[PATCH] D63076: [ELF][RISCV] Support PLT, GOT, copy and relative relocations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> Still disagree about the behaviour of R_RISCV_CALL. You can't just choose the ABI that you want. Please at least put it back to R_PC. @ruiu do you have an opinion on this?

@jrtc27 `R_PC` and `R_PLT_PC` do not behave in the way you thought. In fact, when used as a PLT relocation, `R_PC` and `R_PLT_PC` work almost the same way. The difference is: `R_PLT_PC` is considered a PLT-generating relocation but `R_PC`.

- If the target symbol is non-preemptable, an `R_PLT_PC` gets optimized to an `R_PC`, and the `R_PC` is considered a link-time constant.
- Otherwise, `R_PLT_PC` is still a link-time constant. **However, `R_PC` will trigger the creation of a canonical PLT** and behave as if an `R_PLT_PC` was used:

  // Relocations.cpp:1026
    if (Sym.isFunc()) {
      if (Config->Pie && Config->EMachine == EM_386)
        errorOrWarn("symbol '" + toString(Sym) +
                    "' cannot be preempted; recompile with -fPIE" +
                    getLocation(Sec, Sym, Offset));
      if (!Sym.isInPlt())
        addPltEntry<ELFT>(In.Plt, In.GotPlt, In.RelaPlt, Target->PltRel, Sym);
      if (!Sym.isDefined())
        replaceWithDefined(
            Sym, In.Plt,
            Target->PltHeaderSize + Target->PltEntrySize * Sym.PltIndex, 0);
      Sym.NeedsPltAddr = true;  /// marks the creation of a canonical PLT
      Sec.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
      return;
    }

@ruiu There is visible difference, but the difference is related to **whether a canonical PLT is created**, not as @jrtc27 thought that it changed the bindings. I reported the ld.bfd bug at https://sourceware.org/bugzilla/show_bug.cgi?id=24685

  % riscv-ld -m elf64lriscv a.o b.so -o bfd; riscv-objdump -d bfd
  
  0000000000010250 <_start>:
     10250:       00000097                auipc   ra,0x0
     10254:       020080e7                jalr    32(ra) # 10270 <foo>
     10258:       00000097                auipc   ra,0x0
     1025c:       fe8080e7                jalr    -24(ra) # 10240 <bar at plt>
     10260:       00000097                auipc   ra,0x0
     10264:       fe0080e7                jalr    -32(ra) # 10240 <bar at plt>
     10268:       00000097                auipc   ra,0x0
     1026c:       fc8080e7                jalr    -56(ra) # 10230 <weak at plt>
  % readelf --dyn-s bfd
  ...
       1: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND weak
       2: 0000000000010240     0 FUNC    GLOBAL DEFAULT  UND bar  # this is a canonical PLT, it is however not needed. See 
  ...

With this patch, the 4 calls bind the symbols the same as ld.bfd:

  % myld.lld a.o b.so -o a; riscv-objdump -d a
  0000000000011000 <_start>:                                                                                     
     11000:       00000097                auipc   ra,0x0                  
     11004:       020080e7                jalr    32(ra) # 11020 <foo>                
     11008:       00000097                auipc   ra,0x0                   
     1100c:       048080e7                jalr    72(ra) # 11050 <bar at plt>
     11010:       00000097                auipc   ra,0x0   
     11014:       040080e7                jalr    64(ra) # 11050 <bar at plt>                                       
     11018:       00000097                auipc   ra,0x0
     1101c:       048080e7                jalr    72(ra) # 11060 <weak at plt>

The call sites are: `<foo>, <bar at plt>, <bar at plt>, <weak at plt>`, the same as ld.bfd. **This is the behavior that matters.**.

The difference is that we don't create the unnecessary canonical PLT:

  % readelf --dyn-s a
       1: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND bar
       2: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND weak

@jrtc27 You might consider ld.bfd's behavior as a bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24683 However, I think that is exactly what it should behave (and that is why I think the distinction of `R_RISCV_PLT` doesn't make sense).


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63076/new/

https://reviews.llvm.org/D63076