[ELF] - Do not merge sections from SHT_GROUP when -relocatable

[grimar]

Summary

This is PR34506.

Imagine we have 2 sections the same name but different COMDAT groups:

.section        .foo,"axG",@progbits,bar,comdat
.section        .foo,"axG",@progbits,zed,comdat

When linking relocatable we do not merge SHT_GROUP sections. But still would merge
both input sections .foo into single output section .foo.
As a result we will have 2 different SHT_GROUPs containing the same section, what
is wrong.

Patch fixes the issue, preventing merging SHF_GROUP sections with any others.

[ruiu]

This does not seems like a correct fix. Why do we merge sections in the first place? If we don't do that for -r, it shouldn't have been the issue, no?

[grimar]

In D37574#863756, @ruiu wrote:

This does not seems like a correct fix. Why do we merge sections in the first place? If we don't do that for -r, it shouldn't have been the issue, no?

What is intention of relocatable link and why it is called "partial linking" ? As far I understand intention is to speedup final link.
When we have multiple .text sections for example, then relocatable link merge them by name to output .text. Merging of sections helps to reduce amount of
input sections for latter final link. We also merge SHT_REL[A] sections when doing that, it should probably help to speedup futher relocation processing as well.
As a last resort is also consistent with GNU linkers, it may be dangerous to deviate without solid reason as all kind of tools may rely on this behavior.

[ruiu]

What is intention of relocatable link and why it is called "partial linking" ? As far I understand intention is to speedup final link.
When we have multiple .text sections for example, then relocatable link merge them by name to output .text. Merging of sections helps to reduce amount of
input sections for latter final link. We also merge SHT_REL[A] sections when doing that, it should probably help to speedup futher relocation processing as well.
As a last resort is also consistent with GNU linkers, it may be dangerous to deviate without solid reason as all kind of tools may rely on this behavior.

I don't agree. As we have found when you tried to implement the -r option, a lot of information that can be merged are passed through instead of being merged for a relocatable link. If sections have to be merged for some reason, that is fine, but both "-r is for speeding up the linking" and "doing something different than GNU linkers is dangerous" are not convincing arguments.

[grimar]

• Original version of patch was silly. Rui, will you be ok with this version ?

[ruiu]

ELF/OutputSections.cpp

210 ↗

(On Diff #115390)

s/- r/-r/

215 ↗

(On Diff #115390)

This comment is not correct. Assume that there's only one section group containing a section .foo, and there's other object file having a section .foo which is not a member of any section group. It'll end up with only one section group, but we still shouldn't merge the member .foo with non-member .foo, because it is semantically incorrect.

[grimar]

• Addressed review comments.

ELF/OutputSections.cpp

215 ↗

(On Diff #115390)

Right. Updated comment and added regular (non-group) section to testcase.

[ruiu]

LGTM with this change.

ELF/OutputSections.cpp

254–262 ↗

(On Diff #115616)

This comment becomes a bit too lengthy. Instead of appending more and more comments, it is sometimes better to rewrite. I'd rewrite this:

Sections with SHT_GROUP or SHF_GROUP attributes reach here only when the -r option is given. A section with SHT_GROUP defines a "section group", and its members have SHF_GROUP attribute. Usually these flags have already been stripped by InputFiles.cpp as section groups are processed and uniquified. However, for the -r option, we want to pass through all section groups as-is because adding/removing members or merging them with other groups change their semantics.

[grimar]

ELF/OutputSections.cpp

254–262 ↗

(On Diff #115616)

Thanks !

lld/trunk/ELF/OutputSections.cpp

void OutputSectionFactory::addInputSec(InputSectionBase *IS,
   }
 
   // If we have destination output section - use it directly.
   if (OS) {
     addSection(IS, OutsecName, OS);
     return;
   }
 
-  // Sections with the SHT_GROUP attribute reach here only when the -r option
-  // is given. Such sections define "section groups", and InputFiles.cpp has
-  // dedup'ed section groups by their signatures. For the -r, we want to pass
-  // through all SHT_GROUP sections without merging them because merging them
-  // creates broken section contents.
-  if (IS->Type == SHT_GROUP) {
+  // Sections with SHT_GROUP or SHF_GROUP attributes reach here only when the -r
+  // option is given. A section with SHT_GROUP defines a "section group", and
+  // its members have SHF_GROUP attribute. Usually these flags have already been
+  // stripped by InputFiles.cpp as section groups are processed and uniquified.
+  // However, for the -r option, we want to pass through all section groups
+  // as-is because adding/removing members or merging them with other groups
+  // change their semantics.
+  if (IS->Type == SHT_GROUP || (IS->Flags & SHF_GROUP)) {
     addSection(IS, OutsecName, nullptr);
     return;
   }
 
   // Imagine .zed : { *(.foo) *(.bar) } script. Both foo and bar may have
   // relocation sections .rela.foo and .rela.bar for example. Most tools do
   // not allow multiple REL[A] sections for output section. Hence we
   // should combine these relocation sections into single output.

lld/trunk/test/ELF/relocatable-comdat2.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld -r %t.o -o %t
+# RUN: llvm-readobj -elf-section-groups -s %t | FileCheck %s
+
+## Check .foo was not merged.
+# CHECK: Sections [
+# CHECK:  Name: .foo
+# CHECK:  Name: .foo
+# CHECK:  Name: .foo
+
+# CHECK:      Groups {
+# CHECK-NEXT:   Group {
+# CHECK-NEXT:     Name: .group
+# CHECK-NEXT:     Index: 2
+# CHECK-NEXT:     Type: COMDAT
+# CHECK-NEXT:     Signature: bar
+# CHECK-NEXT:     Section(s) in group [
+# CHECK-NEXT:       .foo (3)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Group {
+# CHECK-NEXT:     Name: .group
+# CHECK-NEXT:     Index: 4
+# CHECK-NEXT:     Type: COMDAT
+# CHECK-NEXT:     Signature: zed
+# CHECK-NEXT:     Section(s) in group [
+# CHECK-NEXT:       .foo (5)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT: }
+
+.section .foo,"axG",@progbits,bar,comdat
+.section .foo,"axG",@progbits,zed,comdat
+.section .foo,"ax",@progbits