[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

1402–1411

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

1402–1411

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

clang/lib/Frontend/CompilerInvocation.cpp

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,
                                 bool DefaultDiagColor) {
+  Optional<DiagnosticsEngine> IgnoringDiags;
+  if (!Diags) {
+    IgnoringDiags.emplace(new DiagnosticIDs(), new DiagnosticOptions(),
+                          new IgnoringDiagConsumer());
+    Diags = &*IgnoringDiags;
+  }
+
   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)
         << 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)
         << 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)
         << 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");
   // Keep VerifyPrefixes in its original order for the sake of diagnostics, and
   // then sort it to prepare for fast lookup using std::binary_search.
-  if (!checkVerifyPrefixes(Opts.VerifyPrefixes, Diags)) {
+  if (!checkVerifyPrefixes(Opts.VerifyPrefixes, *Diags)) {
     Opts.VerifyDiagnostics = false;
     Success = false;
   }
   else
     llvm::sort(Opts.VerifyPrefixes);
   DiagnosticLevelMask DiagMask = DiagnosticLevelMask::None;
   Success &= parseDiagnosticLevelMask("-verify-ignore-unexpected=",
     Args.getAllArgValues(OPT_verify_ignore_unexpected_EQ),
-    Diags, DiagMask);
+    *Diags, DiagMask);
   if (Args.hasArg(OPT_verify_ignore_unexpected))
     DiagMask = DiagnosticLevelMask::All;
   Opts.setVerifyIgnoreUnexpected(DiagMask);
   Opts.ElideType = !Args.hasArg(OPT_fno_elide_type);
   Opts.ShowTemplateTree = Args.hasArg(OPT_fdiagnostics_show_template_tree);
   Opts.ErrorLimit = getLastArgIntValue(Args, OPT_ferror_limit, 0, Diags);
   Opts.MacroBacktraceLimit =
       getLastArgIntValue(Args, OPT_fmacro_backtrace_limit,
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)
         << 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;