[clangd] IncludeCleaner: Do not process locations in built-in files

[kbobyrev]

Summary

Doing otherwise leads to crashing. Way to reproduce: open "gmock/gmock.h" in
the LLVM source tree.

[kbobyrev]

Also handle the Command-Line defines.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/IncludeCleaner.cpp

133

I couldn't figure out why macros defined via CLI are ending up in <built-in> file, despite having the right line directive in https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp#L1257. Looks like a bug in SourceManager. But still let's check for that too, just to be on the safe side (i.e. isWrittenInCommandLineFile)

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

245

you can change this to int y = CLI + __llvm__ to explicitly check for both CLI and built-in macros.

[kbobyrev]

Resolve review comments.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

245

Ahh, that's a nice built-in! Thank you!

[sammccall]

Sorry to be late to the party, just some efficiency concerns

clang-tools-extra/clangd/IncludeCleaner.cpp

133

This part of the function is a hot path: we haven't deduplicated FileIDs yet. And isWrittenInBulitinFile/CommandLineFile are not cheap (seriously, go look at the implementation of getPresumedLoc. I'm not sure *why* we need to handle the 'presumed' cases there, either).

I think the simple/good thing is to allow this to hit Files.insert(FID) and then filter those out later instead.

This could be in a simple walk over the DenseSet at the end, but translateToHeaderIDs is actually looking at the FileEntries for each header anyway. My guess is we're crashing in this code:

const FileEntry *FE = SM.getFileEntryForID(FID);
assert(FE); // this assert passes, we get a fake FE for "<built in>", "<command line>" or "<scratch>"
// Option 1: we could bail out here with a simple check on FE->getName().
const auto File = Includes.getID(FE);
assert(File); // this assert fails. Option 2: turn this assert into an if instead.

148

FWIW, the same seems to apply here (though at least we've deduplicated file IDs).

I don't think we need to do the expensive check to avoid adding scratch buffers to the list, when we can just filter them out at the end with a cheaper check.
(In any case I don't think it makes much sense to check for scratch *before* calling add(), but check for builtin/cli *inside* add()).

[sammccall]

I'll try some things out and send a patch...

[kbobyrev]

In D112608#3090910, @sammccall wrote:

Sorry to be late to the party, just some efficiency concerns

No problem, thank you for the comments, I'll send a follow-up!

clang-tools-extra/clangd/IncludeCleaner.cpp

133

Hmm, good idea, I'll resolve this in a follow-up!

P.S. Yes, we're crashing on assert(FE) in here.

148

Yeah, I was trying to figure out if this would apply to the macro expansions/spellings but couldn't figure out a reasonable example, so I left it out. I agree that it should probably be handled, too, I just wasn't sure when this could happen.

If we do post-filtering, I guess we can handle all of the locations at once, so probably safe to even remove these checks.

[kbobyrev]

In D112608#3090971, @sammccall wrote:

I'll try some things out and send a patch...

Oh, okay, I won't do mine then to avoid crashing into each other.

clang-tools-extra/clangd/IncludeCleaner.cpp

struct ReferencedFiles {
   ReferencedFiles(const SourceManager &SM) : SM(SM) {}
   llvm::DenseSet<FileID> Files;
   llvm::DenseSet<FileID> Macros;
   const SourceManager &SM;
 
   void add(SourceLocation Loc) { add(SM.getFileID(Loc), Loc); }
 
   void add(FileID FID, SourceLocation Loc) {
-    if (FID.isInvalid())
+    // Check if Loc is not written in a physical file.
+    if (FID.isInvalid() || SM.isWrittenInBuiltinFile(Loc) ||
+        SM.isWrittenInCommandLineFile(Loc))
       return;
     assert(SM.isInFileID(Loc, FID));
     if (Loc.isFileID()) {
       Files.insert(FID);
       return;
     }
     // Don't process the same macro FID twice.
     if (!Macros.insert(FID).second)
       return;
     const auto &Exp = SM.getSLocEntry(FID).getExpansion();
     // For token pasting operator in macros, spelling and expansion locations
     // can be within a temporary buffer that Clang creates (scratch space or
     // ScratchBuffer). That is not a real file we can include.
     if (!SM.isWrittenInScratchSpace(Exp.getSpellingLoc()))
       add(Exp.getSpellingLoc());
     if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocStart()))
       add(Exp.getExpansionLocStart());
     if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocEnd()))
       add(Exp.getExpansionLocEnd());
   }
 };
 

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

TEST(IncludeCleaner, GetUnusedHeaders) {
   std::vector<std::string> UnusedIncludes;
   for (const auto &Include : computeUnusedIncludes(AST))
     UnusedIncludes.push_back(Include->Written);
   EXPECT_THAT(UnusedIncludes,
               UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"",
                                    "<system_header.h>"));
 }
 
-TEST(IncludeCleaner, ScratchBuffer) {
+TEST(IncludeCleaner, VirtualBuffers) {
   TestTU TU;
   TU.Filename = "foo.cpp";
   TU.Code = R"cpp(
     #include "macro_spelling_in_scratch_buffer.h"
 
     using flags::FLAGS_FOO;
 
+    // CLI will come from a define, __llvm__ is a built-in. In both cases, they
+    // come from non-existent files.
+    int y = CLI + __llvm__;
+
     int concat(a, b) = 42;
     )cpp";
   // The pasting operator in combination with DEFINE_FLAG will create
   // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
   // FileEntry.
   TU.AdditionalFiles["macro_spelling_in_scratch_buffer.h"] = R"cpp(
     #define DEFINE_FLAG(X) \
     namespace flags { \
     int FLAGS_##X; \
     } \
 
     DEFINE_FLAG(FOO)
 
     #define ab x
     #define concat(x, y) x##y
     )cpp";
+  TU.ExtraArgs = {"-DCLI=42"};
   ParsedAST AST = TU.build();
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
   auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
   auto Entry = SM.getFileManager().getFile(
       testPath("macro_spelling_in_scratch_buffer.h"));
   ASSERT_TRUE(Entry);
   auto FID = SM.translateFile(*Entry);