ilya-biryukov added inline comments.
================
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > ilya-biryukov wrote:
> > > > > > This is racy, `FileInputs` is only accessed from a worker thread
> > > > > > now.
> > > > > > I'm afraid we'll need a separate variable with a lock around it
> > > > > > (could reuse one lock for preamble and compile command, probably)
> > > > > It looks like we could also guard `FileInputs`? Adding another
> > > > > variable seems to make the state more complicated.
> > > > `FileInputs` are currently accessed without locks in a bunch of places
> > > > and it'd be nice to keep it that way (the alternative is more
> > > > complicated code).
> > > > Doing the same thing we do for preamble looks like the best alternative
> > > > here.
> > > The reason I think it would be easier to guard `FileInputs` with mutex
> > > instead of pulling a new variable is that CompileCommand is usually used
> > > along with other fields in `FileInputs`. I think we could manage this
> > > with relatively few changes. Please take a look at the new changes.
> > Unfortunately we can't share `Inputs.FS` safely as the vfs implementations
> > are not thread-safe.
> It seems to me that the behavior for `FS` hasn't change in this patch. And
> `FileInputs` (incl. `Inputs.FS`) has always been available/shared across
> worker threads. We don't seem to have systematic way to prevent raciness
> before this patch, and it would be good to have it somehow, but it's probably
> out of the scope.
>
> Maybe I'm missing something or misinterpreting. Could you elaborate?
> And FileInputs (incl. Inputs.FS) has always been available/shared across
> worker threads
I don't think that's the case, we did not a public API to get the hands on
`ASTWorker::FileInputs.FS` and we're getting one now.
Even though the current patch does not add such racy accesses, it certainly
makes it much easier to do it accidentally from the clients of `ASTWorker` in
the future (one just needs to access `getCurrentInputs().FS`).
The (not very) systematic way to prevent raciness that we use now is to **not**
protect the members which are potentially racy with locks and document they are
accessed only from a worker thread.
Having a separate copy of the compile command is a small price to pay (both in
terms of performance and complexity) to avoid exposing non-thread-safe members
of `ASTWorker` in its public interface.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
