[ARM] RELA relocations for 32bit ARM ignore the addend.

[wolfgangp]

Summary

Fixes PR 50473.

The test case uses llvm-dwarfdump to verify correct resolution.

[MaskRay]

llvm/lib/Object/RelocationResolver.cpp

314

The code is written in the assumption that targets preferring RELA always have Addend while targets preferring REL always have LocData.

Are you trying to add RELA support to ARM? I think it is unpopular but maybe you have specific needs which are not specified.

No objection, but a comment is needed.

llvm/test/DebugInfo/ARM/dwarfdump-rela.yaml

35

rela

[MaskRay]

llvm/lib/Object/RelocationResolver.cpp

314

I think adding LocData and Addend is wrong. When RELA is used, LocData (implicit addend) should be ignored.

[peter.smith]

I've not seen SHT_RELA used for debug data before and I think all of the other LLVM tools assume SHT_REL. Are you planning to add SHT_RELA support to the other LLVM tools?

llvm/lib/Object/RelocationResolver.cpp

314

Agreed. The ELF spec says in http://www.sco.com/developers/gabi/latest/ch4.reloc.html
"As specified previously, only Elf32_Rela and Elf64_Rela entries contain an explicit addend. Entries of type Elf32_Rel and Elf64_Rel store an implicit addend in the location to be modified. Depending on the processor architecture, one form or the other might be necessary or more convenient. Consequently, an implementation for a particular machine may use one form exclusively or either form depending on context."

"The typical application of an ELF relocation is to determine the referenced symbol value, extract the addend (either from the field to be relocated or from the addend field contained in the relocation record, as appropriate for the type of relocation record), apply the expression implied by the relocation type to the symbol and addend, extract the desired part of the expression result, and place it in the field to be relocated."

[wolfgangp]

In D105214#2852339, @peter.smith wrote:

I've not seen SHT_RELA used for debug data before and I think all of the other LLVM tools assume SHT_REL. Are you planning to add SHT_RELA support to the other LLVM tools?

We have at least one internal test case generated by a proprietary ARM compiler that is using RELA relocations, which is handled just fine by dwarfdump (David Anderson's dwarfdump, that is), so there is some incentive for us to handle it. Also, the code I'm planning to change is used by all llvm tools, so all tools should benefit. From what I'm seeing there is SHT_RELA awareness in the other tools as well.

llvm/lib/Object/RelocationResolver.cpp

314

Ah, thanks for pointing that out. It is curious, though, because some RISCV relocations clearly seem to use both addend and location data (see here).
Accordingly, when I zero out LocData for RELA relocations, some RISCV test cases start failing. I may have to restrict zeroing out LocData to ARM platforms, which I don't particularly like.

[MaskRay]

llvm/lib/Object/RelocationResolver.cpp

314

The special RISC-V relocations are R_RISCV_SUB*. They do label differences and the explicit addend is always zero. It can be implemented by "consecutive relocations" but processing the relocation consecutively can also be correct. If anything, it should not be used as examples for other targets because it imposes many restrictions that other targets may not have.

[wolfgangp]

Ignore LocData for RELA relocations except for RISCV, which uses both LocData and Addend.

Updated the test case by adding non-zero data to the location to verify it is ignored.

[wolfgangp]

Ping. Any comments on this more RISCV-specific approach?

llvm/test/DebugInfo/ARM/dwarfdump-rela.yaml

35

It it's OK with you, I fancy leaving the name as 'rel', just to make sure we don't key off the name of the section instead of the section type.

[MaskRay]

llvm/lib/Object/RelocationResolver.cpp

318

Add an assert

llvm/test/DebugInfo/ARM/dwarfdump-rela.yaml

25

Delete AddressAlign: 0x1

34

Delete EntSize: 0x0

35

I think it is not the test's purpose to test this, so I'd avoid it.

[MaskRay]

I am fine with this patch, but want to hear from @peter.smith.

I added @Higuoxing who may comment on the yaml2obj test.

[peter.smith]

With MaskRay's proposed addition of an assert I am happy with it.

[wolfgangp]

Addressed the review comments:

• added an assert that verifies that either the addend or LocData is 0
• renamed the relocation section into .rela.xxx
• deleted some unnecessary section properties

llvm/lib/Object/RelocationResolver.cpp

static bool supportsARM(uint64_t Type) {
   case ELF::R_ARM_ABS32:
   case ELF::R_ARM_REL32:
     return true;
   default:
     return false;
   }
 }
 
 static uint64_t resolveARM(uint64_t Type, uint64_t Offset, uint64_t S,
-                           uint64_t LocData, int64_t /*Addend*/) {
+                           uint64_t LocData, int64_t Addend) {
+  // Support both RELA and REL relocations. The caller is responsible
+  // for supplying the correct values for LocData and Addend, i.e.
+  // Addend == 0 for REL and LocData == 0 for RELA.
+  assert((LocData == 0 || Addend == 0) &&
+         "one of LocData and Addend must be 0");
   switch (Type) {
   case ELF::R_ARM_ABS32:
-    return (S + LocData) & 0xFFFFFFFF;
+    return (S + LocData + Addend) & 0xFFFFFFFF;
   case ELF::R_ARM_REL32:
-    return (S + LocData - Offset) & 0xFFFFFFFF;
+    return (S + LocData + Addend - Offset) & 0xFFFFFFFF;
   }
   llvm_unreachable("Invalid relocation type");
 }
 
 static bool supportsAVR(uint64_t Type) {
   switch (Type) {
   case ELF::R_AVR_16:
   case ELF::R_AVR_32:
if (Obj->isELF()) {
         if (auto *Elf64LEObj = dyn_cast<ELF64LEObjectFile>(Obj))
           return Elf64LEObj->getRelSection(R.getRawDataRefImpl())->sh_type;
         if (auto *Elf32BEObj = dyn_cast<ELF32BEObjectFile>(Obj))
           return Elf32BEObj->getRelSection(R.getRawDataRefImpl())->sh_type;
         auto *Elf64BEObj = cast<ELF64BEObjectFile>(Obj);
         return Elf64BEObj->getRelSection(R.getRawDataRefImpl())->sh_type;
       };
 
-      if (GetRelSectionType() == ELF::SHT_RELA)
+      if (GetRelSectionType() == ELF::SHT_RELA) {
         Addend = getELFAddend(R);
+        // RISCV relocations use both LocData and Addend.
+        if (Obj->getArch() != Triple::riscv32 &&
+            Obj->getArch() != Triple::riscv64)
+          LocData = 0;
+      }
     }
 
     return Resolver(R.getType(), R.getOffset(), S, LocData, Addend);
   }
 
   // Sometimes the caller might want to use its own specific implementation of
   // the resolver function. E.g. this is used by LLD when it resolves debug
   // relocations and assumes that all of them have the same computation (S + A).

llvm/test/DebugInfo/ARM/dwarfdump-rela.yaml

+# Test if we are handling RELA relocations for ARM correctly using llvm-dwarfdump.
+#
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-dwarfdump -i %t | FileCheck %s
+
+# CHECK:      DW_TAG_compile_unit
+# CHECK-NEXT: DW_AT_name {{.*}}("correct")
+# CHECK-NEXT: DW_AT_type ([[TYPEDIE:0x[0-9A-Fa-f]+]] "correct")
+# CHECK:      [[TYPEDIE]]: DW_TAG_base_type
+# CHECK-NEXT: DW_AT_name {{.*}}("correct")
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_ARM
+  Flags:           [ EF_ARM_EABI_VER5 ]
+Sections:
+  # A rudimentary v5 compile unit with an AT_name and an AT_type referencing
+  # a rudimentary DW_TAG_base_type DIE with an AT_name attribute.
+  - Name:            .debug_info
+    Type:            SHT_PROGBITS
+    Content:         17000000050001040000000001A000000000000000020000000000
+  - Name:            .debug_abbrev
+    Type:            SHT_PROGBITS
+    Content:         011101030E49130000022400030E0000
+  - Name:            .debug_str
+    Type:            SHT_PROGBITS
+  - Name:            .rela.debug_info
+    Type:            SHT_RELA
+    Link:            .symtab
+    AddressAlign:    0x4
+    Offset:          0x929
+    Info:            .debug_info
+    Relocations:
+      - Offset:          0x8
+        Symbol:          .debug_abbrev
+        Type:            R_ARM_ABS32
+      # The compile unit name is found via a R_ARM_ABS32 relocation.
+      - Offset:          0xD
+        Symbol:          .debug_str
+        Type:            R_ARM_ABS32
+        Addend:          6
+      # The DW_TAG_base_type is found via a R_ARM_REL32 relocation.
+      # This is completely artificial and unlikely to be ever generated
+      # by a compiler or other tool. We make sure that the relocation is
+      # resolved by (Symbol - Offset + Addend).
+      - Offset:          0x11
+        Symbol:          .debug_info
+        Type:            R_ARM_REL32
+        Addend:          0x26
+      - Offset:          0x16
+        Symbol:          .debug_str
+        Type:            R_ARM_ABS32
+        Addend:          6
+  - Type:            SectionHeaderTable
+    Sections:
+      - Name:            .symtab
+      - Name:            .strtab
+      - Name:            .shstrtab
+      - Name:            .debug_info
+      - Name:            .debug_abbrev
+      - Name:            .debug_str
+      - Name:            .rela.debug_info
+Symbols:
+  - Name:            test.cpp
+    Type:            STT_FILE
+    Index:           SHN_ABS
+  - Name:            .debug_info
+    Type:            STT_SECTION
+    Section:         .debug_info
+  - Name:            .debug_abbrev
+    Type:            STT_SECTION
+    Section:         .debug_abbrev
+  - Name:            .debug_str
+    Type:            STT_SECTION
+    Section:         .debug_str
+  - Name:            .debug_info_cudie
+    Type:            STT_OBJECT
+    Section:         .debug_info
+    Value:           0xC
+DWARF:
+  debug_str:
+    - 'wrong'
+    - 'correct'
+...