[PATCH] D112977: [lld-macho] Handle weak vs strong case in symbol resolution
Vy Nguyen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 8 16:33:53 PST 2021
oontvoo marked an inline comment as done.
oontvoo added a comment.
Thanks!
================
Comment at: lld/MachO/SymbolTable.cpp:83
+ concatIsec->wasCoalesced = true;
+ concatIsec->symbols.erase(llvm::find(concatIsec->symbols, defined));
+ }
----------------
int3 wrote:
> oontvoo wrote:
> > int3 wrote:
> > > IMO this is redundant since we should never be using coalesced sections in any meaningful capacity. Maybe comment it out and leave a note as to why it's not needed?
> > This was needed because only [[ https://github.com/llvm/llvm-project/blob/1837a837b36b075011c2616edfa20b8161e58a72/lld/MachO/InputSection.h#L119 | **coalesced-weak** ]] sections are omitted from output.
> >
> > Eg., without this, you could see my new tests emitting the following (commentary is mine, of course):
> >
> >
> > ```
> > $ otool -jtV /Users/vyng/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/weak-definition-gc.s.tmp/weak-strong-mixed.dylib
> > /Users/vyng/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/weak-definition-gc.s.tmp/weak-strong-mixed.dylib:
> > (__TEXT,__text) section
> > >>>>>> all this stuff is from the weak symbol and should not have made it here.
> > 0000000000000440 55 pushq %rbp
> > 0000000000000441 4889e5 movq %rsp, %rbp
> > 0000000000000444 5d popq %rbp
> > 0000000000000445 c3 retq
> > <<<<<<< END
> > _foo:
> > 0000000000000446 3333 xorl (%rbx), %esi
> > 0000000000000448 3333 xorl (%rbx), %esi
> > 000000000000044a c3 retq
> > ```
> >
> > We could change the logic everywhere but I thought the easiest way was to remove the weak symbol here (as if it'd never been added when we already had a strong symbol)
> >
> > WDYT?
> huh interesting. I thought we would have omitted the section content regardless of whether that symbol were removed from the coalesced section... but I'm too lazy to dig into it right now. I also realized that `ConcatInputSection::foldIdentical` also takes care to remove the symbols, and this is basically a truncated version of that. Yeah ship it, and maybe I'll look into it another day
`foldIdenticalSections()` isn't run for this test. (It's not run by default ....)
But yeah, i'll have a look in a separate patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112977/new/
https://reviews.llvm.org/D112977
More information about the llvm-commits
mailing list