Page MenuHomePhabricator

[clangd] Discard diagnostics from another SourceManager.
ClosedPublic

Authored by adamcz on Aug 11 2020, 9:53 AM.

Details

Summary

This can happen when building implicit modules, as demonstrated in test.
The CompilerInstance uses the same StoredDiags, but different
SourceManager. This used to crash clangd when it tried to relocate the
diagnostic to the main file, which, according to SourceManager from the
diagnostic, is a fake <module-includes> file.

Diff Detail

Event Timeline

adamcz created this revision.Aug 11 2020, 9:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 9:53 AM
adamcz requested review of this revision.Aug 11 2020, 9:53 AM
adamcz updated this revision to Diff 284811.Aug 11 2020, 10:20 AM

log dropped dianostic

Hey Sam. What's your opinion on this?

The options we have are:

  1. Drop the diagnostics, like this change is doing
  2. Relocate the diagnostic to the beginning of the "real" main file, like we do with SourceManager-less diagnostics
  3. Try to find the place where we import the module and relocate the diagnostic there

Right now this change is just an example of approach 1. Not necessarily meant to be submitted as-is, since it silently drops possibly valuable diagnostics, although it does prevent a crash.

Option 3 sounds best, but it's very tricky. At the point HandleDiagnostic() is called, the module is not imported yet, so we would have to either delay the relocation until later or do some tricks with Preprocessor object (not sure how that would work).

Let me know what you think about this.

kadircet added inline comments.Aug 11 2020, 12:22 PM
clang-tools-extra/clangd/Diagnostics.cpp
209

can't we rather record the mainfilename once and make use of it here?
i believe the rest of the logic operates on the local sourcelocations/files associated with the sourcemanager inside the diagnostic in process.

adamcz added inline comments.Aug 12 2020, 4:35 AM
clang-tools-extra/clangd/Diagnostics.cpp
209

getMainFileRange() doesn't.

We can easily get main file from the original source manager or record it in BeginSourceFile. The thing is that it's not easy to find a place in the main file to put this diagnostic at. We can, of course, put it at the top of the file, but normally we try to find the #include location in the main file. That's done in the getMainFileRange() above and it mixes the location of the diagnostic (which is for the new SourceManager) and the main file buffer (which is for the OrigSrcMgr).

I'm a bit worried that if we put a diagnostic relating to #include at the top of the file, where another #include may be, it will become very confusing.

kadircet added inline comments.Aug 12 2020, 6:21 AM
clang-tools-extra/clangd/Diagnostics.cpp
209

getMainFileRange() doesn't.

ah right i forgot about that bit :/

I'm a bit worried that if we put a diagnostic relating to #include at the top of the file, where another #include may be, it will become very confusing.

I totally agree.

A crazy idea, we can register a callback to PP on BeginSourceFile to track file changes, than whenever we see a diagnostic from a different sourcemanager, we can map it back to last position a filechange was triggered. Ofc, this goes with the assumption that we see a filechanged event on #include "modular.h" line, but I guess we should at least get an includedirective callback or something.

adamcz added inline comments.Aug 12 2020, 6:31 AM
clang-tools-extra/clangd/Diagnostics.cpp
209

Right, that's a good idea, thanks. Sam suggested something like that in parallel, during in-person discussion too. I'll look into that in the near future.

There's the other issue of only seeing this diagnostics when a module is built, but not when a module was loaded from cache, so we will not be consistent unless we serialize them somehow.

sammccall accepted this revision.Aug 12 2020, 6:43 AM

Yeah I think this is the right approach for now, because the cached modules don't include diagnostics and ensuring that they're persisted and available is a larger project.

clang-tools-extra/clangd/Diagnostics.cpp
568

maybe a FIXME: errors from implicitly built modules should be surfaced somehow (but then must also be surfaced when the module was cached)

568

Can we be a bit more specific: this happens when an #include causes a module to be implicitly built, using a separate SourceManager.

clang-tools-extra/clangd/unittests/ModulesTests.cpp
44

nit: strict

This revision is now accepted and ready to land.Aug 12 2020, 6:43 AM
adamcz updated this revision to Diff 285961.Aug 17 2020, 4:26 AM
adamcz marked 3 inline comments as done.

addressed review comments

clang-tools-extra/clangd/Diagnostics.cpp
568

It's not just #include, could be "import". But yes, we can be more specific. Done.

This revision was automatically updated to reflect the committed changes.