WIP [clang] adds capabilities for SARIF to be written to file

[cjdb]

Summary

The original -fdiagnostics-format=sarif wrote directly to standard
error. This commit renames that mode to sarif-stderr and introduces a
sarif-file mode that allows the compiler to write .sarif files to
the current working directory. It also removes SARIF as an option so
that there's a single canonical approach for each option.

IMPORTANT: There are two different designs for writing SARIF to file, so I've pulled the common parts of the design out into their own patch. This ensures it only gets reviewed once, and so that the divergence is apparent in the other two patches. Once a design is settled on, this patch will be joined with the selected design.

Depends on D145201.

[cjdb]

adds dependency

[cjdb]

tidies up some stuff that I overlooked

[cjdb]

merges in D145438. This patch is ready for review now.

[erichkeane]

Patch topic needs "WIP" out of it I think?

Just some WIP comments, I don't believe I'd be the one to approve this anyway.

clang/include/clang/Basic/DiagnosticOptions.h

109

typically Clang uses the 'American' spelling of things.

110

I'm a touch disturbed by there being 3 very similar file names with very similar comments here. Is there a reason we need all 3?

clang/include/clang/Driver/Options.td

5851

Another 'TODO' here?

clang/include/clang/Frontend/CompilerInstance.h

610

This comment seems inaccurate.

clang/include/clang/Frontend/SARIFDiagnosticPrinter.h

39

What do we do with the DiagnosticsOptions here? Does this have to be a pointer, instead of a ref?

EDIT: I see the rest of the printers seem to do this, but I don't know if we ever have a case where these are null. I guess having them consistent here is important, but if the next person came through and made these all 'refs', it would be nice :)

clang/lib/Driver/ToolChains/Clang.cpp

4036

are we intentionally getting rid of the SARIF spellings intentionally? If so, and because we have this warning, should we report they are no longer supported?

clang/lib/Frontend/CompilerInstance.cpp

357

Oh my... I'll leave it to the Clang code owner/driver code owner (@jansvoboda11) to decide what they think of this.

clang/lib/Frontend/SARIFDiagnosticPrinter.cpp

37

Isn't TargetPath a 'Path'? If so, are you just creating a dot-file? Aren't those really nasty on Windows, and not a nice thing to do here on Linux?

45

I know these are nice/readable/etc, but llvm doesn't use these.

Also, you KNOW what EC equals here: it is a default constructed std::error_code, so the 'if' condition here is always hit. I suspect just removing the whole body and replacing with an llvm_unreachable is appropriate instead.

[cjdb]

clang/include/clang/Basic/DiagnosticOptions.h

110

Agreed. I tried to get this to work with -serialize-diagnostics but that seems to activate a path that causes a different diagnostic engine to take over based on the value of DiagnosticSerializationFile. I didn't notice DiagnosticLogFile, so I'll see what happens if that is used.

clang/lib/Driver/ToolChains/Clang.cpp

4036

Yes. SARIF support isn't documented at all, and as such, I suspect the number of people who know about it (and use it for more than simple curiosity) would be hand-countable, so I'm not sure that it's worth the effort?

(The reason it's not documented is because I forgot to ask for that last year, but that's currently to our advantage.)

clang/lib/Frontend/SARIFDiagnosticPrinter.cpp

37

TargetPath should be a filename, not a directory. When implicit, this is whatever the compiler will generate as its target program. When explicit... I should probably add a check and diagnostic for this.

45

I'm not sure this code is unreachable, hence the assert message. If the file can't be opened for some reason, it's not appropriate for Clang to simply crash.