[PATCH] D59686: [RISCV] Don't evaluatePCRelLo if a relocation will be forced (e.g. due to linker relaxation)

[Alex Bradbury via Phabricator via llvm-commits]

asb created this revision.
asb added reviewers: apazos, shiva0217, rogfer01.
Herald added subscribers: benna, psnobl, jocewei, PkmX, rkruppe, the_o, brucehoult, MartinMosbeck, edward-jones, zzheng, kito-cheng, niosHD, sabuasal, simoncook, johnrusso, rbar.
Herald added a project: LLVM.

A pcrel_lo will point to the associated pcrel_hi fixup which in turn points to the real target. RISCVMCExpr::evaluatePCRelLo will work around this indirection in order to allow the fixup to be evaluate properly. However, if relocations are forced (e.g. due to linker relaxation is enabled) then its evaluation is undesired and will result in a relocation with the wrong target.

This patch modifies evaluatePCRelLo so it will not try to evaluate if the fixup will be forced as a relocation. A new helper method is added to RISCVAsmBackend to query this.


Repository:
  rL LLVM

https://reviews.llvm.org/D59686

Files:
  lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
  lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
  test/MC/RISCV/linker-relaxation.s


Index: test/MC/RISCV/linker-relaxation.s
===================================================================
--- test/MC/RISCV/linker-relaxation.s
+++ test/MC/RISCV/linker-relaxation.s
@@ -121,14 +121,10 @@
 # RELAX-FIXUP: fixup A - offset: 0, value: %pcrel_hi(bar), kind: fixup_riscv_pcrel_hi20
 # RELAX-FIXUP: fixup B - offset: 0, value: 0, kind: fixup_riscv_relax
 
-# TODO/FIXME: The generated PCREL_LO relocations are incorrect.
-# RISCVMCExpr::evaluatePCRelLo should not be evaluating the fixup when linker
-# relaxation is enabled.
-
 addi t1, t1, %pcrel_lo(2b)
 # NORELAX-RELOC-NOT: R_RISCV_PCREL_LO12_I
 # NORELAX-RELOC-NOT: R_RISCV_RELAX
-# RELAX-RELOC: R_RISCV_PCREL_LO12_I bar 0x4
+# RELAX-RELOC: R_RISCV_PCREL_LO12_I .Ltmp1 0x0
 # RELAX-RELOC: R_RISCV_RELAX - 0x0
 # RELAX-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp1), kind: fixup_riscv_pcrel_lo12_i
 # RELAX-FIXUP: fixup B - offset: 0, value: 0, kind: fixup_riscv_relax
@@ -136,7 +132,7 @@
 sb t1, %pcrel_lo(2b)(a2)
 # NORELAX-RELOC-NOT: R_RISCV_PCREL_LO12_S
 # NORELAX-RELOC-NOT: R_RISCV_RELAX
-# RELAX-RELOC: R_RISCV_PCREL_LO12_S bar 0x8
+# RELAX-RELOC: R_RISCV_PCREL_LO12_S .Ltmp1 0x0
 # RELAX-RELOC: R_RISCV_RELAX - 0x0
 # RELAX-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp1), kind: fixup_riscv_pcrel_lo12_s
 # RELAX-FIXUP: fixup B - offset: 0, value: 0, kind: fixup_riscv_relax
Index: lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
===================================================================
--- lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
+++ lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
@@ -11,9 +11,11 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "RISCV.h"
 #include "RISCVMCExpr.h"
+#include "MCTargetDesc/RISCVAsmBackend.h"
+#include "RISCV.h"
 #include "RISCVFixupKinds.h"
+#include "llvm/MC/MCAsmLayout.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCStreamer.h"
@@ -87,6 +89,16 @@
   // (<real target> + <offset from this fixup to the auipc fixup>).  The Fixup
   // is pcrel relative to the VK_RISCV_PCREL_LO fixup, so we need to add the
   // offset to the VK_RISCV_PCREL_HI Fixup from VK_RISCV_PCREL_LO to correct.
+
+  // Don't try to evaluate if the fixup will be forced as a relocation (e.g.
+  // as linker relaxation is enabled). If we evaluated pcrel_lo in this case,
+  // the modified fixup will be converted into a relocation that no longer
+  // points to the pcrel_hi as the linker requires.
+  auto &RAB =
+      static_cast<RISCVAsmBackend &>(Layout->getAssembler().getBackend());
+  if (RAB.willForceRelocations())
+    return false;
+
   MCValue AUIPCLoc;
   if (!getSubExpr()->evaluateAsValue(AUIPCLoc, *Layout))
     return false;
Index: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
===================================================================
--- lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
+++ lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
@@ -41,11 +41,18 @@
 
   void setForceRelocs() { ForceRelocs = true; }
 
+  // Returns true if relocations will be forced for shouldForceRelocation by
+  // default. This will be true if relaxation is enabled or had previously
+  // been enabled.
+  bool willForceRelocations() const {
+    return ForceRelocs || STI.getFeatureBits()[RISCV::FeatureRelax];
+  }
+
   // Generate diff expression relocations if the relax feature is enabled or had
   // previously been enabled, otherwise it is safe for the assembler to
   // calculate these internally.
   bool requiresDiffExpressionRelocations() const override {
-    return STI.getFeatureBits()[RISCV::FeatureRelax] || ForceRelocs;
+    return willForceRelocations();
   }
 
   // Return Size with extra Nop Bytes for alignment directive in code section.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59686.191837.patch
Type: text/x-patch
Size: 3783 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190322/e3c09e84/attachment.bin>