[ELF] - Don't emit broken relocations for SHF_MERGE sections when --emit-relocs is used.

[grimar]

Summary

Previously our relocations we rewrote were broken for that case.
We emited incorrect addend and broken relocation info field
because did not produce section symbol for mergeable synthetic sections.

Also helps to simplify D40026.

[ruiu]

LGTM

ELF/Writer.cpp

495

Synthetic sections don't have any relocations except mergeable sections. Mergeable output sections may have relocations because mergeable input sections may have relocations.

test/ELF/emit-relocs-mergeable-i386.s

6

Check if

test/ELF/emit-relocs-mergeable.s

6

Check if

[grimar]

ELF/Writer.cpp

495

Sorry, I think I do not understand your suggested comment meaning.

You're saying that "Synthetic sections don't have any relocations except mergeable sections".

My testcase have '.foo' section that have '.foo.rela' section. Latter has relocations that refer to section symbol for '.strings',
used for relocation calulation. '.strings' in my case is a synthetic section. And it does not have '.strings.rela' or something,
so it never have relocations in my understanding.

So synthetic mergeable section '.strings' does not have relocations. But your suggested comment saying different.
I am going to commit this comment as is with fix for "-r" -> "-emit-relocs". Lets discuss the better wording if you think it
is not good now and I'll be happy to change it in followup.

[ruiu]

So, synthetic sections have no relocations, right? But mergeable synthetic section is an exception. I don't see any problem with my comment.

[grimar]

In D40070#927204, @ruiu wrote:

So, synthetic sections have no relocations, right?

Right.

But mergeable synthetic section is an exception.

No. Mergeable synthetic sections also have no relocations.

We have synthetic section .debug_str, for example. And .debug_info that uses it.
.debug_info has relocations (has '.rela.debug_info' section). But .debug_str not.
.debug_str is used for relocation calculation for different section, which is .debug_info,
but it does not have relocations by itself.

[ruiu]

Then it contradicts with the other comment in the same function.

// Create one STT_SECTION symbol for each output section we might
// have a relocation with.

[grimar]

In D40070#927225, @ruiu wrote:

Then it contradicts with the other comment in the same function.

// Create one STT_SECTION symbol for each output section we might
// have a relocation with.

Sorry, but I see no issue in this comment, probably my english is not good enough to see mistake in wording here.
What I think it is saying is that we create section symbols so that relocations that uses section symbols can use it, at least that
is what actually we do I believe.

[ruiu]

Aah, I'd think the comment is actually ambiguous. I was thinking of a section that a relocation is applied to, but it can also be read as it is talking about a section that a relocation's symbol belongs to, and the latter interpretation is correct. Let's update that comment too. I think something like "Create a section symbol for each output section so that we can represent relocations that point to the section. If we know that no relocation is referring to a section (that happens if the section is a synthetic one), we don't create a section symbol for that section" should work.

[grimar]

In D40070#927255, @ruiu wrote:

Aah, I'd think the comment is actually ambiguous. I was thinking of a section that a relocation is applied to, but it can also be read as it is talking about a section that a relocation's symbol belongs to, and the latter interpretation is correct. Let's update that comment too. I think something like "Create a section symbol for each output section so that we can represent relocations that point to the section. If we know that no relocation is referring to a section (that happens if the section is a synthetic one), we don't create a section symbol for that section" should work.

Sounds good for me, thanks !

So second comment will remain the same right ? (or do you prefer something different)

// We do not create symbol for synthetic sections because do not have
// relocations that might use it, but SHF_MERGE sections is an exception
// used for --emit-relocs. After applying merging optimisation we want to
// rewrite relocations and need section symbol for synthetic mergeable
// sections to refer to.

[ruiu]

Yes, that comment is okay, but since we are updating the first comment, and the comment has already mentioned synthetic sections, we can simplify it. I'd write "Unlike other synthetic sections, mergeable output sections contain data copied from input sections, and there may be a relocation pointing to its contents if -r or -emit-reloc are given."

Given that, LGTM. Thanks!

ELF/InputSection.cpp

if (Sym.Type == STT_SECTION) {
       // hopefully creates a frame that is ignored at runtime.
       SectionBase *Section = cast<Defined>(Sym).Section;
       if (Section == &InputSection::Discarded) {
         P->setSymbolAndType(0, 0, false);
         continue;
       }
 
       if (Config->IsRela) {
-        P->r_addend += Sym.getVA() - Section->getOutputSection()->Addr;
+        P->r_addend =
+            Sym.getVA(getAddend<ELFT>(Rel)) - Section->getOutputSection()->Addr;
       } else if (Config->Relocatable) {
         const uint8_t *BufLoc = Sec->Data.begin() + Rel.r_offset;
         Sec->Relocations.push_back({R_ABS, Type, Rel.r_offset,
                                     Target->getImplicitAddend(BufLoc, Type),
                                     &Sym});
       }
     }
 

ELF/Writer.cpp

for (BaseCommand *Base : Script->SectionCommands) {
     auto I = llvm::find_if(Sec->SectionCommands, [](BaseCommand *Base) {
       if (auto *ISD = dyn_cast<InputSectionDescription>(Base))
         return !ISD->Sections.empty();
       return false;
     });
     if (I == Sec->SectionCommands.end())
       continue;
     InputSection *IS = cast<InputSectionDescription>(*I)->Sections[0];
-    if (isa<SyntheticSection>(IS) || IS->Type == SHT_REL ||
-        IS->Type == SHT_RELA)
+
+    // Relocations are not using REL[A] section symbols.
+    if (IS->Type == SHT_REL || IS->Type == SHT_RELA)
+      continue;
+
+    // We do not create symbol for synthetic sections because do not have
+    // relocations that might use it, but SHF_MERGE sections is an exception
+    // used for -r. After applying merging optimisation we want to rewrite
+    // relocations and need section symbol for synthetic mergeable sections to
+    // refer to.
+    if (isa<SyntheticSection>(IS) && !(IS->Flags & SHF_MERGE))
       continue;
 
     auto *Sym = make<Defined>("", /*IsLocal=*/true, /*StOther=*/0, STT_SECTION,
                               /*Value=*/0, /*Size=*/0, IS);
     InX::SymTab->addSymbol(Sym);
   }
 }
 

test/ELF/emit-relocs-mergeable-i386.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=i686-pc-linux %s -o %t1
+# RUN: ld.lld --emit-relocs %t1 -o %t2
+# RUN: llvm-readobj -sections -section-data %t2 | FileCheck %s
+
+## Check we produce proper relocations when doing merging of SHF_MERGE sections.
+
+## Check addends of relocations are: 0x0, 0x8, 0x8, 0x4
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .foo
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:     SHF_EXECINSTR
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Offset:
+# CHECK-NEXT:   Size:
+# CHECK-NEXT:   Link:
+# CHECK-NEXT:   Info:
+# CHECK-NEXT:   AddressAlignment:
+# CHECK-NEXT:   EntrySize:
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 00000000 08000000 08000000 04000000
+# CHECK-NEXT:   )
+# CHECK-NEXT: }
+
+## Check that offsets for AAA is 0x0, for BBB is 0x8 and CCC has offset 0x4.
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .strings
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_MERGE
+# CHECK-NEXT:     SHF_STRINGS
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Offset:
+# CHECK-NEXT:   Size:
+# CHECK-NEXT:   Link:
+# CHECK-NEXT:   Info:
+# CHECK-NEXT:   AddressAlignment:
+# CHECK-NEXT:   EntrySize:
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     |AAA.CCC.BBB.|
+# CHECK-NEXT:   )
+# CHECK-NEXT: } 
+
+.section .strings,"MS",@progbits,1,unique,10
+.Linfo_string0:
+  .asciz "AAA"
+.Linfo_string1:
+  .asciz "BBB"
+
+.section .strings,"MS",@progbits,1,unique,20
+.Linfo_string2:
+  .asciz "BBB"
+.Linfo_string3:
+  .asciz "CCC"
+
+.section .foo,"ax",@progbits
+.long .Linfo_string0
+.long .Linfo_string1
+.long .Linfo_string2
+.long .Linfo_string3

test/ELF/emit-relocs-mergeable.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1
+# RUN: ld.lld --emit-relocs %t1 -o %t2
+# RUN: llvm-readobj -sections -section-data -r %t2 | FileCheck %s
+
+## Check we produce proper relocations when doing merging of SHF_MERGE sections.
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .strings
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [ 
+# CHECK-NEXT:     SHF_MERGE
+# CHECK-NEXT:     SHF_STRINGS
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Offset:
+# CHECK-NEXT:   Size: 12
+# CHECK-NEXT:   Link:
+# CHECK-NEXT:   Info:
+# CHECK-NEXT:   AddressAlignment:
+# CHECK-NEXT:   EntrySize:
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 41414100 43434300 42424200 |AAA.CCC.BBB.|
+# CHECK-NEXT:   )
+# CHECK-NEXT: }
+
+# CHECK:      Relocations [
+# CHECK-NEXT:   Section {{.*}} .rela.foo {
+# CHECK-NEXT:     0x201000 R_X86_64_64 .strings 0x0
+# CHECK-NEXT:     0x201008 R_X86_64_64 .strings 0x8
+# CHECK-NEXT:     0x201010 R_X86_64_64 .strings 0x8
+# CHECK-NEXT:     0x201018 R_X86_64_64 .strings 0x4
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+.section .strings,"MS",@progbits,1,unique,10
+.Linfo_string0:
+  .asciz "AAA"
+.Linfo_string1:
+  .asciz "BBB"
+
+.section .strings,"MS",@progbits,1,unique,20
+.Linfo_string2:
+  .asciz "BBB"
+.Linfo_string3:
+  .asciz "CCC"
+
+.section .foo,"ax",@progbits
+.quad .Linfo_string0
+.quad .Linfo_string1
+.quad .Linfo_string2
+.quad .Linfo_string3

test/ELF/emit-relocs.s

 # CHECK-NEXT:     Value: 0x201000
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Local
 # CHECK-NEXT:     Type: Section
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: .text
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name:
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Local
+# CHECK-NEXT:     Type: Section
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: .comment
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: fn
 # CHECK-NEXT:     Value: 0x201000
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Global
 # CHECK-NEXT:     Type: Function
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: .text
 # CHECK-NEXT:   }