This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix clangd crash when including a header
ClosedPublic

Authored by qdelacru on Aug 13 2021, 10:44 AM.

Details

Summary

Fixes https://github.com/clangd/clangd/issues/819

SourceLocation of macros change when a header file is included above it. This is not checked when creating a PreamblePatch, resulting in reusing previously built preamble with an incorrect source location for the macro in the example test case.
This patch stores the SourceLocation in the struct TextualPPDirective so that it gets checked when comparing old vs new preambles.

Also creates a preamble patch for code completion parsing so that clangd does not crash when following the example test case with a large file.

Diff Detail

Event Timeline

qdelacru created this revision.Aug 13 2021, 10:44 AM
qdelacru requested review of this revision.Aug 13 2021, 10:44 AM
qdelacru updated this revision to Diff 366358.Aug 13 2021, 2:51 PM

Thanks, I can see the problem now (sorry for the late reply, i was on leave last week).

It is amazing that this hasn't bitten us yet during code complete flow (it probably did, but clangd would recover after restart so probably people didn't notice).
I still don't think that we'd like to parse all the new includes during code completion flow hence we should just create a preamble patch for the macro directives that moved around. (I don't think we should make the parser more resilient instead).
Do you mind adding a new factory function to PreamblePatch for creating a macro-directive only patch that can be used in CodeComplete flow to prevent crashes (while paying for some extra latency :/ )?

As for storing source locations, i think we can just store the offsets (we already have them in spellDirective when we decompose sourcelocation.

Does that make sense?

clang-tools-extra/clangd/Preamble.cpp
511

this is the condition i am suggesting to drop.

clang-tools-extra/clangd/unittests/PreambleTests.cpp
553

we actually have a more problematic case, you can just insert whitespace to the first line rather than the include directive. now all the offsets are wrong but there's nothing to patch.

qdelacru updated this revision to Diff 369581.Aug 30 2021, 5:19 PM

Create macro directive only preamble patch for code completion

kadircet added inline comments.Sep 3 2021, 2:34 AM
clang-tools-extra/clangd/Preamble.cpp
148

nit: i'd put offset before text, since it's faster to compare.

533

sorry for not being explicit about this on the first round but rather than duplicating the logic, can we have a shared private factory function that takes an extra parameter to control whether includes/macros should be included in the patch? It can probably look something like this:

class PreamblePatch {
public:
  enum class PatchType {
     MacroDirectives,
     All,
  };
  // these two will just call the create with relevant patchtype
  static PreamblePatch createFullPatch(FileName, Modified, Baseline);
  static PreamblePatch createMacroPatch(FileName, Modified, Baseline);
private:
  static PreamblePatch create(filename, Modified, Baseline, PatchType);
}
qdelacru updated this revision to Diff 372005.Sep 10 2021, 5:02 PM
kadircet accepted this revision.Sep 14 2021, 12:34 AM

Thanks a lot, LGTM!

Do you want me to land this for you?

This revision is now accepted and ready to land.Sep 14 2021, 12:34 AM

Yes please land this. Thanks!

This revision was automatically updated to reflect the committed changes.