This is an archive of the discontinued LLVM Phabricator instance.

[COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32
ClosedPublic

Authored by TomTan on Jan 25 2019, 6:05 PM.

Details

Summary

Windows ARM64 has PIC relocation model and uses jump table kind EK_LabelDifference32. This produces jump table entry as .word LBB123 - LJTI1_2 which represents the distance between the block and jump table. A new relocation type (IMAGE_REL_ARM64_REL32) is needed to do the fixup correctly if they are in different COFF section. This change saves the jump table to the same COFF section as the associated code. An ideal fix could be utilizing IMAGE_REL_ARM64_REL32 relocation type.

Diff Detail

Event Timeline

TomTan created this revision.Jan 25 2019, 6:05 PM
pcc added a subscriber: pcc.Jan 25 2019, 6:26 PM

There is no relocation type for linker to do fixup for this entry correctly if they are in different COFF section.

Looks like there is now. From the PE spec:

IMAGE_REL_ARM64_REL32
0x0011
The 32-bit relative address from the byte following the relocation.

According to archive.org, this was added in the 11/04/2018 revision of the spec. Assuming that support for this was added to the linker recently, we'd probably need to make emitting this conditional on a new enough toolset, though.

Please upload your diffs with extra context (diff -U99999), to allow browsing the surrounding code here on phabricator.

The code change looks good to me, but I'm a little wary about the test; while it uses a CHECK-NEXT on the spot where there previously was a .section .rdata, "dr". Maybe make the line for .LJTI0_0 a CHECK-NEXT and it's at least a bit clearer that there really is no .section between the last instruction and the jump table itself.

In D57277#1372291, @pcc wrote:

There is no relocation type for linker to do fixup for this entry correctly if they are in different COFF section.

Looks like there is now. From the PE spec:

IMAGE_REL_ARM64_REL32
0x0011
The 32-bit relative address from the byte following the relocation.

According to archive.org, this was added in the 11/04/2018 revision of the spec. Assuming that support for this was added to the linker recently, we'd probably need to make emitting this conditional on a new enough toolset, though.

Empirically testing, MSVC 2017 15.4 errors out on this relocation, while 15.5 succeeds (and seems to handle it similarly to the MSVC 2019 previews). 15.4 was the first public version that included the compiler for ARM64 at all, while it only became really officially supported around 15.8 or so. I'll add support for the new relocation in LLD (and using a newer Clang with an older LLD probably isn't something to worry about here IMO).

So given this, it should be fine to use it, but this patch also is totally fine with me.

Thanks Peter and Martin. Yes, IMAGE_REL_ARM64_REL32 can do the relocation work here. I saw Martin added this relocation type to both LLVM and LLD. My change in AArch64AsmPrinter::EmitJumpTableInfo could be reverted when IMAGE_REL_ARM64_REL32 is attached to the jump table entries, but the relocation model is PIC for Windows ARM64.

TomTan updated this revision to Diff 183745.Jan 26 2019, 7:44 PM
TomTan edited the summary of this revision. (Show Details)

Added CHECK-NEXT on the jump table label based on Martin's feedback.

mstorsjo accepted this revision.Jan 27 2019, 11:31 AM

LGTM. While using IMAGE_REL_ARM64_REL32 would be nice, this looks good to me in the meantime.

This revision is now accepted and ready to land.Jan 27 2019, 11:31 AM

Thanks for the review, Martin. Could you please help commit this? I don't have commit access.

Thanks for the review, Martin. Could you please help commit this? I don't have commit access.

Sure, I can do that. I'll wait to give the others a bit of time to comment on it first though.

efriedma added inline comments.
lib/Target/AArch64/AArch64AsmPrinter.cpp
583–592

I'm a little concerned that this is changing the behavior in more cases than you expect. Do you have any idea if you're changing the behavior for MachO targets?

TomTan marked an inline comment as done.Jan 28 2019, 3:03 PM
TomTan added inline comments.
lib/Target/AArch64/AArch64AsmPrinter.cpp
583–592

Yes, MachO target will also be changed. This is conservative change and should not break there. Or do you suggest checking Windows target here for JTInDiffSection?

efriedma added inline comments.Jan 28 2019, 3:14 PM
lib/Target/AArch64/AArch64AsmPrinter.cpp
583–592

I'd rather not change the MachO behavior. (Putting it into the text section is probably fine for normal binaries, but it could break more exotic execute-only environments.)

Checking for Windows explicitly is probably the simplest; this is target-specific code anyway. Or I guess you could write a MachO implementation of shouldPutJumpTableInFunctionSection, but I'm not sure what that should look like off the top of my head.

TomTan updated this revision to Diff 183989.Jan 28 2019, 4:43 PM

Added COFF target check.

TomTan marked 3 inline comments as done.Jan 28 2019, 4:45 PM
TomTan added inline comments.
lib/Target/AArch64/AArch64AsmPrinter.cpp
583–592

Makes sense, thanks. Added the check of COFF target for Windows.

This revision was automatically updated to reflect the committed changes.