[clang-tools-extra] r333370 - [clangd] Fix leak sanitizers warnings in clangd

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Mon May 28 05:11:37 2018
New Revision: 333370

URL: http://llvm.org/viewvc/llvm-project?rev=333370&view=rev
Log:
[clangd] Fix leak sanitizers  warnings in clangd

The commit includes two changes:
1. Set DisableFree to false when building the ParsedAST.
   This is sane default, since clangd never wants to leak the AST.
2. Make sure CompilerInstance created in code completion is passed to
   the FrontendAction::BeginSourceFile call.
   We have to do this to make sure the memory buffers of remapped
   files are properly freed.

Our tests do not produce any warnings under asan anymore.
The changes are mostly trivial, just moving the code around. So
sending without review.

Modified:
    clang-tools-extra/trunk/clangd/ClangdUnit.cpp
    clang-tools-extra/trunk/clangd/CodeComplete.cpp
    clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=333370&r1=333369&r2=333370&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Mon May 28 05:11:37 2018
@@ -153,6 +153,10 @@ ParsedAST::Build(std::unique_ptr<clang::
                  std::unique_ptr<llvm::MemoryBuffer> Buffer,
                  std::shared_ptr<PCHContainerOperations> PCHs,
                  IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
+  assert(CI);
+  // Command-line parsing sets DisableFree to true by default, but we don't want
+  // to leak memory in clangd.
+  CI->getFrontendOpts().DisableFree = false;
   const PrecompiledPreamble *PreamblePCH =
       Preamble ? &Preamble->Preamble : nullptr;
 

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=333370&r1=333369&r2=333370&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Mon May 28 05:11:37 2018
@@ -699,26 +699,13 @@ bool semaCodeComplete(std::unique_ptr<Co
     log("Couldn't create CompilerInvocation");;
     return false;
   }
-  CI->getFrontendOpts().DisableFree = false;
+  auto &FrontendOpts = CI->getFrontendOpts();
+  FrontendOpts.DisableFree = false;
+  FrontendOpts.SkipFunctionBodies = true;
   CI->getLangOpts()->CommentOpts.ParseAllComments = true;
-
-  std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
-      llvm::MemoryBuffer::getMemBufferCopy(Input.Contents, Input.FileName);
-
-  // The diagnostic options must be set before creating a CompilerInstance.
-  CI->getDiagnosticOpts().IgnoreWarnings = true;
-  // We reuse the preamble whether it's valid or not. This is a
-  // correctness/performance tradeoff: building without a preamble is slow, and
-  // completion is latency-sensitive.
-  auto Clang = prepareCompilerInstance(
-      std::move(CI), Input.Preamble, std::move(ContentsBuffer),
-      std::move(Input.PCHs), std::move(Input.VFS), DummyDiagsConsumer);
-
   // Disable typo correction in Sema.
-  Clang->getLangOpts().SpellChecking = false;
-
-  auto &FrontendOpts = Clang->getFrontendOpts();
-  FrontendOpts.SkipFunctionBodies = true;
+  CI->getLangOpts()->SpellChecking = false;
+  // Setup code completion.
   FrontendOpts.CodeCompleteOpts = Options;
   FrontendOpts.CodeCompletionAt.FileName = Input.FileName;
   auto Offset = positionToOffset(Input.Contents, Input.Pos);
@@ -731,6 +718,18 @@ bool semaCodeComplete(std::unique_ptr<Co
            FrontendOpts.CodeCompletionAt.Column) =
       offsetToClangLineColumn(Input.Contents, *Offset);
 
+  std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
+      llvm::MemoryBuffer::getMemBufferCopy(Input.Contents, Input.FileName);
+  // The diagnostic options must be set before creating a CompilerInstance.
+  CI->getDiagnosticOpts().IgnoreWarnings = true;
+  // We reuse the preamble whether it's valid or not. This is a
+  // correctness/performance tradeoff: building without a preamble is slow, and
+  // completion is latency-sensitive.
+  // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
+  // the remapped buffers do not get freed.
+  auto Clang = prepareCompilerInstance(
+      std::move(CI), Input.Preamble, std::move(ContentsBuffer),
+      std::move(Input.PCHs), std::move(Input.VFS), DummyDiagsConsumer);
   Clang->setCodeCompletionConsumer(Consumer.release());
 
   SyntaxOnlyAction Action;

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=333370&r1=333369&r2=333370&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Mon May 28 05:11:37 2018
@@ -18,6 +18,7 @@
 #include "TestFS.h"
 #include "index/MemIndex.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -1040,6 +1041,25 @@ TEST(CompletionTest, DocumentationFromCh
               Contains(AllOf(Not(IsDocumented()), Named("func"))));
 }
 
+TEST(CompletionTest, CompleteOnInvalidLine) {
+  auto FooCpp = testPath("foo.cpp");
+
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  MockFSProvider FS;
+  FS.Files[FooCpp] = "// empty file";
+
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  // Run completion outside the file range.
+  Position Pos;
+  Pos.line = 100;
+  Pos.character = 0;
+  EXPECT_THAT_EXPECTED(
+      runCodeComplete(Server, FooCpp, Pos, clangd::CodeCompleteOptions()),
+      Failed());
+}
+
+
 } // namespace
 } // namespace clangd
 } // namespace clang