[clangd] Use TargetOpts from preamble when building ASTs #88381
Conversation
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: kadir çetinkaya (kadircet) ChangesBuilding ASTs with compile flags that are incompatible to the ones used In an ideal world, we should be re-using not only TargetOpts, but the Unfortunately current contracts of ThreadSafeFS makes this a non-safe Full diff: https://github.com/llvm/llvm-project/pull/88381.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 3ff759415f7c8b..b6e784db4719fe 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -451,6 +451,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
DiagnosticConsumer *DiagConsumer = &ASTDiags;
IgnoreDiagnostics DropDiags;
if (Preamble) {
+ CI->TargetOpts = Preamble->TargetOpts;
Patch = PreamblePatch::createFullPatch(Filename, Inputs, *Preamble);
Patch->apply(*CI);
}
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index f181c7befec156..d579e90adc49df 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();
diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h
index 37da3833748a9c..160b884beb56bb 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/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
index 500b72b9b327a0..4bb76cd6ab8304 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
|
| @@ -451,6 +451,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, | |||
| DiagnosticConsumer *DiagConsumer = &ASTDiags; | |||
| IgnoreDiagnostics DropDiags; | |||
| if (Preamble) { | |||
| CI->TargetOpts = Preamble->TargetOpts; | |||
| Patch = PreamblePatch::createFullPatch(Filename, Inputs, *Preamble); | |||
There was a problem hiding this comment.
Idea: should we make this part of PreamblePatch?
It seems to fit the concept (take inputs from preamble, patch up compiler invocation based on it). Although I'm not sure if I'm misreading its intention.
There was a problem hiding this comment.
well, idea also crossed my mind while putting together the patch but wasn't so sure if that would be the best place. since preamble-patch'ing was mostly for adjusting the compilation to "patch" changes in the preamble contents.
but considering the fact that this actually needs to be done on all the places that re-uses a preamble (CodeCompletion, SignatureHelp), I guess PreamblePatch is a nice common ground (I was leaning towards prepareCompilerInstance but unfortunately that one can't depend on Preamble.h :/)
| @@ -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"; | |||
There was a problem hiding this comment.
I'm not sure if we'll hit this in practice, but this test will fail if the specified targets are disabled during builds.
We might need to guard it against wrong build configurations.
I'd wait until some buildbot fails, though, maybe we never actually run Clangd tests when the set of targets is small.
There was a problem hiding this comment.
yeah happy to wait and see if some build bots fail
That was my remembrance, but apparently it was wrong :/ Changed the logic to cover those cases as well && added some tests. |
|
All good from my side, but asked @sam-mccall or @usx95 to check the context propagation bit as I'm missing context to make sure it's correct. |
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.
|
@kadircet FYI, I noticed our Linux on Power down stream build failues related to this change. The upstream builds are also faling see https://lab.llvm.org/buildbot/#/builders/57/builds/34415 for example. This symbol is define in Can you have a look and try to fix it? |
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.