[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

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