[clang-tools-extra] 34e39eb - [clangd] Change PreambleOnlyAction with content truncation

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2020-05-26T07:37:03+02:00
New Revision: 34e39eb2adc2b3f16c2c2c0607a904ee55705c01

URL: https://github.com/llvm/llvm-project/commit/34e39eb2adc2b3f16c2c2c0607a904ee55705c01
DIFF: https://github.com/llvm/llvm-project/commit/34e39eb2adc2b3f16c2c2c0607a904ee55705c01.diff

LOG: [clangd] Change PreambleOnlyAction with content truncation

Summary:
Lexing until the token location is past preamble bound could be wrong
in some cases as preprocessor lexer can lex multiple tokens in a single call.

Reviewers: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D79426

Added: 
    

Modified: 
    clang-tools-extra/clangd/Preamble.cpp
    clang-tools-extra/clangd/unittests/PreambleTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 9d9c5eff8c68..d3eaa92d4c1a 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -104,24 +104,6 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
   const SourceManager *SourceMgr = nullptr;
 };
 
-// Runs preprocessor over preamble section.
-class PreambleOnlyAction : public PreprocessorFrontendAction {
-protected:
-  void ExecuteAction() override {
-    Preprocessor &PP = getCompilerInstance().getPreprocessor();
-    auto &SM = PP.getSourceManager();
-    PP.EnterMainSourceFile();
-    auto Bounds = ComputePreambleBounds(getCompilerInstance().getLangOpts(),
-                                        SM.getBuffer(SM.getMainFileID()), 0);
-    Token Tok;
-    do {
-      PP.Lex(Tok);
-      assert(SM.isInMainFile(Tok.getLocation()));
-    } while (Tok.isNot(tok::eof) &&
-             SM.getDecomposedLoc(Tok.getLocation()).second < Bounds.Size);
-  }
-};
-
 /// Gets the includes in the preamble section of the file by running
 /// preprocessor over \p Contents. Returned includes do not contain resolved
 /// paths. \p VFS and \p Cmd is used to build the compiler invocation, which
@@ -142,8 +124,15 @@ scanPreambleIncludes(llvm::StringRef Contents,
                                    "failed to create compiler invocation");
   CI->getDiagnosticOpts().IgnoreWarnings = true;
   auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents);
+  // This means we're scanning (though not preprocessing) the preamble section
+  // twice. However, it's important to precisely follow the preamble bounds used
+  // elsewhere.
+  auto Bounds =
+      ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
+  auto PreambleContents =
+      llvm::MemoryBuffer::getMemBufferCopy(Contents.substr(0, Bounds.Size));
   auto Clang = prepareCompilerInstance(
-      std::move(CI), nullptr, std::move(ContentsBuffer),
+      std::move(CI), nullptr, std::move(PreambleContents),
       // Provide an empty FS to prevent preprocessor from performing IO. This
       // also implies missing resolved paths for includes.
       new llvm::vfs::InMemoryFileSystem, IgnoreDiags);
@@ -152,7 +141,7 @@ scanPreambleIncludes(llvm::StringRef Contents,
                                    "compiler instance had no inputs");
   // We are only interested in main file includes.
   Clang->getPreprocessorOpts().SingleFileParseMode = true;
-  PreambleOnlyAction Action;
+  PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]))
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "failed BeginSourceFile");

diff  --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
index c1801980b1d5..db615e6e66e1 100644
--- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -118,6 +118,11 @@ TEST(PreamblePatchTest, IncludeParsing) {
         ^#include "a.h"
         #include <b
         ^#include <b.h>)cpp",
+      // Directive is not part of preamble if it is not the token immediately
+      // followed by the hash (#).
+      R"cpp(
+        ^#include "a.h"
+        #/**/include <b.h>)cpp",
   };
 
   for (const auto Case : Cases) {