[analyzer] Fix SARIF column location assertion crash

[Scarlet1ssimo]

Summary

It meant to fix #60541

LocInfo is 0 based, but column number is 1 based. So in the corner case,
LocInfo.second + 1 == SM.getExpansionColumnNumber(Loc)

[steakhal]

Looks sensible to me.
Do you have a test for triggering the previous assertion?

[Scarlet1ssimo]

In D143867#4121835, @steakhal wrote:

Looks sensible to me.
Do you have a test for triggering the previous assertion?

I do have prepared a test case like:

int testA(void) { return 0/0; }
// RUN: %clang_analyze_cc1 -analyzer-checker=core %s -analyzer-output=sarif -o -

Notice you must put the buggy code at the very first line to trigger the assertion.

Do you think it's necessary to put this test case into the test suite?

[steakhal]

In D143867#4121841, @Scarlet1ssimo wrote:

In D143867#4121835, @steakhal wrote:

Looks sensible to me.
Do you have a test for triggering the previous assertion?

I do have prepared a test case like:

int testA(void) { return 0/0; }
// RUN: %clang_analyze_cc1 -analyzer-checker=core %s -analyzer-output=sarif -o -

Notice you must put the buggy code at the very first line to trigger the assertion.

The test passes on main. Are you sure about the reproducer?
I copy-pasted your code as-is into a test file, but check-clang-analysis still passes.

Do you think it's necessary to put this test case into the test suite?

Generally, we only merge changes with tests, but I'm not the one who sets the rules.

[Scarlet1ssimo]

The test passes on main. Are you sure about the reproducer?

This happens on versions from release/10.x to release/15.x. Since 16.x, this file gets refactored and no such problem ever.
I'm new to llvm. Are we still responsible for older versions?

[steakhal]

In D143867#4121874, @Scarlet1ssimo wrote:

The test passes on main. Are you sure about the reproducer?

This happens on versions from release/10.x to release/15.x. Since 16.x, this file gets refactored and no such problem ever.
I'm new to llvm. Are we still responsible for older versions?

We don't plan to backport a change like this to previous versions.
Thanks for your report and investigation though.

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

static json::Object createArtifactLocation(const FileEntry &FE,
   return json::Object{{"uri", FileURI}, {"index", Index}};
 }
 
 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) &&
+  assert(LocInfo.second + 1 >= SM.getExpansionColumnNumber(Loc) &&
          "position in file is before column number?");
 
   Optional<MemoryBufferRef> Buf = SM.getBufferOrNone(LocInfo.first);
   assert(Buf && "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