[PATCH] D112977: [lld-macho] Fix failed assertion in registerCompactUnwind

Wed Nov 3 08:34:45 PDT 2021

oontvoo added inline comments.

================
Comment at: lld/MachO/SymbolTable.cpp:71-72

+        defined->referencedDynamically |= isReferencedDynamically;
+        defined->noDeadStrip |= noDeadStrip;
+
----------------
int3 wrote:
> oontvoo wrote:
> > int3 wrote:
> > > is this what ld64 does for these two attributes too? (can we test it?)
> > No, this is NOT what LD64 does. (In fact this was one of the differences I was trying to note in the draft doc a couple weeks ago).
> > LD64 picks one of the two (based on things like scope,  alignment, whether symbols are auto-hide ) and doesn't merge anything.
> > I guess we could imitate that here ... didn't want to change that since this current approach seems to be working fine...
> > 
> > 
> gotcha. Nonetheless, this is changing our behavior slightly, since we are now setting `referencedDynamically` and `noDeadStrip` when coalescing a weak with a non-weak, whereas before we would only do it when both symbols were weak. Can we keep the previous behavior for now?
^ is done but. leaving open in case  thakis@ wants to chime in

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112977