[RISCV] Don't force absolute FK_Data_X fixups to relocs

[edward-jones]

Summary

The current behavior of shouldForceRelocation forces relocations for the majority of fixups when relaxation is enabled. This makes sense for fixups which incorporate symbols but is unnecessary for simple data fixups where the fixup target is already resolved to an absolute value.

[edward-jones]

Rebased against master

[asb]

Hi Ed, can you please add a test that shows this new behaviour? Perhaps it makes sense in test/MC/RISCV/linker-relaxation.s? (FK_Data_{4,8} produce R_RISCV_{32,64} when a relocation is emitted).

[edward-jones]

Added test, rebased

[asb]

Hi Ed, I'm struggling a bit to follow the test case you're adding here. You're checking for the absence of R_RISCV_{32,64} but the relocations produced by these expressions are surely going to be R_RISCV_{ADD,SUB}{32,64}?

In the general case, aren't we required to emit relocations for these symbol differences? The symbols might be defined in a code section and relaxation might lead to the symbol location changing?

[edward-jones]

I realize that I hadn't associated this change with D61584

Normally you would get a R_RISCV_{ADD,SUB}{32,64} for the difference expression in the test case, however with the changes in D61584 a difference expression in .data will instead be folded (by MCExpr.cpp: EvaluateSymbolicAdd) to a FK_Data_{4,8} fixup which this change can then collapse to a constant value.

At the start of a CIE in .eh_frame there is a difference expression which expresses the length of the CIE. Before D61584 and this change the length field would end up zeroed with a R_RISCV_{32,64} or R_RISCV_{ADD,SUB}{32,64} relocation. This posed a problem because the linker parses the .eh_frame section ignoring the relocations, and the zero length would cause the parsing to error out.

[asb]

Thanks for the clarification Ed. This looks good to me - thanks!

[edward-jones]

Rebase, and fix another test of FDE

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
                                             const MCFixup &Fixup,
                                             const MCValue &Target) {
   bool ShouldForce = false;
 
   switch ((unsigned)Fixup.getKind()) {
   default:
     break;
+  case FK_Data_1:
+  case FK_Data_2:
+  case FK_Data_4:
+  case FK_Data_8:
+    if (Target.isAbsolute())
+      return false;
+    break;
   case RISCV::fixup_riscv_got_hi20:
   case RISCV::fixup_riscv_tls_got_hi20:
   case RISCV::fixup_riscv_tls_gd_hi20:
     return true;
   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.

llvm/trunk/test/MC/RISCV/fde-reloc.s

 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+relax < %s \
 # RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX-RELOC %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=-relax < %s \
 # RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX-RELOC %s
 
 func:
   .cfi_startproc
   ret
   .cfi_endproc
 
 # TODO: Should produce R_RISCV_32_PCREL for the FDE pc relocation. Many of the
 # ADD32/SUB32 relocations also can be safely resolved even with linker
 # relaxation enabled. This test is written to capture current behaviour, in
 # preparation for follow-on patches to fix it.
 
 # RELAX-RELOC:   Section (4) .rela.eh_frame {
-# RELAX-RELOC-NEXT:   0x0 R_RISCV_32 - 0x10
-# RELAX-RELOC-NEXT:   0x14 R_RISCV_32 - 0x10
-# RELAX-RELOC-NEXT:   0x18 R_RISCV_32 - 0x18
 # RELAX-RELOC-NEXT:   0x1C R_RISCV_ADD32 - 0x0
 # RELAX-RELOC-NEXT:   0x1C R_RISCV_SUB32 - 0x0
 # RELAX-RELOC-NEXT:   0x20 R_RISCV_ADD32 - 0x0
 # RELAX-RELOC-NEXT:   0x20 R_RISCV_SUB32 - 0x0
 # RELAX-RELOC-NEXT: }
 
 # NORELAX-RELOC:        Section (4) .rela.eh_frame {
 # NORELAX-RELOC-NEXT:    0x1C R_RISCV_32_PCREL - 0x0
 # NORELAX-RELOC-NEXT:  }

llvm/trunk/test/MC/RISCV/linker-relaxation.s

 
 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 .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
+
+# Check that a relocation is not emitted for a symbol difference which has
+# been folded to a fixup with an absolute value. This can happen when a
+# difference expression refers to two symbols, at least one of which is
+# not defined at the point it is referenced. Then during *assembler*
+# relaxation when both symbols have become defined the difference may be folded
+# down to a fixup simply containing the absolute value. We want to ensure that
+# we don't force a relocation to be emitted for this absolute value even
+# when linker relaxation is enabled. The reason for this is that one instance
+# where this pattern appears in in the .eh_frame section (the CIE 'length'
+# field), and the .eh_frame section cannot be parsed by the linker unless the
+# fixup has been resolved to a concrete value instead of a relocation.
+  .data
+lo:
+  .word hi-lo
+  .quad hi-lo
+# NORELAX-RELOC-NOT: R_RISCV_32
+# NORELAX-RELOC-NOT: R_RISCV_64
+# RELAX-RELOC-NOT: R_RISCV_32
+# RELAX-RELOC-NOT: R_RISCV_64
+hi: