[PATCH] D112977: [lld-macho] Handle weak vs strong case in symbol resolution

[Vy Nguyen via Phabricator via llvm-commits]

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