[ELF][Hexagon] Replace R_HEXAGON_GOT with R_GOTPLT

[MaskRay]

Summary

R_GOTPLT is relative to .got.plt since D59594. If R_HEXAGON_GOT
relocations always have 0 r_addend, they can use R_GOTPLT instead.

[sidneym]

ELF/InputSection.cpp

676 ↗

(On Diff #215318)

Removing this will produce unexpected output. GOT is the address in the GOT (G). GOTREL is the difference between the location and table (S+A - GOT)

[sidneym]

ELF/InputSection.cpp

676 ↗

(On Diff #215318)

Ah, looking at the code more carefully I think this ok. I was reading this as GOTPLTREL not GOTPLT. Let me try to look back and see why this wasn't done in the first place.

[sidneym]

ELF/InputSection.cpp

676 ↗

(On Diff #215318)

R_HEXAGON_GOT was added before R_GOTPLT. I think it's ok to use R_GOTPLT.

[ruiu]

Nice! Thank you for doing this.

[MaskRay]

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

[ruiu]

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

[arichardson]

In D66274#1634810, @ruiu wrote:

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

I had to do this for the CHERI fork of LLD since we added a few new entries. We currently always use a 128-bit mask (https://github.com/CTSRD-CHERI/llvm-project/blob/master/lld/ELF/Relocations.cpp#L139) but I guess we could reorder the enum to have a fast path for commonly checked values and do the second compare only for less common entries. I can submit a patch if you would like to use the 128-bit variant.

[ruiu]

In D66274#1634895, @arichardson wrote:

In D66274#1634810, @ruiu wrote:

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

I had to do this for the CHERI fork of LLD since we added a few new entries. We currently always use a 128-bit mask (https://github.com/CTSRD-CHERI/llvm-project/blob/master/lld/ELF/Relocations.cpp#L139) but I guess we could reorder the enum to have a fast path for commonly checked values and do the second compare only for less common entries. I can submit a patch if you would like to use the 128-bit variant.

Nice. We probably need your code in a near future. I'm not too worried to always use a 128-bit mask -- I don't think that's a large value, and we can optimize it after it raises in a profiling.

[sidneym]

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

At the moment no. I hope to add TLS relocations shortly and I don't think they will require new target-specific enums.

lld/trunk/ELF/Arch/Hexagon.cpp

RelExpr Hexagon::getRelExpr(RelType type, const Symbol &s,
   case R_HEX_B22_PCREL:
   case R_HEX_PLT_B22_PCREL:
   case R_HEX_B22_PCREL_X:
   case R_HEX_B32_PCREL_X:
     return R_PLT_PC;
   case R_HEX_GOT_11_X:
   case R_HEX_GOT_16_X:
   case R_HEX_GOT_32_6_X:
-    return R_HEXAGON_GOT;
+    return R_GOTPLT;
   default:
     return R_ABS;
   }
 }
 
 static uint32_t findMaskR6(uint32_t insn) {
   // There are (arguably too) many relocation masks for the DSP's
   // R_HEX_6_X type.  The table below is used to select the correct mask

lld/trunk/ELF/InputSection.cpp

static uint64_t getRelocTargetVA(const InputFile *file, RelType type, int64_t a,
   case R_RELAX_TLS_GD_TO_IE_GOT_OFF:
     return sym.getGotOffset() + a;
   case R_AARCH64_GOT_PAGE_PC:
   case R_AARCH64_RELAX_TLS_GD_TO_IE_PAGE_PC:
     return getAArch64Page(sym.getGotVA() + a) - getAArch64Page(p);
   case R_GOT_PC:
   case R_RELAX_TLS_GD_TO_IE:
     return sym.getGotVA() + a - p;
-  case R_HEXAGON_GOT:
-    return sym.getGotVA() - in.gotPlt->getVA();
   case R_MIPS_GOTREL:
     return sym.getVA(a) - in.mipsGot->getGp(file);
   case R_MIPS_GOT_GP:
     return in.mipsGot->getGp(file) + a;
   case R_MIPS_GOT_GP_PC: {
     // R_MIPS_LO16 expression has R_MIPS_GOT_GP_PC type iif the target
     // is _gp_disp symbol. In that case we should use the following
     // formula for calculation "AHL + GP - P + 4". For details see p. 4-19 at

lld/trunk/ELF/Relocations.h

enum RelExpr {
   // Even though RelExpr is intended to be a target-neutral representation
   // of a relocation type, there are some relocations whose semantics are
   // unique to a target. Such relocation are marked with R_<TARGET_NAME>.
   R_AARCH64_GOT_PAGE_PC,
   R_AARCH64_PAGE_PC,
   R_AARCH64_RELAX_TLS_GD_TO_IE_PAGE_PC,
   R_AARCH64_TLSDESC_PAGE,
   R_ARM_SBREL,
-  R_HEXAGON_GOT,
   R_MIPS_GOTREL,
   R_MIPS_GOT_GP,
   R_MIPS_GOT_GP_PC,
   R_MIPS_GOT_LOCAL_PAGE,
   R_MIPS_GOT_OFF,
   R_MIPS_GOT_OFF32,
   R_MIPS_TLSGD,
   R_MIPS_TLSLD,

lld/trunk/ELF/Relocations.cpp

 static bool needsPlt(RelExpr expr) {
   return oneof<R_PLT_PC, R_PPC32_PLTREL, R_PPC64_CALL_PLT, R_PLT>(expr);
 }
 
 // Returns true if Expr refers a GOT entry. Note that this function
 // returns false for TLS variables even though they need GOT, because
 // TLS variables uses GOT differently than the regular variables.
 static bool needsGot(RelExpr expr) {
-  return oneof<R_GOT, R_GOT_OFF, R_HEXAGON_GOT, R_MIPS_GOT_LOCAL_PAGE,
-               R_MIPS_GOT_OFF, R_MIPS_GOT_OFF32, R_AARCH64_GOT_PAGE_PC,
-               R_GOT_PC, R_GOTPLT>(expr);
+  return oneof<R_GOT, R_GOT_OFF, R_MIPS_GOT_LOCAL_PAGE, R_MIPS_GOT_OFF,
+               R_MIPS_GOT_OFF32, R_AARCH64_GOT_PAGE_PC, R_GOT_PC, R_GOTPLT>(
+      expr);
 }
 
 // True if this expression is of the form Sym - X, where X is a position in the
 // file (PC, or GOT for example).
 static bool isRelExpr(RelExpr expr) {
   return oneof<R_PC, R_GOTREL, R_GOTPLTREL, R_MIPS_GOTREL, R_PPC64_CALL,
                R_PPC64_RELAX_TOC, R_AARCH64_PAGE_PC, R_RELAX_GOT_PC,
                R_RISCV_PC_INDIRECT>(expr);
 }
 
 // Returns true if a given relocation can be computed at link-time.
 //
 // For instance, we know the offset from a relocation to its target at
 // link-time if the relocation is PC-relative and refers a
 // non-interposable function in the same executable. This function
 // will return true for such relocation.
 //
 // If this function returns false, that means we need to emit a
 // dynamic relocation so that the relocation will be fixed at load-time.
 static bool isStaticLinkTimeConstant(RelExpr e, RelType type, const Symbol &sym,
                                      InputSectionBase &s, uint64_t relOff) {
   // These expressions always compute a constant
-  if (oneof<R_DTPREL, R_GOTPLT, R_GOT_OFF, R_HEXAGON_GOT, R_TLSLD_GOT_OFF,
+  if (oneof<R_DTPREL, R_GOTPLT, R_GOT_OFF, R_TLSLD_GOT_OFF,
             R_MIPS_GOT_LOCAL_PAGE, R_MIPS_GOTREL, R_MIPS_GOT_OFF,
             R_MIPS_GOT_OFF32, R_MIPS_GOT_GP_PC, R_MIPS_TLSGD,
             R_AARCH64_GOT_PAGE_PC, R_GOT_PC, R_GOTONLY_PC, R_GOTPLTONLY_PC,
             R_PLT_PC, R_TLSGD_GOT, R_TLSGD_GOTPLT, R_TLSGD_PC, R_PPC32_PLTREL,
             R_PPC64_CALL_PLT, R_PPC64_RELAX_TOC, R_RISCV_ADD, R_TLSDESC_CALL,
             R_TLSDESC_PC, R_AARCH64_TLSDESC_PAGE, R_HINT, R_TLSLD_HINT,
             R_TLSIE_HINT>(e))
     return true;