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

[Andrew Ng via Phabricator via llvm-commits]

andrewng added a comment.

> 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.

No, this was not specifically done for `SHT_LLVM_ADDRSIG` but for some downstream sections.

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

I did consider this but decided not to reset `sh_link` to `0`. Do you have a reason for why resetting `sh_link` to `0` in this scenario would be better/preferable?



================
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)
----------------
MaskRay wrote:
> 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?
> If the manual loop unswitching necessary? Can we just keep the `else` part?

It's not needed but felt like a reasonable optimisation. Would you rather drop the optimisation?




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

https://reviews.llvm.org/D150859