[LLD][ELF][AArch64] Add missing LO12 relocation to usesOnlyLowPageBits

[peter.smith]

Summary

The R_AARCH64_TLSLE_ADD_TPREL_LO12_NC relocation also only uses the low page bits, but is missing from the usesOnlyLowPageBits function. At present the usesOnlyLowPageBits function is not called for this relocation but I think it is worth adding for the completeness of usesOnlyLowPageBits.

[grimar]

I would try to avoid dead code. So until it is possible to add a test case for this change, I would not do that probably.

[peter.smith]

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

[grimar]

In D46249#1086321, @peter.smith wrote:

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

Yeah, maybe. I guess it already contains entries it never uses, btw?

[peter.smith]

In D46249#1086324, @grimar wrote:

In D46249#1086321, @peter.smith wrote:

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

Yeah, maybe. I guess it already contains entries it never uses, btw?

I think the ones that are already there can be used as they are one of R_GOT, R_PLT, R_TLSDESC, or (!AbsVal && !RelE) can be true. The local exec relocations are always AbsVal = true and all have a RelExpr of R_TLS.

I'm not hugely fussed either way, I was just unsure of whether to add new LO12_NC relocations to that function or not.

[grimar]

In D46249#1086334, @peter.smith wrote:

In D46249#1086324, @grimar wrote:

In D46249#1086321, @peter.smith wrote:

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

Yeah, maybe. I guess it already contains entries it never uses, btw?

I think the ones that are already there can be used as they are one of R_GOT, R_PLT, R_TLSDESC, or (!AbsVal && !RelE) can be true. The local exec relocations are always AbsVal = true and all have a RelExpr of R_TLS.

I'm not hugely fussed either way, I was just unsure of whether to add new LO12_NC relocations to that function or not.

Ok. So, personally my opinion - I would not add any code that is unproven to be useful. This applies to this patch.

[peter.smith]

Ok I'll abandon this one and I'll update D46255 to remove the new TLSLE relocations I added to usesOnlyLowPageBits as the same argument applies.

ELF/Arch/AArch64.cpp

bool AArch64::usesOnlyLowPageBits(RelType Type) const {
   case R_AARCH64_LDST128_ABS_LO12_NC:
   case R_AARCH64_LDST16_ABS_LO12_NC:
   case R_AARCH64_LDST32_ABS_LO12_NC:
   case R_AARCH64_LDST64_ABS_LO12_NC:
   case R_AARCH64_LDST8_ABS_LO12_NC:
   case R_AARCH64_TLSDESC_ADD_LO12:
   case R_AARCH64_TLSDESC_LD64_LO12:
   case R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
+  case R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
     return true;
   }
 }
 
 RelType AArch64::getDynRel(RelType Type) const {
   if (Type == R_AARCH64_ABS32 || Type == R_AARCH64_ABS64)
     return Type;
   return R_AARCH64_NONE;