[PATCH] D124056: [ELF] Fix wrapping symbols produced during LTO codegen

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

In D124056#3468783 <https://reviews.llvm.org/D124056#3468783>, @MaskRay wrote:

> Thanks for the patch. I think it is an important improvement.

Thanks for the review!

> I am also curious whether you want to do anything with D33621 <https://reviews.llvm.org/D33621>: the chunk of code is a bit of a hack
>
>   if (Res.LinkerRedefined)
>     GV->setLinkage(GlobalValue::WeakAnyLinkage);

Yeah, I've been meaning to look into that as well ... there's at least one issue where the logic to restore the original binding of the symbol in the linker got dropped (https://github.com/llvm/llvm-project/issues/51346). I need to understand the LTO logic better first though.



================
Comment at: lld/ELF/SymbolTable.cpp:47
+    sym->isUsedInRegularObj = true;
+  else if (sym->isUndefined())
     sym->isUsedInRegularObj = false;
----------------
MaskRay wrote:
> It will be good to have a comment for the `if (sym->isUndefined())` condition.
Note that I'm tightening the condition further in D124065, which is a follow-up to this diff. Would you prefer adding a comment here and then changing it in D124065, or just adding the comment in D124065? (I think it'll be easier to explain the logic of the conditional with the change in D124065.)


================
Comment at: lld/ELF/Symbols.h:135
   //
   // This is also used to retain __wrap_foo when foo is referenced.
   uint8_t referenced : 1;
----------------
MaskRay wrote:
> Is this sentence stale now?
Hmm. In a way it is because `referencedAfterWrap` will be used for the retaining now, but `referenced` on the pre-wrapped symbol is still used to set `referencedAfterWrap` on the wrapped symbol. I think it's fine to remove the sentence though, since the `referencedAfterWrap` comment also mentions that property being set based on if the original symbol is referenced.


================
Comment at: lld/ELF/Symbols.h:141
+  // true for __wrap_foo if foo is referenced).
+  uint8_t referencedAfterWrapping : 1;
+
----------------
MaskRay wrote:
> The name may be too long. We don't need to pursue perfect grammar, we can just call it `referencedAfterWrap`
Sure, I'll change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124056