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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SymbolTable.cpp:83
+          concatIsec->wasCoalesced = true;
+          concatIsec->symbols.erase(llvm::find(concatIsec->symbols, defined));
+        }
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112977