Align mutable fields of x86 PIC data #4604
Conversation
|
@0dvictor this looks good to me but would appreciate your expert opinion on the x codegen changes. |
|
The changes look good to me. |
|
FYI, #4627 removes the |
|
Yeah I knew the deletion was coming. I modified the .inc and .pasm files because they were still in use on 32-bit when I started writing this change. Should I revert them? I've already had to rebase this and re-test once due to a conflict with the removal of an |
|
@jdmpapin can you fix the merge conflicts please? |
The resolveAndPopulateVTableDispatch function is used only by TR::X86UnresolvedVirtualCallSnippet, which is never created. Furthermore, if a TR::X86UnresolvedVirtualCallSnippet were to be created, this part of PicBuilder wouldn't work properly in its current state. For instance, it calls jitResolveVirtualMethod passing the same address twice, once as the JIT return address, and once as the VPIC data address, even though those addresses should be different. This commit deletes both resolveAndPopulateVTableDispatch and TR::X86UnresolvedVirtualCallSnippet to prevent further bit-rot. Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>
PicBuilder on x86 was modified in bada7c8 to locate fields of the VPIC data using named constants rather than hard-coded offsets, but there were a few remaining places where the code continued to rely on implicit assumptions about the layout. In particular, - cpAddr was assumed to have offset 0, with pointers to the beginning of the VPIC data being treated as pointers to cpAddr; and - on 32-bit, cmpRegImm4ModRM was assumed to be the last byte. PicBuilder is updated to avoid relying on these implicit assumptions, in order to facilitate the rearrangement of the fields of the VPIC data. Because of jitResolveVirtualMethod(), cpIndex must still always follow immediately after cpAddr. Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>
Because directMethod is written at runtime, and might be concurrently read by another thread, it must be properly aligned to ensure that accesses to it are atomic. Currently it's possible to observe tearing, which can lead PicBuilder to attempt direct dispatch with a bogus J9Method pointer. The necessary alignment is achieved by reordering the fields of the VPIC data so that the pointer-sized fields are at the end. The VPIC data immediately precedes a patchable instruction, which itself is aligned on an 8-byte boundary, so this rearrangement ensures that all of the pointer-sized fields (including directMethod) are naturally aligned. Fixes eclipse-openj9#4446 Signed-off-by: Devin Papineau <devinmp@ca.ibm.com> Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>
Because the interface class and itable offset can be written at runtime, and might be read concurrently by another thread, they need to be aligned so that the accesses will be atomic. Currently it's possible to observe tearing, which can lead PicBuilder to attempt direct dispatch with a bogus J9Method pointer. Such tearing might also interfere with standard interface dispatch. The start of the snippet is now adjusted to effect the alignment. Fixes eclipse-openj9#4446 Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>
jdmpapin
force-pushed
the
pic-data-alignment
branch
from
7d20fac to
3a6ddd3
Compare
|
Updated to leave the (now deleted) old asm files unmodified |
|
@0dvictor are you good with this now? |
|
LGTM. |
|
Jenkins test sanity xlinux,win jdk8,jdk11 |
|
Is this good to go now? |
To prevent tearing, this series aligns fields of the x86 VPIC and IPIC data that are written at runtime. The IPIC data is aligned simply by adjusting the start of the snippet. In the VPIC data snippet, the end of the VPIC data is already aligned, so we get the necessary field alignment by reordering the fields. This reordering requires some changes to PicBulider to remove lingering implicit assumptions about data layout. There is an unused and (almost certainly) broken function in PicBuilder which is removed instead of modified. See the individual commit messages for more.