Undefined Behaviour in AMD64 Trampoline

[mgaudet]

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

../compiler/runtime/Trampoline.cpp:226:7: runtime error: store to misaligned address 0x00010ee41142 for type 'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x00010ee41142: note: pointer points here
 00 00  ff 25 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
              ^ 
../compiler/runtime/Trampoline.cpp:228:7: runtime error: store to misaligned address 0x00010ee41146 for type 'int64_t' (aka 'long long'), which requires 8 byte alignment
0x00010ee41146: note: pointer points here
 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00
             ^ 

While this is working today, there is always the possibility of future compiler breakage.

[0xdaryl]

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?

[0xdaryl]

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. ???

[Leonardo2718]

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.

[mgaudet]

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.

[mstoodle]

Great, another case of a compiler rule making perfectly readable code into more awkward code.

Seems like a good beginner work item.