[PATCH] D60657: [RISCV] Fix evaluation of %pcrel_lo

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

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

The following testcase

  function:
  .Lpcrel_label1:
  	auipc	a0, %pcrel_hi(other_function)
  	addi	a1, a0, %pcrel_lo(.Lpcrel_label1)
  	.p2align	2          # Causes a new fragment to be emitted
  
  	.type	other_function, at function
  other_function:
  	ret

exposes an odd behaviour in which only the `%pcrel_hi` relocation is evaluated but not the `%pcrel_lo`.

  $ ./bin/llvm-mc -triple riscv64 -filetype obj t.s | ./bin/llvm-objdump  -d -r -
  
  <stdin>:	file format ELF64-riscv
  
  Disassembly of section .text:
  0000000000000000 function:
         0:	17 05 00 00	auipc	a0, 0
         4:	93 05 05 00	mv	a1, a0
  		0000000000000004:  R_RISCV_PCREL_LO12_I	other_function+4
  
  0000000000000008 other_function:
         8:	67 80 00 00	ret

The reason seems to be that in `RISCVAsmBackend::shouldForceRelocation` we only consider the fragment but in `RISCVMCExpr::evaluatePCRelLo` we consider the section. This usually works but there are cases where the section may still be the same but the fragment may be another one. In that case we end forcing a `%pcrel_lo` relocation without any `%pcrel_hi`.

This patch makes `RISCVAsmBackend::shouldForceRelocation` use the section, if any, to determine if the relocation must be forced or not.

This issue seems related to D57240 <https://reviews.llvm.org/D57240>.


Repository:
  rL LLVM

https://reviews.llvm.org/D60657

Files:
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
  llvm/test/MC/RISCV/pcrel-fixups.s


Index: llvm/test/MC/RISCV/pcrel-fixups.s
===================================================================
--- /dev/null
+++ llvm/test/MC/RISCV/pcrel-fixups.s
@@ -0,0 +1,44 @@
+# RUN: llvm-mc -triple riscv32 -mattr=-relax -filetype obj %s \
+# RUN:    | llvm-objdump  -d -r - | FileCheck --check-prefix NORELAX %s
+# RUN: llvm-mc -triple riscv32 -mattr=+relax -filetype obj %s \
+# RUN:    | llvm-objdump  -d -r - | FileCheck --check-prefix RELAX %s
+# RUN: llvm-mc -triple riscv64 -mattr=-relax -filetype obj %s \
+# RUN:    | llvm-objdump  -d -r - | FileCheck --check-prefix NORELAX %s
+# RUN: llvm-mc -triple riscv64 -mattr=+relax -filetype obj %s \
+# RUN:    | llvm-objdump  -d -r - | FileCheck --check-prefix RELAX %s
+
+# Fixups for %pcrel_hi / %pcrel_lo can be evaluated within a section,
+# regardless of the fragment containing the target address.
+
+function:
+.Lpcrel_label1:
+	auipc	a0, %pcrel_hi(other_function)
+	addi	a1, a0, %pcrel_lo(.Lpcrel_label1)
+# NORELAX: auipc	a0, 0
+# NORELAX: addi	a1, a0, 16
+
+# RELAX: auipc	a0, 0
+# RELAX: R_RISCV_PCREL_HI20	other_function
+# RELAX: R_RISCV_RELAX	*ABS*
+# RELAX: mv	a1, a0
+# RELAX: R_RISCV_PCREL_LO12_I	.Lpcrel_label1
+# RELAX: R_RISCV_RELAX	*ABS*
+
+	.p2align	2   # Cause a new fragment be emitted here
+.Lpcrel_label2:
+	auipc	a0, %pcrel_hi(other_function)
+	addi	a1, a0, %pcrel_lo(.Lpcrel_label2)
+# NORELAX: auipc	a0, 0
+# NORELAX: addi	a1, a0, 8
+
+# RELAX: auipc	a0, 0
+# RELAX: R_RISCV_PCREL_HI20	other_function
+# RELAX: R_RISCV_RELAX	*ABS*
+# RELAX: mv	a1, a0
+# RELAX: R_RISCV_PCREL_LO12_I	.Lpcrel_label2
+# RELAX: R_RISCV_RELAX	*ABS*
+
+	.type	other_function, at function
+other_function:
+	ret
+
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
===================================================================
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -53,11 +53,14 @@
     case RISCV::fixup_riscv_got_hi20:
       ShouldForce = true;
       break;
-    case RISCV::fixup_riscv_pcrel_hi20:
-      ShouldForce = T->getValue()->findAssociatedFragment() !=
-                    Fixup.getValue()->findAssociatedFragment();
+    case RISCV::fixup_riscv_pcrel_hi20: {
+      MCFragment *TFragment = T->getValue()->findAssociatedFragment();
+      MCFragment *FixupFragment = Fixup.getValue()->findAssociatedFragment();
+      ShouldForce = !TFragment || !FixupFragment ||
+                    TFragment->getParent() != FixupFragment->getParent();
       break;
     }
+    }
     break;
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60657.195028.patch
Type: text/x-patch
Size: 2559 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190413/a5c0b6a8/attachment.bin>