[PATCH] D139069: [lld-macho] Ignored aliases to weak symbols should not retain section data

[Jez Ng via Phabricator via llvm-commits]

int3 marked 3 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:624
 
+static bool isIgnoredName(StringRef name) {
+  return name.startswith("l") || name.startswith("L");
----------------
int3 wrote:
> oontvoo wrote:
> > super nit ...
> will add the comment. honestly I don't think either name is super good heh (I went back and forth before deciding on this). ld64 calls them "ignored labels" which doesn't seem great either because the rest of the code doesn't use the word "label"
> 
> but the symbol isn't exactly ignored either :/ it's used at parse time, just immediately dropped as part of the parse...
> 
> will mull on this a bit
MC calls these "private labels" so let's go with that (see https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCAsmInfo.cpp#L46)


================
Comment at: lld/MachO/UnwindInfoSection.cpp:209
   if (!p.second && d->unwindEntry) {
-    assert(!p.first->second->unwindEntry);
+    assert(p.first->second == d || !p.first->second->unwindEntry);
     p.first->second = d;
----------------
oontvoo wrote:
> i wonder if this assert could be stricter (ie., how do we know this `p.first->second == d` is true because of the alias-symbol reuse  (from InputFiles ~line 872) and not some bug where the same set of symbols are being added twice ....
I feel that too, but not sure there's a great solution here

we could delete the duplicate (aliased) symbols after parsing the relocations but that seems like unnecessary overhead (in terms of iterating over all the symbols an extra time)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139069