Output "rule" information in SARIF

[aaron.ballman]

Summary

SARIF allows you to export descriptions about rules that are present in the SARIF log. This patch exposes the help text table generated into Checkers.inc as the rule's "full description" and exports all of the rules present in the analysis output.

This information is useful for analysis result viewers like CodeSonar.

[george.karpenkov]

Minor nit: let's create a custom substitution for your diff command, like diff_plist.
Otherwise LGTM.

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

2

In order to avoid duplication in diff commands between the tests, it might be better to define a custom command in lit.cfg, as we did for diff_plist
(alternatively, create a custom flag to generate stable output)

[aaron.ballman]

Updated based on review feedback.

[aaron.ballman]

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

2

Oh, how neat! I didn't notice that we did that for plists, but that's a nice solution here.

[george.karpenkov]

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

10

You have probably noticed that seeing those changes is much nicer in diff than in FileCheck :P

[aaron.ballman]

Thank you for the quick review and feedback -- I've commit in r345874.

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

10

YES! :-D

[Szelethus]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

237–242

Hmmm, this won't handle checkers loaded from plugins.

[Szelethus]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

237–242

I don't immediately know the solution is to this, but when you invoke clang with -cc1 -analyzer-checker-help, plugin checkers are also displayed, and this is line that magically does it.
Maybe store the plugins in AnalyzerOptions, and move ClangCheckerRegistry to include/clang/StaticAnalyzer/Frontend?

[aaron.ballman]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

237–242

Oof, this is a good point. Thank you for bringing it up! I'll look into this as soon as I have the chance.

Do we have any tests that use plugins (preferably on Windows) so that I have a way to test this functionality out?

[Szelethus]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

237–242

I'm working on the exact same issue, which is why I spotted this! There are some in unittests/, but I plan on enhancing them a lot in the coming days. I'll make sure to add you as subscriber on related paches, until then, unless users of the Saris output are likely to use the StaticAnalyzer with plugins, I think feel free to wait on my progress. I expect to come out with a patch during the week. But you never know, I guess.

https://github.com/llvm-mirror/clang/blob/master/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

[aaron.ballman]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

237–242

I'm fine with waiting. I don't expect to see heavy use of the Sarif exporter until the spec is finalized, as it's hard to write a Sarif viewer against a moving target. So we've got a few months to find these sort of things out.

Thank you for CCing me on the related patches!

[Szelethus]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

237–242

Well, yea, skeletons are falling out of the closet left and right, so I'll need to clean a lot of other things up before finding a neat solution to this. Please let me know when you need to make progress on this.

[aaron.ballman]

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

237–242

It's not a high priority for me -- I'm not certain about the intersection of users of the static analyzer with plugins and folks who want to export SARIF. I'm sure such people will exist someday, but it seems like a small audience to fret over for the moment.

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

       {"codeFlows", json::Array{createCodeFlow(Path, Files)}},
       {"locations",
        json::Array{createLocation(createPhysicalLocation(
            Diag.getLocation().asRange(),
            *Diag.getLocation().asLocation().getFileEntry(), SMgr, Files))}},
       {"ruleId", Diag.getCheckName()}};
 }
 
+static StringRef getRuleDescription(StringRef CheckName) {
+  return llvm::StringSwitch<StringRef>(CheckName)
+#define GET_CHECKERS
+#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN)        \
+  .Case(FULLNAME, HELPTEXT)
+#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
+#undef CHECKER
+#undef GET_CHECKERS
+      ;
+}
+
+static json::Object createRule(const PathDiagnostic &Diag) {
+  StringRef CheckName = Diag.getCheckName();
+  return json::Object{
+      {"fullDescription", createMessage(getRuleDescription(CheckName))},
+      {"name", createMessage(CheckName)}};
+}
+
+static json::Object createRules(std::vector<const PathDiagnostic *> &Diags) {
+  json::Object Rules;
+  llvm::StringSet<> Seen;
+
+  llvm::for_each(Diags, [&](const PathDiagnostic *D) {
+    StringRef RuleID = D->getCheckName();
+    std::pair<llvm::StringSet<>::iterator, bool> P = Seen.insert(RuleID);
+    if (P.second)
+      Rules[RuleID] = createRule(*D);
+  });
+
+  return Rules;
+}
+
+static json::Object
+createResources(std::vector<const PathDiagnostic *> &Diags) {
+  return json::Object{{"rules", createRules(Diags)}};
+}
+
 static json::Object createRun(std::vector<const PathDiagnostic *> &Diags) {
   json::Array Results;
   json::Object Files;
 
   llvm::for_each(Diags, [&](const PathDiagnostic *D) {
     Results.push_back(createResult(*D, Files));
   });
 
   return json::Object{{"tool", createTool()},
+                      {"resources", createResources(Diags)},
                       {"results", std::move(Results)},
                       {"files", std::move(Files)}};
 }
 
 void SarifDiagnostics::FlushDiagnosticsImpl(
     std::vector<const PathDiagnostic *> &Diags, FilesMade *) {
   // We currently overwrite the file if it already exists. However, it may be
   // useful to add a feature someday that allows the user to append a run to an

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-10-10",
   "runs": [
     {
       "files": {
         "file:sarif-diagnostics-taint-test.c": {
           "fileLocation": {
             "uri": "file:sarif-diagnostics-taint-test.c"
           },
-          "length": 510,
+          "length": 413,
           "mimeType": "text/plain",
           "roles": [
             "resultFile"
           ]
         }
       },
+      "resources": {
+        "rules": {
+          "debug.TaintTest": {
+            "fullDescription": {
+              "text": "Mark tainted symbols as such."
+            },
+            "name": {
+              "text": "debug.TaintTest"
+            }
+          }
+        }
+      },      
       "results": [
         {
           "codeFlows": [
             {
               "threadFlows": [
                 {
                   "locations": [
                     {

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-10-10",
+  "runs": [
+    {
+      "files": {
+        "file:sarif-multi-diagnostic-test.c": {
+          "fileLocation": {
+            "uri": "file:sarif-multi-diagnostic-test.c"
+          },
+          "length": 665,
+          "mimeType": "text/plain",
+          "roles": [
+            "resultFile"
+          ]
+        }
+      },
+      "resources": {
+        "rules": {
+          "core.CallAndMessage": {
+            "fullDescription": {
+              "text": "Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers)"
+            },
+            "name": {
+              "text": "core.CallAndMessage"
+            }
+          },
+          "core.DivideZero": {
+            "fullDescription": {
+              "text": "Check for division by zero"
+            },
+            "name": {
+              "text": "core.DivideZero"
+            }
+          },
+          "debug.TaintTest": {
+            "fullDescription": {
+              "text": "Mark tainted symbols as such."
+            },
+            "name": {
+              "text": "debug.TaintTest"
+            }
+          }
+        }
+      },
+      "results": [
+        {
+          "codeFlows": [
+            {
+              "threadFlows": [
+                {
+                  "locations": [
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "Calling 'f'"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 5,
+                            "endLine": 24,
+                            "startColumn": 3,
+                            "startLine": 24
+                          }
+                        }
+                      }
+                    },
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "tainted"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 17,
+                            "endLine": 9,
+                            "startColumn": 11,
+                            "startLine": 9
+                          }
+                        }
+                      }
+                    }
+                  ]
+                }
+              ]
+            }
+          ],
+          "locations": [
+            {
+              "physicalLocation": {
+                "fileLocation": {
+                  "uri": "file:sarif-multi-diagnostic-test.c"
+                },
+                "region": {
+                  "endColumn": 17,
+                  "endLine": 9,
+                  "startColumn": 11,
+                  "startLine": 9
+                }
+              }
+            }
+          ],
+          "message": {
+            "text": "tainted"
+          },
+          "ruleId": "debug.TaintTest"
+        },
+        {
+          "codeFlows": [
+            {
+              "threadFlows": [
+                {
+                  "locations": [
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "Calling 'g'"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 5,
+                            "endLine": 25,
+                            "startColumn": 3,
+                            "startLine": 25
+                          }
+                        }
+                      }
+                    },
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "'fp' declared without an initial value"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 10,
+                            "endLine": 13,
+                            "startColumn": 3,
+                            "startLine": 13
+                          }
+                        }
+                      }
+                    },
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "Called function pointer is an uninitialized pointer value"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 8,
+                            "endLine": 14,
+                            "startColumn": 3,
+                            "startLine": 14
+                          }
+                        }
+                      }
+                    }
+                  ]
+                }
+              ]
+            }
+          ],
+          "locations": [
+            {
+              "physicalLocation": {
+                "fileLocation": {
+                  "uri": "file:sarif-multi-diagnostic-test.c"
+                },
+                "region": {
+                  "endColumn": 8,
+                  "endLine": 14,
+                  "startColumn": 3,
+                  "startLine": 14
+                }
+              }
+            }
+          ],
+          "message": {
+            "text": "Called function pointer is an uninitialized pointer value"
+          },
+          "ruleId": "core.CallAndMessage"
+        },
+        {
+          "codeFlows": [
+            {
+              "threadFlows": [
+                {
+                  "locations": [
+                    {
+                      "importance": "important",
+                      "location": {
+                        "message": {
+                          "text": "Assuming 'i' is equal to 0"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 12,
+                            "endLine": 18,
+                            "startColumn": 7,
+                            "startLine": 18
+                          }
+                        }
+                      }
+                    },
+                    {
+                      "importance": "unimportant",
+                      "location": {
+                        "message": {
+                          "text": "Taking true branch"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 3,
+                            "endLine": 18,
+                            "startColumn": 3,
+                            "startLine": 18
+                          }
+                        }
+                      }
+                    },
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "Division by zero"
+                        },
+                        "physicalLocation": {
+                          "fileLocation": {
+                            "uri": "file:sarif-multi-diagnostic-test.c"
+                          },
+                          "region": {
+                            "endColumn": 14,
+                            "endLine": 19,
+                            "startColumn": 14,
+                            "startLine": 19
+                          }
+                        }
+                      }
+                    }
+                  ]
+                }
+              ]
+            }
+          ],
+          "locations": [
+            {
+              "physicalLocation": {
+                "fileLocation": {
+                  "uri": "file:sarif-multi-diagnostic-test.c"
+                },
+                "region": {
+                  "endColumn": 14,
+                  "endLine": 19,
+                  "startColumn": 14,
+                  "startLine": 19
+                }
+              }
+            }
+          ],
+          "message": {
+            "text": "Division by zero"
+          },
+          "ruleId": "core.DivideZero"
+        }
+      ],
+      "tool": {
+        "fullName": "clang static analyzer",
+        "language": "en-US",
+        "name": "clang",
+        "version": "clang version 8.0.0 (trunk 345822) (llvm/trunk 345824)"
+      }
+    }
+  ],
+  "version": "2.0.0-csd.2.beta.2018-10-10"
+}

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 -U1 -w -I ".*file:.*sarif-diagnostics-taint-test.c" -I '"version":' -I "2\.0\.0\-csd\.[0-9]*\.beta\." - %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 - | %diff_sarif %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;
 }

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
+#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}}
+}
+
+void g(void) {
+  void (*fp)(int);
+  fp(12); // expected-warning {{Called function pointer is an uninitialized pointer value}}
+}
+
+int h(int i) {
+  if (i == 0)
+    return 1 / i; // expected-warning {{Division by zero}}
+  return 0;
+}
+
+int main(void) {
+  f();
+  g();
+  h(0);
+  return 0;
+}
+

test/Analysis/lit.local.cfg

 # -*- Python -*- vim: set ft=python ts=4 sw=4 expandtab tw=79:
 
 import site
 
 # Load the custom analyzer test format, which runs the test again with Z3 if it
 # is available.
 site.addsitedir(os.path.dirname(__file__))
 import analyzer_test
 config.test_format = analyzer_test.AnalyzerTest(
         config.test_format.execute_external, config.use_z3_solver)
 
 # Diff command used by Clang Analyzer tests (when comparing .plist files
 # with reference output)
 config.substitutions.append(('%diff_plist',
     'diff -u -w -I "<string>/" -I "<string>.:" -I "version" -'))
 
+# 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\." -'''))
+
 if not config.root.clang_staticanalyzer:
     config.unsupported = True