This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix an assertion failure in background index.
ClosedPublic

Authored by hokein on Dec 13 2018, 4:54 AM.

Details

Summary

When indexing a file which contains an uncompilable error, we will
trigger an assertion failure -- the IndexFileIn data is not set, but we
access them in the backgound index.

Event Timeline

hokein created this revision.Dec 13 2018, 4:54 AM

I think the description is misleading, you are saying that we were triggering an assertion and you are fixing that. But in reality, we were not triggering any assertions, this patch is adding the assertions right?

clangd/index/Background.cpp
393

I don't think it is a good idea to assert and die just for one file, since we are actually trying to index all the files. We should rather check these exists and propagate the error in case of failure. You can have a look at https://reviews.llvm.org/D55224, same lines.

Also with your changes in the endsourcefileaction this assertion is not possible to be triggered, since you will fill them with "empty" versions.

clangd/index/IndexAction.cpp
144 ↗(On Diff #178036)

I think this is same as if (!HasUncompilableErrors) so you can keep the old one just by negating it.

150 ↗(On Diff #178036)

I don't think it is a good idea to respond with an "empty" symbol slab rather than not replying. They have different semantics. The former implies everything went well, just couldn't find any symbols in the file, whereas the latter implies there was an error.

So I believe we should keep this code as it was.

hokein updated this revision to Diff 178203.Dec 14 2018, 1:41 AM
hokein marked 5 inline comments as done.

Keep the old behavior.

I think the description is misleading, you are saying that we were triggering an assertion and you are fixing that. But in reality, we were not triggering any assertions, this patch is adding the assertions right?

Ah, sorry for the confusion caused by the assertion I added. The description is correct, and this patch is trying to fix an assertion (not the assertion I added, it is the assertion in llvm::Optional, we are accessing an empty Optional).

clangd/index/Background.cpp
393

check these exists seems a wired way to me, We should explicitly check the error here.

The assertion I added here is to guarantee that we have all the data when indexing a file successfully.

clangd/index/IndexAction.cpp
150 ↗(On Diff #178036)

Sounds fair, I kept the old behavior.
but we have different behavior in clangd-indexer -- clangd-indexer will emit an empty index file when processing a problematic cpp file... a tradeoff is that we will reindex the problematic cpp file since we don't emit any index file, and the backgroundindex thinks this file is not being indexed.

Ah you are right, the Optional.. Thanks!

clangd/index/Background.cpp
384

Then don't need to keep these checks anymore in the endsourcefileaction of indexingaction right?

hokein marked an inline comment as done.Dec 14 2018, 3:14 AM
hokein added inline comments.
clangd/index/Background.cpp
384

We need indeed. We have other clients (clangd-indexer, and our internal indexer) using the IndexingAction -- we don't want symbols from broken TUs because they causes many troublesomes.

kadircet accepted this revision.Dec 14 2018, 3:36 AM

LGTM, thanks!

clangd/index/Background.cpp
384

Ah you are right.

This revision is now accepted and ready to land.Dec 14 2018, 3:36 AM
hokein marked an inline comment as done.Dec 14 2018, 4:39 AM

Thanks for the review.

This revision was automatically updated to reflect the committed changes.