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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:82
+
+## The signature symbol is local. Assume the intention is to localize the group.
+## Drop GRP_COMDAT so that the linker will suppress deduplication.
----------------
"is" confused me slightly, suggesting it was the original state of the symbol.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:90
+
+## The signature symbol is non-local. Keep GRP_COMDAT.
+# RUN: llvm-readelf -s --section-groups %t | FileCheck %s --check-prefix=WEAK-SIG
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:91
+## The signature symbol is non-local. Keep GRP_COMDAT.
+# RUN: llvm-readelf -s --section-groups %t | FileCheck %s --check-prefix=WEAK-SIG
+
----------------
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?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1074-1075
+  // 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.
+  if ((FlagWord & GRP_COMDAT) && Sym && Sym->Binding == STB_LOCAL)
----------------
Possibly worth adding a reference to the gABI list discussion for further justification?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1076-1077
+  // deduplication.
+  if ((FlagWord & GRP_COMDAT) && Sym && Sym->Binding == STB_LOCAL)
+    this->FlagWord &= ~GRP_COMDAT;
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106782/new/

https://reviews.llvm.org/D106782