[clang] Hide the `TargetOptions` pointer from `CompilerInvocation` #106271

jansvoboda11 · 2024-08-27T18:54:47Z

This PR hides the reference-counted pointer that holds TargetOptions from the public API of CompilerInvocation. This gives CompilerInvocation an exclusive control over the lifetime of this member, which will eventually be leveraged to implement a copy-on-write behavior.

There are two clients that currently share ownership of that pointer:

TargetInfo - This was refactored to hold a non-owning reference to TargetOptions. The options object is typically owned by the CompilerInvocation or by the new CompilerInstance::AuxTargetOpts for the auxiliary target. This needed a bit of care in ASTUnit::Parse() to keep the CompilerInvocation alive.
clangd::PreambleData - This was refactored to exclusively own the TargetOptions that get moved out of the CompilerInvocation.

llvmbot · 2024-08-27T18:55:20Z

@llvm/pr-subscribers-lldb
@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Jan Svoboda (jansvoboda11)

Changes

This PR hides the reference-counter pointer that holds TargetOptions from the public API of CompilerInvocation. This gives CompilerInvocation an exclusive control over the lifetime of this member, which will eventually be leveraged to implement a copy-on-write behavior.

There are two clients that currently share ownership of that pointer:

TargetInfo - This was refactored to hold a non-owning reference to TargetOptions. The options object is typically owned by the CompilerInvocation or by the new CompilerInstance::AuxTargetOpts for the auxiliary target. This needed a bit of care in ASTUnit::Parse() to keep the CompilerInvocation alive.
clangd::PreambleData - This was refactored to exclusively own the TargetOptions that get moved out of the CompilerInvocation.

Full diff: https://github.com/llvm/llvm-project/pull/106271.diff

25 Files Affected:

(modified) clang-tools-extra/clangd/Preamble.cpp (+5-2)
(modified) clang-tools-extra/clangd/Preamble.h (+1-1)
(modified) clang-tools-extra/clangd/SystemIncludeExtractor.cpp (+1-1)
(modified) clang-tools-extra/modularize/ModularizeUtilities.cpp (+1-1)
(modified) clang/include/clang/Basic/TargetInfo.h (+3-4)
(modified) clang/include/clang/Frontend/ASTUnit.h (+4)
(modified) clang/include/clang/Frontend/CompilerInstance.h (+3)
(modified) clang/include/clang/Frontend/CompilerInvocation.h (-1)
(modified) clang/lib/Basic/Targets.cpp (+4-3)
(modified) clang/lib/Frontend/ASTUnit.cpp (+5-1)
(modified) clang/lib/Frontend/ChainedIncludesSource.cpp (+1-1)
(modified) clang/lib/Frontend/CompilerInstance.cpp (+3-3)
(modified) clang/lib/Interpreter/Interpreter.cpp (+1-1)
(modified) clang/tools/clang-import-test/clang-import-test.cpp (+1-1)
(modified) clang/unittests/Analysis/MacroExpansionContextTest.cpp (+1-1)
(modified) clang/unittests/Basic/SourceManagerTest.cpp (+1-1)
(modified) clang/unittests/CodeGen/TestCompiler.h (+1-2)
(modified) clang/unittests/Frontend/UtilsTest.cpp (+1-1)
(modified) clang/unittests/Lex/HeaderSearchTest.cpp (+1-1)
(modified) clang/unittests/Lex/LexerTest.cpp (+1-1)
(modified) clang/unittests/Lex/ModuleDeclStateTest.cpp (+1-1)
(modified) clang/unittests/Lex/PPCallbacksTest.cpp (+1-1)
(modified) clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp (+1-1)
(modified) clang/unittests/Lex/PPDependencyDirectivesTest.cpp (+1-1)
(modified) clang/unittests/Lex/PPMemoryAllocationsTest.cpp (+1-1)

diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index dd13b1a9e5613d..59917c9b64ecd9 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -714,7 +714,10 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
     Result->Marks = CapturedInfo.takeMarks();
     Result->StatCache = StatCache;
     Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
-    Result->TargetOpts = CI.TargetOpts;
+    // Move the options instead of copying them. The invocation doesn't need
+    // them anymore.
+    Result->TargetOpts =
+        std::make_unique<TargetOptions>(std::move(CI.getTargetOpts()));
     if (PreambleCallback) {
       trace::Span Tracer("Running PreambleCallback");
       auto Ctx = CapturedInfo.takeLife();
@@ -936,7 +939,7 @@ void PreamblePatch::apply(CompilerInvocation &CI) const {
   // ParsedASTTest.PreambleWithDifferentTarget.
   // Make sure this is a deep copy, as the same Baseline might be used
   // concurrently.
-  *CI.TargetOpts = *Baseline->TargetOpts;
+  CI.getTargetOpts() = *Baseline->TargetOpts;
 
   // No need to map an empty file.
   if (PatchContents.empty())
diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h
index be8fed4ab88cdd..7f2eb233ee1aa4 100644
--- a/clang-tools-extra/clangd/Preamble.h
+++ b/clang-tools-extra/clangd/Preamble.h
@@ -103,7 +103,7 @@ struct PreambleData {
   // 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;
+  std::unique_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/SystemIncludeExtractor.cpp b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
index d4b9b173d149da..58ddc29ae33b7c 100644
--- a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
+++ b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
@@ -256,7 +256,7 @@ bool isValidTarget(llvm::StringRef Triple) {
   DiagnosticsEngine Diags(new DiagnosticIDs, new DiagnosticOptions,
                           new IgnoringDiagConsumer);
   llvm::IntrusiveRefCntPtr<TargetInfo> Target =
-      TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+      TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   return bool(Target);
 }
 
diff --git a/clang-tools-extra/modularize/ModularizeUtilities.cpp b/clang-tools-extra/modularize/ModularizeUtilities.cpp
index b202b3aae8f8a3..517d7454d161b8 100644
--- a/clang-tools-extra/modularize/ModularizeUtilities.cpp
+++ b/clang-tools-extra/modularize/ModularizeUtilities.cpp
@@ -53,7 +53,7 @@ ModularizeUtilities::ModularizeUtilities(std::vector<std::string> &InputPaths,
       Diagnostics(
           new DiagnosticsEngine(DiagIDs, DiagnosticOpts.get(), &DC, false)),
       TargetOpts(new ModuleMapTargetOptions()),
-      Target(TargetInfo::CreateTargetInfo(*Diagnostics, TargetOpts)),
+      Target(TargetInfo::CreateTargetInfo(*Diagnostics, *TargetOpts)),
       FileMgr(new FileManager(FileSystemOpts)),
       SourceMgr(new SourceManager(*Diagnostics, *FileMgr, false)),
       HeaderInfo(new HeaderSearch(std::make_shared<HeaderSearchOptions>(),
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index a58fb5f9792720..63dd84991314eb 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -216,7 +216,7 @@ enum OpenCLTypeKind : uint8_t {
 ///
 class TargetInfo : public TransferrableTargetInfo,
                    public RefCountedBase<TargetInfo> {
-  std::shared_ptr<TargetOptions> TargetOpts;
+  TargetOptions *TargetOpts;
   llvm::Triple Triple;
 protected:
   // Target values set by the ctor of the actual target implementation.  Default
@@ -302,9 +302,8 @@ class TargetInfo : public TransferrableTargetInfo,
   /// \param Opts - The options to use to initialize the target. The target may
   /// modify the options to canonicalize the target feature information to match
   /// what the backend expects.
-  static TargetInfo *
-  CreateTargetInfo(DiagnosticsEngine &Diags,
-                   const std::shared_ptr<TargetOptions> &Opts);
+  static TargetInfo *CreateTargetInfo(DiagnosticsEngine &Diags,
+                                      TargetOptions &Opts);
 
   virtual ~TargetInfo();
 
diff --git a/clang/include/clang/Frontend/ASTUnit.h b/clang/include/clang/Frontend/ASTUnit.h
index 080844893c13c9..f223ea6372b9d1 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -140,6 +140,10 @@ class ASTUnit {
   /// LoadFromCommandLine available.
   std::shared_ptr<CompilerInvocation> Invocation;
 
+  /// Optional owned invocation, just used to keep the invocation alive for the
+  /// members initialized in transferASTDataFromCompilerInstance.
+  std::shared_ptr<CompilerInvocation> ModifiedInvocation;
+
   /// Fake module loader: the AST unit doesn't need to load any modules.
   TrivialModuleLoader ModuleLoader;
 
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 3464654284f199..5e0a1ca3401678 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -86,6 +86,9 @@ class CompilerInstance : public ModuleLoader {
   /// The target being compiled for.
   IntrusiveRefCntPtr<TargetInfo> Target;
 
+  /// Options for the auxiliary target.
+  std::unique_ptr<TargetOptions> AuxTargetOpts;
+
   /// Auxiliary Target info.
   IntrusiveRefCntPtr<TargetInfo> AuxTarget;
 
diff --git a/clang/include/clang/Frontend/CompilerInvocation.h b/clang/include/clang/Frontend/CompilerInvocation.h
index 9daa0a1ecf9488..e18498600799bc 100644
--- a/clang/include/clang/Frontend/CompilerInvocation.h
+++ b/clang/include/clang/Frontend/CompilerInvocation.h
@@ -268,7 +268,6 @@ class CompilerInvocation : public CompilerInvocationBase {
   /// Base class internals.
   /// @{
   using CompilerInvocationBase::LangOpts;
-  using CompilerInvocationBase::TargetOpts;
   using CompilerInvocationBase::DiagnosticOpts;
   std::shared_ptr<HeaderSearchOptions> getHeaderSearchOptsPtr() {
     return HSOpts;
diff --git a/clang/lib/Basic/Targets.cpp b/clang/lib/Basic/Targets.cpp
index 0b8e565345b6a4..b54075ac5a3150 100644
--- a/clang/lib/Basic/Targets.cpp
+++ b/clang/lib/Basic/Targets.cpp
@@ -748,9 +748,10 @@ std::unique_ptr<TargetInfo> AllocateTarget(const llvm::Triple &Triple,
 using namespace clang::targets;
 /// CreateTargetInfo - Return the target info object for the specified target
 /// options.
-TargetInfo *
-TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
-                             const std::shared_ptr<TargetOptions> &Opts) {
+TargetInfo *TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
+                                         TargetOptions &OptsRef) {
+  TargetOptions *Opts = &OptsRef;
+
   llvm::Triple Triple(llvm::Triple::normalize(Opts->Triple));
 
   // Construct the target
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 84e273a3949ef0..9acebc67786e0e 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -615,7 +615,7 @@ class ASTInfoCollector : public ASTReaderListener {
 
     this->TargetOpts = std::make_shared<TargetOptions>(TargetOpts);
     Target =
-        TargetInfo::CreateTargetInfo(PP.getDiagnostics(), this->TargetOpts);
+        TargetInfo::CreateTargetInfo(PP.getDiagnostics(), *this->TargetOpts);
 
     updated();
     return false;
@@ -1501,6 +1501,10 @@ void ASTUnit::transferASTDataFromCompilerInstance(CompilerInstance &CI) {
     Target = &CI.getTarget();
   Reader = CI.getASTReader();
   HadModuleLoaderFatalFailure = CI.hadModuleLoaderFatalFailure();
+  if (Invocation != CI.getInvocationPtr()) {
+    // This happens when Parse creates a copy of \c Invocation to modify.
+    ModifiedInvocation = CI.getInvocationPtr();
+  }
 }
 
 StringRef ASTUnit::getMainFileName() const {
diff --git a/clang/lib/Frontend/ChainedIncludesSource.cpp b/clang/lib/Frontend/ChainedIncludesSource.cpp
index c1a9f25a8798c7..ad08ebdad4a1d2 100644
--- a/clang/lib/Frontend/ChainedIncludesSource.cpp
+++ b/clang/lib/Frontend/ChainedIncludesSource.cpp
@@ -127,7 +127,7 @@ IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource(
     Clang->setInvocation(std::move(CInvok));
     Clang->setDiagnostics(Diags.get());
     Clang->setTarget(TargetInfo::CreateTargetInfo(
-        Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
+        Clang->getDiagnostics(), Clang->getInvocation().getTargetOpts()));
     Clang->createFileManager();
     Clang->createSourceManager(Clang->getFileManager());
     Clang->createPreprocessor(TU_Prefix);
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 1364641a9b71e1..cde4c7e874e240 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -107,7 +107,7 @@ void CompilerInstance::setAuxTarget(TargetInfo *Value) { AuxTarget = Value; }
 bool CompilerInstance::createTarget() {
   // Create the target instance.
   setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(),
-                                         getInvocation().TargetOpts));
+                                         getInvocation().getTargetOpts()));
   if (!hasTarget())
     return false;
 
@@ -117,14 +117,14 @@ bool CompilerInstance::createTarget() {
       (getLangOpts().CUDA || getLangOpts().OpenMPIsTargetDevice ||
        getLangOpts().SYCLIsDevice) &&
       !getFrontendOpts().AuxTriple.empty()) {
-    auto TO = std::make_shared<TargetOptions>();
+    auto &TO = AuxTargetOpts = std::make_unique<TargetOptions>();
     TO->Triple = llvm::Triple::normalize(getFrontendOpts().AuxTriple);
     if (getFrontendOpts().AuxTargetCPU)
       TO->CPU = *getFrontendOpts().AuxTargetCPU;
     if (getFrontendOpts().AuxTargetFeatures)
       TO->FeaturesAsWritten = *getFrontendOpts().AuxTargetFeatures;
     TO->HostTriple = getTarget().getTriple().str();
-    setAuxTarget(TargetInfo::CreateTargetInfo(getDiagnostics(), TO));
+    setAuxTarget(TargetInfo::CreateTargetInfo(getDiagnostics(), *TO));
   }
 
   if (!getTarget().hasStrictFP() && !getLangOpts().ExpStrictFP) {
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 7209a33272ef22..723b7ebfa82249 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -119,7 +119,7 @@ CreateCI(const llvm::opt::ArgStringList &Argv) {
   Clang->getPreprocessorOpts().addRemappedFile("<<< inputs >>>", MB);
 
   Clang->setTarget(TargetInfo::CreateTargetInfo(
-      Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
+      Clang->getDiagnostics(), Clang->getInvocation().getTargetOpts()));
   if (!Clang->hasTarget())
     return llvm::createStringError(llvm::errc::not_supported,
                                    "Initialization failed. "
diff --git a/clang/tools/clang-import-test/clang-import-test.cpp b/clang/tools/clang-import-test/clang-import-test.cpp
index 2473e16a546dc0..98007ff25b44cf 100644
--- a/clang/tools/clang-import-test/clang-import-test.cpp
+++ b/clang/tools/clang-import-test/clang-import-test.cpp
@@ -206,7 +206,7 @@ std::unique_ptr<CompilerInstance> BuildCompilerInstance() {
   Ins->setInvocation(std::move(Inv));
 
   TargetInfo *TI = TargetInfo::CreateTargetInfo(
-      Ins->getDiagnostics(), Ins->getInvocation().TargetOpts);
+      Ins->getDiagnostics(), Ins->getInvocation().getTargetOpts());
   Ins->setTarget(TI);
   Ins->getTarget().adjust(Ins->getDiagnostics(), Ins->getLangOpts());
   Ins->createFileManager();
diff --git a/clang/unittests/Analysis/MacroExpansionContextTest.cpp b/clang/unittests/Analysis/MacroExpansionContextTest.cpp
index 54b209e7b28c18..0a555f36a09375 100644
--- a/clang/unittests/Analysis/MacroExpansionContextTest.cpp
+++ b/clang/unittests/Analysis/MacroExpansionContextTest.cpp
@@ -39,7 +39,7 @@ class MacroExpansionContextTest : public ::testing::Test {
         Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
         SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions()) {
     TargetOpts->Triple = "x86_64-pc-linux-unknown";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
     LangOpts.CPlusPlus20 = 1; // For __VA_OPT__
   }
 
diff --git a/clang/unittests/Basic/SourceManagerTest.cpp b/clang/unittests/Basic/SourceManagerTest.cpp
index 45840f5188cdcd..b9dcd74e3b4891 100644
--- a/clang/unittests/Basic/SourceManagerTest.cpp
+++ b/clang/unittests/Basic/SourceManagerTest.cpp
@@ -45,7 +45,7 @@ class SourceManagerTest : public ::testing::Test {
       SourceMgr(Diags, FileMgr),
       TargetOpts(new TargetOptions) {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   FileSystemOptions FileMgrOpts;
diff --git a/clang/unittests/CodeGen/TestCompiler.h b/clang/unittests/CodeGen/TestCompiler.h
index 891489cb511a4f..fa2cc10f8c3532 100644
--- a/clang/unittests/CodeGen/TestCompiler.h
+++ b/clang/unittests/CodeGen/TestCompiler.h
@@ -44,8 +44,7 @@ struct TestCompiler {
     Tr.setEnvironment(Triple::EnvironmentType::UnknownEnvironment);
     compiler.getTargetOpts().Triple = Tr.getTriple();
     compiler.setTarget(clang::TargetInfo::CreateTargetInfo(
-        compiler.getDiagnostics(),
-        std::make_shared<clang::TargetOptions>(compiler.getTargetOpts())));
+        compiler.getDiagnostics(), compiler.getTargetOpts()));
 
     const clang::TargetInfo &TInfo = compiler.getTarget();
     PtrSize = TInfo.getPointerWidth(clang::LangAS::Default) / 8;
diff --git a/clang/unittests/Frontend/UtilsTest.cpp b/clang/unittests/Frontend/UtilsTest.cpp
index ae014d3f86b5aa..854fef9e39c360 100644
--- a/clang/unittests/Frontend/UtilsTest.cpp
+++ b/clang/unittests/Frontend/UtilsTest.cpp
@@ -33,7 +33,7 @@ TEST(BuildCompilerInvocationTest, RecoverMultipleJobs) {
   Opts.VFS = new llvm::vfs::InMemoryFileSystem();
   std::unique_ptr<CompilerInvocation> CI = createInvocation(Args, Opts);
   ASSERT_TRUE(CI);
-  EXPECT_THAT(CI->TargetOpts->Triple, testing::StartsWith("i386-"));
+  EXPECT_THAT(CI->getTargetOpts().Triple, testing::StartsWith("i386-"));
 }
 
 // buildInvocationFromCommandLine should not translate -include to -include-pch,
diff --git a/clang/unittests/Lex/HeaderSearchTest.cpp b/clang/unittests/Lex/HeaderSearchTest.cpp
index b0375d5985f2ef..6bd5ca1331683d 100644
--- a/clang/unittests/Lex/HeaderSearchTest.cpp
+++ b/clang/unittests/Lex/HeaderSearchTest.cpp
@@ -36,7 +36,7 @@ class HeaderSearchTest : public ::testing::Test {
         Search(std::make_shared<HeaderSearchOptions>(), SourceMgr, Diags,
                LangOpts, Target.get()) {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   void addSearchDir(llvm::StringRef Dir) {
diff --git a/clang/unittests/Lex/LexerTest.cpp b/clang/unittests/Lex/LexerTest.cpp
index 47aa2c131a304d..3bfd101733b610 100644
--- a/clang/unittests/Lex/LexerTest.cpp
+++ b/clang/unittests/Lex/LexerTest.cpp
@@ -48,7 +48,7 @@ class LexerTest : public ::testing::Test {
       TargetOpts(new TargetOptions)
   {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   std::unique_ptr<Preprocessor> CreatePP(StringRef Source,
diff --git a/clang/unittests/Lex/ModuleDeclStateTest.cpp b/clang/unittests/Lex/ModuleDeclStateTest.cpp
index 15306ba22bf67e..ca277aca4be274 100644
--- a/clang/unittests/Lex/ModuleDeclStateTest.cpp
+++ b/clang/unittests/Lex/ModuleDeclStateTest.cpp
@@ -58,7 +58,7 @@ class ModuleDeclStateTest : public ::testing::Test {
         Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
         SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
     TargetOpts->Triple = "x86_64-unknown-linux-gnu";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   std::unique_ptr<Preprocessor>
diff --git a/clang/unittests/Lex/PPCallbacksTest.cpp b/clang/unittests/Lex/PPCallbacksTest.cpp
index f3cdb1dfb28742..e9750d21a662a9 100644
--- a/clang/unittests/Lex/PPCallbacksTest.cpp
+++ b/clang/unittests/Lex/PPCallbacksTest.cpp
@@ -139,7 +139,7 @@ class PPCallbacksTest : public ::testing::Test {
         Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
         SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions()) {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem;
diff --git a/clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp b/clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
index 84c4cc3a1b2dc0..e9019fe298f704 100644
--- a/clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
+++ b/clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
@@ -36,7 +36,7 @@ class PPConditionalDirectiveRecordTest : public ::testing::Test {
       TargetOpts(new TargetOptions)
   {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   FileSystemOptions FileMgrOpts;
diff --git a/clang/unittests/Lex/PPDependencyDirectivesTest.cpp b/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
index 6ff87f720a559f..83e42cacf188a1 100644
--- a/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
+++ b/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -35,7 +35,7 @@ class PPDependencyDirectivesTest : public ::testing::Test {
         Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
         SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
     TargetOpts->Triple = "x86_64-apple-macos12";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   FileSystemOptions FileMgrOpts;
diff --git a/clang/unittests/Lex/PPMemoryAllocationsTest.cpp b/clang/unittests/Lex/PPMemoryAllocationsTest.cpp
index f7fb097b1f7faf..e500c1b4cd670e 100644
--- a/clang/unittests/Lex/PPMemoryAllocationsTest.cpp
+++ b/clang/unittests/Lex/PPMemoryAllocationsTest.cpp
@@ -31,7 +31,7 @@ class PPMemoryAllocationsTest : public ::testing::Test {
         Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
         SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+    Target = TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
   }
 
   FileSystemOptions FileMgrOpts;

github-actions · 2024-08-27T18:59:10Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang/include/clang/Basic/TargetInfo.h

benlangmuir

Seems reasonable

clang/include/clang/Basic/TargetInfo.h

…e()`

…lvm#106271) This PR hides the reference-counted pointer that holds `TargetOptions` from the public API of `CompilerInvocation`. This gives `CompilerInvocation` an exclusive control over the lifetime of this member, which will eventually be leveraged to implement a copy-on-write behavior. There are two clients that currently share ownership of that pointer: * `TargetInfo` - This was refactored to hold a non-owning reference to `TargetOptions`. The options object is typically owned by the `CompilerInvocation` or by the new `CompilerInstance::AuxTargetOpts` for the auxiliary target. This needed a bit of care in `ASTUnit::Parse()` to keep the `CompilerInvocation` alive. * `clangd::PreambleData` - This was refactored to exclusively own the `TargetOptions` that get moved out of the `CompilerInvocation`.

…vs. ptr) See llvm/llvm-project#106271

generation Since llvm/llvm-project#106271, `clang::TargetInfo` does not co-own target options, so there is no longer anything to keep them alive after we discard the throwaway Clang invocation they originate from. Make the importer take ownership of a copy of code generation target options to avoid a use after free.

…s ref vs. ptr) See llvm#106271

…s ref vs. ptr) See llvm#106271 (cherry picked from commit 78fe2cf)

jansvoboda11 requested review from akyrtzi, kadircet and benlangmuir August 27, 2024 18:54

llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 27, 2024

benlangmuir reviewed Aug 27, 2024

View reviewed changes

clang/include/clang/Basic/TargetInfo.h Show resolved Hide resolved

jansvoboda11 requested a review from benlangmuir August 27, 2024 22:59

benlangmuir approved these changes Aug 27, 2024

View reviewed changes

kadircet reviewed Aug 30, 2024

View reviewed changes

clang/include/clang/Basic/TargetInfo.h Show resolved Hide resolved

jansvoboda11 force-pushed the compiler-invocation-target-options-ptr branch from aa23cc2 to d30014f Compare March 11, 2025 17:59

llvmbot added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Mar 11, 2025

jansvoboda11 added 7 commits April 25, 2025 07:40

[clang] TargetInfo does not own TargetOptions

4431265

[clang] Extend the lifetime of CompilerInvocation in `ASTUnit::Pars…

84445a4

…e()`

[clangd] Preamble owns TargetOptions

bd1b2aa

[clang] Hide CompilerInvocation::TargetOpts

5a8c392

Document lifetime on CreateTargetInfo()

f490552

Update ToolChainTest.cpp

61b0397

Update ParseHLSLRootSignatureTest.cpp & LLDB

1721948

jansvoboda11 force-pushed the compiler-invocation-target-options-ptr branch from d30014f to 1721948 Compare April 25, 2025 16:18

jansvoboda11 requested a review from JDevlieghere as a code owner April 25, 2025 16:18

llvmbot added lldb HLSL HLSL Language Support labels Apr 25, 2025

jansvoboda11 merged commit 985410f into llvm:main Apr 28, 2025
14 checks passed

jansvoboda11 deleted the compiler-invocation-target-options-ptr branch April 28, 2025 14:43

AnthonyLatsis added a commit to AnthonyLatsis/swift that referenced this pull request Jun 13, 2025

Adjust calls to clang::TargetInfo::CreateTargetInfo (now takes ref …

9322c9f

…vs. ptr) See llvm/llvm-project#106271

AnthonyLatsis mentioned this pull request Jul 9, 2025

ClangImporter: Take ownership of clang::TargetOptions used for code generation swiftlang/swift#82912

Merged

AnthonyLatsis added a commit to swiftlang/llvm-project that referenced this pull request Jul 9, 2025

[Swift] lldb: Adjust calls to clang::DiagnosticsEngine ctor (now take…

78fe2cf

…s ref vs. ptr) See llvm#106271

AnthonyLatsis added a commit to swiftlang/llvm-project that referenced this pull request Jul 15, 2025

[Swift] lldb: Adjust calls to clang::DiagnosticsEngine ctor (now take…

0f38e95

…s ref vs. ptr) See llvm#106271 (cherry picked from commit 78fe2cf)

AnthonyLatsis added a commit to swiftlang/llvm-project that referenced this pull request Jul 15, 2025

[Swift] lldb: Adjust calls to clang::DiagnosticsEngine ctor (now take…

cc52794

…s ref vs. ptr) See llvm#106271 (cherry picked from commit 78fe2cf)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Hide the `TargetOptions` pointer from `CompilerInvocation` #106271

[clang] Hide the `TargetOptions` pointer from `CompilerInvocation` #106271

Uh oh!

jansvoboda11 commented Aug 27, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 27, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Aug 27, 2024 •

edited

Loading

Uh oh!

Uh oh!

benlangmuir left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[clang] Hide the TargetOptions pointer from CompilerInvocation #106271

[clang] Hide the TargetOptions pointer from CompilerInvocation #106271

Uh oh!

Conversation

jansvoboda11 commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[clang] Hide the `TargetOptions` pointer from `CompilerInvocation` #106271

[clang] Hide the `TargetOptions` pointer from `CompilerInvocation` #106271

jansvoboda11 commented Aug 27, 2024 •

edited

Loading

llvmbot commented Aug 27, 2024 •

edited

Loading

github-actions bot commented Aug 27, 2024 •

edited

Loading