[llvm-objcopy] Drop GRP_COMDAT if the group signature is localized

[MaskRay]

Summary

See GRP_COMDAT group with STB_LOCAL signature
objcopy PR: https://sourceware.org/bugzilla/show_bug.cgi?id=27931

GRP_COMDAT deduplication is purely based on the signature symbol name in
ld.lld/GNU ld/gold. The local/global status is not part of the equation.

If the signature symbol is localized by --localize-hidden or
--keep-global-symbol, the intention is likely to make the group fully
localized. Drop GRP_COMDAT to suppress deduplication.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/group.test

82

"is" confused me slightly, suggesting it was the original state of the symbol.

90

91

This line is testing the yaml2obj output, which I don't think is what you want. Did you mean to dump %t1 or do something else?

llvm/tools/llvm-objcopy/ELF/Object.cpp

1074–1075

Possibly worth adding a reference to the gABI list discussion for further justification?

1076–1077

If I'm reading this correctly, this will happen even if no operations have been performed to make the symbol STB_LOCAL (i.e. it was STB_LOCAL in the input). Perhaps we should have an explicit test case for that too.

[MaskRay]

address comments

llvm/test/tools/llvm-objcopy/ELF/group.test

91

Ah, right. I intended to test %t1

[jhenderson]

LGTM, with two nits.

llvm/test/tools/llvm-objcopy/ELF/group.test

120

Seems like you can drop this? (possibly also the Link field, but I don't remember what the default is for group sections).

127

Perhaps simplify to just [SHF_GROUP]?

[MaskRay]

simplify test

[uweigand]

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

[MaskRay]

In D106782#2905730, @uweigand wrote:

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

A latent problem triggered by the change. Fixed by c5d8bd5a35cbd325c6ccd42afa91bad06d261f07

[uweigand]

In D106782#2905801, @MaskRay wrote:

In D106782#2905730, @uweigand wrote:

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

A latent problem triggered by the change. Fixed by c5d8bd5a35cbd325c6ccd42afa91bad06d261f07

Thanks! The build bot is back to green now.

llvm/test/tools/llvm-objcopy/ELF/group-reorder.test

Symbols:
   - Name:            .foo
     Type:            STT_SECTION
     Section:         .foo
   - Name:            .bar
     Type:            STT_SECTION
     Section:         .bar
   - Name:            bar
     Type:            STT_FUNC
+    Binding:         STB_GLOBAL
     Section:         .foo

llvm/test/tools/llvm-objcopy/ELF/group.test

 # SECTIONS-NEXT: .text.foo
 # SECTIONS-NEXT: .strtab
 # SECTIONS-NEXT: .shstrtab
 
 ## Show that llvm-objcopy can handle a group section with a zero sh_link and
 ## sh_info fields.
 # RUN: llvm-objcopy --allow-broken-links -R .symtab %t3 %t4
 # RUN: cmp %t3 %t4
+
+## The signature symbol becomes local. Assume the intention is to localize the group.
+## Drop GRP_COMDAT so that the linker will suppress deduplication.
+# RUN: llvm-objcopy --keep-global-symbol=bar %t %t5
+# RUN: llvm-readelf -s --section-groups %t5 | FileCheck %s --check-prefix=LOCAL-SIG
+
+# LOCAL-SIG:  LOCAL  DEFAULT [[#]] foo
+# LOCAL-SIG:  (unknown) group section [ 1] `.group' [foo] contains 1 sections:
+
+## The signature symbol remains non-local. Keep GRP_COMDAT.
+# RUN: llvm-readelf -s --section-groups %t1 | FileCheck %s --check-prefix=WEAK-SIG
+
+# WEAK-SIG:   WEAK   DEFAULT [[#]] foo
+# WEAK-SIG:   COMDAT group section [ 1] `.group' [foo] contains 1 sections:
+
+# RUN: llvm-objcopy --globalize-symbol=foo %t %t6
+# RUN: llvm-readelf -s --section-groups %t6 | FileCheck %s --check-prefix=GLOBAL-SIG
+
+# GLOBAL-SIG: GLOBAL DEFAULT [[#]] foo
+# GLOBAL-SIG: COMDAT group section [ 1] `.group' [foo] contains 1 sections:
+
+## If the signature is initially local and no operation has been performed to
+## specifically localize it, it isn't clear whether we should drop GRP_COMDAT.
+## The current convention is that compilers should not produce such input, so
+## our choice does not matter.
+# RUN: yaml2obj --docnum=2 %s -o %t.localsig
+# RUN: llvm-objcopy %t.localsig %t.localsig.out
+# RUN: llvm-readelf -s --section-groups %t.localsig.out | FileCheck %s --check-prefix=LOCAL-SIG
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:  .group
+    Type:  SHT_GROUP
+    Info:  foo
+    Members:
+      - SectionOrType: GRP_COMDAT
+      - SectionOrType: .text.foo
+  - Name:  .text.foo
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_GROUP ]
+Symbols:
+  - Name:    foo
+    Type:    STT_FUNC
+    Section: .text.foo

llvm/test/tools/llvm-objcopy/ELF/remove-section-in-group.test

- Name:     .foo
     Type:     SHT_PROGBITS
     Flags:    [ SHF_ALLOC, SHF_GROUP ]
   - Name:     .bar
     Type:     SHT_PROGBITS
     Flags:    [ SHF_ALLOC, SHF_GROUP ]
 Symbols:
   - Name:     foo_bar_grp
     Section:  .group
+    Binding:  STB_GLOBAL

llvm/test/tools/llvm-objcopy/ELF/strip-dwo-groups.test

- Name:  .rela.text.group2
     Type:  SHT_RELA
     Flags: [ SHF_GROUP ]
     Info:  .text.group2
   - Name: .debug_after.dwo
     Type: SHT_PROGBITS
 Symbols:
   - Name:    debug_before
     Section: .debug_before.dwo
+  - Name:    debug_after
+    Section: .debug_after.dwo
   - Name:    group2
     Section: .text.group2
+    Binding: STB_WEAK
   - Name:    group1
     Section: .text.group1
-  - Name:    debug_after
-    Section: .debug_after.dwo
+    Binding: STB_WEAK

llvm/tools/llvm-objcopy/ELF/Object.cpp

if (ToRemove(LinkSection)) {
     LinkSection = nullptr;
   }
   return Error::success();
 }
 
 void GroupSection::finalize() {
   this->Info = Sym ? Sym->Index : 0;
   this->Link = SymTab ? SymTab->Index : 0;
+  // Linker deduplication for GRP_COMDAT is based on Sym->Name. The local/global
+  // status is not part of the equation. If Sym is localized, the intention is
+  // likely to make the group fully localized. Drop GRP_COMDAT to suppress
+  // deduplication. See https://groups.google.com/g/generic-abi/c/2X6mR-s2zoc
+  if ((FlagWord & GRP_COMDAT) && Sym && Sym->Binding == STB_LOCAL)
+    this->FlagWord &= ~GRP_COMDAT;
 }
 
 Error GroupSection::removeSectionReferences(
     bool AllowBrokenLinks, function_ref<bool(const SectionBase *)> ToRemove) {
   if (ToRemove(SymTab)) {
     if (!AllowBrokenLinks)
       return createStringError(
           llvm::errc::invalid_argument,