[lld]: Fix RISC-V relaxation bug

[MaskRay]

Summary

When multiple symbol names alias to the same symbol (which is the case
when we use --wrap), the RISC-V relaxation steps are executed multiple
times for the same symbol, causing erroneous addresses being assigned to
symbols. This commit fixes this by only operating on unique symbols
(addressed by the symbol pointers).

Signed-off-by: Kishore Ganesh <[e-mail]>

[reames]

lld/ELF/Arch/RISCV.cpp

506

It looks like you're only using existance in the map, so this can be a DenseSet<Symbol *>

532

It looks like symMap is only used in the scope of this loop, so you can declare it locally here.

539

I suspect we should really be checking for some property of the symbol here, but am not sufficiently knowledgeable of ELF minutia to know which one. Maybe @MaskRay or @jrtc27 might have a suggestion here?

[MaskRay]

When multiple symbol names alias to the same symbol (which is the case when we use --wrap),

This is the uncommon in wrap.s, where foo and __wrap_foo are defined in one file. This is not how --wrap is used in the wild, but if we identify a bug, we should fix it.

the RISC-V relaxation steps are executed multiple times for the same symbol, causing erroneous addresses being assigned to symbols.

A relocatable object file (%t2) indeed has multiple instances of the same symbol. Can you describe how this leads to incorrect addresses?

For riscv-wrap.s, the %t3 is identical without or with this patch.

lld/test/ELF/Inputs/riscv-wrap.s

3

Omit __real_foo. Having a .globl for an undefined symbol is a no-op, but not conventional.

Avoid .protected if STV_PROTECTED doesn't lead to behavior differences.

Add a comment before .align 5.

nop below is not indented.

lld/test/ELF/riscv-wrap.s

2

The test case copied from wrap.s needs some polishing. You may find some recent tests that use a IMHO cleaner style:

• .o for relocatable object files. %t.o and %t to make the primary relocatable object file name match the linker output file name.
• prefer split-file when it makes the test more readable.

[jobnoorman]

This seems to have been (accidentally) fixed by D149735: symbol values are not calculated anymore by repeated subtraction (which caused this bug) but always overwritten with their new value (which fixes this).

@kishore-rivos: this patch is based on an old LLVM version where D149735 wasn't landed yet. Could you try updating your local copy?

It could still make sense to filter-out symbol aliases for performance reasons I suppose.

[asb]

It would likely be worth separating out the tests from this patch to ensure the bug isn't reintroduced in any future refactorings.

[reames]

In D150220#4345935, @asb wrote:

It would likely be worth separating out the tests from this patch to ensure the bug isn't reintroduced in any future refactorings.

Agreed. @jobnoorman could I ask you to reduce a test here? @kishore-rivos is an intern who has finished his internship. I can take a shot at it, but I'm not really a linker person and I think you have a much better chance of getting a clean test on first attempt.

[jobnoorman]

In D150220#4354466, @reames wrote:

In D150220#4345935, @asb wrote:

It would likely be worth separating out the tests from this patch to ensure the bug isn't reintroduced in any future refactorings.

Agreed. @jobnoorman could I ask you to reduce a test here? @kishore-rivos is an intern who has finished his internship. I can take a shot at it, but I'm not really a linker person and I think you have a much better chance of getting a clean test on first attempt.

Sure! D150940

[MaskRay]

Thanks for the report. Obsoleted by D151768

lld/ELF/Arch/RISCV.cpp

struct SymbolAnchor {
   bool end; // true for the anchor of st_value+st_size
 };
 } // namespace
 
 struct elf::RISCVRelaxAux {
   // This records symbol start and end offsets which will be adjusted according
   // to the nearest relocDeltas element.
   SmallVector<SymbolAnchor, 0> anchors;
+  llvm::DenseMap<Symbol *, int> symMap;
   // For relocations[i], the actual offset is r_offset - (i ? relocDeltas[i-1] :
   // 0).
   std::unique_ptr<uint32_t[]> relocDeltas;
   // For relocations[i], the actual type is relocTypes[i].
   std::unique_ptr<RelType[]> relocTypes;
   SmallVector<uint32_t, 0> writes;
 };
 
for (InputSection *sec : getInputSections(*osec, storage)) {
             std::make_unique<uint32_t[]>(sec->relocs().size());
         sec->relaxAux->relocTypes =
             std::make_unique<RelType[]>(sec->relocs().size());
       }
     }
   }
   // Store anchors (st_value and st_value+st_size) for symbols relative to text
   // sections.
   for (InputFile *file : ctx.objectFiles)
     for (Symbol *sym : file->getSymbols()) {
       auto *d = dyn_cast<Defined>(sym);
       if (!d || d->file != file)
         continue;
       if (auto *sec = dyn_cast_or_null<InputSection>(d->section))
         if (sec->flags & SHF_EXECINSTR && sec->relaxAux) {
+          bool found =
+              sec->relaxAux->symMap.find(sym) != sec->relaxAux->symMap.end();
+          if (!found) {
             // If sec is discarded, relaxAux will be nullptr.
             sec->relaxAux->anchors.push_back({d->value, d, false});
             sec->relaxAux->anchors.push_back({d->value + d->size, d, true});
+            sec->relaxAux->symMap.insert({sym, 1});
+          }
         }
     }
   // Sort anchors by offset so that we can find the closest relocation
   // efficiently. For a zero size symbol, ensure that its start anchor precedes
   // its end anchor. For two symbols with anchors at the same offset, their
   // order does not matter.
   for (OutputSection *osec : outputSections) {
     if (!(osec->flags & SHF_EXECINSTR))

lld/test/ELF/Inputs/riscv-wrap.s

+.global foo 
+.weak __wrap_foo 
+.protected __wrap_foo
+.global __real_foo
+nop
+foo:
+    addi x1,x1,1
+.align 5
+__wrap_foo:
+    addi x1,x1,1
+    call __real_foo
 No newline at end of file

lld/test/ELF/riscv-wrap.s

+# REQUIRES: riscv 
+## Test RISCV's handling of --wrap when R_RISCV_ALIGN is present 
+# RUN: rm -rf %t && mkdir %t && cd %t 
+# RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=+relax %s -o %t1 
+# RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=+relax %p/Inputs/riscv-wrap.s  -o %t2 
+# RUN: ld.lld -Ttext=0x100000 -o %t3 %t1 %t2 --wrap foo
+# RUN: llvm-objdump -d --print-imm-hex %t3 | FileCheck %s 
+# CHECK: 0000000000100040 <__wrap_foo>:
+## CHECK-NEXT:   100040: 93 80 10 00   addi    ra, ra, 1  
+.global _start
+_start:
+    call foo
+    ret