[LLD][ELF] make default for get{ARM,AArch64}UndefinedRelativeWeakVA unreachable

[peter.smith]

Summary

The get{ARM,AArch64}UndefinedRelativeWeakVA() functions should only be called for PC-relative relocations. Complete the supported pc-relative relocations in the switch statement and make the default case unreachable.

The R_ARM_TARGET relocation can be evaluated as R_ARM_REL32 but it is only used in the context of exception tables, and is never output with respect to a weak reference so it does not appear in the switch statement.

[ruiu]

ELF/InputSection.cpp

405–406 ↗

(On Diff #102304)

What does this comment explain?

437–438 ↗

(On Diff #102304)

Ditto

444 ↗

(On Diff #102304)

to the place ...?

[peter.smith]

I'll update the comments and move the llvm_unreachable.

ELF/InputSection.cpp

405–406 ↗

(On Diff #102304)

In the ARM ABI there is a general rule for handling pc-relative relocations. There is a further special case for the subset of pc-relative instructions that are also branch relocations. I wanted to explain why the branch relocations resolved to the address of the next instruction, but the other relocations did not.

I'll update the comments to make this more explicit.

444 ↗

(On Diff #102304)

I'm using the long form for P in the PC-relative relocation calculation (S + A) - P. The place is commonly used in ELF processor supplements [*], but I'll make it clearer.

For example in the AMD64 ABI:

P Represents the place (section offset or address) of the storage unit being relocated (computed using r_offset).

[peter.smith]

Updated diff with comment changes. Let me know if you want any changes, otherwise I'll commit tomorrow?

[ruiu]

ELF/InputSection.cpp

405–406 ↗

(On Diff #102304)

Thank you for adding the explanation.

So, does this comment explain return P + 2 + A? I'd move this after case instead of writing it before case.

425 ↗

(On Diff #102502)

Move this after the last case too.

[peter.smith]

Updated diff to move comments and make it clearer that the +2 is for a 16-bit thumb instruction and +4 for ARM.

[peter.smith]

I've committed as r305673 as the last changes were only comment related. I'm happy to alter further if needed.

lld/trunk/ELF/InputSection.cpp

if (Body.Type == STT_SECTION) {
             {R_ABS, Type, Rel.r_offset, Target->getImplicitAddend(BufLoc, Type),
              &Body});
       }
     }
 
   }
 }
 
+// The ARM and AArch64 ABI handle pc-relative relocations to undefined weak
+// references specially. The general rule is that the value of the symbol in
+// this context is the address of the place P. A further special case is that
+// branch relocations to an undefined weak reference resolve to the next
+// instruction.
 static uint32_t getARMUndefinedRelativeWeakVA(uint32_t Type, uint32_t A,
                                               uint32_t P) {
   switch (Type) {
+  // Unresolved branch relocations to weak references resolve to next
+  // instruction, this will be either 2 or 4 bytes on from P.
   case R_ARM_THM_JUMP11:
     return P + 2 + A;
   case R_ARM_CALL:
   case R_ARM_JUMP24:
   case R_ARM_PC24:
   case R_ARM_PLT32:
   case R_ARM_PREL31:
   case R_ARM_THM_JUMP19:
   case R_ARM_THM_JUMP24:
     return P + 4 + A;
   case R_ARM_THM_CALL:
     // We don't want an interworking BLX to ARM
     return P + 5 + A;
-  default:
+  // Unresolved non branch pc-relative relocations
+  // R_ARM_TARGET2 which can be resolved relatively is not present as it never
+  // targets a weak-reference.
+  case R_ARM_MOVW_PREL_NC:
+  case R_ARM_MOVT_PREL:
+  case R_ARM_REL32:
+  case R_ARM_THM_MOVW_PREL_NC:
+  case R_ARM_THM_MOVT_PREL:
     return P + A;
   }
+  llvm_unreachable("ARM pc-relative relocation expected\n");
 }
 
+// The comment above getARMUndefinedRelativeWeakVA applies to this function.
 static uint64_t getAArch64UndefinedRelativeWeakVA(uint64_t Type, uint64_t A,
                                                   uint64_t P) {
   switch (Type) {
+  // Unresolved branch relocations to weak references resolve to next
+  // instruction, this is 4 bytes on from P.
   case R_AARCH64_CALL26:
   case R_AARCH64_CONDBR19:
   case R_AARCH64_JUMP26:
   case R_AARCH64_TSTBR14:
     return P + 4 + A;
-  default:
+  // Unresolved non branch pc-relative relocations
+  case R_AARCH64_PREL16:
+  case R_AARCH64_PREL32:
+  case R_AARCH64_PREL64:
+  case R_AARCH64_ADR_PREL_LO21:
     return P + A;
   }
+  llvm_unreachable("AArch64 pc-relative relocation expected\n");
 }
 
 // ARM SBREL relocations are of the form S + A - B where B is the static base
 // The ARM ABI defines base to be "addressing origin of the output segment
 // defining the symbol S". We defined the "addressing origin"/static base to be
 // the base of the PT_LOAD segment containing the Body.
 // The procedure call standard only defines a Read Write Position Independent
 // RWPI variant so in practice we should expect the static base to be the base