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

[TomTan]

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.

[pcc]

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.

[mstorsjo]

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.

[mstorsjo]

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.

[TomTan]

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]

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

[mstorsjo]

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

[TomTan]

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

[mstorsjo]

In D57277#1373043, @TomTan wrote:

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]

lib/Target/AArch64/AArch64AsmPrinter.cpp

583 ↗

(On Diff #183745)

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]

lib/Target/AArch64/AArch64AsmPrinter.cpp

583 ↗

(On Diff #183745)

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]

lib/Target/AArch64/AArch64AsmPrinter.cpp

583 ↗

(On Diff #183745)

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]

Added COFF target check.

[TomTan]

lib/Target/AArch64/AArch64AsmPrinter.cpp

583 ↗

(On Diff #183745)

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

[efriedma]

LGTM

llvm/trunk/lib/Target/AArch64/AArch64AsmPrinter.cpp

 
 void AArch64AsmPrinter::EmitJumpTableInfo() {
   const MachineJumpTableInfo *MJTI = MF->getJumpTableInfo();
   if (!MJTI) return;
 
   const std::vector<MachineJumpTableEntry> &JT = MJTI->getJumpTables();
   if (JT.empty()) return;
 
+  const Function &F = MF->getFunction();
   const TargetLoweringObjectFile &TLOF = getObjFileLowering();
-  MCSection *ReadOnlySec = TLOF.getSectionForJumpTable(MF->getFunction(), TM);
+  bool JTInDiffSection =
+      !STI->isTargetCOFF() ||
+      !TLOF.shouldPutJumpTableInFunctionSection(
+          MJTI->getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32,
+          F);
+  if (JTInDiffSection) {
+      // Drop it in the readonly section.
+      MCSection *ReadOnlySec = TLOF.getSectionForJumpTable(F, TM);
       OutStreamer->SwitchSection(ReadOnlySec);
+  }
 
   auto AFI = MF->getInfo<AArch64FunctionInfo>();
   for (unsigned JTI = 0, e = JT.size(); JTI != e; ++JTI) {
     const std::vector<MachineBasicBlock*> &JTBBs = JT[JTI].MBBs;
 
     // If this jump table was deleted, ignore it.
     if (JTBBs.empty()) continue;
 

llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp

if (TT.isOSBinFormatCOFF())
     return "e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128";
   if (LittleEndian)
     return "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128";
   return "E-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128";
 }
 
 static Reloc::Model getEffectiveRelocModel(const Triple &TT,
                                            Optional<Reloc::Model> RM) {
-  // AArch64 Darwin is always PIC.
-  if (TT.isOSDarwin())
+  // AArch64 Darwin and Windows are always PIC.
+  if (TT.isOSDarwin() || TT.isOSWindows())
     return Reloc::PIC_;
   // On ELF platforms the default static relocation model has a smart enough
   // linker to cope with referencing external symbols defined in a shared
   // library. Hence DynamicNoPIC doesn't need to be promoted to PIC.
   if (!RM.hasValue() || *RM == Reloc::DynamicNoPIC)
     return Reloc::Static;
   return *RM;
 }

llvm/trunk/test/CodeGen/AArch64/win64-jumptable.ll

+; RUN: llc -o - %s -mtriple=aarch64-windows -aarch64-enable-compress-jump-tables=0 | FileCheck %s
+
+define void @f(i32 %x) {
+entry:
+  switch i32 %x, label %sw.epilog [
+    i32 0, label %sw.bb
+    i32 1, label %sw.bb1
+    i32 2, label %sw.bb2
+    i32 3, label %sw.bb3
+  ]
+
+sw.bb:                                            ; preds = %entry
+  tail call void @g(i32 0) #2
+  br label %sw.epilog
+
+sw.bb1:                                           ; preds = %entry
+  tail call void @g(i32 1) #2
+  br label %sw.epilog
+
+sw.bb2:                                           ; preds = %entry
+  tail call void @g(i32 2) #2
+  br label %sw.epilog
+
+sw.bb3:                                           ; preds = %entry
+  tail call void @g(i32 3) #2
+  br label %sw.epilog
+
+sw.epilog:                                        ; preds = %entry, %sw.bb3, %sw.bb2, %sw.bb1, %sw.bb
+  tail call void @g(i32 10) #2
+  ret void
+}
+
+declare void @g(i32)
+
+; CHECK:    .text
+; CHECK:    f:
+; CHECK:    .seh_proc f
+; CHECK:    b g
+; CHECK-NEXT: .p2align  2
+; CHECK-NEXT: .LJTI0_0:
+; CHECK:    .word .LBB0_2-.LJTI0_0
+; CHECK:    .word .LBB0_3-.LJTI0_0
+; CHECK:    .word .LBB0_4-.LJTI0_0
+; CHECK:    .word .LBB0_5-.LJTI0_0
+; CHECK:    .section  .xdata,"dr"
+; CHECK:    .seh_handlerdata
+; CHECK:    .text
+; CHECK:    .seh_endproc