[clang] Add -fdiagnostics-format=sarif option for future SARIF output

[abrahamcd]

Summary

Adds sarif option to the existing -fdiagnostics-format flag
for intended future work with SARIF diagnostics. Currently issues a warning
against the use of diagnostics in SARIF mode, then defaults to clang style for
diagnostics.

[abrahamcd]

Edited commit message

[abrahamcd]

Commit message edits did not go through

[cjdb]

Thanks for getting this started! To get this ready for submission, would you be able to add a test please?

[vaibhav.y]

Might be worth hiding it from --help, despite the instability warning.

[abrahamcd]

Adds test file and fixes warning-flags test failure by adding warning group.

[abrahamcd]

In D129886#3656221, @vaibhav.y wrote:

Might be worth hiding it from --help, despite the instability warning.

I already couldn't find any mention of -fdiagnostics-format or sarif when I ran clang --help. Is there somewhere else I should look?

[abrahamcd]

clang/include/clang/Basic/DiagnosticDriverKinds.td

684

Please let me know if there's a better warning group you think this should be in.

[vaibhav.y]

In D129886#3660299, @abrahamcd wrote:

In D129886#3656221, @vaibhav.y wrote:

Might be worth hiding it from --help, despite the instability warning.

I already couldn't find any mention of -fdiagnostics-format or sarif when I ran clang --help. Is there somewhere else I should look?

Interesting, fdiagnostics-format doesn't show up in either --help or --help-hidden.

Please disregard my earlier comment in that case, thanks!

[cjdb]

clang/include/clang/Basic/DiagnosticDriverKinds.td

684

Nope! This is the right way to do it :)

clang/include/clang/Basic/DiagnosticGroups.td

1383 ↗

(On Diff #445554)

Please make sure that there's a newline at the end of each file.

clang/test/Driver/fdiagnostics-format-sarif.cpp

1 ↗

(On Diff #445554)

This test should check not only that we're able to use the flag, but also that it emits the warning.

[denik]

Thanks for adding the flag!

clang/include/clang/Basic/DiagnosticGroups.td

1383 ↗

(On Diff #445554)

I guess you don't need a new entry here since you used InGroup<DiagGroup<"sarif-format-unstable">>.

clang/lib/Driver/ToolChains/Clang.cpp

4009

nit: I would replace it with StringRef.

[abrahamcd]

Update test file to check for warning and address other minor comments

[cjdb]

Thanks for working on this!

[aaron.ballman]

clang/lib/Driver/ToolChains/Clang.cpp

4009–4011

Do we want to be kind to people who spell it SARIF (given that it's an acronym, some folks may spell it that way reflexively)?

[cjdb]

clang/lib/Driver/ToolChains/Clang.cpp

4009–4011

tbh I think it's worth accepting both sarif and SARIF, if we can.

[abrahamcd]

Enabled both "sarif" and "SARIF" as flag spellings.

[vaibhav.y]

Please address the whitespace changes in clang/lib/Driver/ToolChains/Clang.cpp.

clang/lib/Driver/ToolChains/Clang.cpp

81–82

nit: Looks like this an unrelated format change/ rebase issue. There are a few others as well in this file

Would be better to introduce these through another CR, or use git clang-format <commit-start> [<commit-end>] to limit it to the diff.

[abrahamcd]

clang/lib/Driver/ToolChains/Clang.cpp

4009–4011

Ah yeah that would definitely be helpful.

[abrahamcd]

Removed unintended formatting.

clang/lib/Driver/ToolChains/Clang.cpp

81–82

Thanks! Did not realize format changed all that :)

[aaron.ballman]

LGTM!

clang/include/clang/Basic/DiagnosticDriverKinds.td

def err_drv_invalid_range_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "Validator version must be less than or equal to current internal version.">;
 def err_drv_invalid_format_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "Format of validator version is \"<major>.<minor>\" (ex:\"1.4\").">;
 def err_drv_invalid_empty_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "If validator major version is 0, minor version must also be 0.">;
+
+def warn_drv_sarif_format_unstable : Warning<
+  "diagnostic formatting in SARIF mode is currently unstable">;
 }

clang/include/clang/Basic/DiagnosticOptions.h

 /// Options for controlling the compiler diagnostics engine.
 class DiagnosticOptions : public RefCountedBase<DiagnosticOptions>{
   friend bool ParseDiagnosticArgs(DiagnosticOptions &, llvm::opt::ArgList &,
                                   clang::DiagnosticsEngine *, bool);
 
   friend class CompilerInvocation;
 
 public:
-  enum TextDiagnosticFormat { Clang, MSVC, Vi };
+  enum TextDiagnosticFormat { Clang, MSVC, Vi, SARIF };
 
   // Default values.
   enum {
     DefaultTabStop = 8,
     MaxTabStop = 100,
     DefaultMacroBacktraceLimit = 6,
     DefaultTemplateBacktraceLimit = 10,
     DefaultConstexprBacktraceLimit = 10,

clang/include/clang/Driver/Options.td

def diagnostic_log_file : Separate<["-"], "diagnostic-log-file">,
   HelpText<"Filename (or -) to log diagnostics to">,
   MarshallingInfoString<DiagnosticOpts<"DiagnosticLogFile">>;
 def diagnostic_serialized_file : Separate<["-"], "serialize-diagnostic-file">,
   MetaVarName<"<filename>">,
   HelpText<"File for serializing diagnostics in a binary format">;
 
 def fdiagnostics_format : Separate<["-"], "fdiagnostics-format">,
   HelpText<"Change diagnostic formatting to match IDE and command line tools">,
-  Values<"clang,msvc,vi">,
-  NormalizedValuesScope<"DiagnosticOptions">, NormalizedValues<["Clang", "MSVC", "Vi"]>,
+  Values<"clang,msvc,vi,sarif">,
+  NormalizedValuesScope<"DiagnosticOptions">, NormalizedValues<["Clang", "MSVC", "Vi", "SARIF"]>,
   MarshallingInfoEnum<DiagnosticOpts<"Format">, "Clang">;
 def fdiagnostics_show_category : Separate<["-"], "fdiagnostics-show-category">,
   HelpText<"Print diagnostic category">,
   Values<"none,id,name">,
   NormalizedValues<["0", "1", "2"]>,
   MarshallingInfoEnum<DiagnosticOpts<"ShowCategories">, "0">;
 def fno_diagnostics_use_presumed_location : Flag<["-"], "fno-diagnostics-use-presumed-location">,
   HelpText<"Ignore #line directives when displaying diagnostic locations">,

clang/lib/Driver/ToolChains/Clang.cpp

 }
 
 static void CheckCodeGenerationOptions(const Driver &D, const ArgList &Args) {
   // In gcc, only ARM checks this, but it seems reasonable to check universally.
   if (Args.hasArg(options::OPT_static))
     if (const Arg *A =
             Args.getLastArg(options::OPT_dynamic, options::OPT_mdynamic_no_pic))
       D.Diag(diag::err_drv_argument_not_allowed_with) << A->getAsString(Args)
                                                       << "-static";
 }
 
 // Add backslashes to escape spaces and other backslashes.
 // This is used for the space-separated argument list specified with
 // the -dwarf-debug-flags option.
 static void EscapeSpacesAndBackslashes(const char *Arg,
                                        SmallVectorImpl<char> &Res) {
   for (; *Arg; ++Arg) {
     switch (*Arg) {
if (const Arg *A =
     std::string Opt =
         std::string("-fdiagnostics-hotness-threshold=") + A->getValue();
     CmdArgs.push_back(Args.MakeArgString(Opt));
   }
 
   if (const Arg *A = Args.getLastArg(options::OPT_fdiagnostics_format_EQ)) {
     CmdArgs.push_back("-fdiagnostics-format");
     CmdArgs.push_back(A->getValue());
+    if (std::string(A->getValue()) == "sarif") {
+      D.Diag(diag::warn_drv_sarif_format_unstable);
+    }
   }
 
   if (const Arg *A = Args.getLastArg(
           options::OPT_fdiagnostics_show_note_include_stack,
           options::OPT_fno_diagnostics_show_note_include_stack)) {
     const Option &O = A->getOption();
     if (O.matches(options::OPT_fdiagnostics_show_note_include_stack))
       CmdArgs.push_back("-fdiagnostics-show-note-include-stack");

clang/lib/Frontend/TextDiagnostic.cpp

void TextDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
   if (!DiagOpts->ShowLocation)
     return;
 
   if (DiagOpts->ShowColors)
     OS.changeColor(savedColor, true);
 
   emitFilename(PLoc.getFilename(), Loc.getManager());
   switch (DiagOpts->getFormat()) {
+  case DiagnosticOptions::SARIF:
   case DiagnosticOptions::Clang:
     if (DiagOpts->ShowLine)
       OS << ':' << LineNo;
     break;
   case DiagnosticOptions::MSVC:  OS << '('  << LineNo; break;
   case DiagnosticOptions::Vi:    OS << " +" << LineNo; break;
   }
 
   if (DiagOpts->ShowColumn)
     // Compute the column number.
     if (unsigned ColNo = PLoc.getColumn()) {
       if (DiagOpts->getFormat() == DiagnosticOptions::MSVC) {
         OS << ',';
         // Visual Studio 2010 or earlier expects column number to be off by one
         if (LangOpts.MSCompatibilityVersion &&
             !LangOpts.isCompatibleWithMSVC(LangOptions::MSVC2012))
           ColNo--;
       } else
         OS << ':';
       OS << ColNo;
     }
   switch (DiagOpts->getFormat()) {
+  case DiagnosticOptions::SARIF:
   case DiagnosticOptions::Clang:
   case DiagnosticOptions::Vi:    OS << ':';    break;
   case DiagnosticOptions::MSVC:
     // MSVC2013 and before print 'file(4) : error'. MSVC2015 gets rid of the
     // space and prints 'file(4): error'.
     OS << ')';
     if (LangOpts.MSCompatibilityVersion &&
         !LangOpts.isCompatibleWithMSVC(LangOptions::MSVC2015))