[PATCH] D135897: [LLD][ELF] --wrap: __real_foo references should trigger archive extraction for foo

[ben via Phabricator via llvm-commits]

bd1976llvm marked 5 inline comments as done.
bd1976llvm added inline comments.


================
Comment at: lld/test/ELF/wrap-extract-real.ll:19
+# REAL-NEXT:        {{.*}}           {{.*}} FUNC    GLOBAL DEFAULT     2 _start
+# REAL-NEXT:        {{.*}}           {{.*}} FUNC    WEAK   DEFAULT     2 foo
+# REAL-NEXT:        {{.*}}           {{.*}} NOTYPE  GLOBAL DEFAULT   UND __wrap_foo
----------------
MaskRay wrote:
> smeenai wrote:
> > bd1976llvm wrote:
> > > smeenai wrote:
> > > > Why does this become weak? (It's global for the assembly test case, which is what I'd expect.)
> > > Good question! It seems to be a bug. Weak binding is assigned by this piece of code:
> > > 
> > > llvm\lib\LTO\LTO.cpp:
> > >         // For symbols re-defined with linker -wrap and -defsym options,
> > >         // set the linkage to weak to inhibit IPO. The linkage will be
> > >         // restored by the linker.
> > >         if (Res.LinkerRedefined)
> > >           GV->setLinkage(GlobalValue::WeakAnyLinkage);
> > > 
> > > However, LLD doesn't seem to have fulfilled its side of the bargain alluded to in this comment.
> > > 
> > > I'll look at this more tomorrow.
> > Ah, I remember this now; that's https://github.com/llvm/llvm-project/issues/51346. It's independent of your change though, so you can certainly land this without needing to fix that.
> So this is related to a WeakAnyLinkage workaround D33621. It'd be difficult to fix properly but it's good to re-think how to handle it.
Thanks.


================
Comment at: lld/test/ELF/wrap-extract-real.ll:19
+# REAL-NEXT:        {{.*}}           {{.*}} FUNC    GLOBAL DEFAULT     2 _start
+# REAL-NEXT:        {{.*}}           {{.*}} FUNC    WEAK   DEFAULT     2 foo
+# REAL-NEXT:        {{.*}}           {{.*}} NOTYPE  GLOBAL DEFAULT   UND __wrap_foo
----------------
bd1976llvm wrote:
> MaskRay wrote:
> > smeenai wrote:
> > > bd1976llvm wrote:
> > > > smeenai wrote:
> > > > > Why does this become weak? (It's global for the assembly test case, which is what I'd expect.)
> > > > Good question! It seems to be a bug. Weak binding is assigned by this piece of code:
> > > > 
> > > > llvm\lib\LTO\LTO.cpp:
> > > >         // For symbols re-defined with linker -wrap and -defsym options,
> > > >         // set the linkage to weak to inhibit IPO. The linkage will be
> > > >         // restored by the linker.
> > > >         if (Res.LinkerRedefined)
> > > >           GV->setLinkage(GlobalValue::WeakAnyLinkage);
> > > > 
> > > > However, LLD doesn't seem to have fulfilled its side of the bargain alluded to in this comment.
> > > > 
> > > > I'll look at this more tomorrow.
> > > Ah, I remember this now; that's https://github.com/llvm/llvm-project/issues/51346. It's independent of your change though, so you can certainly land this without needing to fix that.
> > So this is related to a WeakAnyLinkage workaround D33621. It'd be difficult to fix properly but it's good to re-think how to handle it.
> Thanks.
Interesting. Thanks.


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

https://reviews.llvm.org/D135897