[PATCH] D150859: [llvm-objcopy][ELF] Preserve sh_link to .symtab when applicable

Thu May 25 11:00:50 PDT 2023

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Sorry for the delay. This looks good me. Is the intention to make `ld.lld --icf=safe` work after certain `llvm-objcopy` operations? If so, it'd be good to state the motivation in the summary.
I think the change is good for other `sh_link->SHT_SYMTAB` sections.

I have a comment whether we should make new symbols (while existing symbol indices are unchanged) reset `sh_link` to zero.

I wonder whether we still consider changing the design of ELF `SHT_LLVM_ADDRSIG`. Mach-O uses relocation-based `__addrsig` (https://discourse.llvm.org/t/problems-with-mach-o-address-significance-table-generation/63392).

================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:690
   uint32_t Index = 0;
-  for (auto &Sym : Symbols)
-    Sym->Index = Index++;
+  if (IndicesChanged)
+    for (auto &Sym : Symbols)
----------------
Add `{}` for `if` and `else`. See https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

If the manual loop unswitching necessary? Can we just keep the `else` part?

================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:2537

+  // If the .symtab indices have not been changed restore the sh_link to .symtab
+  // for sections that were linked to .symtab.
----------------

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symtab-link.test:21
+# RUN: llvm-readobj --sections %t7 | FileCheck %s --check-prefix=LINK-0
+## Remove .text section
+# RUN: llvm-objcopy --remove-section=.text %t %t8
----------------

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

https://reviews.llvm.org/D150859