[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/Analysis/PathDiagnostic.h"
 #include "clang/Basic/Version.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #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"
 
 using namespace llvm;
 using namespace clang;
 using namespace ento;
 
 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,
                             FilesMade *FM) override;
 
   StringRef getName() const override { return "SarifDiagnostics"; }
   PathGenerationScheme getGenerationScheme() const override { return Minimal; }
   bool supportsLogicalOpControlFlow() const override { return true; }
   bool supportsCrossFileDiagnostics() const override { return true; }
 };
 } // end anonymous namespace
 
 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) {
   StringRef Filename = FE.tryGetRealPathName();
   if (Filename.empty())
     Filename = FE.getName();
   return Filename;
 }
static json::Object createArtifactLocation(const FileEntry &FE,
   // the JSON object.
   auto Index = static_cast<unsigned>(std::distance(Artifacts.begin(), I));
   if (I == Artifacts.end())
     Artifacts.push_back(createArtifact(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 };
 
 static StringRef importanceToStr(Importance I) {
   switch (I) {
   case Importance::Important:
     return "important";
case PathDiagnosticPiece::Event:
     return Piece.getTagStr() == "ConditionBRVisitor" ? Importance::Important
                                                      : Importance::Essential;
   case PathDiagnosticPiece::ControlFlow:
     return Importance::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;
   for (const auto &Piece : Pieces) {
     const PathDiagnosticLocation &P = Piece->getLocation();
     Locations.push_back(createThreadFlowLocation(
         createLocation(createPhysicalLocation(
-                           P.asRange(),
+                           LO, P.asRange(),
                            *P.asLocation().getExpansionLoc().getFileEntry(),
                            SMgr, Artifacts),
                        Piece->getString()),
         calculateImportance(*Piece)));
   }
   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);
   const SourceManager &SMgr = Path.front()->getLocation().getManager();
 
   auto Iter = RuleMapping.find(Diag.getCheckerName());
   assert(Iter != RuleMapping.end() && "Rule ID is not in the array index map?");
 
   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()},
       {"ruleId", Diag.getCheckerName()}};
 }
 
 static StringRef getRuleDescription(StringRef CheckName) {
   return llvm::StringSwitch<StringRef>(CheckName)
static json::Object createTool(std::vector<const PathDiagnostic *> &Diags,
   return json::Object{
       {"driver", json::Object{{"name", "clang"},
                               {"fullName", "clang static analyzer"},
                               {"language", "en-US"},
                               {"version", getClangFullVersion()},
                               {"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(
     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
   // existing SARIF file. One danger from that approach is that the size of the
   // file can become large very quickly, so decoding into JSON to append a run
   // may be an expensive operation.
   std::error_code EC;
   llvm::raw_fd_ostream OS(OutputFile, EC, llvm::sys::fs::OF_Text);
   if (EC) {
     llvm::errs() << "warning: could not create file: " << EC.message() << '\n';
     return;
   }
   json::Object Sarif{
       {"$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

 {
   "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
   "runs": [
     {
       "artifacts": [
         {
           "length": 434,
           "location": {
           },
           "mimeType": "text/plain",
           "roles": [
             "resultFile"
           ]
         }
       ],
+      "columnKind": "unicodeCodePoints",
       "results": [
         {
           "codeFlows": [
             {
               "threadFlows": [
                 {
                   "locations": [
                     {

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

 {
   "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
   "runs": [
     {
       "artifacts": [
         {
-          "length": 951,
+          "length": 1077,
           "location": {
           },
           "mimeType": "text/plain",
           "roles": [
             "resultFile"
           ]
         }
       ],
+      "columnKind": "unicodeCodePoints",
       "results": [
         {
           "codeFlows": [
             {
               "threadFlows": [
                 {
                   "locations": [
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "Calling 'f'"
                         },
                         "physicalLocation": {
                           "artifactLocation": {
                             "index": 0,
                           },
                           "region": {
                             "endColumn": 6,
-                            "endLine": 34,
+                            "endLine": 39,
                             "startColumn": 3,
-                            "startLine": 34
+                            "startLine": 39
                           }
                         }
                       }
                     },
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
"runs": [
                           "text": "Calling 'g'"
                         },
                         "physicalLocation": {
                           "artifactLocation": {
                             "index": 0,
                           },
                           "region": {
                             "endColumn": 6,
-                            "endLine": 35,
+                            "endLine": 40,
                             "startColumn": 3,
-                            "startLine": 35
+                            "startLine": 40
                           }
                         }
                       }
                     },
                     {
                       "importance": "essential",
                       "location": {
                         "message": {
                           "text": "'fp' declared without an initial value"
                         },
                         "physicalLocation": {
                           "artifactLocation": {
                             "index": 0,
                           },
                           "region": {
-                            "endColumn": 11,
+                            "endColumn": 12,
                             "endLine": 15,
                             "startColumn": 3,
                             "startLine": 15
                           }
                         }
                       }
                     },
                     {
"runs": [
               }
             }
           ],
           "message": {
             "text": "Potential leak of memory pointed to by 'mem'"
           },
           "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": {
         "driver": {
           "fullName": "clang static analyzer",
           "language": "en-US",
           "name": "clang",
           "rules": [

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

 int leak(int i) {
   void *mem = malloc(8);
   if (i < 4)
     return ERR; // expected-warning {{Potential leak of memory pointed to by 'mem'}}
   free(mem);
   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;
 }