Open mgaudet opened 6 years ago
This particular section of code should be fine as it is initialized by only a single thread on valid code cache addresses and its initialization is guaranteed to finish before any mutator threads start using it.
Casting to uint32_t *
is really more of a convenience to store 4 bytes than a requirement. I wonder if this entire function should be blacklisted with a no_sanitize_undefined
attribute?
Is this the only problem of this kind found? I would think the snippet binary encoding (and instruction binary encoding for that matter) are rife with this kind of pattern, so I'm surprised there weren't more problems found.
Helper trampoline initialization occurs in a code cache long before any binary encoding, so I'm guessing execution aborted upon encountering the problem reported in this issue before any methods could actually be compiled and those other problem areas encountered. ???
This particular section of code should be fine as it is initialized by only a single thread on valid code cache addresses and its initialization is guaranteed to finish before any mutator threads start using it.
I think the core problem here is not a multi-threading issue, but rather that the code relies on undefined behaviour. The compiler is free to produce just about any arbitrary code here. So, while our current compilers are generating code that does what we want, it's not guaranteed to always be the case. It's not unreasonable to expect a future version of one of our compilers to exploit this UB and start generating code that does something completely different than what we want.
Correct.
A good background to Undefined Behaviour is this series of posts by John Regehr
The concern raised here is that future compilers could freely optimize the trampoline ‘incorrectly’ based on undefined behaviour.
Great, another case of a compiler rule making perfectly readable code into more awkward code.
Seems like a good beginner work item.
Running the compiler technology (via testjit or the compiler test, or Tril's
comptest
) with-fsanitize=undefined
https://github.com/eclipse/omr/blob/8595d1c3b98b623cf696f9e1cb8a8ae1c94e0925/compiler/runtime/Trampoline.cpp#L208-L232
While this is working today, there is always the possibility of future compiler breakage.