ioeric marked an inline comment as done.
ioeric added inline comments.
================
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
----------------
ilya-biryukov wrote:
> 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.
I don't think that makes a fundamental difference as `FileInputs.FS` is already
racy. But if "public" API is the concern, we could simply restrict the scope of
the API and make return `CompileCommand` only.
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
