[clang] Use CompilerInstance::createTarget to createTarget

[oToToT]

Summary

As proposed in D97109, I tried to replace all similar target creating logics in clang to a single CompilerInstance::createTarget call introduced in D97493.

This improve the consistency while creating Target.

[oToToT]

clang/lib/Frontend/PrecompiledPreamble.cpp

368

Changing this without other further patch might cause clangd results in Runtime Error while handling files like CUDA.
Should I also include the patch of clangd or other related projects into this patch?

[oToToT]

Kindly ping.

Since the part I modified is quite... old (about 4 to 7 years ago?), I hope I've found the right person to review for this.

[sammccall]

clang/lib/Frontend/ASTUnit.cpp

1153

This part is not used by clangd. However it is used together with PrecompiledPreamble, so maybe all three need to be changed together.

(Just trying to understand whether this is part of the minimal change, but I think you have this right)

clang/lib/Frontend/ChainedIncludesSource.cpp

152

This looks like a good, correct change, but I think it should be in a separate patch.
This affects the behavior of clang itself, which means:

• it's riskier to change
• keeping the inconsistency for a little while longer can't be too bad, because it's long-standing behavior

I suspect -Xclang -chained-include is just really rare in practice.

clang/lib/Frontend/PrecompiledPreamble.cpp

368

What kind of runtime error can this result in and why? What's the current behavior?

My guess is if this has to be consistent between preamble + main AST, then you should change PrecompiledPreamble, ASTUnit, and clangd together in one patch and leave others.

[oToToT]

Since, originally, I think it is OK to submit patch just by project, but I agree that it's better to make patch as minimal as possible.
Thus, I will abandon this and resubmit another patch.
(I think reuploading patch and changing the whole patch logic here might not be great?)

clang/lib/Frontend/PrecompiledPreamble.cpp

368

Yes, this is because the inconsistency between preamble and main AST. I will reorder my patch.

clang/lib/Frontend/ASTUnit.cpp

bool ASTUnit::Parse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
   OriginalSourceFile =
       std::string(Clang->getFrontendOpts().Inputs[0].getFile());
 
   // Set up diagnostics, capturing any diagnostics that would
   // otherwise be dropped.
   Clang->setDiagnostics(&getDiagnostics());
 
   // Create the target instance.
-  Clang->setTarget(TargetInfo::CreateTargetInfo(
-      Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
-  if (!Clang->hasTarget())
+  if (!Clang->createTarget())
     return true;
 
-  // Inform the target of the language options.
-  //
-  // FIXME: We shouldn't need to do this, the target should be immutable once
-  // created. This complexity should be lifted elsewhere.
-  Clang->getTarget().adjust(Clang->getLangOpts());
-
   assert(Clang->getFrontendOpts().Inputs.size() == 1 &&
          "Invocation must have exactly one source file!");
   assert(Clang->getFrontendOpts().Inputs[0].getKind().getFormat() ==
              InputKind::Source &&
          "FIXME: AST inputs not yet supported here!");
   assert(Clang->getFrontendOpts().Inputs[0].getKind().getLanguage() !=
              Language::LLVM_IR &&
          "IR inputs not support here!");
ASTUnit *ASTUnit::LoadFromCompilerInvocationAction(
   AST->OriginalSourceFile =
       std::string(Clang->getFrontendOpts().Inputs[0].getFile());
 
   // Set up diagnostics, capturing any diagnostics that would
   // otherwise be dropped.
   Clang->setDiagnostics(&AST->getDiagnostics());
 
   // Create the target instance.
-  Clang->setTarget(TargetInfo::CreateTargetInfo(
-      Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
-  if (!Clang->hasTarget())
+  if (!Clang->createTarget())
     return nullptr;
 
-  // Inform the target of the language options.
-  //
-  // FIXME: We shouldn't need to do this, the target should be immutable once
-  // created. This complexity should be lifted elsewhere.
-  Clang->getTarget().adjust(Clang->getLangOpts());
-
   assert(Clang->getFrontendOpts().Inputs.size() == 1 &&
          "Invocation must have exactly one source file!");
   assert(Clang->getFrontendOpts().Inputs[0].getKind().getFormat() ==
              InputKind::Source &&
          "FIXME: AST inputs not yet supported here!");
   assert(Clang->getFrontendOpts().Inputs[0].getKind().getLanguage() !=
              Language::LLVM_IR &&
          "IR inputs not support here!");
void ASTUnit::CodeComplete(
   // Set up diagnostics, capturing any diagnostics produced.
   Clang->setDiagnostics(&Diag);
   CaptureDroppedDiagnostics Capture(CaptureDiagsKind::All,
                                     Clang->getDiagnostics(),
                                     &StoredDiagnostics, nullptr);
   ProcessWarningOptions(Diag, Inv.getDiagnosticOpts());
 
   // Create the target instance.
-  Clang->setTarget(TargetInfo::CreateTargetInfo(
-      Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
-  if (!Clang->hasTarget()) {
+  if (!Clang->createTarget()) {
     Clang->setInvocation(nullptr);
     return;
   }
 
-  // Inform the target of the language options.
-  //
-  // FIXME: We shouldn't need to do this, the target should be immutable once
-  // created. This complexity should be lifted elsewhere.
-  Clang->getTarget().adjust(Clang->getLangOpts());
-
   assert(Clang->getFrontendOpts().Inputs.size() == 1 &&
          "Invocation must have exactly one source file!");
   assert(Clang->getFrontendOpts().Inputs[0].getKind().getFormat() ==
              InputKind::Source &&
          "FIXME: AST inputs not yet supported here!");
   assert(Clang->getFrontendOpts().Inputs[0].getKind().getLanguage() !=
              Language::LLVM_IR &&
          "IR inputs not support here!");

clang/lib/Frontend/ChainedIncludesSource.cpp

for (unsigned i = 0, e = includes.size(); i != e; ++i) {
     IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
     IntrusiveRefCntPtr<DiagnosticsEngine> Diags(
         new DiagnosticsEngine(DiagID, &CI.getDiagnosticOpts(), DiagClient));
 
     std::unique_ptr<CompilerInstance> Clang(
         new CompilerInstance(CI.getPCHContainerOperations()));
     Clang->setInvocation(std::move(CInvok));
     Clang->setDiagnostics(Diags.get());
-    Clang->setTarget(TargetInfo::CreateTargetInfo(
-        Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
+    Clang->createTarget();
     Clang->createFileManager();
     Clang->createSourceManager(Clang->getFileManager());
     Clang->createPreprocessor(TU_Prefix);
     Clang->getDiagnosticClient().BeginSourceFile(Clang->getLangOpts(),
                                                  &Clang->getPreprocessor());
     Clang->createASTContext();
 
     auto Buffer = std::make_shared<PCHBuffer>();

clang/lib/Frontend/PrecompiledPreamble.cpp

llvm::ErrorOr<PrecompiledPreamble> PrecompiledPreamble::Build(
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<CompilerInstance> CICleanup(
       Clang.get());
 
   Clang->setInvocation(std::move(PreambleInvocation));
   Clang->setDiagnostics(&Diagnostics);
 
   // Create the target instance.
-  Clang->setTarget(TargetInfo::CreateTargetInfo(
-      Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
-  if (!Clang->hasTarget())
+  if (!Clang->createTarget())
     return BuildPreambleError::CouldntCreateTargetInfo;
 
-  // Inform the target of the language options.
-  //
-  // FIXME: We shouldn't need to do this, the target should be immutable once
-  // created. This complexity should be lifted elsewhere.
-  Clang->getTarget().adjust(Clang->getLangOpts());
-
   if (Clang->getFrontendOpts().Inputs.size() != 1 ||
       Clang->getFrontendOpts().Inputs[0].getKind().getFormat() !=
           InputKind::Source ||
       Clang->getFrontendOpts().Inputs[0].getKind().getLanguage() ==
           Language::LLVM_IR) {
     return BuildPreambleError::BadInputs;
   }
 

clang/tools/clang-import-test/clang-import-test.cpp

std::unique_ptr<CompilerInstance> BuildCompilerInstance() {
   Inv->getLangOpts()->CXXExceptions = true;
   // Needed for testing dynamic_cast.
   Inv->getLangOpts()->RTTI = true;
   Inv->getCodeGenOpts().setDebugInfo(codegenoptions::FullDebugInfo);
   Inv->getTargetOpts().Triple = llvm::sys::getDefaultTargetTriple();
 
   Ins->setInvocation(std::move(Inv));
 
-  TargetInfo *TI = TargetInfo::CreateTargetInfo(
-      Ins->getDiagnostics(), Ins->getInvocation().TargetOpts);
-  Ins->setTarget(TI);
-  Ins->getTarget().adjust(Ins->getLangOpts());
+  Ins->createTarget();
   Ins->createFileManager();
   Ins->createSourceManager(Ins->getFileManager());
   Ins->createPreprocessor(TU_Complete);
 
   return Ins;
 }
 
 std::unique_ptr<ASTContext>