Remove dangerous memsets that don't survive gcc16 dead store optimization

[fridrich]

The project failed to build properly because GCC 16 optimized out a memory operation during the TR::CompilationInfo construction (memset inside createCompilationInfo). As a band-aid, I put a memory barrier (__asm__("" : : : "memory")) between the memset and the subsequent new calls. Just that in C++98, there is no portable way to put a memory barrier, so that was not a real solution for this problem.
Nevertheless, I saw a comment that was advising the proper solution. The goal was to remove the workaround and fix the C++ class initializations properly so that the memory is fully zero-initialized safely without memset hacks on the this pointer or immediately before placement new, ensuring it doesn't crash when running under MALLOC_PERTURB_=63.
And since this is exactly the kind of thing, where code assist agents are useful, I was doing this using Gemini, for the full disclosure. Still building myself with the MALLOC_PERTURB_ and iterating over the crashes until a clean bootcycle-images build was not successful.

After fixing the current crash with gcc16 optimization, I scanned the code for similar constructs and found one potential ticking bomb in runtime/compiler/optimizer/NewInitialization.cpp. Since the fix was trivial, I added that one too.

This pull request will need a similar pull request in OMR that I will create after this one. The part is to initialize properly the TR_StatsEvents in compiler/infra/Statistics.hpp.

[fridrich]

The Clang Format Check, I would fix it if I knew how to get the information from it. And there was no commit hook warning me about anything. Is there some astyle or somesuch command to run?

[fridrich]

The OMR PR related to this one is eclipse-openj9/openj9-omr#286

[dsouzai]

The Clang Format Check, I would fix it if I knew how to get the information from it.

The Clang Format Check job prints out a diff that you can apply to get the proper formatting. You can find more details on the format here.

[fridrich]

The Clang Format Check, I would fix it if I knew how to get the information from it.

The Clang Format Check job prints out a diff that you can apply to get the proper formatting. You can find more details on the format here.

I downloaded the .clang-format from the link mentioned in the doc. I ran the command
GIT_SEQUENCE_EDITOR=true git rebase --interactive --strategy-option=theirs --exec "git clang-format HEAD^ ; git commit --amend --no-edit --all --no-verify" HEAD~2
and force-pushed the commits. Still the clang format check fails. What I am doing wrong?

[fridrich]

As for a possibility of build failing with these fixes only, it is normal, the related commit in openj9-omr from eclipse-openj9/openj9-omr#286 is needed. But that commit can land without this PR being integrated if need be, because the current state with the memset hack will still be working with that commit. Just this PR needs the TR_StatsEvents properly initialized.

[dsouzai]

The Clang Format Check, I would fix it if I knew how to get the information from it.

The Clang Format Check job prints out a diff that you can apply to get the proper formatting. You can find more details on the format here.

I downloaded the .clang-format from the link mentioned in the doc. I ran the command
GIT_SEQUENCE_EDITOR=true git rebase --interactive --strategy-option=theirs --exec "git clang-format HEAD^ ; git commit --amend --no-edit --all --no-verify" HEAD~2
and force-pushed the commits. Still the clang format check fails. What I am doing wrong?

It's likely that you need to use the exact Clang Format version as the one used in the Linter job. If you look at the documentation linked above, it should point you to the Dockerfile that the Linter uses, which specifies the version.

Clang Format unfortunately changes its output, even across minor versions.

At any rate, if you continue to run into issues, applying the diff provided by the format checker job will unblock you.

[fridrich]

At any rate, if you continue to run into issues, applying the diff provided by the format checker job will unblock you.

I must be dumb, can you point me to the log?

[dsouzai]

At any rate, if you continue to run into issues, applying the diff provided by the format checker job will unblock you.

I must be dumb, can you point me to the log?

https://openj9-jenkins.osuosl.org/job/PullRequest-Clang-Format-Check/2942/console

[fridrich]

https://openj9-jenkins.osuosl.org/job/PullRequest-Clang-Format-Check/2942/console

Thanks, that helped. I fixed it manually instead of chasing the exact version of clang-format :(

[fridrich]

Where is this one hanging? Do I need to rebase it?

[fridrich]

I would not mind it to be in for the October release.

[pshipton]

@hzongaro @mpirvu fyi

[fridrich]

Btw, the corresponding omr fix went in, so this one should be perfectly buildable with MALLOC_PERTURB_ using recent omr. I did not rebase because I don't know how many reviews it would invalidate, but just say a word... But the rebase done here locally did not have one single conflict.

[fridrich]

I hit this discrepancy btw:

In RelocationRuntime.hpp:522 (UNCONDITIONAL):

#if 1 // defined(DEBUG) || defined(PROD_WITH_ASSUMES)
      // Detect unexpected scenarios when build has assumes
    uint32_t _numValidations;
    uint32_t _numFailedValidations;
    uint32_t _numInlinedMethodRelos;
    uint32_t _numFailedInlinedMethodRelos;
    uint32_t _numInlinedAllocRelos;
    uint32_t _numFailedInlinedAllocRelos;
#endif

In RelocationRuntime.cpp:245 (CONDITIONAL):

#if defined(DEBUG) || defined(PROD_WITH_ASSUMES)
    _numValidations = 0;
    _numFailedValidations = 0;
    _numInlinedMethodRelos = 0;
    _numFailedInlinedMethodRelos = 0;
    _numInlinedAllocRelos = 0;
    _numFailedInlinedAllocRelos = 0;
#endif

The #if 1 sucks them unconditionally. What is the right thing to do with them? Which condition is good?

RelocationRecord.cpp:3398 calls incNumValidations() unconditionally, incrementing garbage memory

[mpirvu]

LGTM.
A few small suggestions:

1. Remove stale comment: "// The object is zero-initialized before this method is called" from CompilationThread.cpp line 1279
2. Move "_jitConfig = jitConfig;" (line 1282) into the initializer list of CompilationInfo.
3. Move "_vmStateOfCrashedThread = 0;" (line 1288) into the initializer list of CompilationInfo
4. Move "_crashWasDueToOrphanedConstRefs = false;" (line 1289) into the initializer list of CompilationInfo
5. I am in favor of removing that "#if 1" both from RelocationRuntime.hpp and cpp files.
6. Please squash all commits into 1.

[fridrich]

LGTM. A few small suggestions:

1. Remove stale comment: "// The object is zero-initialized before this method is called" from CompilationThread.cpp line 1279

2. Move "_jitConfig = jitConfig;" (line 1282) into the initializer list of CompilationInfo.

3. Move "_vmStateOfCrashedThread = 0;" (line 1288) into the initializer list of CompilationInfo

4. Move "_crashWasDueToOrphanedConstRefs = false;" (line 1289) into the initializer list of CompilationInfo

5. I am in favor of removing that "#if 1" both from RelocationRuntime.hpp and cpp files.

6. Please squash all commits into 1.

Done. Just remember that the commit in omr openj9 branch with the exactly same title is needed so that this works fine.

[mpirvu]

jenkins test sanity all jdk25

[fridrich]

The cmdLineTester_jfr_0 failure (VMAccess.cpp:431, mustHaveVMAccess in "VM API Test aggressive start and stop") looks unrelated. The change only replaces the CompilationInfo memset with explicit initializers, and every member is covered. This test already fails intermittently (#24611, #23261). Only x86-64 Linux failed; the same test passed on ppc64le and s390x. Could someone rerun it or run a Grinder, and if possible share the stack of the asserting thread from the javacore?

[mpirvu]

The JFR test failure is a known issue covered by #24693

[fridrich]

If this gets integrated, what is the probability it could make it for 0.63.0?

[fridrich]

I checked yesterday, the omr patch is not strictly necessary, since it only fixes stuff like zeroing *this in constructor and I was finding it quite bad C++ and fixed it. But I run my builds without it and with MALLOC_PERTURB_ the bootcycle builds passed just fine, with gcc 16.2.0, without this patch, they clearly were crashing showing poisoned memory, without the omr patch, they passed.

[mpirvu]

The other test failures have all been seen before.

AIX failed jdk_foreign_0

       Failed test cases: 
        TEST: java/foreign/TestBufferStackStress.java
        TEST: java/foreign/TestBufferStackStress2.java

Exception in thread "" java.lang.IncompatibleClassChangeError and timeout tracked here: #22224 (comment)

openjdk on xlinux failed jdk_util_other_1

12:20:56  ACTION: main -- Error. '/home/jenkins/workspace/Test_openjdk25_j9_sanity.openjdk_x86-64_linux_Personal_testList_2/jdkbinary/j2sdk-image/bin/java' timed out after 2400000 ms (elapsed time including timeout handling 29176708 ms)
12:20:56  REASON: User specified action: run main/othervm/timeout=300 -Djava.locale.providers=CLDR,SPI DateFormatProviderTest 

Tracked here: #24811

openjdk mac failed jdk_lang_0
This is an infra issue: java.lang.Exception: No JUnit driver -- install JUnit JAR file(s) next to jtreg.jar

openjdk windows failed jdk_security4_1

07:48:19  java.lang.Exception: Timestamp is Fri Sep 25 22:47:19 UTC 2026, actual difference 39554 is not 39600
07:48:19  	at Renewal.checkRough(Renewal.java:166)
07:48:19  	at Renewal.checkKinit(Renewal.java:112)
07:48:19  	at Renewal.main(Renewal.java:72)
07:48:19  	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
07:48:19  	at java.base/java.lang.reflect.Method.invoke(Method.java:571)
07:48:19  	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
07:48:19  	at java.base/java.lang.Thread.run(Thread.java:1485)
07:48:19  
07:48:19  JavaTest Message: Test threw exception: java.lang.Exception: Timestamp is Fri Sep 25 22:47:19 UTC 2026, actual difference 39554 is not 39600

Tracked here: #17749

[mpirvu]

Since all test failure have been accounted for, this PR is ready to be merged.

[pshipton]

@fridrich pls cherry pick your commit against https://github.com/eclipse-openj9/openj9/tree/v0.63.0-release and create a PR.

[keithc-ca]

I believe the previous new expression was correct; adding parentheses is not appropriate:

    Candidate *candidate = new (trStackMemory()) Candidate;

[pshipton]

Why? It seems the parentheses will cause zero init of the members which aren't explicitly initialized in the constructor.

[fridrich]

The parentheses call the Candidate constructor in which I took care to properly initialize every member of the struct.

BTW, I really love that idea of MALLOC_PERTURB_ and the fact that SUSE build service builds always setting it to 63, so that we can know immediately from the log whether we read after free or whether we use uninitialized memory. Something like this in the CI workflow could be good. But yes, that would require to build bootcycle-images so that one actually tests the currently built virtual machine.

[keithc-ca]

Thank you, @fridrich. It's refreshing to see this sort of improvement.

[fridrich]

Thank you, @fridrich. It's refreshing to see this sort of improvement.

Glad to help. First approach was memory barrier, but that one is not portable if we don't use C++11. But this is cleaner and more C++ like :)

[fridrich]

@fridrich pls cherry pick your commit against https://github.com/eclipse-openj9/openj9/tree/v0.63.0-release and create a PR.

#24818