llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: kadir çetinkaya (kadircet) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/88381.diff 4 Files Affected: - (modified) clang-tools-extra/clangd/ParsedAST.cpp (+1) - (modified) clang-tools-extra/clangd/Preamble.cpp (+1) - (modified) clang-tools-extra/clangd/Preamble.h (+5) - (modified) clang-tools-extra/clangd/unittests/ParsedASTTests.cpp (+32-7) ``````````diff 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 `````````` </details> https://github.com/llvm/llvm-project/pull/88381 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits