[PATCH] D120377: [lld-macho] Implement -why_live (without perf overhead)

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/MarkLive.cpp:111
+    if (!symbols.empty())
+      lld::errs() << std::string(indent, ' ') << toString(*symbols.front())
+                  << " from " << toString(symbols.front()->getFile()) << "\n";
----------------
oontvoo wrote:
> thakis wrote:
> > int3 wrote:
> > > thakis wrote:
> > > > can you add a comment on why this doesn't use message()?
> > > oh yeah... I was thinking about how we would handle parallelization in the future, and I realized that `message()` wasn't ideal here because it would take one lock per call, whereas we want to take one lock for the whole of `printWhyLiveImpl` so that the lines wouldn't get interleaved with other concurrent messages. I'll add that comment.
> > Oh, good point. But maybe it's nicer to accumulate the message in a string and only call message() once at the end? Then we need to hold the lock for a shorter time.
> > 
> > (Are our bumpptr allocators in a per-thread tls?)
> 
> > (Are our bumpptr allocators in a per-thread tls?)
> 
> No - the allocators are shared (and very much not thread-safe :\ )
> But maybe it's nicer to accumulate the message in a string and only call message() once at the end? Then we need to hold the lock for a shorter time.

Good point!

Also, in case you missed it, there's this discussion going on: https://discourse.llvm.org/t/parallel-thread-safe-algorithms-allocators-and-containers/60472


================
Comment at: lld/MachO/MarkLive.cpp:187
+              referentIsec = d->isec;
+            enqueue(isec, 0, makeEntry(referentIsec, nullptr));
+          }
----------------
thakis wrote:
> thakis wrote:
> > Do you want a `makeEntry(nullptr, nullptr)` for Undefineds here, or a `nullptr`?
> > 
> > Hm, I guess an Undefined never has relocs. Do we always have a Defined here? If so, we should probably just `cast<>` without conditional.
> > 
> > If there are non-Undefined non-Defined cases, the original question if you want `makeEntry(nullptr, nullptr)` stands for that :)
> (this is still open)
Oh whoops, almost forgot about this one.

We could have dylib or undefined symbols here too, yeah. In fact, I'd neglected to add a test for that... and adding that test revealed a null deref issue due to this `makeEntry(nullptr, nullptr)` thing. Good catch :)

I'll have `makeEntry()` itself do the null check so that we don't have an extra conditional in the non-why-live-enabled case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120377