[analyzer] Fix SARIF column locations

[jranieri-grammatech]

Summary

This fixes SARIF column locations to be end-exclusive and be counted in terms of Unicode characters.

[aaron.ballman]

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

31

You don't really need the whole preprocessor, just the language options, right? If so, why not thread the language options through rather than the PP?

150

Can you add some text to the assertion to explain what's gone wrong if the assert triggers? e.g. assert(!Loc.isInvalid() && "invalid location when adjusting column position"); or something (same for the others asserts you've added)?

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

34

Formatting of the curly brace looks wrong.

35–36

What file encoding is used by this file (and is there a BOM)? Have you tried this test on multiple platforms by any chance?

[jranieri-grammatech]

Addressed review comments.

[jranieri-grammatech]

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

35–36

The file is encoded in UTF-8 without a BOM. I have not tried the test on other platforms.

[aaron.ballman]

Mostly LG aside from a question about an assertion. Adding other reviewers outside of GrammaTech in case there are other concerns we've missed.

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

157

I don't think this API is able to return a null buffer, but it can return an invalid buffer under legitimate circumstances, I believe. For instance, if the source location is in the scratch space used for things like macros defined on the command line with -D. I think you should pass the optional invalid parameter to the call to getBuffer() and then do no adjustments if the buffer is invalid. WDYT?

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

35–36

Okay, I think that's reasonable, thanks!

[jranieri-grammatech]

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

157

Good catch that this can't return NULL.

I tested out defining things on the command line, but this code deals with expansion locations and wasn't affected by it. Unless you can think of a way to reproduce it, I think I'd rather pass in the invalid argument and assert that it isn't invalid.

[aaron.ballman]

LGTM! I think we've had sufficient time for other reviewers to lodge concerns and we can deal with any other issues post-commit. Do you need me to commit on your behalf?

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

157

I think I'd rather pass in the invalid argument and assert that it isn't invalid.

That's fine by me -- I can't think of another way to reproduce it.

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 

 namespace {
 class SarifDiagnostics : public PathDiagnosticConsumer {
   std::string OutputFile;
+  const LangOptions &LO;
 
 public:
-  SarifDiagnostics(AnalyzerOptions &, const std::string &Output)
-      : OutputFile(Output) {}
+  SarifDiagnostics(AnalyzerOptions &, const std::string &Output,
+                   const LangOptions &LO)
+      : OutputFile(Output), LO(LO) {}
   ~SarifDiagnostics() override = default;
 
   void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,

 
 void ento::createSarifDiagnosticConsumer(
     AnalyzerOptions &AnalyzerOpts, PathDiagnosticConsumers &C,
-    const std::string &Output, const Preprocessor &,
+    const std::string &Output, const Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &) {
-  C.push_back(new SarifDiagnostics(AnalyzerOpts, Output));
+  C.push_back(new SarifDiagnostics(AnalyzerOpts, Output, PP.getLangOpts()));
 }
 
 static StringRef getFileName(const FileEntry &FE) {

   return json::Object{{"uri", FileURI}, {"index", Index}};
 }
 
-static json::Object createTextRegion(SourceRange R, const SourceManager &SM) {
+static unsigned int adjustColumnPos(const SourceManager &SM, SourceLocation Loc,
+                                    unsigned int TokenLen = 0) {
+  assert(!Loc.isInvalid() && "invalid Loc when adjusting column position");
+
+  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedExpansionLoc(Loc);
+  assert(LocInfo.second > SM.getExpansionColumnNumber(Loc) &&
+         "position in file is before column number?");
+
+  bool InvalidBuffer = false;
+  const MemoryBuffer *Buf = SM.getBuffer(LocInfo.first, &InvalidBuffer);
+  assert(!InvalidBuffer && "got an invalid buffer for the location's file");
+  assert(Buf->getBufferSize() >= (LocInfo.second + TokenLen) &&
+         "token extends past end of buffer?");
+
+  // Adjust the offset to be the start of the line, since we'll be counting
+  // Unicode characters from there until our column offset.
+  unsigned int Off = LocInfo.second - (SM.getExpansionColumnNumber(Loc) - 1);
+  unsigned int Ret = 1;
+  while (Off < (LocInfo.second + TokenLen)) {
+    Off += getNumBytesForUTF8(Buf->getBuffer()[Off]);
+    Ret++;
+  }
+
+  return Ret;
+}
+
+static json::Object createTextRegion(const LangOptions &LO, SourceRange R,
+                                     const SourceManager &SM) {
   json::Object Region{
       {"startLine", SM.getExpansionLineNumber(R.getBegin())},
-      {"startColumn", SM.getExpansionColumnNumber(R.getBegin())},
+      {"startColumn", adjustColumnPos(SM, R.getBegin())},
   };
   if (R.getBegin() == R.getEnd()) {
-    Region["endColumn"] = SM.getExpansionColumnNumber(R.getBegin());
+    Region["endColumn"] = adjustColumnPos(SM, R.getBegin());
   } else {
     Region["endLine"] = SM.getExpansionLineNumber(R.getEnd());
-    Region["endColumn"] = SM.getExpansionColumnNumber(R.getEnd()) + 1;
+    Region["endColumn"] = adjustColumnPos(
+        SM, R.getEnd(),
+        Lexer::MeasureTokenLength(R.getEnd(), SM, LO));
   }
   return Region;
 }
 
-static json::Object createPhysicalLocation(SourceRange R, const FileEntry &FE,
+static json::Object createPhysicalLocation(const LangOptions &LO,
+                                           SourceRange R, const FileEntry &FE,
                                            const SourceManager &SMgr,
                                            json::Array &Artifacts) {
   return json::Object{
       {{"artifactLocation", createArtifactLocation(FE, Artifacts)},
-       {"region", createTextRegion(R, SMgr)}}};
+       {"region", createTextRegion(LO, R, SMgr)}}};
 }
 
 enum class Importance { Important, Essential, Unimportant };

   return Importance::Unimportant;
 }
 
-static json::Object createThreadFlow(const PathPieces &Pieces,
+static json::Object createThreadFlow(const LangOptions &LO,
+                                     const PathPieces &Pieces,
                                      json::Array &Artifacts) {
   const SourceManager &SMgr = Pieces.front()->getLocation().getManager();
   json::Array Locations;

     const PathDiagnosticLocation &P = Piece->getLocation();
     Locations.push_back(createThreadFlowLocation(
         createLocation(createPhysicalLocation(
-                           P.asRange(),
+                           LO, P.asRange(),
                            *P.asLocation().getExpansionLoc().getFileEntry(),
                            SMgr, Artifacts),
                        Piece->getString()),

   return json::Object{{"locations", std::move(Locations)}};
 }
 
-static json::Object createCodeFlow(const PathPieces &Pieces,
+static json::Object createCodeFlow(const LangOptions &LO,
+                                   const PathPieces &Pieces,
                                    json::Array &Artifacts) {
   return json::Object{
-      {"threadFlows", json::Array{createThreadFlow(Pieces, Artifacts)}}};
+      {"threadFlows", json::Array{createThreadFlow(LO, Pieces, Artifacts)}}};
 }
 
-static json::Object createResult(const PathDiagnostic &Diag,
+static json::Object createResult(const LangOptions &LO,
+                                 const PathDiagnostic &Diag,
                                  json::Array &Artifacts,
                                  const StringMap<unsigned> &RuleMapping) {
   const PathPieces &Path = Diag.path.flatten(false);

 
   return json::Object{
       {"message", createMessage(Diag.getVerboseDescription())},
-      {"codeFlows", json::Array{createCodeFlow(Path, Artifacts)}},
+      {"codeFlows", json::Array{createCodeFlow(LO, Path, Artifacts)}},
       {"locations",
        json::Array{createLocation(createPhysicalLocation(
-           Diag.getLocation().asRange(),
+           LO, Diag.getLocation().asRange(),
            *Diag.getLocation().asLocation().getExpansionLoc().getFileEntry(),
            SMgr, Artifacts))}},
       {"ruleIndex", Iter->getValue()},

                               {"rules", createRules(Diags, RuleMapping)}}}};
 }
 
-static json::Object createRun(std::vector<const PathDiagnostic *> &Diags) {
+static json::Object createRun(const LangOptions &LO,
+                              std::vector<const PathDiagnostic *> &Diags) {
   json::Array Results, Artifacts;
   StringMap<unsigned> RuleMapping;
   json::Object Tool = createTool(Diags, RuleMapping);
   
   llvm::for_each(Diags, [&](const PathDiagnostic *D) {
-    Results.push_back(createResult(*D, Artifacts, RuleMapping));
+    Results.push_back(createResult(LO, *D, Artifacts, RuleMapping));
   });
 
   return json::Object{{"tool", std::move(Tool)},
                       {"results", std::move(Results)},
-                      {"artifacts", std::move(Artifacts)}};
+                      {"artifacts", std::move(Artifacts)},
+                      {"columnKind", "unicodeCodePoints"}};
 }
 
 void SarifDiagnostics::FlushDiagnosticsImpl(

       {"$schema",
        "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json"},
       {"version", "2.1.0"},
-      {"runs", json::Array{createRun(Diags)}}};
+      {"runs", json::Array{createRun(LO, Diags)}}};
   OS << llvm::formatv("{0:2}\n", json::Value(std::move(Sarif)));
 }

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

           ]
         }
       ],
+      "columnKind": "unicodeCodePoints",
       "results": [
         {
           "codeFlows": [

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

     {
       "artifacts": [
         {
-          "length": 951,
+          "length": 1077,
           "location": {
           },
           "mimeType": "text/plain",

           ]
         }
       ],
+      "columnKind": "unicodeCodePoints",
       "results": [
         {
           "codeFlows": [

                           },
                           "region": {
                             "endColumn": 6,
-                            "endLine": 34,
+                            "endLine": 39,
                             "startColumn": 3,
-                            "startLine": 34
+                            "startLine": 39
                           }
                         }
                       }

                           },
                           "region": {
                             "endColumn": 6,
-                            "endLine": 35,
+                            "endLine": 40,
                             "startColumn": 3,
-                            "startLine": 35
+                            "startLine": 40
                           }
                         }
                       }

                             "index": 0,
                           },
                           "region": {
-                            "endColumn": 11,
+                            "endColumn": 12,
                             "endLine": 15,
                             "startColumn": 3,
                             "startLine": 15

           },
           "ruleId": "unix.Malloc",
           "ruleIndex": 3
+        },
+        {
+          "codeFlows": [
+            {
+              "threadFlows": [
+                {
+                  "locations": [
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "'løçål' initialized to 0"
+                        },
+                        "physicalLocation": {
+                          "artifactLocation": {
+                            "index": 0,
+                          },
+                          "region": {
+                            "endColumn": 12,
+                            "endLine": 34,
+                            "startColumn": 3,
+                            "startLine": 34
+                          }
+                        }
+                      }
+                    },
+                    {
+                      "importance": "essential",
+                      "location": {
+                        "message": {
+                          "text": "Division by zero"
+                        },
+                        "physicalLocation": {
+                          "artifactLocation": {
+                            "index": 0,
+                          },
+                          "region": {
+                            "endColumn": 20,
+                            "startColumn": 20,
+                            "startLine": 35
+                          }
+                        }
+                      }
+                    }
+                  ]
+                }
+              ]
+            }
+          ],
+          "locations": [
+            {
+              "physicalLocation": {
+                "artifactLocation": {
+                  "index": 0,
+                },
+                "region": {
+                  "endColumn": 20,
+                  "startColumn": 20,
+                  "startLine": 35
+                }
+              }
+            }
+          ],
+          "message": {
+            "text": "Division by zero"
+          },
+          "ruleId": "core.DivideZero",
+          "ruleIndex": 2
         }
       ],
       "tool": {

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

   return 0;
 }
 
+int unicode() {
+  int løçål = 0;
+  /* ☃ */ return 1 / løçål; // expected-warning {{Division by zero}}
+}
+
 int main(void) {
   f();
   g();
   h(0);
   leak(0);
+  unicode();
   return 0;
 }