This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Avoid relations being overwritten in a header shard
ClosedPublic

Authored by nridge on Sep 7 2020, 3:56 PM.

Diff Detail

Event Timeline

nridge created this revision.Sep 7 2020, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2020, 3:56 PM
nridge requested review of this revision.Sep 7 2020, 3:56 PM

+@kadircet, he is tracking on this -- we had some discussion internally last week, but we don't reply on that thread yet (sorry).

nridge updated this revision to Diff 293045.Sep 20 2020, 5:23 PM

Address review comments

kadircet added inline comments.Sep 21 2020, 1:30 AM
clang-tools-extra/clangd/index/FileIndex.cpp
151

instead of (or in addition to) providing the link, can we give a short summary? e.g.

Subject and/or Object files might be part of multiple TUs. In such cases there will be a race
and last TU to write the shard will win and all the other relations will be lost. We are storing
relations in both places, as we do for symbols, to reduce such races. Note that they might still
happen if same translation unit produces different relations under different configurations
but that's something clangd doesn't handle in general.
348

can you move this outside of the for loop, so that we do it only once.

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
232

do we still need this test?

395

s/ShardSource/ShardHeader

clang-tools-extra/clangd/unittests/FileIndexTests.cpp
571–572

maybe mention that Sym1 belongs to a.h in the comment. stored in a.h (where Sym1 is stored) even tho the reference is dangling as Sym3 is unknown

nridge updated this revision to Diff 294577.Sep 27 2020, 4:57 PM
nridge marked 5 inline comments as done.

Address review comments

kadircet added inline comments.Oct 2 2020, 2:01 AM
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
232

this one was marked as resolved but i still don't see the reasoning. can we test this in fileindextests instead?

we already test the sharding logic, we need to test the merging logic now. can we rather test it at FileSymbols layer directly? or is there something extra that i am misisng?

nridge added a comment.Oct 2 2020, 8:13 AM

Sorry, I had a response to that comment but accidentally left it as unsubmitted...

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
232

I think it's still useful, yes. If someone makes a change to the way relations are stored in the future, they could regress this use case without a test specifically for it.

kadircet accepted this revision.Oct 5 2020, 1:56 AM

thanks! (and sorry for taking too long on this one.)

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
232

okay, i still think it is better to test on FileSymbols layer but not that important.

This revision is now accepted and ready to land.Oct 5 2020, 1:56 AM
nridge updated this revision to Diff 297482.Oct 11 2020, 12:24 PM

Slightly simplified test (base class does not need to be a template to trigger the issue)

nridge marked an inline comment as done.Oct 11 2020, 12:31 PM
nridge added inline comments.
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
232

The reason I'd like to have a test at this layer is so that we have a test that closely approximates the steps to reproduce: create files with certain contents and inclusion relationships between them, index them, and perform a particular query. I feel like internal abstractions like FileSymbols are liable to change over time through refactorings, and so tests written against them are less robust.

Note, I don't think using BackgroundIndex is important for the kind of test I'd like; FileIndex would be fine too. However, I could not see how to use FileIndex with files that contain #include statements (didn't see a way to provide a MockFS or another way to get the includes to be resolved).

That said, I did try to write an additional test using FileSymbols, but I found that:

  • it only exercises the merging logic, not the sharding logic
  • I couldn't get the test to fail even if I omit the merging logic of this patch, because both MemIndex and Dex build a DenseMap for relations and therefore end up implicitly deduplicating anyways
This revision was landed with ongoing or failed builds.Oct 11 2020, 12:33 PM
This revision was automatically updated to reflect the committed changes.