[clang][cli] NFC: Ensure non-null DiagnosticsEngine in ParseDiagnosticArgs

[jansvoboda11]

Summary

Before this patch, ParseDiagnosticArgs can be called with a nullptr DiagnosticsEngine *. This happens early on in the compilation process, where no proper DiagnosticEngine exists, because the diagnostic options (passed through command line) are not known yet.

This patch ensures nullptr is replaced by an ignoring DiagnosticEngine in ParseDiagnosticArgs, which allows to switch from pointer to a reference in some utility functions.

Besides simplifying the code, this patch enables a future patch (D84673) that ports diagnostic options to the new marshalling infrastructure.

[dexonsmith]

This is needed for a future patch, where we start using normalizers in contexts where no Diags are available.

Can you explain what those contexts are? I'm wondering if they can be changed to create a DiagnosticsEngine instead of having to account for it being maybe missing in all these places.

[jansvoboda11]

This will make more sense after looking at D84673. In short: parsing of diagnostic options can happen outside of CompilerInvocation::createFromArgs, that's why ParseDiagnosticArgs is a free function. Ideally, we'd like to reuse the existing marshalling infrastructure here as well.

ParseDiagnosticArgs is usually called with DiagnosticEngine *Diags = nullptr. That's because the call happens early on during compiler execution, where we don't have a diagnostic engine yet. This call needs to happen first, to obtain the diagnostic options that are necessary for the engine instantiation.

This patch accommodates this free function by turning all DiagnosticEngine references to pointers. Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

[jansvoboda11]

In D93701#2477239, @jansvoboda11 wrote:

Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Something like CompilerInstance::createDiagnostics(new DiagnosticOptions(), new IgnoreDiagnostics()) could work. I'll give that a try.

[dexonsmith]

In D93701#2479574, @jansvoboda11 wrote:

In D93701#2477239, @jansvoboda11 wrote:

Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Something like CompilerInstance::createDiagnostics(new DiagnosticOptions(), new IgnoreDiagnostics()) could work. I'll give that a try.

I think you can also construct a DiagnosticsEngine directly on the stack, without going through CompilerInstance, if that helps.

[dexonsmith]

In D93701#2477239, @jansvoboda11 wrote:

This will make more sense after looking at D84673. In short: parsing of diagnostic options can happen outside of CompilerInvocation::createFromArgs, that's why ParseDiagnosticArgs is a free function. Ideally, we'd like to reuse the existing marshalling infrastructure here as well.

ParseDiagnosticArgs is usually called with DiagnosticEngine *Diags = nullptr. That's because the call happens early on during compiler execution, where we don't have a diagnostic engine yet. This call needs to happen first, to obtain the diagnostic options that are necessary for the engine instantiation.

This patch accommodates this free function by turning all DiagnosticEngine references to pointers. Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Also, please try put this level of detail in the commit message.

[jansvoboda11]

Instead of making Diags optional everywhere, pass an ignoring Diags into ParseDiagnosticArgs.

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

1400–1402

Instead of this change (and updating callers), I suggest adding:

Optional<DiagnosticsEngine> IgnoredDiags;
if (!Diags) {
  IgnoredDiags.emplace(new DiagnosticIDs(), new DiagnosticOptions(),
                       new IgnoringDiagConsumer());
  Diags = &*IgnoredDiags;
}

[dexonsmith]

With the above adjustment (reverting the change to the ParseDiagnosticArgs signature), this LGTM.

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

1400–1402

That's a nice way to keep the signature intact. Thanks!

clang/include/clang/Frontend/CompilerInvocation.h

 //===- CompilerInvocation.h - Compiler Invocation Helper Data ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 class PreprocessorOptions;
 class TargetOptions;
 
 /// Fill out Opts based on the options given in Args.
 ///
 /// Args must have been created from the OptTable returned by
 /// createCC1OptTable().
 ///
-/// When errors are encountered, return false and, if Diags is non-null,
-/// report the error(s).
+/// When errors are encountered, return false and report the error(s).
 bool ParseDiagnosticArgs(DiagnosticOptions &Opts, llvm::opt::ArgList &Args,
-                         DiagnosticsEngine *Diags = nullptr,
+                         DiagnosticsEngine &Diags,
                          bool DefaultDiagColor = true);
 
 class CompilerInvocationBase {
 public:
   /// Options controlling the language variant.
   std::shared_ptr<LangOptions> LangOpts;
 
   /// Options controlling the target.

clang/lib/Frontend/CompilerInvocation.cpp

 //===- CompilerInvocation.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
Diags.Report(diag::err_drv_optimization_remark_pattern)
         << RegexError << RpassArg->getAsString(Args);
     Pattern.reset();
   }
   return Pattern;
 }
 
 static bool parseDiagnosticLevelMask(StringRef FlagName,
                                      const std::vector<std::string> &Levels,
-                                     DiagnosticsEngine *Diags,
+                                     DiagnosticsEngine &Diags,
                                      DiagnosticLevelMask &M) {
   bool Success = true;
   for (const auto &Level : Levels) {
     DiagnosticLevelMask const PM =
       llvm::StringSwitch<DiagnosticLevelMask>(Level)
         .Case("note",    DiagnosticLevelMask::Note)
         .Case("remark",  DiagnosticLevelMask::Remark)
         .Case("warning", DiagnosticLevelMask::Warning)
         .Case("error",   DiagnosticLevelMask::Error)
         .Default(DiagnosticLevelMask::None);
     if (PM == DiagnosticLevelMask::None) {
       Success = false;
-      if (Diags)
-        Diags->Report(diag::err_drv_invalid_value) << FlagName << Level;
+      Diags.Report(diag::err_drv_invalid_value) << FlagName << Level;
     }
     M = M | PM;
   }
   return Success;
 }
 
 static void parseSanitizerKinds(StringRef FlagName,
                                 const std::vector<std::string> &Sanitizers,
for (auto *A : Args) {
     }
   }
   return ShowColors == Colors_On ||
          (ShowColors == Colors_Auto &&
           llvm::sys::Process::StandardErrHasColors());
 }
 
 static bool checkVerifyPrefixes(const std::vector<std::string> &VerifyPrefixes,
-                                DiagnosticsEngine *Diags) {
+                                DiagnosticsEngine &Diags) {
   bool Success = true;
   for (const auto &Prefix : VerifyPrefixes) {
     // Every prefix must start with a letter and contain only alphanumeric
     // characters, hyphens, and underscores.
     auto BadChar = llvm::find_if(Prefix, [](char C) {
       return !isAlphanumeric(C) && C != '-' && C != '_';
     });
     if (BadChar != Prefix.end() || !isLetter(Prefix[0])) {
       Success = false;
-      if (Diags) {
-        Diags->Report(diag::err_drv_invalid_value) << "-verify=" << Prefix;
-        Diags->Report(diag::note_drv_verify_prefix_spelling);
-      }
+      Diags.Report(diag::err_drv_invalid_value) << "-verify=" << Prefix;
+      Diags.Report(diag::note_drv_verify_prefix_spelling);
     }
   }
   return Success;
 }
 
 bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
-                                DiagnosticsEngine *Diags,
+                                DiagnosticsEngine &Diags,
                                 bool DefaultDiagColor) {
   bool Success = true;
 
   Opts.DiagnosticLogFile =
       std::string(Args.getLastArgValue(OPT_diagnostic_log_file));
   if (Arg *A =
           Args.getLastArg(OPT_diagnostic_serialized_file, OPT__serialize_diags))
     Opts.DiagnosticSerializationFile = A->getValue();
   Opts.IgnoreWarnings = Args.hasArg(OPT_w);
bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
   StringRef ShowOverloads =
     Args.getLastArgValue(OPT_fshow_overloads_EQ, "all");
   if (ShowOverloads == "best")
     Opts.setShowOverloads(Ovl_Best);
   else if (ShowOverloads == "all")
     Opts.setShowOverloads(Ovl_All);
   else {
     Success = false;
-    if (Diags)
-      Diags->Report(diag::err_drv_invalid_value)
+    Diags.Report(diag::err_drv_invalid_value)
         << Args.getLastArg(OPT_fshow_overloads_EQ)->getAsString(Args)
         << ShowOverloads;
   }
 
   StringRef ShowCategory =
     Args.getLastArgValue(OPT_fdiagnostics_show_category, "none");
   if (ShowCategory == "none")
     Opts.ShowCategories = 0;
   else if (ShowCategory == "id")
     Opts.ShowCategories = 1;
   else if (ShowCategory == "name")
     Opts.ShowCategories = 2;
   else {
     Success = false;
-    if (Diags)
-      Diags->Report(diag::err_drv_invalid_value)
+    Diags.Report(diag::err_drv_invalid_value)
         << Args.getLastArg(OPT_fdiagnostics_show_category)->getAsString(Args)
         << ShowCategory;
   }
 
   StringRef Format =
     Args.getLastArgValue(OPT_fdiagnostics_format, "clang");
   if (Format == "clang")
     Opts.setFormat(DiagnosticOptions::Clang);
   else if (Format == "msvc")
     Opts.setFormat(DiagnosticOptions::MSVC);
   else if (Format == "msvc-fallback") {
     Opts.setFormat(DiagnosticOptions::MSVC);
     Opts.CLFallbackMode = true;
   } else if (Format == "vi")
     Opts.setFormat(DiagnosticOptions::Vi);
   else {
     Success = false;
-    if (Diags)
-      Diags->Report(diag::err_drv_invalid_value)
+    Diags.Report(diag::err_drv_invalid_value)
         << Args.getLastArg(OPT_fdiagnostics_format)->getAsString(Args)
         << Format;
   }
 
   Opts.ShowSourceRanges = Args.hasArg(OPT_fdiagnostics_print_source_range_info);
   Opts.ShowParseableFixits = Args.hasArg(OPT_fdiagnostics_parseable_fixits);
   Opts.ShowPresumedLoc = !Args.hasArg(OPT_fno_diagnostics_use_presumed_location);
   Opts.VerifyDiagnostics = Args.hasArg(OPT_verify) || Args.hasArg(OPT_verify_EQ);
   if (Args.hasArg(OPT_verify))
     Opts.VerifyPrefixes.push_back("expected");
Opts.SpellCheckingLimit = getLastArgIntValue(
       DiagnosticOptions::DefaultSpellCheckingLimit, Diags);
   Opts.SnippetLineLimit = getLastArgIntValue(
       Args, OPT_fcaret_diagnostics_max_lines,
       DiagnosticOptions::DefaultSnippetLineLimit, Diags);
   Opts.TabStop = getLastArgIntValue(Args, OPT_ftabstop,
                                     DiagnosticOptions::DefaultTabStop, Diags);
   if (Opts.TabStop == 0 || Opts.TabStop > DiagnosticOptions::MaxTabStop) {
     Opts.TabStop = DiagnosticOptions::DefaultTabStop;
-    if (Diags)
-      Diags->Report(diag::warn_ignoring_ftabstop_value)
+    Diags.Report(diag::warn_ignoring_ftabstop_value)
         << Opts.TabStop << DiagnosticOptions::DefaultTabStop;
   }
   Opts.MessageLength =
       getLastArgIntValue(Args, OPT_fmessage_length_EQ, 0, Diags);
 
   addDiagnosticArgs(Args, OPT_W_Group, OPT_W_value_Group, Opts.Warnings);
   addDiagnosticArgs(Args, OPT_R_Group, OPT_R_value_Group, Opts.Remarks);
 
   return Success;
bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
 
   Success &= ParseAnalyzerArgs(*Res.getAnalyzerOpts(), Args, Diags);
   ParseDependencyOutputArgs(Res.getDependencyOutputOpts(), Args);
   if (!Res.getDependencyOutputOpts().OutputFile.empty() &&
       Res.getDependencyOutputOpts().Targets.empty()) {
     Diags.Report(diag::err_fe_dependency_file_requires_MT);
     Success = false;
   }
-  Success &= ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, &Diags,
+  Success &= ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, Diags,
                                  /*DefaultDiagColor=*/false);
   ParseCommentArgs(LangOpts.CommentOpts, Args);
   // FIXME: We shouldn't have to pass the DashX option around here
   InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags,
                                       LangOpts.IsHeaderFile);
   ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
   Success &= ParseCodeGenArgs(Res.getCodeGenOpts(), Args, DashX, Diags,
                               Res.getTargetOpts(), Res.getFrontendOpts());

clang/lib/Tooling/Tooling.cpp

 //===- Tooling.cpp - Running clang standalone tools -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
bool ToolInvocation::run() {
   std::vector<const char*> Argv;
   for (const std::string &Str : CommandLine)
     Argv.push_back(Str.c_str());
   const char *const BinaryName = Argv[0];
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   unsigned MissingArgIndex, MissingArgCount;
   llvm::opt::InputArgList ParsedArgs = driver::getDriverOptTable().ParseArgs(
       ArrayRef<const char *>(Argv).slice(1), MissingArgIndex, MissingArgCount);
-  ParseDiagnosticArgs(*DiagOpts, ParsedArgs);
+  DiagnosticsEngine DummyDiags(new DiagnosticIDs(), new DiagnosticOptions(),
+                               new IgnoringDiagConsumer());
+  ParseDiagnosticArgs(*DiagOpts, ParsedArgs, DummyDiags);
   TextDiagnosticPrinter DiagnosticPrinter(
       llvm::errs(), &*DiagOpts);
   DiagnosticsEngine Diagnostics(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
       DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
 
   const std::unique_ptr<driver::Driver> Driver(
       newDriver(&Diagnostics, BinaryName, &Files->getVirtualFileSystem()));

clang/tools/driver/driver.cpp

 //===-- driver.cpp - Clang GCC-Compatible Driver --------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 CreateAndPopulateDiagOpts(ArrayRef<const char *> argv, bool &UseNewCC1Process) {
   auto *DiagOpts = new DiagnosticOptions;
   unsigned MissingArgIndex, MissingArgCount;
   InputArgList Args = getDriverOptTable().ParseArgs(
       argv.slice(1), MissingArgIndex, MissingArgCount);
   // We ignore MissingArgCount and the return value of ParseDiagnosticArgs.
   // Any errors that would be diagnosed here will also be diagnosed later,
   // when the DiagnosticsEngine actually exists.
-  (void)ParseDiagnosticArgs(*DiagOpts, Args);
+  DiagnosticsEngine DummyDiags(new DiagnosticIDs(), new DiagnosticOptions(),
+                               new IgnoringDiagConsumer());
+  (void)ParseDiagnosticArgs(*DiagOpts, Args, DummyDiags);
 
   UseNewCC1Process =
       Args.hasFlag(clang::driver::options::OPT_fno_integrated_cc1,
                    clang::driver::options::OPT_fintegrated_cc1,
                    /*Default=*/CLANG_SPAWN_CC1);
 
   return DiagOpts;
 }