[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;
+}
----------------
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?  


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60607/new/

https://reviews.llvm.org/D60607