[RISCV] Don't fold symbol diff

[edward-jones]

Summary

When emitting the difference between two symbols, the standard behavior is that the difference will be resolved to an absolute value if both of the symbols are offsets from the same data fragment. This is undesirable on architectures such as RISC-V where relaxation in the linker may cause the computed difference to become invalid. This caused an issue when compiling to object code, where the size of a function in the debug information was already calculated even though it could change as a consequence of relaxation in the subsequent linking stage.

This patch inhibits the resolution of symbol differences to absolute values where the target's AsmBackend has declared that it does not want these to be folded.

[asb]

Thanks, this seems sensible to me. Could you add a test please?

[edward-jones]

Working on creating a test at the moment, however while trying to create a test I realized this change doesn't actually work when using llc to generate an object file. It seems that when compiling with llc, the RISCV::FeatureRelax bit is never set, so RISCV::requiresDiffExpressionRelocations always returns false. This happens even with "target-features"="+relax" in the function attributes.

[edward-jones]

Added a test

[kito-cheng]

lib/MC/MCObjectStreamer.cpp

59 ↗

(On Diff #144682)

I got compile error here:

MCObjectStreamer.cpp:94:7: error: ‘TAB’ was not declared in this scope

[simoncook]

lib/MC/MCObjectStreamer.cpp

59 ↗

(On Diff #144682)

Hi Kito. Ed is out of the office today so I can't update this diff right now (I'll ask him to do this on Monday), but I've rebased it and this line should now be

if (getAssembler().getBackendPtr()->requiresDiffExpressionRelocations())

[asb]

Looks good to me, thanks!

include/llvm/MC/MCObjectStreamer.h

172 ↗

(On Diff #146558)

hi -> Hi

[asb]

include/llvm/MC/MCObjectStreamer.h

172 ↗

(On Diff #146558)

On second thought, I'd be tempted to keep this helper as a static function and add an MCAssembler & parameter to it. It only has two callsites so it's not much hassle to update.

[asb]

What did you think about keeping absoluteSymbolDiff as static and updating the callsites?

[edward-jones]

In D45773#1109391, @asb wrote:

What did you think about keeping absoluteSymbolDiff as static and updating the callsites?

Agreed, keeping it static is a better idea. I'll make the change and update this patch.

[edward-jones]

Updated to incorporate Alex's suggestion - keeping absoluteSymbolDiff static

[asb]

Thanks Ed, looks good to me.

llvm/trunk/include/llvm/MC/MCObjectStreamer.h

public:
   void EmitAddrsigSym(const MCSymbol *Sym) override;
 
   void FinishImpl() override;
 
   /// Emit the absolute difference between two symbols if possible.
   ///
   /// Emit the absolute difference between \c Hi and \c Lo, as long as we can
   /// compute it.  Currently, that requires that both symbols are in the same
-  /// data fragment.  Otherwise, do nothing and return \c false.
+  /// data fragment and that the target has not specified that diff expressions
+  /// require relocations to be emitted. Otherwise, do nothing and return
+  /// \c false.
   ///
   /// \pre Offset of \c Hi is greater than the offset \c Lo.
   void emitAbsoluteSymbolDiff(const MCSymbol *Hi, const MCSymbol *Lo,
                               unsigned Size) override;
 
   void emitAbsoluteSymbolDiffAsULEB128(const MCSymbol *Hi,
                                        const MCSymbol *Lo) override;
 
   bool mayHaveInstructions(MCSection &Sec) const override;
 };
 
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/MC/MCObjectStreamer.cpp

for (MCSymbol *Sym : PendingLabels) {
     Sym->setFragment(F);
     Sym->setOffset(FOffset);
   }
   PendingLabels.clear();
 }
 
 // As a compile-time optimization, avoid allocating and evaluating an MCExpr
 // tree for (Hi - Lo) when Hi and Lo are offsets into the same fragment.
-static Optional<uint64_t> absoluteSymbolDiff(const MCSymbol *Hi,
-                                             const MCSymbol *Lo) {
+static Optional<uint64_t>
+absoluteSymbolDiff(MCAssembler &Asm, const MCSymbol *Hi, const MCSymbol *Lo) {
   assert(Hi && Lo);
+  if (Asm.getBackendPtr()->requiresDiffExpressionRelocations())
+    return None;
+
   if (!Hi->getFragment() || Hi->getFragment() != Lo->getFragment() ||
       Hi->isVariable() || Lo->isVariable())
     return None;
 
   return Hi->getOffset() - Lo->getOffset();
 }
 
 void MCObjectStreamer::emitAbsoluteSymbolDiff(const MCSymbol *Hi,
                                               const MCSymbol *Lo,
                                               unsigned Size) {
-  if (Optional<uint64_t> Diff = absoluteSymbolDiff(Hi, Lo)) {
+  if (Optional<uint64_t> Diff = absoluteSymbolDiff(getAssembler(), Hi, Lo)) {
     EmitIntValue(*Diff, Size);
     return;
   }
   MCStreamer::emitAbsoluteSymbolDiff(Hi, Lo, Size);
 }
 
 void MCObjectStreamer::emitAbsoluteSymbolDiffAsULEB128(const MCSymbol *Hi,
                                                        const MCSymbol *Lo) {
-  if (Optional<uint64_t> Diff = absoluteSymbolDiff(Hi, Lo)) {
+  if (Optional<uint64_t> Diff = absoluteSymbolDiff(getAssembler(), Hi, Lo)) {
     EmitULEB128IntValue(*Diff);
     return;
   }
   MCStreamer::emitAbsoluteSymbolDiffAsULEB128(Hi, Lo);
 }
 
 void MCObjectStreamer::reset() {
   if (Assembler)

llvm/trunk/test/CodeGen/RISCV/fixups-diff.ll

+; RUN: llc -filetype=obj -mtriple=riscv32 -mattr=+relax %s -o - \
+; RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX %s
+; RUN: llc -filetype=obj -mtriple=riscv32 -mattr=-relax %s -o - \
+; RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX %s
+
+; Check that a difference between two symbols in the same fragment
+; causes relocations to be emitted if and only if relaxation is enabled.
+;
+; This specific test is checking that the size of the function in
+; the debug information is represented by a relocation. This isn't
+; an assembly test as the assembler takes a different path through
+; LLVM, which is already covered by the fixups-expr.s test.
+
+source_filename = "tmp.c"
+target datalayout = "e-m:e-p:32:32-i64:64-n32-S128"
+target triple = "riscv32"
+
+define i32 @main() !dbg !7 {
+entry:
+  %retval = alloca i32, align 4
+  store i32 0, i32* %retval, align 4
+  ret i32 0
+}
+
+; RELAX: 0x22 R_RISCV_ADD32 - 0x0
+; RELAX: 0x22 R_RISCV_SUB32 - 0x0
+; RELAX: 0x2B R_RISCV_ADD32 - 0x0
+; RELAX: 0x2B R_RISCV_SUB32 - 0x0
+; NORELAX-NOT: R_RISCV_ADD32
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "fixups-diff.ll", directory: "test/MC/RISCV")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: false, unit: !0)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocation(line: 2, column: 3, scope: !7)