[analyzer] SARIF: Add EOF newline; replace diff_sarif

[hubert.reinterpretcast]

Summary

This patch applies a change similar to rC363069, but for SARIF files.

The %diff_sarif lit substitution invokes diff with a non-portable -I option. The intended effect can be achieved by normalizing the inputs to diff beforehand. Such normalization can be done with grep -Ev, which is also used by other tests.

Additionally, this patch updates the SARIF output to have a newline at the end of the file. This makes it so that the SARIF file qualifies as a POSIX text file, which increases the consumability of the generated file in relation to various tools.

[Szelethus]

Added Aaron as he wrote the this diagnostic output type :)

[hubert.reinterpretcast]

@aaron.ballman, for similar cases in the plist output, it has been proposed

• that the reference expected file be committed into the tree pre-normalized, and
• that tool be modified such that the output file has a newline at the end of the file.

Does that sound good for this format?

[hubert.reinterpretcast]

Ping.

[aaron.ballman]

In D62952#1535088, @hubert.reinterpretcast wrote:

@aaron.ballman, for similar cases in the plist output, it has been proposed

• that the reference expected file be committed into the tree pre-normalized, and
• that tool be modified such that the output file has a newline at the end of the file.

Does that sound good for this format?

In general, that seems reasonable, but I would prefer to take care of more of the work in lit.local.cfg than have to deal with that atrocious RUN line in every test case. Is there a way to retain a similarly succinct solution as diff_sarif?

[hubert.reinterpretcast]

In D62952#1548377, @aaron.ballman wrote:

In general, that seems reasonable, but I would prefer to take care of more of the work in lit.local.cfg than have to deal with that atrocious RUN line in every test case. Is there a way to retain a similarly succinct solution as diff_sarif?

There'd be no atrocious RUN line if we went with modifying the expected files beforehand and having the tool output a newline.

The unchanged:

// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %diff_sarif %S/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif -

becomes:

// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %normalize_sarif | diff -U1 -b %S/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif -

As in, %diff_sarif gets replaced with %normalize_sarif | diff -U1 -b and that's it.

[aaron.ballman]

In D62952#1548580, @hubert.reinterpretcast wrote:

In D62952#1548377, @aaron.ballman wrote:

In general, that seems reasonable, but I would prefer to take care of more of the work in lit.local.cfg than have to deal with that atrocious RUN line in every test case. Is there a way to retain a similarly succinct solution as diff_sarif?

There'd be no atrocious RUN line if we went with modifying the expected files beforehand and having the tool output a newline.

The unchanged:

// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %diff_sarif %S/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif -

becomes:

// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %normalize_sarif | diff -U1 -b %S/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif -

As in, %diff_sarif gets replaced with %normalize_sarif | diff -U1 -b and that's it.

But is there a reason to not keep %diff_sarif and define it in terms of %normalize_sarif | diff -U1 -b within lit.local.cfg? I guess I don't see the benefit to exposing the call to diff (I don't anticipate anyone needing to change the options passed to diff).

[hubert.reinterpretcast]

In D62952#1548593, @aaron.ballman wrote:

But is there a reason to not keep %diff_sarif and define it in terms of %normalize_sarif | diff -U1 -b within lit.local.cfg? I guess I don't see the benefit to exposing the call to diff (I don't anticipate anyone needing to change the options passed to diff).

Yes: The normalization runs on stdin. %diff_sarif would then only make sense if stdin is one of the inputs, and I think the only input we can hardcode to be stdin is the one conventionally considered to be the reference input (which is not what we want).

[aaron.ballman]

In D62952#1548620, @hubert.reinterpretcast wrote:

In D62952#1548593, @aaron.ballman wrote:

But is there a reason to not keep %diff_sarif and define it in terms of %normalize_sarif | diff -U1 -b within lit.local.cfg? I guess I don't see the benefit to exposing the call to diff (I don't anticipate anyone needing to change the options passed to diff).

Yes: The normalization runs on stdin. %diff_sarif would then only make sense if stdin is one of the inputs, and I think the only input we can hardcode to be stdin is the one conventionally considered to be the reference input (which is not what we want).

Ah drat, I think you're right. Oh well! I think your approach makes sense to me then. Thank you for working on this!

[hubert.reinterpretcast]

Update based on review comments from D62949 as confirmed in D62952

Normalized versions of the reference expected SARIF output files were
checked in under r363788. The patch has been updated with that revision
as the base. Made the SARIF output generation produce a newline at the
end of the file, and modified the RUN lines in the manner discussed.

[aaron.ballman]

LGTM!

cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

if (EC) {
     llvm::errs() << "warning: could not create file: " << EC.message() << '\n';
     return;
   }
   json::Object Sarif{
       {"$schema",
        "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28"},
       {"version", "2.0.0-csd.2.beta.2018-11-28"},
       {"runs", json::Array{createRun(Diags)}}};
-  OS << llvm::formatv("{0:2}", json::Value(std::move(Sarif)));
+  OS << llvm::formatv("{0:2}\n", json::Value(std::move(Sarif)));
 }

cfe/trunk/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif

 {
   "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28",
   "runs": [
     {
       "files": [
         {
           "fileLocation": {
           },
-          "length": 415,
+          "length": 434,
           "mimeType": "text/plain",
           "roles": [
             "resultFile"
           ]
         }
       ],
       "resources": {
         "rules": [

cfe/trunk/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif

 {
   "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28",
   "runs": [
     {
       "files": [      
         {
           "fileLocation": {
           },
-          "length": 667,
+          "length": 686,
           "mimeType": "text/plain",
           "roles": [
             "resultFile"
           ]
         }
       ],
       "resources": {
         "rules": [

cfe/trunk/test/Analysis/diagnostics/sarif-diagnostics-taint-test.c

-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %diff_sarif %S/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif -
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %normalize_sarif | diff -U1 -b %S/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif -
 #include "../Inputs/system-header-simulator.h"
 
 int atoi(const char *nptr);
 
 void f(void) {
   char s[80];
   scanf("%s", s);
   int d = atoi(s); // expected-warning {{tainted}}
 }
 
 int main(void) {
   f();
   return 0;
 }

cfe/trunk/test/Analysis/diagnostics/sarif-multi-diagnostic-test.c

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %diff_sarif %S/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif -
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | %normalize_sarif | diff -U1 -b %S/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif -
 #include "../Inputs/system-header-simulator.h"
 
 int atoi(const char *nptr);
 
 void f(void) {
   char s[80];
   scanf("%s", s);
   int d = atoi(s); // expected-warning {{tainted}}

cfe/trunk/test/Analysis/lit.local.cfg

 # Filtering command used by Clang Analyzer tests (when comparing .plist files
 # with reference output)
 config.substitutions.append(('%normalize_plist',
     "grep -Ev '%s|%s|%s'" %
         ('^[[:space:]]*<string>.* version .*</string>[[:space:]]*$',
          '^[[:space:]]*<string>/.*</string>[[:space:]]*$',
          '^[[:space:]]*<string>.:.*</string>[[:space:]]*$')))
 
-# Diff command for testing SARIF output to reference output.
-config.substitutions.append(('%diff_sarif',
-    '''diff -U1 -w -I ".*file:.*%basename_t" -I '"version":' -I "2\.0\.0\-csd\.[0-9]*\.beta\."'''))
+# Filtering command for testing SARIF output against reference output.
+config.substitutions.append(('%normalize_sarif',
+    "grep -Ev '^[[:space:]]*(%s|%s|%s)[[:space:]]*$'" %
+        ('"uri": "file:.*%basename_t"',
+         '"version": ".* version .*"',
+         '"version": "2\.0\.0-csd\.[0-9]*\.beta\.[0-9-]{10}"')))
 
 if not config.root.clang_staticanalyzer:
     config.unsupported = True