This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fixed a data race.
ClosedPublic

Authored by ilya-biryukov on Aug 7 2017, 7:50 AM.

Details

Summary

Calling addDocument after removeDocument could have resulted in an
invalid program state (AST and Preamble for the valid document could
have been incorrectly removed).
This commit also includes an improved CppFile::cancelRebuild
implementation that allows to cancel reparse without waiting for
ongoing rebuild to finish.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Aug 7 2017, 7:50 AM
klimek edited edge metadata.Aug 7 2017, 8:22 AM

Tests?

Tests?

TSAN does not catch this (as it's a logical error) and it requires a rather bizarre timing of file reparses to trigger.
I couldn't come up with an example that would reproduce this.

klimek added a comment.Aug 7 2017, 8:48 AM

Tests?

TSAN does not catch this (as it's a logical error) and it requires a rather bizarre timing of file reparses to trigger.
I couldn't come up with an example that would reproduce this.

Yea, we'll probably want to add a "smash it hard with parallel" requests kind of test to catch these things. You're right, there is probably not a better solution for this specific bug.

Yea, we'll probably want to add a "smash it hard with parallel" requests kind of test to catch these things. You're right, there is probably not a better solution for this specific bug.

Added a relevant test to https://reviews.llvm.org/D36261. It actually found this problem (after repeatedly running it for some time).

klimek added inline comments.Aug 11 2017, 3:19 AM
clangd/ClangdUnit.cpp
714 ↗(On Diff #109993)

Somewhat orthogonal question - why CppFile? We do support other languages in Clangd, right? (C / Obj-C)
I'd have expected this to be called ClangdUnit, especially given the file name.

735 ↗(On Diff #109993)

Now I know why I don't like the shard_ptr, this is all pretty awkward.

738 ↗(On Diff #109993)

Why do we need that instead of just calling the bound shared_ptr This?

ilya-biryukov added inline comments.Aug 11 2017, 3:41 AM
clangd/ClangdUnit.cpp
714 ↗(On Diff #109993)

Just wanted to give it a different name when the class changed semantics. Didn't rename the file yet, though, because was afraid that git would not detect renames and we'll lose history.
ClangdUnit used to provide implementations for all features (codeComplete, etc.), CppFile manages AST and Preamble., i.e. doing a different thing.
But I agree, CppFile is a probably a bad name, but I would still go with something not ending with Unit to not bring up parallels with ASTUnit, that provides a very different interface.

How about ClangdFile, ClangdFileAST?

735 ↗(On Diff #109993)

Do you mean shared_from_this() specifically?

738 ↗(On Diff #109993)

This lambda is a deferred computation and might get executed on a different thread, so we want to keep a shared_ptr to our class around (weak_ptr would also work) to make sure this class does not get deleted before this lambda is called.
This local var is for capture in a lambda that is passed to wait in the code below (we could've captured That, but that would be an extra shared_ptr copy).

A follow-up after discussion offline: we should make an attempt to simplify the threading code in ClangdUnit.h by moving the async worker loop of CppFile into the class itself, rather than relying on external scheduling to do the right thing.
@klimek, are there any other issues we need to address in this specific patch or are we good to go with submitting it?

bkramer accepted this revision.Aug 14 2017, 12:59 AM

I think this can be committed now. It's still a bit awkward but we can address that later.

This revision is now accepted and ready to land.Aug 14 2017, 12:59 AM

Thanks. I'll make sure to address the awkwardness in further commits.

Closed by commit rL310818: [clangd] Fixed a data race. (authored by ibiryukov). · Explain WhyAug 14 2017, 1:19 AM
This revision was automatically updated to reflect the committed changes.