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;
+}
----------------
ioeric wrote:
> 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.
Inside `ASTWorker`, `FileInputs` can still be guarded with `Mutex` as a whole.
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
