[PATCH] D53814: Allow the analyzer to output to a SARIF file
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 30 08:39:42 PDT 2018
aaron.ballman added inline comments.
================
Comment at: Analysis/diagnostics/sarif-diagnostics-taint-test.c:18
+// Test the basics for sanity.
+// CHECK: sarifLog['version'].startswith("2.0.0")
+// CHECK: sarifLog['runs'][0]['tool']['fullName'] == "clang static analyzer"
----------------
aaron.ballman wrote:
> george.karpenkov wrote:
> > aaron.ballman wrote:
> > > george.karpenkov wrote:
> > > > 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.
> > > 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.
> > 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?
> 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.
>
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.
https://reviews.llvm.org/D53814
More information about the cfe-commits
mailing list