Page MenuHomePhabricator

Allow the analyzer to output to a SARIF file
ClosedPublic

Authored by aaron.ballman on Oct 29 2018, 6:33 AM.

Details

Summary

SARIF (https://github.com/oasis-tcs/sarif-spec) is a new draft standard interchange format for static analysis results that allows result viewers to be decoupled from the tool producing the analysis results. This patch allows users to specify SARIF as the output from the clang static analyzer so that the results can be read in by other tools. There are several such tools for consuming SARIF, such as extensions to Visual Studio and VSCode, as well as static analyzers like CodeSonar.

SARIF is JSON-based and the latest provisional specification can be found at: https://github.com/oasis-tcs/sarif-spec/blob/master/Documents/ProvisionalDrafts/sarif-v2.0-csd02-provisional.docx. GrammaTech sponsored the work to produce this patch and we will make any necessary changes if the draft standard changes before publication.

Diff Detail

Event Timeline

aaron.ballman created this revision.Oct 29 2018, 6:33 AM
ZaMaZaN4iK added inline comments.
StaticAnalyzer/Core/CMakeLists.txt
43

Sort alphabetically

StaticAnalyzer/Core/SarifDiagnostics.cpp
89

I don't know about const corectness policy in LLVM. But I prefer here const char C .

95

Probably this piece of code will be better to write separately as function

Patch context is missing.

Analysis/diagnostics/sarif-check.py
22 ↗(On Diff #171490)

Wow, this is super neat!
Since you are quite active in LLVM community, would you think it's better to have this tool in llvm/utils next to FileCheck? The concept is very general, and I think a lot of people really feel FileCheck limitations.

Analysis/diagnostics/sarif-diagnostics-taint-test.c
19

Would it make more sense to just use diff + json pretty-formatter to write a test?
With this test I can't even quite figure out how the output should look like.

StaticAnalyzer/Core/SarifDiagnostics.cpp
75

Nitpicking style, but I don't see why for-each loop, preferably with a range wrapping the iterators would not be more readable.

183

"Note" are notes which do not have to be attached to a particular path element.

244

I like closures, but what's wrong with just using a for loop here?

255

Usually we overwrite the file and note that on stderr in such cases.

george.karpenkov requested changes to this revision.Oct 29 2018, 9:20 AM

Minor style notes + context missing.

I think using diff would be better than a custom python tool.

This revision now requires changes to proceed.Oct 29 2018, 9:20 AM
aaron.ballman added inline comments.Oct 29 2018, 10:07 AM
Analysis/diagnostics/sarif-check.py
22 ↗(On Diff #171490)

The concept was pulled from test\TableGen\JSON-check.py, so it likely could be generalized. However, I suspect that each JSON test may want to expose different helper capabilities, so perhaps it won't be super general? I don't know enough about good Python design to know.

Analysis/diagnostics/sarif-diagnostics-taint-test.c
19

I'm not super comfortable with that approach, but perhaps I'm thinking of something different than what you're proposing. The reason I went with this approach is because diff would be fragile (depends heavily on field ordering, which the JSON support library doesn't give control over) and the physical layout of the file isn't what needs to be tested anyway. SARIF has a fair amount of optional data that can be provided as well, so using a purely textual diff worried me that exporting additional optional data in the future would require extensive unrelated changes to all SARIF diffs in the test directory.

The goal for this test was to demonstrate that we can validate that the interesting bits of information are present in the output without worrying about the details.

Also, the python approach allows us to express relationships between data items more easily than a textual diff tool would. I've not used that here, but I could imagine a test where we want to check that each code location has a corresponding file entry in another list.

StaticAnalyzer/Core/SarifDiagnostics.cpp
75

I tend to prefer using algorithms when the logic is simple -- it makes it more clear that the loop is an unimportant detail. I don't have strong opinions on this particular loop, however.

89

We typically do not put top-level const on locals, so I'd prefer to leave it off here rather than be inconsistent.

183

Good to know!

244

Same as above: clarity of exposition. This one I'd feel pretty strongly about keeping as an algorithm given how trivial the loop body is.

255

We took the decision internally to overwrite as well, but the SARIF format allows for multiple runs within the same output file (so you can compare analysis results for the same project over time). I think this will eventually have to be user-controlled because I can see some users wanting to append to the run and others wanting to overwrite. However, these log files can become quite large in practice (GBs of data), so "read in the JSON and add to it" may be implausible, hence why I punted for now.

I'll update the comment so it's not a FIXME.

aaron.ballman marked 3 inline comments as done.

Updated based on initial review feedback, and added more context to the patch.

george.karpenkov requested changes to this revision.Oct 29 2018, 2:57 PM

I don't think a new PathGenerationScheme is needed, unless you plan changes to BugReporter.cpp.

The code is fine otherwise, but could we try to use diff for testing?

Analysis/diagnostics/sarif-diagnostics-taint-test.c
19

Using a sample file + diff would have an advantage of being easier to read (just glance at the "Inputs/blah.serif" to see a sample output), and consistent with how we already do checking for plists.

depends heavily on field ordering

Is it an issue in practice though? I would assume that JSON support library would not switch field ordering too often (and even if it does, we can have a python wrapper testing that)

SARIF has a fair amount of optional data

Would diff --ignore-matching-lines be enough for those?

StaticAnalyzer/Core/SarifDiagnostics.cpp
70

+1, I would use this in other consumers.

clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
128

Do you actually need a new generation scheme here?
I'm pretty sure that using "Minimal" would give you the same effect.

This revision now requires changes to proceed.Oct 29 2018, 2:57 PM
MTC added a subscriber: MTC.Oct 29 2018, 7:48 PM
aaron.ballman marked 9 inline comments as done.Oct 30 2018, 6:20 AM
aaron.ballman added inline comments.
Analysis/diagnostics/sarif-diagnostics-taint-test.c
19

Diff testing was what I originally tried and I abandoned it because it was not viable. The optional data cannot be handled by ignoring matching lines because the lines won't match from system to system. For instance, there are absolute URIs to files included in the output, clang git hashes and version numbers that will change from revision to revision, etc.

What you see as an advantage, I see as more of a distraction -- the actual layout of the information in the file isn't that important so long as it validates as valid SARIF (unfortunately, there are no cross-platform command line tools to validate SARIF yet). What is important are just a few pieces of the content information (where are the diagnostics, what source ranges and paths are involved, etc) compared to the overall whole.

I can give diff testing another shot, but I was unable to find a way to make -I work so that I could ignore everything that needs to be ignored due to natural variance. Do you have ideas there?

To give you an idea of the ultimate form of the output:

{
  "$schema": "http://json.schemastore.org/sarif-2.0.0",
  "runs": [
    {
      "files": {
        "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c": {
          "fileLocation": {
            "uri": "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
          },
          "length": 3850,
          "mimeType": "text/plain",
          "roles": [
            "resultFile"
          ]
        }
      },
      "results": [
        {
          "codeFlows": [
            {
              "threadFlows": [
                {
                  "locations": [
                    {
                      "importance": "essential",
                      "location": {
                        "message": {
                          "text": "Calling 'f'"
                        },
                        "physicalLocation": {
                          "fileLocation": {
                            "uri": "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
                          },
                          "region": {
                            "endColumn": 5,
                            "endLine": 119,
                            "startColumn": 3,
                            "startLine": 119
                          }
                        }
                      },
                      "step": 1
                    },
                    {
                      "importance": "essential",
                      "location": {
                        "message": {
                          "text": "tainted"
                        },
                        "physicalLocation": {
                          "fileLocation": {
                            "uri": "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
                          },
                          "region": {
                            "endColumn": 17,
                            "endLine": 115,
                            "startColumn": 11,
                            "startLine": 115
                          }
                        }
                      },
                      "step": 2
                    }
                  ]
                }
              ]
            }
          ],
          "locations": [
            {
              "physicalLocation": {
                "fileLocation": {
                  "uri": "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
                },
                "region": {
                  "endColumn": 17,
                  "endLine": 115,
                  "startColumn": 11,
                  "startLine": 115
                }
              }
            }
          ],
          "message": {
            "text": "tainted"
          },
          "ruleId": "debug.TaintTest"
        }
      ],
      "tool": {
        "fullName": "clang static analyzer",
        "language": "en-US",
        "name": "clang",
        "version": "clang version 8.0.0 (https://github.com/llvm-project/clang.git a5ccb257a7a70928ede717a7c282f5fc8cbed310) (https://github.com/llvm-mirror/llvm.git 73cebd79c512f7129eca16b0f3a7abd21d2881e8)"
      }
    }
  ],
  "version": "2.0.0-beta.2018-09-26"
}

In this file, the variable things are: the file URIs in multiple places, the clang version information, and to a lesser extent, the SARIF version information (we care that it says 2.0.0 but don't care about the -beta stuff). Other pieces of information are likely to become variable in the future (like the language field). This log file is ~100 lines long for displaying information about one diagnostic with two interesting locations on the code path, which is why I think the diff testing is a distraction. I believe that more complex examples will result in even larger output files.

I might be able to use FileCheck more directly, e.g.,

CHECK: "threadFlows":
CHECK-NEXT: "locations":
CHECK-NEXT: "{"
CHECK-NEXT: "importance": "essential"
...

But I feel like that's just a more verbose version of what's being done in the patch with less flexibility.

StaticAnalyzer/Core/SarifDiagnostics.cpp
70

Not certain I understand this comment.

clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
128

I don't think it's currently needed, I've removed but updated the comment.

Updated based on review feedback -- removed the Sarif path generation scheme as it isn't currently needed, and replaced a FIXME with a better comment.

Testing remains an open question, however.

Switched to using diff for testing.

aaron.ballman added inline comments.Oct 30 2018, 8:39 AM
Analysis/diagnostics/sarif-diagnostics-taint-test.c
19

I was finally able to convince diff to do what I needed. You have to list a fair amount of things to ignore on the RUN line and there's a disconnect between the input and the expected output.

One odd behavior I noticed was that splitting this into two RUN lines (so diff is its own RUN line) fails on my system because diff then does not understand the -I option! Using the pipe syntax seems to work for me. I think lit is somehow finding two different diff programs.

I'm still not convinced this an improvement over the previous style of testing, but at least we have something working for comparison purposes now.

george.karpenkov accepted this revision.Oct 30 2018, 9:07 AM

I much prefer this version.
We've had the same problem with diffing plist output.
One thing we have learned is using a FileCheck was definitely a bad idea, as it leads to unreadable, unmaintainable, and very hard to update tests,
so either diff or your custom tool is way better.

As for the ultimate solution, I'm still not sure: I agree that maintaining those -I flags is annoying.
One option is having an extra flag to produce "stable" output, which does not include absolute URLs/versions/etc.

This revision is now accepted and ready to land.Oct 30 2018, 9:07 AM

I much prefer this version.
We've had the same problem with diffing plist output.
One thing we have learned is using a FileCheck was definitely a bad idea, as it leads to unreadable, unmaintainable, and very hard to update tests,
so either diff or your custom tool is way better.

As for the ultimate solution, I'm still not sure: I agree that maintaining those -I flags is annoying.

We can go with this approach until we need something more complicated. I suspect that as we add SARIF features, we may want to bring back the Python script for handling things like "Does every file in the 'files' list appear only once and do the files listed correspond exactly to ones in the diagnostic locations?". Diff definitely won't handle that sort of thing.

One option is having an extra flag to produce "stable" output, which does not include absolute URLs/versions/etc.

Worth thinking about. SARIF has the ability to output relative paths as well as absolute paths. It also has the notion of redacted paths so that you can remove sensitive information from analysis reports. So there's plenty of room for changes here.

Thank you for the reviews! I've commit in r345628.