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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D150859#4373263 <https://reviews.llvm.org/D150859#4373263>, @andrewng wrote:

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

The section content may be a digest of existing symbols. Adding a symbol nullifies its assumption that its content is comprehensive.
However, I don't have an example that this breaks.



================
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)
----------------
andrewng wrote:
> 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?
> 
> 
I'd drop this optimization. It likely doesn't affect performance. There are complex symbol table operations elsewhere..


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

https://reviews.llvm.org/D150859