[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

747

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.

2226–2236

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

747

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.

2226–2236

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

747

Great!

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

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

 }
 
 static void addDiagnosticArgs(ArgList &Args, OptSpecifier Group,
                               OptSpecifier GroupWithValue,
                               std::vector<std::string> &Diagnostics) {
   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.
+      // its name (minus the "W" or "R" at the beginning) to the diagnostics.
       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.
+      // This is -Wfoo= or -Rfoo=, where foo is the name of the diagnostic
+      // group. Add only the group name to the diagnostics.
       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);
+      Diagnostics.push_back(A->getValue());
     }
   }
 }
 
 // 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,
                                       std::vector<std::string> &Funcs) {
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.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.Warnings) {
+    // This option is automatically generated from UndefPrefixes.
+    if (Warning == "undef-prefix")
+      continue;
+    Args.push_back(SA(StringRef("-W") + Warning));
+  }
+
+  for (const auto &Remark : Opts.Remarks)
+    Args.push_back(SA(StringRef("-R") + 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;
   }
 
   // The key paths of diagnostic options defined in Options.td start with
#undef DIAG_OPTION_WITH_MARSHALLING
   llvm::sys::Process::UseANSIEscapeCodes(Opts.UseANSIEscapeCodes);
 
   if (Arg *A =
           Args.getLastArg(OPT_diagnostic_serialized_file, OPT__serialize_diags))
     Opts.DiagnosticSerializationFile = A->getValue();
   Opts.ShowColors = parseShowColorsArgs(Args, DefaultDiagColor);
 
   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;
   }
#undef DIAG_OPTION_WITH_MARSHALLING
   }
 
   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::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 might not get queried properly during parsing, but
+        // should be generated from DiagnosticOptions.
+
+        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);