Remove dangerous memsets that don't survive gcc16 dead store optimization - #24564
Remove dangerous memsets that don't survive gcc16 dead store optimization#24564fridrich wants to merge 2 commits into
Conversation
|
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 |
|
The OMR PR related to this one is eclipse-openj9/openj9-omr#286 |
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. |
38f2a77 to
86df88e
Compare
I downloaded the .clang-format from the link mentioned in the doc. I ran the command |
|
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. |
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. |
I must be dumb, can you point me to the log? |
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 :( |
Paired with OMR commit with the same title
The project failed to build properly because GCC 16 optimized out a memory operation during the
TR::CompilationInfoconstruction (memsetinsidecreateCompilationInfo). As a band-aid, I put a memory barrier (__asm__("" : : : "memory")) between thememsetand the subsequentnewcalls. 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
memsethacks on thethispointer or immediately before placement new, ensuring it doesn't crash when running underMALLOC_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 cleanbootcycle-imagesbuild 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_StatsEventsincompiler/infra/Statistics.hpp.