sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:724
std::move(Req.WantDiags));
+ // Set it after notifying ASTPeer about the preamble to prevent any races.
+ BuiltFirst.notify();
----------------
kadircet wrote:
> sammccall wrote:
> > hmm, we're notifying the ASTPeer, and then setting builtfirst which is
> > being waited on... by astpeer.
> > This suggests to me the AST thread should own the notification, not the
> > preamble thread.
> > And in fact it already has a notification for the first preamble being
> > built! why can't we use that one?
> umm, it is a little bit more complicated.
>
> The above call only inserts the "update request" onto ASTPeer's queue and
> doesn't update the preamble inside ASTPeer. This enables ASTPeer to access
> `LatestPreamble` without holding the lock, as it is the only writer. We can
> change that, update the `LatestPreamble` while queueing the request.
As discussed, the relationship between the two is complicated.
At least we should move the second into ASTPeer to reduce the surface area
between the two.
As a followup, we can "explode" the existing notification into CV + Mutex +
bool.
The bool can naturally become the "present" bit on LatestPreamble if it turns
into an Optional<shared_ptr<>> - we must document none vs null!
The Mutex already exists.
Then we don't need the second notification: we just wait on
`!preambletasks.empty() || LatestPreamble.hasValue()`
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:658
std::move(CompilerInvocationDiags), WantDiags);
+ // Block until first preamble is ready, as patching an empty preamble would
+ // imply rebuilding it from scratch.
----------------
This isn't the natural place to block, rather where the preamble would be
consumed.
But that's too late, we'd be running on the worker thread with the PreambleTask
scheduled and so we'd deadlock.
This needs a comment :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80293/new/
https://reviews.llvm.org/D80293
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
