https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/88381
From 233f413b5921ff23c171fa5ada5623ad1a8e3d82 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya <kadir...@google.com>
Date: Thu, 4 Apr 2024 10:57:44 +0200
Subject: [PATCH 1/2] [clangd] Propagate context into stdlib indexing thread
Some FS implementations rely on snapshots available in the context.
---
clang-tools-extra/clangd/ClangdServer.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp
b/clang-tools-extra/clangd/ClangdServer.cpp
index 5790273d625ef14..4eeed83b3e37516 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -112,7 +112,10 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
// Index outlives TUScheduler (declared first)
FIndex(FIndex),
// shared_ptr extends lifetime
- Stdlib(Stdlib)]() mutable {
+ Stdlib(Stdlib),
+ // We have some FS implementations that rely on infomration in
+ // the context.
+ Ctx(Context::current().clone())]() mutable {
clang::noteBottomOfStack();
IndexFileIn IF;
IF.Symbols = indexStandardLibrary(std::move(CI), Loc, *TFS);
From d396ab92ba577bdda05e43acb069de4aad60b083 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya <kadir...@google.com>
Date: Thu, 11 Apr 2024 14:02:43 +0200
Subject: [PATCH 2/2] [clangd] Use TargetOpts from preamble when building ASTs
Building ASTs with compile flags that are incompatible to the ones used
for the Preamble are not really supported by clang and can trigger
crashes.
In an ideal world, we should be re-using not only TargetOpts, but the
full ParseInputs from the Preamble to prevent such failures.
Unfortunately current contracts of ThreadSafeFS makes this a non-safe
change for certain implementations. As there are no guarantees that the
same ThreadSafeFS is going to be valid in the Context::current() we're
building the AST in.
---
clang-tools-extra/clangd/Preamble.cpp | 7 ++++
clang-tools-extra/clangd/Preamble.h | 5 +++
.../clangd/unittests/CodeCompleteTests.cpp | 25 ++++++++++++
.../clangd/unittests/ParsedASTTests.cpp | 39 +++++++++++++++----
4 files changed, 69 insertions(+), 7 deletions(-)
diff --git a/clang-tools-extra/clangd/Preamble.cpp
b/clang-tools-extra/clangd/Preamble.cpp
index f181c7befec156a..d5818e0ca309b03 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -700,6 +700,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
Result->Marks = CapturedInfo.takeMarks();
Result->StatCache = StatCache;
Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
+ Result->TargetOpts = CI.TargetOpts;
if (PreambleCallback) {
trace::Span Tracer("Running PreambleCallback");
auto Ctx = CapturedInfo.takeLife();
@@ -913,6 +914,12 @@ PreamblePatch
PreamblePatch::createMacroPatch(llvm::StringRef FileName,
}
void PreamblePatch::apply(CompilerInvocation &CI) const {
+ // Make sure the compilation uses same target opts as the preamble. Clang has
+ // no guarantees around using arbitrary options when reusing PCHs, and
+ // different target opts can result in crashes, see
+ // ParsedASTTest.PreambleWithDifferentTarget.
+ CI.TargetOpts = Baseline->TargetOpts;
+
// No need to map an empty file.
if (PatchContents.empty())
return;
diff --git a/clang-tools-extra/clangd/Preamble.h
b/clang-tools-extra/clangd/Preamble.h
index 37da3833748a9c6..160b884beb56bb7 100644
--- a/clang-tools-extra/clangd/Preamble.h
+++ b/clang-tools-extra/clangd/Preamble.h
@@ -30,6 +30,7 @@
#include "clang-include-cleaner/Record.h"
#include "support/Path.h"
#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetOptions.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PrecompiledPreamble.h"
#include "clang/Lex/Lexer.h"
@@ -97,6 +98,10 @@ struct PreambleData {
// Version of the ParseInputs this preamble was built from.
std::string Version;
tooling::CompileCommand CompileCommand;
+ // Target options used when building the preamble. Changes in target can
cause
+ // crashes when deserializing preamble, this enables consumers to use the
+ // same target (without reparsing CompileCommand).
+ std::shared_ptr<TargetOptions> TargetOpts = nullptr;
PrecompiledPreamble Preamble;
std::vector<Diag> Diags;
// Processes like code completions and go-to-definitions will need #include
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index 8fbac73cb653bcc..96d1ee1f0add735 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -4160,7 +4160,32 @@ TEST(CompletionTest, DoNotCrash) {
auto Completions = completions(Case);
}
}
+TEST(CompletionTest, PreambleFromDifferentTarget) {
+ constexpr std::string_view PreambleTarget = "x86_64";
+ constexpr std::string_view Contents =
+ "int foo(int); int num; int num2 = foo(n^";
+ Annotations Test(Contents);
+ auto TU = TestTU::withCode(Test.code());
+ TU.ExtraArgs.emplace_back("-target");
+ TU.ExtraArgs.emplace_back(PreambleTarget);
+ auto Preamble = TU.preamble();
+ ASSERT_TRUE(Preamble);
+ // Switch target to wasm.
+ TU.ExtraArgs.pop_back();
+ TU.ExtraArgs.emplace_back("wasm32");
+
+ MockFS FS;
+ auto Inputs = TU.inputs(FS);
+ auto Result = codeComplete(testPath(TU.Filename), Test.point(),
+ Preamble.get(), Inputs, {});
+ auto Signatures =
+ signatureHelp(testPath(TU.Filename), Test.point(), *Preamble, Inputs,
{});
+
+ // Make sure we don't crash.
+ EXPECT_THAT(Result.Completions, Not(testing::IsEmpty()));
+ EXPECT_THAT(Signatures.signatures, Not(testing::IsEmpty()));
+}
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
index 500b72b9b327a04..4bb76cd6ab8304a 100644
--- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -12,10 +12,7 @@
//===----------------------------------------------------------------------===//
#include "../../clang-tidy/ClangTidyCheck.h"
-#include "../../clang-tidy/ClangTidyModule.h"
-#include "../../clang-tidy/ClangTidyModuleRegistry.h"
#include "AST.h"
-#include "CompileCommands.h"
#include "Compiler.h"
#include "Config.h"
#include "Diagnostics.h"
@@ -32,7 +29,6 @@
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TokenKinds.h"
-#include "clang/Lex/PPCallbacks.h"
#include "clang/Tooling/Syntax/Tokens.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Testing/Annotations/Annotations.h"
@@ -41,6 +37,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <memory>
+#include <string_view>
#include <utility>
#include <vector>
@@ -347,9 +344,8 @@ TEST(ParsedASTTest, CollectsMainFileMacroExpansions) {
}
for (const auto &R : AST.getMacros().UnknownMacros)
MacroExpansionPositions.push_back(R.StartOffset);
- EXPECT_THAT(
- MacroExpansionPositions,
- testing::UnorderedElementsAreArray(TestCase.points()));
+ EXPECT_THAT(MacroExpansionPositions,
+ testing::UnorderedElementsAreArray(TestCase.points()));
}
MATCHER_P(withFileName, Inc, "") { return arg.FileName == Inc; }
@@ -768,6 +764,35 @@ TEST(ParsedASTTest, GracefulFailureOnAssemblyFile) {
<< "Should not try to build AST for assembly source file";
}
+TEST(ParsedASTTest, PreambleWithDifferentTarget) {
+ constexpr std::string_view kPreambleTarget = "x86_64";
+ // Specifically picking __builtin_va_list as it triggers crashes when
+ // switching to wasm.
+ // It's due to different predefined types in different targets.
+ auto TU = TestTU::withHeaderCode("void foo(__builtin_va_list);");
+ TU.Code = "void bar() { foo(2); }";
+ TU.ExtraArgs.emplace_back("-target");
+ TU.ExtraArgs.emplace_back(kPreambleTarget);
+ const auto Preamble = TU.preamble();
+
+ // Switch target to wasm.
+ TU.ExtraArgs.pop_back();
+ TU.ExtraArgs.emplace_back("wasm32");
+
+ IgnoreDiagnostics Diags;
+ MockFS FS;
+ auto Inputs = TU.inputs(FS);
+ auto CI = buildCompilerInvocation(Inputs, Diags);
+ ASSERT_TRUE(CI) << "Failed to build compiler invocation";
+
+ auto AST = ParsedAST::build(testPath(TU.Filename), std::move(Inputs),
+ std::move(CI), {}, Preamble);
+
+ ASSERT_TRUE(AST);
+ // We use the target from preamble, not with the most-recent flags.
+ EXPECT_EQ(AST->getASTContext().getTargetInfo().getTriple().getArchName(),
+ llvm::StringRef(kPreambleTarget));
+}
} // namespace
} // namespace clangd
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
