[clang][cli] Generate and round-trip Diagnostic options

[jansvoboda11]

Summary

This patch implements generation of remaining diagnostic options and tests it by performing parse-generate-parse round trip.

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

745

It's not obvious why this renders the args instead of calling A->getAsString() to get the literal command-line argument and putting it directly in DiagnosticsAsWritten. If that'll work, I suggest switching to that, since it's a bit more straightforward; if there's some reason you can't do that, please leave a comment explaining why.

2230–2240

Does this mean that if some client is programmatically modifying the Warnings array (which is what CompilerInstance reads) they need to also update WarningsAsWritten array identically to get command-line generation to work?

If so, then this is a bit non-obvious / error-prone. Maybe these can encapsulated together:

struct DiagnosticArg {
  std::string Name;
  std::string AsWritten;
};
std::vector<DiagnosticArg> Warnings;
std::vector<DiagnosticArg> Remarks;

[jansvoboda11]

Remove the need for {Warnings,Remarks}AsWritten field

[jansvoboda11]

Got rid of {Warnings,Remarks}AsWritten entirely.

clang/lib/Frontend/CompilerInvocation.cpp

745

I removed DiagnosticsAsWritten completely.

Previously, the code looped over A->getValues(), which gave me the impression it handles a flag that might indeed have multiple values (e.g. -Wsomething=a,b,c). If the code only stored {a,b,c} into Diagnostics, GenerateDiagnosticArgs could never reconstruct the original argument.

However, after looking closely at all -W and -R options, this never happens and A->getValues() always has exactly one element. I've refactored the code and clarified the comment for the branch above.

2230–2240

I double-checked and managed to get rid of this entirely, see the other comment.

[probinson]

I don't claim to be very familiar with this area but "round-trip" and "test" would make me think those bits should be in a lit test or unittest. As it is, it's not obvious what is functional and what is testing here.
Possibly I am misunderstanding something fundamental about how these options work?

[dexonsmith]

LGTM!

In D96274#2551206, @probinson wrote:

I don't claim to be very familiar with this area but "round-trip" and "test" would make me think those bits should be in a lit test or unittest. As it is, it's not obvious what is functional and what is testing here.
Possibly I am misunderstanding something fundamental about how these options work?

The idea is to eventually / soon turn -round-trip-args on by default in asserts builds. The option changes "parse -cc1" to "parse1 => generate1 => parse2 => generate2", returning the result of "parse2" (instead of "parse1") so that new options cannot be added without adding matching generate code, and comparing "generate1" against "generate2" to ensure generation is consistent / deterministic. Having it on by default in asserts builds will add coverage for all -cc1 options used in any test.

Note that the per-option-group round-tripping is temporary, to enable some of this verification to land incrementally. The following patch combines it into a single high-level operation:
https://reviews.llvm.org/D96280

clang/lib/Frontend/CompilerInvocation.cpp

745

Great!

clang/include/clang/Basic/DiagnosticOptions.h

 //===- DiagnosticOptions.h --------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
public:
   std::string DiagnosticLogFile;
 
   /// The file to serialize diagnostics to (non-appending).
   std::string DiagnosticSerializationFile;
 
   /// The list of -W... options used to alter the diagnostic mappings, with the
   /// prefixes removed.
   std::vector<std::string> Warnings;
+  /// The list of -W... options as they were written on the command line.
+  std::vector<std::string> WarningsAsWritten;
 
   /// The list of prefixes from -Wundef-prefix=... used to generate warnings
   /// for undefined macros.
   std::vector<std::string> UndefPrefixes;
 
   /// The list of -R... options used to alter the diagnostic mappings, with the
   /// prefixes removed.
   std::vector<std::string> Remarks;
+  /// The list of -R... options as they were written on the command line.
+  std::vector<std::string> RemarksAsWritten;
 
   /// The prefixes for comment directives sought by -verify ("expected" by
   /// default).
   std::vector<std::string> VerifyPrefixes;
 
 public:
   // Define accessors/mutators for diagnostic options of enumeration type.
 #define DIAGOPT(Name, Bits, Default)

clang/include/clang/Driver/Options.td

def fspell_checking_limit : Separate<["-"], "fspell-checking-limit">, MetaVarName<"<N>">,
   MarshallingInfoStringInt<DiagnosticOpts<"SpellCheckingLimit">, "DiagnosticOptions::DefaultSpellCheckingLimit">;
 def fcaret_diagnostics_max_lines :
   Separate<["-"], "fcaret-diagnostics-max-lines">, MetaVarName<"<N>">,
   HelpText<"Set the maximum number of source lines to show in a caret diagnostic">,
   MarshallingInfoStringInt<DiagnosticOpts<"SnippetLineLimit">, "DiagnosticOptions::DefaultSnippetLineLimit">;
 def verify_EQ : CommaJoined<["-"], "verify=">,
   MetaVarName<"<prefixes>">,
   HelpText<"Verify diagnostic output using comment directives that start with"
-           " prefixes in the comma-separated sequence <prefixes>">,
-  MarshallingInfoStringVector<DiagnosticOpts<"VerifyPrefixes">>;
+           " prefixes in the comma-separated sequence <prefixes>">;
 def verify : Flag<["-"], "verify">,
   HelpText<"Equivalent to -verify=expected">;
 def verify_ignore_unexpected : Flag<["-"], "verify-ignore-unexpected">,
   HelpText<"Ignore unexpected diagnostic messages">;
 def verify_ignore_unexpected_EQ : CommaJoined<["-"], "verify-ignore-unexpected=">,
   HelpText<"Ignore unexpected diagnostic messages">;
 def Wno_rewrite_macros : Flag<["-"], "Wno-rewrite-macros">,
   HelpText<"Silence ObjC rewriting warnings">,

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
 //
 //===----------------------------------------------------------------------===//
 
private:
   /// table-gen definition
   ///
   /// \param Args - The argument list containing the arguments to parse
   /// \param Diags - The DiagnosticsEngine associated with CreateFromArgs
   /// \returns - True if parsing was successful, false otherwise
   bool parseSimpleArgs(const llvm::opt::ArgList &Args,
                        DiagnosticsEngine &Diags);
 
+  /// Parse command line options from DiagnosticOptions.
+  static bool ParseDiagnosticArgsRoundTrip(CompilerInvocation &Res,
+                                           DiagnosticOptions &Opts,
+                                           llvm::opt::ArgList &Args,
+                                           DiagnosticsEngine *Diags = nullptr,
+                                           bool DefaultDiagColor = true);
+
+  /// Generate command line options from DiagnosticOptions.
+  static void GenerateDiagnosticArgs(const DiagnosticOptions &Opts,
+                                     SmallVectorImpl<const char *> &Args,
+                                     StringAllocator SA, bool DefaultDiagColor);
+
   /// Parse command line options that map to LangOptions.
   static bool ParseLangArgsImpl(LangOptions &Opts, llvm::opt::ArgList &Args,
                                 InputKind IK, const llvm::Triple &T,
                                 std::vector<std::string> &Includes,
                                 DiagnosticsEngine &Diags);
 
   static bool ParseLangArgs(CompilerInvocation &Res, LangOptions &Opts,
                             llvm::opt::ArgList &Args, InputKind IK,

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
 //
 //===----------------------------------------------------------------------===//
 
#endif
   Diags.Report(diag::remark_cc1_round_trip_generated)
       << OptsName << 2 << SerializeArgs(GeneratedStrings2);
 
   return Success2;
 }
 
 static void addDiagnosticArgs(ArgList &Args, OptSpecifier Group,
                               OptSpecifier GroupWithValue,
-                              std::vector<std::string> &Diagnostics) {
+                              std::vector<std::string> &Diagnostics,
+                              std::vector<std::string> &DiagnosticsAsWritten) {
+  ArgStringList Rendered;
+
   for (auto *A : Args.filtered(Group)) {
     if (A->getOption().getKind() == Option::FlagClass) {
       // The argument is a pure flag (such as OPT_Wall or OPT_Wdeprecated). Add
       // its name (minus the "W" or "R" at the beginning) to the warning list.
       Diagnostics.push_back(
           std::string(A->getOption().getName().drop_front(1)));
     } else if (A->getOption().matches(GroupWithValue)) {
       // This is -Wfoo= or -Rfoo=, where foo is the name of the diagnostic group.
       Diagnostics.push_back(
           std::string(A->getOption().getName().drop_front(1).rtrim("=-")));
     } else {
       // Otherwise, add its value (for OPT_W_Joined and similar).
       for (const auto *Arg : A->getValues())
         Diagnostics.emplace_back(Arg);
     }
+
+    A->render(Args, Rendered);
   }
+
+  DiagnosticsAsWritten.reserve(Rendered.size());
+  for (const char *Arg : Rendered)
+    DiagnosticsAsWritten.emplace_back(Arg);
 }
 
 // Parse the Static Analyzer configuration. If \p Diags is set to nullptr,
 // it won't verify the input.
 static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
                                  DiagnosticsEngine *Diags);
 
 static void getAllNoBuiltinFuncValues(ArgList &Args,
PARSE_OPTION_WITH_MARSHALLING(Args, Diags, Success, ID, FLAGS, PARAM,        \
                                 IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER,      \
                                 MERGER, TABLE_INDEX)
 #include "clang/Driver/Options.inc"
 #undef OPTION_WITH_MARSHALLING
 
   return Success;
 }
 
+void CompilerInvocation::GenerateDiagnosticArgs(
+    const DiagnosticOptions &Opts, SmallVectorImpl<const char *> &Args,
+    StringAllocator SA, bool DefaultDiagColor) {
+  const DiagnosticOptions *DiagnosticOpts = &Opts;
+#define DIAG_OPTION_WITH_MARSHALLING(                                          \
+    PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
+    HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, ALWAYS_EMIT, KEYPATH,   \
+    DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER,     \
+    MERGER, EXTRACTOR, TABLE_INDEX)                                            \
+  GENERATE_OPTION_WITH_MARSHALLING(                                            \
+      Args, SA, KIND, FLAGS, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,    \
+      IMPLIED_CHECK, IMPLIED_VALUE, DENORMALIZER, EXTRACTOR, TABLE_INDEX)
+#include "clang/Driver/Options.inc"
+#undef DIAG_OPTION_WITH_MARSHALLING
+
+  if (!Opts.DiagnosticSerializationFile.empty())
+    GenerateArg(Args, OPT_diagnostic_serialized_file,
+                Opts.DiagnosticSerializationFile, SA);
+
+  if (Opts.ShowColors)
+    GenerateArg(Args, OPT_fcolor_diagnostics, SA);
+
+  if (Opts.CLFallbackMode)
+    GenerateArg(Args, OPT_fdiagnostics_format, "msvc-fallback", SA);
+
+  if (Opts.VerifyDiagnostics &&
+      llvm::is_contained(Opts.VerifyPrefixes, "expected"))
+    GenerateArg(Args, OPT_verify, SA);
+
+  for (const auto &Prefix : Opts.VerifyPrefixes)
+    if (Prefix != "expected")
+      GenerateArg(Args, OPT_verify_EQ, Prefix, SA);
+
+  DiagnosticLevelMask VIU = Opts.getVerifyIgnoreUnexpected();
+  if (VIU == DiagnosticLevelMask::None) {
+    // This is the default, don't generate anything.
+  } else if (VIU == DiagnosticLevelMask::All) {
+    GenerateArg(Args, OPT_verify_ignore_unexpected, SA);
+  } else {
+    if (static_cast<unsigned>(VIU & DiagnosticLevelMask::Note) != 0)
+      GenerateArg(Args, OPT_verify_ignore_unexpected_EQ, "note", SA);
+    if (static_cast<unsigned>(VIU & DiagnosticLevelMask::Remark) != 0)
+      GenerateArg(Args, OPT_verify_ignore_unexpected_EQ, "remark", SA);
+    if (static_cast<unsigned>(VIU & DiagnosticLevelMask::Warning) != 0)
+      GenerateArg(Args, OPT_verify_ignore_unexpected_EQ, "warning", SA);
+    if (static_cast<unsigned>(VIU & DiagnosticLevelMask::Error) != 0)
+      GenerateArg(Args, OPT_verify_ignore_unexpected_EQ, "error", SA);
+  }
+
+  for (const auto &Warning : Opts.WarningsAsWritten) {
+    // This option also maps to UndefPrefixes that generates it automatically.
+    if (StringRef(Warning).startswith("-Wundef-prefix="))
+      continue;
+    // TODO: Consider ignoring '-Wno-rewrite-macros' that maps to
+    // NoRewriteMacros.
+    Args.push_back(SA(Warning));
+  }
+
+  for (const auto &Remark : Opts.RemarksAsWritten)
+    Args.push_back(SA(Remark));
+}
+
 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;
if (Arg *A =
           Args.getLastArg(OPT_diagnostic_serialized_file, OPT__serialize_diags))
     Opts.DiagnosticSerializationFile = A->getValue();
   Opts.ShowColors = parseShowColorsArgs(Args, DefaultDiagColor);
 
   if (Args.getLastArgValue(OPT_fdiagnostics_format) == "msvc-fallback")
     Opts.CLFallbackMode = true;
 
   Opts.VerifyDiagnostics = Args.hasArg(OPT_verify) || Args.hasArg(OPT_verify_EQ);
+  Opts.VerifyPrefixes = Args.getAllArgValues(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)) {
     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);
   if (Args.hasArg(OPT_verify_ignore_unexpected))
     DiagMask = DiagnosticLevelMask::All;
   Opts.setVerifyIgnoreUnexpected(DiagMask);
   if (Opts.TabStop == 0 || Opts.TabStop > DiagnosticOptions::MaxTabStop) {
     Opts.TabStop = DiagnosticOptions::DefaultTabStop;
     Diags->Report(diag::warn_ignoring_ftabstop_value)
         << Opts.TabStop << DiagnosticOptions::DefaultTabStop;
   }
 
-  addDiagnosticArgs(Args, OPT_W_Group, OPT_W_value_Group, Opts.Warnings);
-  addDiagnosticArgs(Args, OPT_R_Group, OPT_R_value_Group, Opts.Remarks);
+  addDiagnosticArgs(Args, OPT_W_Group, OPT_W_value_Group, Opts.Warnings,
+                    Opts.WarningsAsWritten);
+
+  addDiagnosticArgs(Args, OPT_R_Group, OPT_R_value_Group, Opts.Remarks,
+                    Opts.RemarksAsWritten);
 
   return Success;
 }
 
+bool CompilerInvocation::ParseDiagnosticArgsRoundTrip(CompilerInvocation &Res,
+                                                      DiagnosticOptions &Opts,
+                                                      ArgList &Args,
+                                                      DiagnosticsEngine *Diags,
+                                                      bool DefaultDiagColor) {
+  IntrusiveRefCntPtr<DiagnosticOptions> DummyOpts(new DiagnosticOptions);
+
+  return RoundTrip(
+      [DefaultDiagColor](CompilerInvocation &Res, ArgList &Args,
+                         DiagnosticsEngine &Diags) {
+        // Query the options we might not have queried properly during parsing,
+        // but should be generated by us.
+        Args.getLastArg(OPT_fcolor_diagnostics);
+        Args.getLastArg(OPT_fno_color_diagnostics);
+        Args.getLastArg(OPT_fdiagnostics_color);
+        Args.getLastArg(OPT_fno_diagnostics_color);
+        Args.getLastArg(OPT_fdiagnostics_color_EQ);
+
+        for (auto *A : Args.filtered(OPT_W_Group))
+          Args.getLastArg(A->getOption().getID());
+        for (auto *A : Args.filtered(OPT_R_Group))
+          Args.getLastArg(A->getOption().getID());
+
+        return clang::ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, &Diags,
+                                          DefaultDiagColor);
+      },
+      [DefaultDiagColor](CompilerInvocation &Res,
+                         SmallVectorImpl<const char *> &Args,
+                         CompilerInvocation::StringAllocator SA) {
+        GenerateDiagnosticArgs(Res.getDiagnosticOpts(), Args, SA,
+                               DefaultDiagColor);
+      },
+      [&DummyOpts](CompilerInvocation &Res) {
+        Res.DiagnosticOpts.swap(DummyOpts);
+      },
+      Res, Args, *Diags, "DiagnosticOptions");
+}
+
 /// Parse the argument to the -ftest-module-file-extension
 /// command-line argument.
 ///
 /// \returns true on error, false on success.
 static bool parseTestModuleFileExtensionArg(StringRef Arg,
                                             std::string &BlockName,
                                             unsigned &MajorVersion,
                                             unsigned &MinorVersion,
else
       Diags.Report(diag::err_drv_unknown_argument_with_suggestion)
           << ArgString << Nearest;
     Success = false;
   }
 
   Success &= Res.parseSimpleArgs(Args, Diags);
 
   Success &= ParseAnalyzerArgs(Res, *Res.getAnalyzerOpts(), Args, Diags);
-  Success &= ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, &Diags,
+  Success &=
+      ParseDiagnosticArgsRoundTrip(Res, Res.getDiagnosticOpts(), Args, &Diags,
                                    /*DefaultDiagColor=*/false);
   Success &= ParseFrontendArgs(Res, Res.getFrontendOpts(), Args, Diags,
                                LangOpts.IsHeaderFile);
   // FIXME: We shouldn't have to pass the DashX option around here
   InputKind DashX = Res.getFrontendOpts().DashX;
   ParseTargetArgs(Res, Res.getTargetOpts(), Args, Diags);
   llvm::Triple T(Res.getTargetOpts().Triple);
   ParseHeaderSearchArgs(Res, Res.getHeaderSearchOpts(), Args, Diags,
                         Res.getFileSystemOpts().WorkingDir);
#define OPTION_WITH_MARSHALLING(                                               \
     PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
     HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, ALWAYS_EMIT, KEYPATH,   \
     DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER,     \
     MERGER, EXTRACTOR, TABLE_INDEX)                                            \
   GENERATE_OPTION_WITH_MARSHALLING(Args, SA, KIND, FLAGS, SPELLING,            \
                                    ALWAYS_EMIT, this->KEYPATH, DEFAULT_VALUE,  \
                                    IMPLIED_CHECK, IMPLIED_VALUE, DENORMALIZER, \
                                    EXTRACTOR, TABLE_INDEX)
-
-#define DIAG_OPTION_WITH_MARSHALLING OPTION_WITH_MARSHALLING
-
 #include "clang/Driver/Options.inc"
-
-#undef DIAG_OPTION_WITH_MARSHALLING
 #undef OPTION_WITH_MARSHALLING
 
   llvm::Triple T(TargetOpts->Triple);
 
   GenerateAnalyzerArgs(*AnalyzerOpts, Args, SA);
+  GenerateDiagnosticArgs(*DiagnosticOpts, Args, SA, false);
   GenerateFrontendArgs(FrontendOpts, Args, SA, LangOpts->IsHeaderFile);
   GenerateTargetArgs(*TargetOpts, Args, SA);
   GenerateHeaderSearchArgs(*HeaderSearchOpts, Args, SA);
   GenerateLangArgs(*LangOpts, Args, SA,  T);
   GenerateCodeGenArgs(CodeGenOpts, Args, SA, T, FrontendOpts.OutputFile,
                       &*LangOpts);
   GeneratePreprocessorArgs(*PreprocessorOpts, Args, SA, *LangOpts, FrontendOpts,
                            CodeGenOpts);