[PATCH] D57240: [RISCV] Don't incorrectly force relocation for %pcrel_lo

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill created this revision.
lewis-revill added reviewers: asb, PkmX, jrtc27.
Herald added subscribers: llvm-commits, jocewei, rkruppe, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, shiva0217, kito-cheng, niosHD, sabuasal, apazos, simoncook, johnrusso, rbar.

This patch updatess the logic introduced by D54029 <https://reviews.llvm.org/D54029>, which attempted to check if a %pcrel_lo relocation should be forced based on whether the previous %pcrel_hi fixup would have a relocation. The check forced the relocation if the fragment containing the symbol associated with the %pcrel_hi and the fragment containing the symbol referenced by the %pcrel_hi were different.

However there are some occassions where the symbol referenced by %pcrel_hi is in a different fragment to the %pcrel_hi itself but the assembler determines it can be resolved. The %pcrel_lo would be incorrectly emitted in this case, resulting in an invalid object file that would not link (dangerous relocation: %pcrel_lo missing matching %pcrel_hi).

      

This patch explicitly stores %pcrel_hi fixups that were previously resolved and uses this to check whether a relocation should be forced for a %pcrel_lo fixup. A %pcrel_lo relocation should never be forced if the corresponding %pcrel_hi fixup was resolved without a relocation.


Repository:
  rL LLVM

https://reviews.llvm.org/D57240

Files:
  lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
  lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
  test/MC/RISCV/fixups.s


Index: test/MC/RISCV/fixups.s
===================================================================
--- test/MC/RISCV/fixups.s
+++ test/MC/RISCV/fixups.s
@@ -24,6 +24,8 @@
 # CHECK-FIXUP: fixup A - offset: 0, value: %lo(val), kind: fixup_riscv_lo12_s
 # CHECK-INSTR: sw a0, 1656(t1)
 
+# Create a new fragment boundary here.
+.align 0
 1:
 auipc t1, %pcrel_hi(.LBB0)
 # CHECK-FIXUP: fixup A - offset: 0, value: %pcrel_hi(.LBB0), kind: fixup_riscv_pcrel_hi20
Index: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
===================================================================
--- lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
+++ lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
@@ -11,6 +11,7 @@
 
 #include "MCTargetDesc/RISCVFixupKinds.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
@@ -22,6 +23,7 @@
 
 class RISCVAsmBackend : public MCAsmBackend {
   const MCSubtargetInfo &STI;
+  SmallSet<const MCFixup *, 8> ResolvedPCRelHiFixups;
   uint8_t OSABI;
   bool Is64Bit;
   bool ForceRelocs = false;
Index: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
===================================================================
--- lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -36,10 +36,13 @@
   case RISCV::fixup_riscv_tls_got_hi20:
   case RISCV::fixup_riscv_tls_gd_hi20:
     return true;
+  case RISCV::fixup_riscv_pcrel_hi20:
+    ResolvedPCRelHiFixups.insert(&Fixup);
+    break;
   case RISCV::fixup_riscv_pcrel_lo12_i:
   case RISCV::fixup_riscv_pcrel_lo12_s:
-    // For pcrel_lo12, force a relocation if the target of the corresponding
-    // pcrel_hi20 is not in the same fragment or if the target is something
+    // For pcrel_lo12, force a relocation if the corresponding pcrel_hi20 hasn't
+    // already been resolved without a relocation or if the target is something
     // other than a pcrel_hi20 (such as a got_hi20)..
     const MCFixup *T = cast<RISCVMCExpr>(Fixup.getValue())->getPCRelHiFixup();
     if (!T) {
@@ -58,8 +61,8 @@
       ShouldForce = true;
       break;
     case RISCV::fixup_riscv_pcrel_hi20:
-      ShouldForce = T->getValue()->findAssociatedFragment() !=
-                    Fixup.getValue()->findAssociatedFragment();
+      if (ResolvedPCRelHiFixups.count(T) == 0)
+        ShouldForce = true;
       break;
     }
     break;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57240.183545.patch
Type: text/x-patch
Size: 2486 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190125/4a76d2df/attachment.bin>