[clangd] Handle PresumedLocations in IncludeCollector

[kadircet]

Summary

This will enable extraction of correct line locations in preamble patch
for includes.

[sammccall]

clang-tools-extra/clangd/Headers.cpp

46

Is it possible that doing this for every include we process in the preamble might be significantly expensive?

You could consider doing something like:

FileID BuiltinFile;
bool InBuiltin;
void FileChanged(Loc, Reason, _, PrevFID) override {
  if (Reason == EnterFile && !BuiltinFile && SM.isWrittenInBuiltInFile(Loc)) {
    BuiltinFile = SM.getFileID(Loc);
    InBuiltin = true;
  } else if (Reason == ExitFile && InBuiltinFile && BuiltinFile == PrevFID) {
    InBuiltin = false;
  }
}

and then only run this for InBuiltin. (In practice fairly soon during processing, BuiltinFile will be set and InBuiltin will have become false, so all of this will be short-circuited)

47

wouldn't be SM.getFileEntryForID(PreLoc.FID) be more efficient?

49

could use a comment to explain what's going on at this point: we're transforming the parameters so we can process this as if it occurred in the main file.

But actually it might be clearer to extract a function that both cases can call.

51

This part looks a little iffy to me, with all the coordinate transforms.

If we're synthesizing the include, chars don't have to match 1:1 right?
e.g. if the original code was # include /* foo */ "bar.h" // baz and we synthesize #include "bar.h", how is this going to get the coordinates of "bar.h" right?

This seems awkward to resolve. R isn't actually used much though, go-to-definition looks at its line number only, and DocumentLink uses it (but it seems OK to just to do approximate re-lexing there). Maybe we can just drop it?

---

(Original comment disregarding above problem)

Isn't it the case that the filename expansion location has to be in the same file as the hashloc?
So can we do something like:

FilenameRange = SM.getExpansionRange(FilenameRange);
if (SM.getFileID(FilenameRange.start()) == SM.getFileID(FilenameRange.end()) == SM.getFileID(OrigHashLoc)) {
  // all in the same file
  // compute NewStart = OrigStart - OrigHashLoc + NewHashLoc, etc
} else {
  FilenameRange = CharSourceRange();
}

[kadircet]

clang-tools-extra/clangd/Headers.cpp

47

well, that would've been even easier to just check for PreLoc.FID == SM.getMainFileID(), but unfortunately PresumedLoc.FID is invalid in presence of line directives :/

but we can do something like storing the MainFilePath and then just compare the PreLoc.getFileName with it.

51

This part looks a little iffy to me, with all the coordinate transforms.

If we're synthesizing the include, chars don't have to match 1:1 right?
e.g. if the original code was # include /* foo */ "bar.h" // baz and we synthesize #include "bar.h", how is this going to get the coordinates of "bar.h" right?

well, the patching actually ensures both # and "filename" starts at the correct offset, by padding them with whitespaces ignoring any comments and such.

This seems awkward to resolve. R isn't actually used much though, go-to-definition looks at its line number only, and DocumentLink uses it (but it seems OK to just to do approximate re-lexing there). Maybe we can just drop it?

I am fine with dropping it too, the padding looks really ugly in the patching code :D.

Regarding go-to-def, I suppose we can keep storing the include line, since we calculate it anyway while getting the presumed location for HashLoc.

For DocumentLink, I suppose we can either lex while handling the request or store those separately in parsedast. I would go with the former.

WDYT?

[kadircet]

clang-tools-extra/clangd/Headers.cpp

51

sent out D79315.

[kadircet]

• Perform line mapping only for includes found through built-in file.
• Drop filename range mapping.
• Make use of FileChanged events to track being in built-in file.

[sammccall]

LG, Just readability stuff.

clang-tools-extra/clangd/Headers.cpp

61

nit: Inc -> Include

61

this structure is not terrible but the reverse-chronological-order hurts readability a bit.

You could try:

// If an include is part of the preamble patch, translate #line directives.
if (BuiltinFileInStack) {
  auto PreLoc = ...;
  if (...) {
    HashLoc = ...; // now we'll hit the case below
  }
}
// Record main-file inclusions (including those mapped from the preamble patch).
if (isInsideMainFile(HashLoc, SM)) {
  // add the include
}

75

maybe delimit this with a comment like
// Record include graph (not just for main-file includes)

76

can -> might, line -> #line, ... as part of a preamble patch
(just to hint a little more clearly who created the mapping)

78

PreLoc -> Presumed?
(these abbreviations aren't common)

79

For clarity I think we could do this only with an invalid file ID and pull out a function, ending up with:

if (Presumed.getFileID().isInvalid() && isMainFile(Presumed.getFilename()))
  AddMainFileInc(HashLoc);

86

nit: I'd find it slightly clearer to handle this case first in the if/else, as it kind of "motivates" the more complicated one.

113

Using the name stack for a boolean was pretty confusing to me. I think InBuiltinFile plus the comment would be enough, otherwise maybe UnderBuiltinFile or so?

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

309

I don't think testPath is needed here: it should be on the include path, and FS.Files adds the qualifiers where needed

314

Can we use more aribtrary placeholder here, like 42? :)
We could easily hit 1 by mistake.

324

changing plus to comma here seems clearer

[kadircet]

• Address comments

clang-tools-extra/clangd/Headers.cpp

61

well this was the initial version :D moved away from it since you asked for an explicit function both cases can call, reverting back to it.

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

314

unfortunately we can't, at least not without having that line available in the main file. Since we need to get a real offset into it in the end.

changing it to be line 3 as a middle ground :P

[kadircet]

• Try to fix windows builds

[kadircet]

• use basename in tests as having unescaped backslashes breaks windows build bots. going to address that separetely in line directive generation logic.

clang-tools-extra/clangd/Headers.cpp

 //===--- Headers.cpp - Include headers ---------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Headers.h"
 #include "Compiler.h"
 #include "SourceCode.h"
 #include "support/Logger.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
+bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) {
+  auto FE = SM.getFileManager().getFile(FileName);
+  return FE && *FE == SM.getFileEntryForID(SM.getMainFileID());
+}
+
 class RecordHeaders : public PPCallbacks {
 public:
   RecordHeaders(const SourceManager &SM, IncludeStructure *Out)
       : SM(SM), Out(Out) {}
 
   // Record existing #includes - both written and resolved paths. Only #includes
   // in the main file are collected.
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           llvm::StringRef FileName, bool IsAngled,
-                          CharSourceRange FilenameRange, const FileEntry *File,
-                          llvm::StringRef /*SearchPath*/,
+                          CharSourceRange /*FilenameRange*/,
+                          const FileEntry *File, llvm::StringRef /*SearchPath*/,
                           llvm::StringRef /*RelativePath*/,
                           const Module * /*Imported*/,
                           SrcMgr::CharacteristicKind FileKind) override {
+    auto MainFID = SM.getMainFileID();
+    // If an include is part of the preamble patch, translate #line directives.
+    if (InBuiltinFile) {
+      auto Presumed = SM.getPresumedLoc(HashLoc);
+      // Presumed locations will have an invalid file id when #line directive
+      // changes the filename.
+      if (Presumed.getFileID().isInvalid() &&
+          isMainFile(Presumed.getFilename(), SM)) {
+        // Now we'll hit the case below.
+        HashLoc = SM.translateLineCol(MainFID, Presumed.getLine(),
+                                      Presumed.getColumn());
+      }
+    }
+
+    // Record main-file inclusions (including those mapped from the preamble
+    // patch).
     if (isInsideMainFile(HashLoc, SM)) {
       Out->MainFileIncludes.emplace_back();
       auto &Inc = Out->MainFileIncludes.back();
       Inc.Written =
           (IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str();
       Inc.Resolved = std::string(File ? File->tryGetRealPathName() : "");
       Inc.HashOffset = SM.getFileOffset(HashLoc);
       Inc.HashLine =
           SM.getLineNumber(SM.getFileID(HashLoc), Inc.HashOffset) - 1;
       Inc.FileKind = FileKind;
       Inc.Directive = IncludeTok.getIdentifierInfo()->getPPKeywordID();
     }
+
+    // Record include graph (not just for main-file includes)
     if (File) {
       auto *IncludingFileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc));
       if (!IncludingFileEntry) {
         assert(SM.getBufferName(HashLoc).startswith("<") &&
                "Expected #include location to be a file or <built-in>");
         // Treat as if included from the main file.
-        IncludingFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+        IncludingFileEntry = SM.getFileEntryForID(MainFID);
       }
       Out->recordInclude(IncludingFileEntry->getName(), File->getName(),
                          File->tryGetRealPathName());
     }
   }
 
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                   SrcMgr::CharacteristicKind FileType,
+                   FileID PrevFID) override {
+    switch (Reason) {
+    case PPCallbacks::EnterFile:
+      if (BuiltinFile.isInvalid() && SM.isWrittenInBuiltinFile(Loc)) {
+        BuiltinFile = SM.getFileID(Loc);
+        InBuiltinFile = true;
+      }
+      break;
+    case PPCallbacks::ExitFile:
+      if (PrevFID == BuiltinFile)
+        InBuiltinFile = false;
+      break;
+    case PPCallbacks::RenameFile:
+    case PPCallbacks::SystemHeaderPragma:
+      break;
+    }
+  }
+
 private:
   const SourceManager &SM;
+  // Set after entering the <built-in> file.
+  FileID BuiltinFile;
+  // Indicates whether <built-in> file is part of include stack.
+  bool InBuiltinFile = false;
+
   IncludeStructure *Out;
 };
 
 } // namespace
 
 bool isLiteralInclude(llvm::StringRef Include) {
   return Include.startswith("<") || Include.startswith("\"");
 }

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

 #include "Compiler.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
+using ::testing::Not;
 using ::testing::UnorderedElementsAre;
 
 class HeadersTest : public ::testing::Test {
 public:
   HeadersTest() {
     CDB.ExtraClangFlags = {SearchDirArg.c_str()};
     FS.Files[MainFile] = "";
     // Make sure directory sub/ exists.
TEST(Headers, NoHeaderSearchInfo) {
   EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile),
             std::string("<x>"));
   EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
 
   EXPECT_EQ(Inserter.calculateIncludePath(Inserting, "sub2/main2.cpp"),
             llvm::None);
 }
 
+TEST_F(HeadersTest, PresumedLocations) {
+  std::string HeaderFile = "implicit_include.h";
+
+  // Line map inclusion back to main file.
+  std::string HeaderContents =
+      llvm::formatv("#line 0 \"{0}\"", llvm::sys::path::filename(MainFile));
+  HeaderContents += R"cpp(
+#line 3
+#include <a.h>)cpp";
+  FS.Files[HeaderFile] = HeaderContents;
+
+  // Including through non-builtin file has no effects.
+  FS.Files[MainFile] = "#include \"implicit_include.h\"\n\n";
+  EXPECT_THAT(collectIncludes().MainFileIncludes,
+              Not(Contains(Written("<a.h>"))));
+
+  // Now include through built-in file.
+  CDB.ExtraClangFlags = {"-include", testPath(HeaderFile)};
+  EXPECT_THAT(collectIncludes().MainFileIncludes,
+              Contains(AllOf(IncludeLine(2), Written("<a.h>"))));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang