RFR: 8354668: Missing REX2 prefix accounting in ZGC barriers leads to incorrect encoding

[Axel Boldt-Christmas]

On Tue, 15 Apr 2025 13:50:40 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> ZGC bookkeeps multiple place holders in barrier code snippets through relocations, these are later used to patch appropriate contents (mostly immediate values) in instruction encoding. While most of the relocation records the patching offsets from the end of the instruction, SHL instruction, which is used for pointer coloring, computes the patching offset from the starting address of the instruction.
> 
> Thus, in case the destination register operand of SHL instruction is an extended GPR register, we miss accounting additional REX2 prefix byte in patch offset, thereby corrupting the encoding since runtime patches the primary opcode byte resulting into ILLEGAL instruction exception.
> 
> This patch fixes reported failures by computing the relocation offset of SHL instruction from end of instruction, thereby making the patch offset agnostic to REX/REX2 prefix.
> 
> Please review and share your feedback.
> 
> Best Regards,
> Jatin
> 
> PS: Validation were performed using latest Intel Software Development Emulator after modifying static register allocation order in x86_64.ad file giving preference to EGPRs.

Looks good but need to communicate with JVMCI implementors.

Also pre-exisiting but maybe `ZBarrierRelocationFormatLoadGoodAfterShl` should be called `ZBarrierRelocationFormatLoadGoodAfterShX` as we use it for both shr and shl.

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.hpp line 52:

> 50: #endif // COMPILER2
> 51: 
> 52: const int ZBarrierRelocationFormatLoadGoodAfterShl = 0;

Suggestion:

const int ZBarrierRelocationFormatLoadGoodAfterShl  = 0;

src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp line 223:

> 221:       return true;
> 222: #if INCLUDE_ZGC
> 223:     case Z_BARRIER_RELOCATION_FORMAT_LOAD_GOOD_BEFORE_SHL:

Should probably communicate with the JVMCI / Graal @dougxc so we can both update this exported symbol name to reflect the new behaviour, and give them the opportunity to adapt to the new relocation patching.

-------------

Changes requested by aboldtch (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24664#pullrequestreview-2768666320
PR Review Comment: https://git.openjdk.org/jdk/pull/24664#discussion_r2044778342
PR Review Comment: https://git.openjdk.org/jdk/pull/24664#discussion_r2044814373