[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 440270.
vaibhav.y added a comment.

Factor dependency on `Lexer::MeasureTokenLength` into externally provided 
functor

Introduces a type: `TokenLengthMetric` which measures the length of a token
starting at the given SLoc


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,321 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+static unsigned int MeasureTokenLengthAt(FullSourceLoc Loc,
+ const LangOptions &LangOpts) {
+  return Lexer::MeasureTokenLength(Loc, Loc.getManager(), LangOpts);
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  FullSourceRange getFakeFullSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return FullSourceRange{FullSourceLoc{BeginLoc, SourceMgr},
+   FullSourceLoc{EndLoc, SourceMgr}};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts, MeasureTokenLengthAt};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts, MeasureTokenLengthAt};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray(

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Basic/Sarif.cpp:161
+Region["endColumn"] = adjustColumnPos(
+R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0),
+  R.getEnd().getManager(), LO));

aaron.ballman wrote:
> vaibhav.y wrote:
> > aaron.ballman wrote:
> > > I didn't catch this during the review -- but this is a layering violation 
> > > that caused link errors on some of the build bots. Lexer can call into 
> > > Basic, but Basic cannot call into Lexer. So we'll need to find a 
> > > different way to handle this.
> > Would moving the code to Support, having it depend on Basic & Lex work?
> I don't think so -- Support is supposed to be a pretty low-level interface; 
> it currently only relies on LLVM's Support library. I think the layering is 
> supposed to be: Support -> Basic -> Lex.
> 
> As I see it, there are a couple of options as to where this could live. It 
> could live in the Frontend library, as that's where all the diagnostic 
> consumer code in Clang lives. But that library might be a bit "heavy" to pull 
> into other tools (maybe? I don't know). It could also live in AST -- that 
> already links in Basic and Lex. But that feels like a somewhat random place 
> for this to live as this has very little to do with the AST itself.
> 
> Another approach, which might be better, is to require the user of this 
> interface to pass in the token length calculation themselves in the places 
> where it's necessary. e.g., `json::Object whatever(SourceLocation Start, 
> SourceLocation End, unsigned EndLen)` and then you can remove the reliance on 
> the lexer entirely while keeping the interface in Basic. I'm not certain how 
> obnoxious this suggestion is, but I think it's my preferred approach for the 
> moment (but not a strongly held position yet). WDYT of this approach?
I think the approach to injecting the function is better here. I've tried to 
make the smallest change possiblew with passing in a function whose interface 
is almost identical to `Lexer::MeasureTokenLength`. The intent was to hint at 
this being the "canonical metric" for token lengths (with an example in the 
tests for the same).

I tried passing start, end locs but couldn't find a strong use case yet since 
`end` would likely always be: `Lexer::getLocForEndOfToken(start, 0)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 440279.
vaibhav.y added a comment.

Discard unused includes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,321 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+static unsigned int MeasureTokenLengthAt(FullSourceLoc Loc,
+ const LangOptions &LangOpts) {
+  return Lexer::MeasureTokenLength(Loc, Loc.getManager(), LangOpts);
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  FullSourceRange getFakeFullSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return FullSourceRange{FullSourceLoc{BeginLoc, SourceMgr},
+   FullSourceLoc{EndLoc, SourceMgr}};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts, MeasureTokenLengthAt};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts, MeasureTokenLengthAt};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The to

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Basic/Sarif.cpp:161
+Region["endColumn"] = adjustColumnPos(
+R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0),
+  R.getEnd().getManager(), LO));

aaron.ballman wrote:
> vaibhav.y wrote:
> > aaron.ballman wrote:
> > > vaibhav.y wrote:
> > > > aaron.ballman wrote:
> > > > > I didn't catch this during the review -- but this is a layering 
> > > > > violation that caused link errors on some of the build bots. Lexer 
> > > > > can call into Basic, but Basic cannot call into Lexer. So we'll need 
> > > > > to find a different way to handle this.
> > > > Would moving the code to Support, having it depend on Basic & Lex work?
> > > I don't think so -- Support is supposed to be a pretty low-level 
> > > interface; it currently only relies on LLVM's Support library. I think 
> > > the layering is supposed to be: Support -> Basic -> Lex.
> > > 
> > > As I see it, there are a couple of options as to where this could live. 
> > > It could live in the Frontend library, as that's where all the diagnostic 
> > > consumer code in Clang lives. But that library might be a bit "heavy" to 
> > > pull into other tools (maybe? I don't know). It could also live in AST -- 
> > > that already links in Basic and Lex. But that feels like a somewhat 
> > > random place for this to live as this has very little to do with the AST 
> > > itself.
> > > 
> > > Another approach, which might be better, is to require the user of this 
> > > interface to pass in the token length calculation themselves in the 
> > > places where it's necessary. e.g., `json::Object whatever(SourceLocation 
> > > Start, SourceLocation End, unsigned EndLen)` and then you can remove the 
> > > reliance on the lexer entirely while keeping the interface in Basic. I'm 
> > > not certain how obnoxious this suggestion is, but I think it's my 
> > > preferred approach for the moment (but not a strongly held position yet). 
> > > WDYT of this approach?
> > I think the approach to injecting the function is better here. I've tried 
> > to make the smallest change possiblew with passing in a function whose 
> > interface is almost identical to `Lexer::MeasureTokenLength`. The intent 
> > was to hint at this being the "canonical metric" for token lengths (with an 
> > example in the tests for the same).
> > 
> > I tried passing start, end locs but couldn't find a strong use case yet 
> > since `end` would likely always be: `Lexer::getLocForEndOfToken(start, 0)`
> I'm not convinced that the less obtrusive change is a good design in this 
> case. But I also agree that we should not use start/end *locations* either. 
> `SourceLocation` traditionally points to the *start* of a token, so it would 
> be super easy to get the `end` location wrong by forgetting to pass the 
> location for the end of the token.
> 
> My suggestion was to continue to pass the start of the starting token, the 
> start of the ending token, and the length of the ending token. With the 
> callback approach, you have to call through the callback to eventually call 
> `Lexer::MeasureTokenLength()`; with the direct approach, you skip needing to 
> call through a callback (which means at least one less function call on every 
> source location operation).
Ah, I think I misunderstood your initial suggestion (`json::Object 
whatever(SourceLocation Start, SourceLocation End, unsigned EndLen)`) seemed 
like a function call to me, when it seems the suggested change was to pass in 
an object? Apologies, will fix that up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y marked an inline comment as not done.
vaibhav.y added a comment.

Comment isn't done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Basic/Sarif.cpp:161
+Region["endColumn"] = adjustColumnPos(
+R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0),
+  R.getEnd().getManager(), LO));

aaron.ballman wrote:
> vaibhav.y wrote:
> > aaron.ballman wrote:
> > > vaibhav.y wrote:
> > > > aaron.ballman wrote:
> > > > > vaibhav.y wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I didn't catch this during the review -- but this is a layering 
> > > > > > > violation that caused link errors on some of the build bots. 
> > > > > > > Lexer can call into Basic, but Basic cannot call into Lexer. So 
> > > > > > > we'll need to find a different way to handle this.
> > > > > > Would moving the code to Support, having it depend on Basic & Lex 
> > > > > > work?
> > > > > I don't think so -- Support is supposed to be a pretty low-level 
> > > > > interface; it currently only relies on LLVM's Support library. I 
> > > > > think the layering is supposed to be: Support -> Basic -> Lex.
> > > > > 
> > > > > As I see it, there are a couple of options as to where this could 
> > > > > live. It could live in the Frontend library, as that's where all the 
> > > > > diagnostic consumer code in Clang lives. But that library might be a 
> > > > > bit "heavy" to pull into other tools (maybe? I don't know). It could 
> > > > > also live in AST -- that already links in Basic and Lex. But that 
> > > > > feels like a somewhat random place for this to live as this has very 
> > > > > little to do with the AST itself.
> > > > > 
> > > > > Another approach, which might be better, is to require the user of 
> > > > > this interface to pass in the token length calculation themselves in 
> > > > > the places where it's necessary. e.g., `json::Object 
> > > > > whatever(SourceLocation Start, SourceLocation End, unsigned EndLen)` 
> > > > > and then you can remove the reliance on the lexer entirely while 
> > > > > keeping the interface in Basic. I'm not certain how obnoxious this 
> > > > > suggestion is, but I think it's my preferred approach for the moment 
> > > > > (but not a strongly held position yet). WDYT of this approach?
> > > > I think the approach to injecting the function is better here. I've 
> > > > tried to make the smallest change possiblew with passing in a function 
> > > > whose interface is almost identical to `Lexer::MeasureTokenLength`. The 
> > > > intent was to hint at this being the "canonical metric" for token 
> > > > lengths (with an example in the tests for the same).
> > > > 
> > > > I tried passing start, end locs but couldn't find a strong use case yet 
> > > > since `end` would likely always be: `Lexer::getLocForEndOfToken(start, 
> > > > 0)`
> > > I'm not convinced that the less obtrusive change is a good design in this 
> > > case. But I also agree that we should not use start/end *locations* 
> > > either. `SourceLocation` traditionally points to the *start* of a token, 
> > > so it would be super easy to get the `end` location wrong by forgetting 
> > > to pass the location for the end of the token.
> > > 
> > > My suggestion was to continue to pass the start of the starting token, 
> > > the start of the ending token, and the length of the ending token. With 
> > > the callback approach, you have to call through the callback to 
> > > eventually call `Lexer::MeasureTokenLength()`; with the direct approach, 
> > > you skip needing to call through a callback (which means at least one 
> > > less function call on every source location operation).
> > Ah, I think I misunderstood your initial suggestion (`json::Object 
> > whatever(SourceLocation Start, SourceLocation End, unsigned EndLen)`) 
> > seemed like a function call to me, when it seems the suggested change was 
> > to pass in an object? Apologies, will fix that up.
> Sorry for the confusion! Just to make sure we're on the same page -- my 
> suggestion was to change the function interfaces like 
> `SarficDocumentWriter::createPhysicalLocation()` so that they would take an 
> additional `unsigned EndLen` parameter.
> 
> However, now that I dig around a bit, it seems like `CharSourceRange` is what 
> you'd want to use there -- then you can assert that what you're given is a 
> char range and not a token range. So you won't need the `unsigned EndLen` 
> parameter after all!
Interesting!

Asking for my understanding: If a `CharSourceRange` is a valid character range, 
 then the `End` SLoc points to the last character in the range (even if it is 
mid token)? (Unlike slocs where it the first character of the last token).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 440371.
vaibhav.y added a comment.

Use `CharSourceRange` instead of FullSourceRange so we no longer need to compute
the length of the last token. This should already be encoded in the end location
of the `CharSourceRange` by the caller

TODO(@vaibhav.y): Once accepted, drop the commit that introduces: 
`clang:i:FullSourceRange`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,320 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  CharSourceRange getFakeCharSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::jso

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 440374.
vaibhav.y added a comment.

Discard include: `clang/Lex/Lexer.h`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,320 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  CharSourceRange getFakeCharSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/include/clang/Basic/Sarif.h:167
+
+  ThreadFlow setRange(const CharSourceRange &ItemRange) {
+Range = ItemRange;

aaron.ballman wrote:
> Should we assert this source range is not a token range?
I don't have a strong opinion here (since these are validated downstream in 
`createPhysicalLocation`) but it makes sense to be defensive & assert early.

I'll preserve the downstream one as well in case a new type gets added that 
feeds data into `createPhysicalLocation` as well.



Comment at: clang/include/clang/Basic/Sarif.h:280
+  SarifResult setLocations(llvm::ArrayRef DiagLocs) {
+Locations.assign(DiagLocs.begin(), DiagLocs.end());
+return *this;

aaron.ballman wrote:
> In an asserts build, should we additionally have a loop to assert that each 
> location is a char range rather than a token range?
Will do, I had the asserts in `createPhysicalLocation` initially. Adding them 
at the site of creation makes sense as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 441130.
vaibhav.y added a comment.

Assert that `CharSourceRange`s passed to `Threadflow`, `SarifResult` are
character Ranges.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,320 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  CharSourceRange getFakeCharSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getStrin

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

In D109701#3622983 , @aaron.ballman 
wrote:

> I had to roll it back because of failures with test bots:
>
> https://lab.llvm.org/buildbot/#/builders/91/builds/11328
>
> So this was reverted in b46ad1b5be694feefabd4c6cd112cbbd04a7b3a7 
> , can 
> you take a look when you get the chance?

Odd! I'll try to reproduce that at the earliest.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y created this revision.
vaibhav.y added reviewers: aaron.ballman, cjdb, denik, abrahamcd, dbeer1.
Herald added a project: All.
vaibhav.y edited the summary of this revision.
vaibhav.y edited the summary of this revision.
vaibhav.y updated this revision to Diff 449713.
vaibhav.y added a comment.
vaibhav.y updated this revision to Diff 449717.
vaibhav.y updated this revision to Diff 449718.
vaibhav.y updated this revision to Diff 449721.
vaibhav.y edited projects, added clang; removed All.
vaibhav.y added a subscriber: clang.
Herald added a project: All.
vaibhav.y removed a project: All.
Herald added a project: All.
vaibhav.y published this revision for review.
Herald added a subscriber: cfe-commits.

Update diff


vaibhav.y added a comment.

Rebase on main, set result level to warning by default.


vaibhav.y added a comment.

- Add differential revision to individual commits
- Remove unused header


vaibhav.y added a comment.

Fix typo in comment


vaibhav.y added a comment.

Submitting for review:

Some notes:

There are a couple of ways I think we can acheive this, per the spec:

1. The reportingDescriptor (rule) objects can be given a default configuration 
property 
,
 which can set the default warning level and other data such as rule parameters 
etc.
2. The reportingDescriptor objects can omit the default configuration (which 
then allows operating with warning as default), and the level is then set when 
the result is reported.

The first approach would be "more correct", what are your thoughts on this? 
Would we benefit from having per-diagnostic configuration?

There is also the question about the "kind" of results in clang. From my 
reading of the spec 
,
 it seems that "fail" is the only case that applies to us because:

- "pass": Implies no issue was found.
- "open": This value is used by proof-based tools. It could also mean 
additional assertions required
- "informational": The specified rule was evaluated and produced a purely 
informational result that does not indicate the presence of a problem
- "notApplicable": The rule specified by ruleId was not evaluated, because it 
does not apply to the analysis target.

Of these "open" and "notApplicable" seem to be ones that *could* come to use 
but I'm not sure where / what kind of diagnostics would use these. Probably 
clang-tidy's `bugprone-*` suite?

Let me know what you think is a good way to approach this wrt clang's 
diagnostics system.


Previously it was not possible to specify the level of a SarifResult.

This changeset adds support for setting the `level` property[1] of a result. If 
unset, it defaults to "warning", which is the result of an empty default 
configuration on rules[2].

[1]: 
https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317648
[2]: 
https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317855

Test Plan: Updated test cases in BasicTests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131084

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -7,7 +7,6 @@
 //===--===//
 
 #include "clang/Basic/Sarif.h"
-#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
@@ -202,7 +201,7 @@
 TEST_F(SarifDocumentWriterTest, checkSerializingResults) {
   // GIVEN:
   const std::string ExpectedOutput =
-  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[],"columnKind":"unicodeCodePoints","results":[{"message":{"text":""},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[],"columnKind":"unicodeCodePoints","results":[{"level":"warning","message":{"text":""},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang 

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

> Hmm, we can probably use "informational" for notes, warnings, and remarks, 
> but I'm kinda partial to proposing the latter two upstream.

Interesting,  I agree that remarks could fit under "informational".

As for notes: As I understand it they are always child diagnostics of 
warnings/errors, so seems it would be okay to associate these with the "fail" 
kind.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

> A part of my endgame is to see notes be incorporated into their parents, but 
> that's a long way off methinks.

Regarding this, the current best approach the spec provides is using the 
"locationRelationShip" 
,
 but the relationships that exist dont' seem to cover cases needed by us. One 
example is macro-expansion (example 

 from an older proposal for SARIF in clang).

Seems that for locationRelationShip the spec allows producer defined strings:

> A locationRelationship object MAY contain a property named kinds whose value 
> is an array of one or more unique (§3.7.3) strings each of which specifies a 
> relationship between theSource and theTarget (see §3.34.1). If kinds is 
> absent, it SHALL default to [ "relevant" ] (see below for the meaning of 
> "relevant").
>
> When possible, SARIF producers SHOULD use the following values, with the 
> specified meanings.
>
> · "includes": The artifact identified by theSource includes the 
> artifact identified by theTarget.
>
> · "isIncludedBy": The artifact identified by theSource is included by 
> the artifact identified by theTarget.
>
> · "relevant": theTarget is relevant to theSource in a way not covered 
> by other relationship kinds.
>
> If none of these values are appropriate, a SARIF producer MAY use any value.
>
> NOTE: Although "relevant" is a catch-all for any relationship not described 
> by the other values, a producer might still wish to define its own more 
> specific values.
>
> In particular, the values defined for logicalLocation.kind (§3.33.7) and 
> threadFlowLocation.kinds (§3.38.8) might prove useful.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 451585.
vaibhav.y added a comment.

Update diff:

- Create class `SarifReportingConfiguration`:

This should allow rules to specify the `defaultConfiguration` property

Add tests for:

- Creating rules with custom reporting configuration
- Death test for invalid reporting configuration rank
- Death test for creating a result that references a disabled rule


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -7,7 +7,6 @@
 //===--===//
 
 #include "clang/Basic/Sarif.h"
-#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
@@ -147,6 +146,47 @@
   ASSERT_DEATH(Writer.appendResult(EmptyResult), Matcher);
 }
 
+TEST_F(SarifDocumentWriterTest, settingInvalidRankWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A SarifReportingConfiguration is created with an invalid "rank"
+  // * Ranks below 0.0 are invalid
+  // * Ranks above 100.0 are invalid
+
+  // THEN: The builder will crash in either case
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(-1.0),
+   ::testing::HasSubstr("Rule rank cannot be smaller than 0.0"));
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(101.0),
+   ::testing::HasSubstr("Rule rank cannot be larger than 100.0"));
+}
+
+TEST_F(SarifDocumentWriterTest, creatingResultWithDisabledRuleWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A disabled Rule is created, and a result is create referencing this rule
+  const auto &Config = SarifReportingConfiguration::create().disable();
+  auto RuleIdx =
+  Writer.createRule(SarifRule::create().setDefaultConfiguration(Config));
+  const SarifResult &Result = SarifResult::create(RuleIdx);
+
+  // THEN:
+  // SarifResult::create(...) will produce a crash
+  ASSERT_DEATH(
+  Writer.appendResult(Result),
+  ::testing::HasSubstr("Cannot add a result referencing a disabled Rule"));
+}
+
 // Test adding rule and result shows up in the final document
 TEST_F(SarifDocumentWriterTest, addingResultWithValidRuleAndRunIsOk) {
   // GIVEN:
@@ -199,10 +239,10 @@
   EXPECT_TRUE(Artifacts->empty());
 }
 
-TEST_F(SarifDocumentWriterTest, checkSerializingResults) {
+TEST_F(SarifDocumentWriterTest, checkSerializingResultsWithDefaultRuleConfig) {
   // GIVEN:
   const std::string ExpectedOutput =
-  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[],"columnKind":"unicodeCodePoints","results":[{"message":{"text":""},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[],"columnKind":"unicodeCodePoints","results":[{"level":"warning","message":{"text":""},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
 
   SarifDocumentWriter Writer{SourceMgr};
   const SarifRule &Rule =
@@ -213,8 +253,34 @@
 
   // WHEN: A run contains a result
   Writer.createRun("sarif test", "sarif test runner", "1.0.0");
-  unsigned ruleIdx = Writer.createRule(Rule);
-  const SarifResult &Result = SarifResult::create(ruleIdx);
+  unsigned RuleIdx = Writer.createRule(Rule);
+  const SarifResult &Result = SarifResult::create(RuleIdx);
+  Writer.appendResult(Result);
+  std::string Output = serializeSarifDocument(Writer.createDocument());
+
+  // THEN:
+  ASSERT_THAT(Output, ::testing::StrEq(ExpectedOutput));
+}
+
+TEST_F(Sarif

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/include/clang/Basic/Sarif.h:256
   std::string HelpURI;
+  SarifReportingConfiguration DefaultConfiguration;
 

Is this form preferred for expressing a statically known default value?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 451633.
vaibhav.y added a comment.

Autofix clang-tidy readability warnings


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -7,7 +7,6 @@
 //===--===//
 
 #include "clang/Basic/Sarif.h"
-#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
@@ -33,9 +32,9 @@
 
 static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
   std::string Output;
-  llvm::json::Value value(std::move(Doc));
+  llvm::json::Value Value(std::move(Doc));
   llvm::raw_string_ostream OS{Output};
-  OS << llvm::formatv("{0}", value);
+  OS << llvm::formatv("{0}", Value);
   OS.flush();
   return Output;
 }
@@ -86,7 +85,7 @@
   const llvm::json::Object &EmptyDoc = Writer.createDocument();
   std::vector Keys(EmptyDoc.size());
   std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
- [](auto item) { return item.getFirst(); });
+ [](auto Item) { return Item.getFirst(); });
 
   // THEN:
   ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
@@ -113,17 +112,17 @@
   ASSERT_EQ(Runs->size(), 1UL);
 
   // The tool associated with the run was the tool
-  const llvm::json::Object *driver =
+  const llvm::json::Object *Driver =
   Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
-  ASSERT_THAT(driver, testing::NotNull());
+  ASSERT_THAT(Driver, testing::NotNull());
 
-  ASSERT_TRUE(driver->getString("name").has_value());
-  ASSERT_TRUE(driver->getString("fullName").has_value());
-  ASSERT_TRUE(driver->getString("language").has_value());
+  ASSERT_TRUE(Driver->getString("name").has_value());
+  ASSERT_TRUE(Driver->getString("fullName").has_value());
+  ASSERT_TRUE(Driver->getString("language").has_value());
 
-  EXPECT_EQ(driver->getString("name").value(), ShortName);
-  EXPECT_EQ(driver->getString("fullName").value(), LongName);
-  EXPECT_EQ(driver->getString("language").value(), "en-US");
+  EXPECT_EQ(Driver->getString("name").value(), ShortName);
+  EXPECT_EQ(Driver->getString("fullName").value(), LongName);
+  EXPECT_EQ(Driver->getString("language").value(), "en-US");
 }
 
 TEST_F(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
@@ -147,6 +146,47 @@
   ASSERT_DEATH(Writer.appendResult(EmptyResult), Matcher);
 }
 
+TEST_F(SarifDocumentWriterTest, settingInvalidRankWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A SarifReportingConfiguration is created with an invalid "rank"
+  // * Ranks below 0.0 are invalid
+  // * Ranks above 100.0 are invalid
+
+  // THEN: The builder will crash in either case
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(-1.0),
+   ::testing::HasSubstr("Rule rank cannot be smaller than 0.0"));
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(101.0),
+   ::testing::HasSubstr("Rule rank cannot be larger than 100.0"));
+}
+
+TEST_F(SarifDocumentWriterTest, creatingResultWithDisabledRuleWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A disabled Rule is created, and a result is create referencing this rule
+  const auto &Config = SarifReportingConfiguration::create().disable();
+  auto RuleIdx =
+  Writer.createRule(SarifRule::create().setDefaultConfiguration(Config));
+  const SarifResult &Result = SarifResult::create(RuleIdx);
+
+  // THEN:
+  // SarifResult::create(...) will produce a crash
+  ASSERT_DEATH(
+  Writer.appendResult(Result),
+  ::testing::HasSubstr("Cannot add a result referencing a disabled Rule"));
+}
+
 // Test adding rule and result shows up in the final document
 TEST_F(SarifDocumentWriterTest, addingResultWithValidRuleAndRunIsOk) {
   // GIVEN:
@@ -160,9 +200,9 @@
   // WHEN:
   Writer.createRun("sarif test", "sarif test runner");
   unsigned RuleIdx = Writer.createRule(Rule);
-  const SarifResult &result = SarifResult::create(RuleIdx);
+  const SarifResult &Result = SarifResult::create(RuleIdx);
 
-  Writer.appendResult(result);
+  Writer.appendResult(Result);
   const llvm::json::Object &Doc = Writer.createDocument();
 
   // THEN:
@@ -199,10 +239,10 @@
   EXPECT_TRUE(Artifacts->empty());
 

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 451635.
vaibhav.y added a comment.

Run git-clang-format

Rebase on upstream


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -7,7 +7,6 @@
 //===--===//
 
 #include "clang/Basic/Sarif.h"
-#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
@@ -33,9 +32,9 @@
 
 static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
   std::string Output;
-  llvm::json::Value value(std::move(Doc));
+  llvm::json::Value Value(std::move(Doc));
   llvm::raw_string_ostream OS{Output};
-  OS << llvm::formatv("{0}", value);
+  OS << llvm::formatv("{0}", Value);
   OS.flush();
   return Output;
 }
@@ -86,7 +85,7 @@
   const llvm::json::Object &EmptyDoc = Writer.createDocument();
   std::vector Keys(EmptyDoc.size());
   std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
- [](auto item) { return item.getFirst(); });
+ [](auto Item) { return Item.getFirst(); });
 
   // THEN:
   ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
@@ -113,17 +112,17 @@
   ASSERT_EQ(Runs->size(), 1UL);
 
   // The tool associated with the run was the tool
-  const llvm::json::Object *driver =
+  const llvm::json::Object *Driver =
   Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
-  ASSERT_THAT(driver, testing::NotNull());
+  ASSERT_THAT(Driver, testing::NotNull());
 
-  ASSERT_TRUE(driver->getString("name").has_value());
-  ASSERT_TRUE(driver->getString("fullName").has_value());
-  ASSERT_TRUE(driver->getString("language").has_value());
+  ASSERT_TRUE(Driver->getString("name").has_value());
+  ASSERT_TRUE(Driver->getString("fullName").has_value());
+  ASSERT_TRUE(Driver->getString("language").has_value());
 
-  EXPECT_EQ(driver->getString("name").value(), ShortName);
-  EXPECT_EQ(driver->getString("fullName").value(), LongName);
-  EXPECT_EQ(driver->getString("language").value(), "en-US");
+  EXPECT_EQ(Driver->getString("name").value(), ShortName);
+  EXPECT_EQ(Driver->getString("fullName").value(), LongName);
+  EXPECT_EQ(Driver->getString("language").value(), "en-US");
 }
 
 TEST_F(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
@@ -147,6 +146,47 @@
   ASSERT_DEATH(Writer.appendResult(EmptyResult), Matcher);
 }
 
+TEST_F(SarifDocumentWriterTest, settingInvalidRankWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A SarifReportingConfiguration is created with an invalid "rank"
+  // * Ranks below 0.0 are invalid
+  // * Ranks above 100.0 are invalid
+
+  // THEN: The builder will crash in either case
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(-1.0),
+   ::testing::HasSubstr("Rule rank cannot be smaller than 0.0"));
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(101.0),
+   ::testing::HasSubstr("Rule rank cannot be larger than 100.0"));
+}
+
+TEST_F(SarifDocumentWriterTest, creatingResultWithDisabledRuleWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A disabled Rule is created, and a result is create referencing this rule
+  const auto &Config = SarifReportingConfiguration::create().disable();
+  auto RuleIdx =
+  Writer.createRule(SarifRule::create().setDefaultConfiguration(Config));
+  const SarifResult &Result = SarifResult::create(RuleIdx);
+
+  // THEN:
+  // SarifResult::create(...) will produce a crash
+  ASSERT_DEATH(
+  Writer.appendResult(Result),
+  ::testing::HasSubstr("Cannot add a result referencing a disabled Rule"));
+}
+
 // Test adding rule and result shows up in the final document
 TEST_F(SarifDocumentWriterTest, addingResultWithValidRuleAndRunIsOk) {
   // GIVEN:
@@ -160,9 +200,9 @@
   // WHEN:
   Writer.createRun("sarif test", "sarif test runner");
   unsigned RuleIdx = Writer.createRule(Rule);
-  const SarifResult &result = SarifResult::create(RuleIdx);
+  const SarifResult &Result = SarifResult::create(RuleIdx);
 
-  Writer.appendResult(result);
+  Writer.appendResult(Result);
   const llvm::json::Object &Doc = Writer.createDocument();
 
   // THEN:
@@ -199,10 +239,10 @@
   EXPECT_TRUE(Artifacts->empty());

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 414743.
vaibhav.y marked 15 inline comments as done.
vaibhav.y added a comment.
Herald added a project: All.

Fixup comments
Mark clang::FullSourceRange as returning const & to its locs
rebase on upstream main


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,138 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); }, ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  ASSERT_THAT(theRun->get("tool"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("results"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("artifacts"), ::testing::NotNull());
+  const json::Object *driver = theRun->getO

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Hi,

Apologies for the long delay! I was on a short break to focus to other 
projects. I have some changes in mind such as:

- Creating the `SarifLog` object to represent the top-level document. Currently 
we store this as a JSON object which ends up rather mucky. Having a separate 
structure can release internal state in `SarifDocumentWriter`. That way the 
writer will end up only dealing with the serialization of a `SarifLog`.

Regarding how to validate documents, I think having a `SarifLog::validate()` 
that checks everything underneath is the way to go. Ideally I'd prefer handling 
on the interface level, but I'm uncertain what would be a good approach. What 
do you think?

I'll comb through the previous comments again to make sure I'm not missing 
punctuations, will signal when this is ready for review again.

Thanks!




Comment at: clang/include/clang/Basic/Sarif.h:112
+
+  explicit SarifArtifact(const SarifArtifactLocation &Loc) : Location(Loc) {}
+

aaron.ballman wrote:
> Should we delete the default ctor?
Agreed, there's no reason for it to be callable. Will do the same for 
`SarifArtifactLocation`.



Comment at: clang/include/clang/Basic/Sarif.h:340
+  /// Create a new empty SARIF document
+  SarifDocumentWriter() : Closed(true){};
+

aaron.ballman wrote:
> Once you use the default ctor, there's no way to associate language options 
> with the document writer. Is it wise to expose this constructor? (If we 
> didn't, then we could store the `LangOptions` as a const reference instead of 
> making a copy in the other constructor. Given that we never mutate the 
> options, I think that's a win.)
That's a good observation, I will delete this constructor and expose 
`SarifDocumentWriter(const LangOptions &LangOpts)` instead. 



Comment at: clang/lib/Basic/Sarif.cpp:248
+void SarifDocumentWriter::endRun() {
+  // Exit early if trying to close a closed Document
+  if (Closed) {

aaron.ballman wrote:
> (At this point, I'll stop commenting on these -- can you go through the patch 
> and make sure that all comments have appropriate terminating punctuation?)
Ack, sincere apologies again!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 443654.
vaibhav.y added a comment.

Discard most likely culprit in test code causing unexpected crash.

@aaron.ballman I was unable to reproduce the issue on my end using the buildbot
instructions and ASAN, UBSAN.

Is it possible for you to trigger a pre-merge check on the LLVM cluster?

I have suspicions that it was the `SarifResult &&` type in the test so I changed
it to a `const SarifResult &`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,320 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  CharSourceRange getFakeCharSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

>> I have suspicions that it was the `SarifResult &&` type in the test so I 
>> changed it to a `const SarifResult &`.
>>
>> I've tried running it on a RHEL 7, Darwin on my end.
>
> If you think you've got it fixed, I think the best we can do is to re-commit 
> and watch the bots to see how they react. I'll commit again for you and watch 
> them.

Thank you! Let us try that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

In D109701#3642748 , @aaron.ballman 
wrote:

> Unfortunately, it still seems to be causing failures (I had to revert again):
>
> https://lab.llvm.org/buildbot/#/builders/91/builds/11840
>
> It looks to be the same failure as before 
> (https://lab.llvm.org/buildbot/#/builders/91/builds/11328). :-(

Thanks, fortunately this seems to be reproducible. Perhaps there's something 
special about my environment making it work. I'll try to pare down more 
(possibly try to build inside a container).

The buildbot properties say its Linux aurora is there an official container / 
build spec I can use to mimic the environment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

In D109701#3646856 , @abrahamcd wrote:

> Hi! I'm interning with @cjdb and @denik this summer and I was working on 
> adding a `-fdiagnostics-format=sarif` option to start off my project, but I 
> just found that a previous abandoned version of this change (D109697 
> ) was intending to add it. Seeing as the 
> flag is no longer included in this version of the change, is it okay for me 
> to go on and add it myself, or are you still planning on adding it here? 
> Thanks!

Sure, feel free to use D109697  as you see 
fit!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

@aaron.ballman

The culprit turned out to be the difference in release flags on the build 
server vs my environment. I had unfortunately run the configuration command 
once in `Debug` mode, and hadn't re-configured. Not a bright moment :)

The `ASSERT_DEATH` tests that I had written weren't gated by `NDEBUG, 
GTEST_HAS_DEATH_TEST` (other unit tests use some combination two). This was 
causing them to pass on my machine but fail pre-merge (which is 
`RelWithDebInfo`)

I will gate the tests similar to what  
https://github.com/llvm/llvm-project/blob/main/clang/unittests/Serialization/InMemoryModuleCacheTest.cpp#L48-L52
  does, but force a skip instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 444764.
vaibhav.y added a comment.

Gate death tests on `NDEBUG` and available of `GTEST_HAS_DEATH_TEST`

This should fix recent pre-merge failures


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,325 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  CharSourceRange getFakeCharSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, CanCreateEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, CanCreateDocumentWithOneRun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 444960.
vaibhav.y added a comment.

Undo test case renames


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,325 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  CharSourceRange getFakeCharSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, canCreateEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, canCreateDocumentWithOneRun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(dri

[PATCH] D129886: [clang] Add -fdiagnostics-format=sarif for SARIF diagnostics

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Might be worth hiding it from `--help`, despite the instability warning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129886/new/

https://reviews.llvm.org/D129886

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129886: [clang] Add -fdiagnostics-format=sarif option for future SARIF output

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

In D129886#3660299 , @abrahamcd wrote:

> In D129886#3656221 , @vaibhav.y 
> wrote:
>
>> Might be worth hiding it from `--help`, despite the instability warning.
>
> I already couldn't find any mention of `-fdiagnostics-format` or `sarif` when 
> I ran `clang --help`.  Is there somewhere else I should look?

Interesting, `fdiagnostics-format` doesn't show up in either `--help` or 
`--help-hidden`.

You can disregard my earlier comment in that case, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129886/new/

https://reviews.llvm.org/D129886

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129886: [clang] Add -fdiagnostics-format=sarif option for future SARIF output

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Please address the whitespace changes in 
`clang/lib/Driver/ToolChains/Clang.cpp`.




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:80-81
 Args.getLastArg(options::OPT_dynamic, 
options::OPT_mdynamic_no_pic))
-  D.Diag(diag::err_drv_argument_not_allowed_with) << A->getAsString(Args)
-  << "-static";
+  D.Diag(diag::err_drv_argument_not_allowed_with)
+  << A->getAsString(Args) << "-static";
 }

nit: Looks like this an unrelated format change/ rebase issue. There are a few 
others as well in this file

Would be better to introduce these through another CR, or use `git clang-format 
 []` to limit it to the diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129886/new/

https://reviews.llvm.org/D129886

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 375734.
vaibhav.y marked 34 inline comments as done.
vaibhav.y added a comment.

Address comments from upstream review:

- Mark anonymous functions static, improve documentation
- Coding style fixes
- Use Ref types as they are intended (as non-owning lightweight refs)
- SarifDocumentWriter now stores a copy of `clang::LangOptions` passed to it
- getCurrentTool, getCurrentRun return mutable refs instead of raw pointers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,153 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(ruleIdx, emptyResult); },
+   ".*create a run first.*");
+}
+
+// Test adding result for invalid ruleIdx causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWithoutRuleWillCrash) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  // But caller forgot to create a rule for this run:
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(0, emptyResult); },
+   "Trying to reference a rule that doesn't exist");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWIthValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifResult &result = SarifResult::create();
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  writer.appendResult(ruleIdx, result);
+  const json::Object &document = writer.createDocument();

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/include/clang/Basic/Sarif.h:95
+//
+/// Since every in clang artifact MUST have a location (there being no nested
+/// artifacts), the creation method \ref SarifArtifact::create requires a

aaron.ballman wrote:
> How do we expect to handle artifact locations that don't correspond directly 
> to a file? For example, the user can specify macros on the command line and 
> those macros could have a diagnostic result associated with them. Can we 
> handle that sort of scenario?
I hadn't considered `-D` macros. Definitely a valid case to handle. I will 
respond with more information once I read through the related portions of the 
SARIF spec. A (sort of hacky?) solution come to mind after a quick glance:

Setting the artifact URI to: `data:text/plain:` with an offset, and 
saying that it's role is `"referencedOnCommandLine"`. It seems strange since we 
need to copy over the command line, instead of referencing it directly. What do 
you think: 
https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317617

Dropping a TODO comment so I can update later.

`TODO(envp)`



Comment at: clang/include/clang/Basic/Sarif.h:109
+  SarifArtifactLocation Location;
+  SmallVector Roles;
+

aaron.ballman wrote:
> What size would you like this `SmallVector` to have?
Glancing through valid values for roles: my expectation is that each artifact 
will have a _small_ number of roles associated, so `4` seems like a good 
threshold here. I don't really have a strong opinion on the size, so I'm open 
to changing this.



Comment at: clang/include/clang/Basic/Sarif.h:250
+
+  SarifResult() = default;
+

aaron.ballman wrote:
> A default constructed `SarifResult` will have an uninitialized `RuleIdx` -- 
> are you okay with that?
Hrm, I had overlooked that case (since `appendResult` took and index and a 
rule). I'll make it take a rule index upon construction. This will also make it 
so that `appendResult` takes just the `SarifResult`, and not an (Idx, Result), 
which is definitely an artifact of before I added RuleIdx to `SarifResult`.




Comment at: clang/include/clang/Basic/Sarif.h:297-298
+  /// \internal
+  /// Return a pointer to the current tool. If no run exists, this will
+  /// crash.
+  json::Object *getCurrentTool();

aaron.ballman wrote:
> 
Thanks for rewording. I'll also make this return a reference, since the pointer 
returned cannot be null.



Comment at: clang/include/clang/Basic/Sarif.h:388-389
+private:
+  /// Langauge options to use for the current SARIF document
+  const LangOptions *LangOpts;
+

aaron.ballman wrote:
> I think this should be a value type rather than a possibly null pointer type 
> -- this way, the document can always rely on there being valid language 
> options to check, and if the user provides no custom language options, the 
> default `LangOptions` suffice. Alternatively, it seems reasonable to expect 
> the user to have to pass in a valid language options object in order to 
> create a SARIF document. WDYT?
I agree with that, will change it to store an owned value.

I think leaving the two constructors as is is fine as long as the `default` 
variant will also leave `LangOpts` in a valid state.



Comment at: clang/lib/Basic/Sarif.cpp:207-209
+if (statusIter.second) {
+  I = statusIter.first;
+}

aaron.ballman wrote:
> Our usual coding style elides these too. Btw, you can find the coding style 
> document at: https://llvm.org/docs/CodingStandards.html
Thanks, sorry there's so many of these! I definitely need to not auto-pilot 
with style.



Comment at: clang/lib/Basic/Sarif.cpp:219-222
+json::Object *SarifDocumentWriter::getCurrentTool() {
+  assert(hasRun() && "Need to call createRun() before using getcurrentTool!");
+  return Runs.back().getAsObject()->get("tool")->getAsObject();
+}

aaron.ballman wrote:
> Should this return a reference rather than a pointer?
Makes sense to convert to a ref, the pointer returned can never be null anyway



Comment at: clang/lib/Basic/Sarif.cpp:230-232
+  if (!hasRun()) {
+return;
+  }

aaron.ballman wrote:
> Is there a reason why we don't want to assert instead?
Creating a document requires ending any ongoing runs, and it is valid to create 
a document without any runs, so `createDocument()` calls `endRun()`.

I guess having a flag to mark the status of the document, and only calling 
`endRun()` if a an active run exists would likely be better. (`hasRun()` seems 
to have a rather broad responsibility, tracking both the availability & state 
of the current run)

I'm thinking of adding a `Closed` flag to the writer (default `true`), which is 
unset whenever `createRun()` is called, and `endRun()` will set the same flag.

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 385609.
vaibhav.y added a comment.

Summary of changes:

- [clangBasic] Fix SIGSEGV in Sarif builders
- [clangBasic] Fixup documentation typo, add explicit type


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,139 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); },
+   ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  ASSERT_THAT(theRun->get("tool"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("results"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("artifacts"), ::testing::NotNull());
+  const json::Object *driver = theRun->getObject("tool")->getObject("driver"

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 385634.
vaibhav.y added a comment.

Add `Closed` to signal if document is writable

Introduce a flag `Closed` to `clang::SarifDocumentWriter` that tracks

  if the underlying sarif document is accepting results to record. i.e. it
  is in the middle of an active run. This flag enforces that every call to
  `endRun()` must be followed by a `createRun(...)` if the user wants to
  append additional diagnostics
  
  E.g.:
  
  ```cpp
  // Starts with Closed = true
  SarifDocumentWriter writer;
  
  // This sets Closed = false
  writer.createRun();
  
  // append diagnostics to the document
  ...
  
  // end the current run, setting Closed = true
  writer.endRun();
  
  // When Closed = true, adding a new result leads to errors
  // This is now an error, and requires a call to createRun(...)
  writer.appendResult(...)
  ```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,139 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); },
+   ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(d

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 385636.
vaibhav.y added a comment.

Format code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,138 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); }, ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  ASSERT_THAT(theRun->get("tool"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("results"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("artifacts"), ::testing::NotNull());
+  const json::Object *driver = theRun->getObject("tool")->getObject("driver");
+  const json::Array *results = theRun->getArray("results");
+  const json::Array *artifacts = theRun->getArray("artifacts");

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 385813.
vaibhav.y added a comment.

Format code using patch from buildkite


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,138 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); }, ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  ASSERT_THAT(theRun->get("tool"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("results"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("artifacts"), ::testing::NotNull());
+  const json::Object *driver = theRun->getObject("tool")->getObject("driver");
+  const json::Array *results = theRun->getArray("results");
+  const json::Array *artifacts = theR

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Ping.

`clang-format` is the only check failing as of now (attempting to reformat the 
`` links in doc comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 390396.
vaibhav.y marked 6 inline comments as done.
vaibhav.y added a comment.

Rebase on upstream/main:

- [clangBasic] Format code
- [clangBasic] Mark all constructors taking single values `explicit`
- [clangBasic] Convert `StringRef` to `std::string` fields that own the data
- [clangBasic] Fixup outdated comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,138 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); }, ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  ASSERT_THAT(theRun->get("tool"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("results"), ::testing::NotNull());

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/include/clang/Basic/Sarif.h:80-82
+  static SarifArtifactLocation create(StringRef URI) {
+return SarifArtifactLocation{URI};
+  }

aaron.ballman wrote:
> One thing that's worth calling out is that `StringRef` is non-owning which 
> means that the argument passed to create the `SarifArtifactLocation` has to 
> outlive the returned object to avoid dangling references. This makes the 
> class a bit more dangerous to use because `Twine` or automatic `std::string` 
> objects may cause lifetime concerns.
> 
> Should these classes be storing a `std::string` so that the memory is owned 
> by SARIF?
Good point! Will change it to `std::string` to start with.

Some fields such as MimeType, RuleID would probably do better with 
`SmallString`, I haven't been able to find any good measurements on how the 
lengths of those two are distributed, can it be made into `SmallString` in a 
future PR?



Comment at: clang/include/clang/Basic/Sarif.h:116
+  static SarifArtifact create(const SarifArtifactLocation &Loc) {
+return SarifArtifactLocation{Loc};
+  }

aaron.ballman wrote:
> `Loc` is already a `SarifArtifactLocation`, did you mean `SarifArtifact` by 
> any chance?
> 
> (Note, this suggests to me we should mark the ctor's here as `explicit`.)
Agree, I've marked all constructors taking a single parameter as explicit.



Comment at: clang/include/clang/Basic/Sarif.h:243-245
+  // While this cannot be negative, since this type needs to be serialized
+  // to JSON, it needs to be `int64_t`. The best we can do is assert that
+  // a negative value isn't used to create it

aaron.ballman wrote:
> This comment looks incorrect to me.
Ack, fixed that as well. the right rationale for `uint32_t` is that it is the 
largest non-negative type that can be safely promoted to `int64_t` (which is 
what LLVM's json encoder supports)



Comment at: clang/include/clang/Basic/SourceLocation.h:441-456
+bool operator()(const FullSourceLoc &lhs, const FullSourceLoc &rhs) const {
   return lhs.isBeforeInTranslationUnitThan(rhs);
 }
   };
 
   /// Prints information about this FullSourceLoc to stderr.
   ///

aaron.ballman wrote:
> Looks like unrelated formatting changes; feel free to submit these as an NFC 
> change if you'd like, but the changes should be reverted from this patch for 
> clarity.
Ack, these are definitely a result of a bad rebase. 



Comment at: clang/lib/Basic/Sarif.cpp:199
+const SarifArtifactLocation &Location =
+SarifArtifactLocation::create(FileURI).setIndex(Idx);
+const SarifArtifact &Artifact = SarifArtifact::create(Location)

aaron.ballman wrote:
> This seems like it'll be a use-after-free because the local `std::string` 
> will be destroyed before the lifetime of the `SarifArtifactLocation` ends.
Will run it through a pass of asan & msan, is the best way to add: 
`-fsanitize=memory -fsanitize=address` to the test CMakeLists.txt & run them?

I've changed all strings to `std::string`, so this one should no longer be a 
problem but I wonder if there's any others I have missed as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 390448.
vaibhav.y added a comment.

Rename enum members


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,138 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); }, ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  ASSERT_THAT(theRun->get("tool"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("results"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("artifacts"), ::testing::NotNull());
+  const json::Object *driver = theRun->getObject("tool")->getObject("driver");
+  const json::Array *results = theRun->getArray("results");
+  const json::Array *artifacts = theRun->getArray("artif

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y marked 11 inline comments as done.
vaibhav.y added a comment.

Mark completed comments as done


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135987: [clangBasic] Refactor StaticAnalyzer to use `clang::SarifDocumentWriter`

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y created this revision.
vaibhav.y added reviewers: aaron.ballman, cjdb.
Herald added subscribers: steakhal, wenlei, martong.
Herald added a reviewer: NoQ.
Herald added a project: All.
vaibhav.y updated this revision to Diff 472133.
vaibhav.y added a comment.
vaibhav.y added a reviewer: dbeer1.
vaibhav.y retitled this revision from "[clangBasic] Create 
`FullSourceLoc::getDecomposedExpansionLoc`" to "[clangBasic] Refactor 
StaticAnalyzer to use `clang::SarifDocumentWriter`".
vaibhav.y edited the summary of this revision.
Herald added subscribers: manas, ASDenysPetrov, dkrupp, donat.nagy, Szelethus, 
a.sidorin, baloghadamsoftware.
vaibhav.y published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Update unit tests




Comment at: clang/lib/Basic/Sarif.cpp:121-123
+  std::pair LocInfo = Loc.getDecomposedExpansionLoc();
+  assert(LocInfo.second > Loc.getExpansionColumnNumber() &&
+ "position in file is before column number?");

L121 was copied wrong, `libStaticAnalyzer` (sensibly) uses the expansion loc. 
This has been fixed as well.


Refactor StaticAnalyzer to use `clang::SarifDocumentWriter` for serializing 
sarif diagnostics.

Uses `clang::SarifDocumentWriter` to generate SARIF output in the 
StaticAnalyzer.

Various bugfixes are also made to `clang::SarifDocumentWriter`.

Summary of changes:

- `clang/lib/Basic/Sarif.cpp`:
  - Fix bug in adjustColumnPos introduced from prev move, it now uses 
FullSourceLoc::getDecomposedExpansionLoc which provides the correct location 
(in the presence of macros) instead of `FullSourceLoc::getDecomposedLoc`.
  - Fix `createTextRegion` so that it handles caret ranges correctly, this 
should bring it to parity with the previous implementation.
- `clang/test/Analysis/diagnostics/Inputs/expected-sarif`:
  - Update the schema URL to the offical website
  - Add the emitted `defaultConfiguration` sections to all rules
  - Annotate results with the "level" property
- `clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp`:
  - Update `SarifDiagnostics` class to hold a `clang::SarifDocumentWriter` that 
it uses to convert diagnostics to SARIF.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135987

Files:
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  
clang/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
  
clang/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -292,7 +292,7 @@
 TEST_F(SarifDocumentWriterTest, checkSerializingArtifacts) {
   // GIVEN:
   const std::string ExpectedOutput =
-  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0,"uri":"file:///main.cpp"},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
 
   SarifDocumentWriter Writer{SourceMgr};
   const SarifRule &Rule =
@@ -332,7 +332,7 @@
 TEST_F(SarifDocumentWriterTest, checkSeriali

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y marked an inline comment as not done.
vaibhav.y added inline comments.



Comment at: clang/include/clang/Basic/Sarif.h:157
+/// 1. https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317648";>level
 property
+enum class SarifResultLevel { Note, Warning, Error };
+

aaron.ballman wrote:
> Should this include `Remark` for diagnostics like: 
> https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticFrontendKinds.td#L55
> 
> If not, I think the comments should explain how to map remarks to one of 
> these levels.
Ack, if we're adding `remark`, should I also add support the 
"informational"/"fail" result kind that was previously discussed by you? 
Currently there's no `kind`, so we are defaulting to "fail".



Comment at: clang/lib/Basic/Sarif.cpp:25
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/JSON.h"

aaron.ballman wrote:
> Why was this include required?
It provides `llvm_unreachable`. so far it was available transitively, must have 
been added when I implemented `resultLevelToStr(...)`.

I don't feel strongly about keeping this though.



Comment at: clang/lib/Basic/Sarif.cpp:396-399
+  if (Result.LevelOverride.hasValue())
+Ret["level"] = resultLevelToStr(Result.LevelOverride.getValue());
+  else
+Ret["level"] = resultLevelToStr(Rule.DefaultConfiguration.Level);

aaron.ballman wrote:
> (Probably have to re-run clang-format)
Good catch! I notice Optional has `value_or`, is that okay as well here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:75
+emitFilename(FE->getName(), Loc.getManager());
+// FIXME: No current way to add file-only location to SARIF object
+  }

abrahamcd wrote:
> @vaibhav.y , just wanted to confirm, the SarifDiagnosticWriter can only add 
> locations that include row/column numbers, correct?
Yes, this is currently not supported. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131632/new/

https://reviews.llvm.org/D131632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y marked 4 inline comments as done.
vaibhav.y added inline comments.



Comment at: clang/include/clang/Basic/Sarif.h:157
+/// 1. https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317648";>level
 property
+enum class SarifResultLevel { Note, Warning, Error };
+

aaron.ballman wrote:
> vaibhav.y wrote:
> > aaron.ballman wrote:
> > > Should this include `Remark` for diagnostics like: 
> > > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticFrontendKinds.td#L55
> > > 
> > > If not, I think the comments should explain how to map remarks to one of 
> > > these levels.
> > Ack, if we're adding `remark`, should I also add support the 
> > "informational"/"fail" result kind that was previously discussed by you? 
> > Currently there's no `kind`, so we are defaulting to "fail".
> I think we should, but it could be done in a follow-up commit. Remarks aren't 
> the most common of diagnostics (they're infrequent unless you're asking for 
> them), but they certainly exist and we should have some plan for them. Doing 
> it as a separate patch means we don't have to think about things like "What 
> does it mean to have an optimization report in SARIF? What should that look 
> like?" quite yet.
Gotcha, the best way forward for now would be to include the `None` Level. I'll 
add support for alternative result kinds as a follow-up task.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 453427.
vaibhav.y edited the summary of this revision.
vaibhav.y added a comment.

Address review comments:

- Drop include line that added `llvm/Support/ErrorHandling.h`
- Use `llvm::Optional::value_or(...)` instead of bare conditional
- Add `SarifResultLevel::None` type for encoding Remark-like diagnostics.
  - Support for these is not yet complete since it requires a custom result 
kind (this currently defaults to "fail", thus omitted)
  - Add a comment explaining how diagnostic levels are typically mapped to 
sarif result levels


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

Files:
  clang/include/clang/Basic/Sarif.h
  clang/lib/Basic/Sarif.cpp
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -7,7 +7,6 @@
 //===--===//
 
 #include "clang/Basic/Sarif.h"
-#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
@@ -33,9 +32,9 @@
 
 static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
   std::string Output;
-  llvm::json::Value value(std::move(Doc));
+  llvm::json::Value Value(std::move(Doc));
   llvm::raw_string_ostream OS{Output};
-  OS << llvm::formatv("{0}", value);
+  OS << llvm::formatv("{0}", Value);
   OS.flush();
   return Output;
 }
@@ -86,7 +85,7 @@
   const llvm::json::Object &EmptyDoc = Writer.createDocument();
   std::vector Keys(EmptyDoc.size());
   std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
- [](auto item) { return item.getFirst(); });
+ [](auto Item) { return Item.getFirst(); });
 
   // THEN:
   ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
@@ -113,17 +112,17 @@
   ASSERT_EQ(Runs->size(), 1UL);
 
   // The tool associated with the run was the tool
-  const llvm::json::Object *driver =
+  const llvm::json::Object *Driver =
   Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
-  ASSERT_THAT(driver, testing::NotNull());
+  ASSERT_THAT(Driver, testing::NotNull());
 
-  ASSERT_TRUE(driver->getString("name").has_value());
-  ASSERT_TRUE(driver->getString("fullName").has_value());
-  ASSERT_TRUE(driver->getString("language").has_value());
+  ASSERT_TRUE(Driver->getString("name").has_value());
+  ASSERT_TRUE(Driver->getString("fullName").has_value());
+  ASSERT_TRUE(Driver->getString("language").has_value());
 
-  EXPECT_EQ(driver->getString("name").value(), ShortName);
-  EXPECT_EQ(driver->getString("fullName").value(), LongName);
-  EXPECT_EQ(driver->getString("language").value(), "en-US");
+  EXPECT_EQ(Driver->getString("name").value(), ShortName);
+  EXPECT_EQ(Driver->getString("fullName").value(), LongName);
+  EXPECT_EQ(Driver->getString("language").value(), "en-US");
 }
 
 TEST_F(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
@@ -147,6 +146,47 @@
   ASSERT_DEATH(Writer.appendResult(EmptyResult), Matcher);
 }
 
+TEST_F(SarifDocumentWriterTest, settingInvalidRankWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A SarifReportingConfiguration is created with an invalid "rank"
+  // * Ranks below 0.0 are invalid
+  // * Ranks above 100.0 are invalid
+
+  // THEN: The builder will crash in either case
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(-1.0),
+   ::testing::HasSubstr("Rule rank cannot be smaller than 0.0"));
+  EXPECT_DEATH(SarifReportingConfiguration::create().setRank(101.0),
+   ::testing::HasSubstr("Rule rank cannot be larger than 100.0"));
+}
+
+TEST_F(SarifDocumentWriterTest, creatingResultWithDisabledRuleWillCrash) {
+#if defined(NDEBUG) || !GTEST_HAS_DEATH_TEST
+  GTEST_SKIP() << "This death test is only available for debug builds.";
+#endif
+
+  // GIVEN:
+  SarifDocumentWriter Writer{SourceMgr};
+
+  // WHEN:
+  // A disabled Rule is created, and a result is create referencing this rule
+  const auto &Config = SarifReportingConfiguration::create().disable();
+  auto RuleIdx =
+  Writer.createRule(SarifRule::create().setDefaultConfiguration(Config));
+  const SarifResult &Result = SarifResult::create(RuleIdx);
+
+  // THEN:
+  // SarifResult::create(...) will produce a crash
+  ASSERT_DEATH(
+  Writer.appendResult(Result),
+  ::testing::HasSubstr("Cannot add a result referencing a disabled Rule"));
+}
+
 // Test adding rule and result shows up in the final document
 TEST_F(SarifDocumentWriterTest, addingResultWithValidRuleAndRunIsOk) {
   // 

[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Thank you! I don't have merge access, will need your help for that :)

For the commit details, I'd like to use:

- Name: Vaibhav Yenamandra
- email address: vyenaman...@bloomberg.net


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131084/new/

https://reviews.llvm.org/D131084

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:155
+break;
+  }
+}

Does this need an `llvm_unreachable` after the switch?



Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:177
+  // original path as possible, because that helps a user to recognize it.
+  // real_path() expands all links, which sometimes too much. Luckily,
+  // on Windows we can just use llvm::sys::path::remove_dots(), because,





Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:71
+  // other infrastructure necessary when emitting more rich diagnostics.
+  if (!Info.getLocation().isValid()) { // TODO: What is this case? 
+// SARIFDiag->addDiagnosticWithoutLocation(

The location maybe if the diagnostic's source is located in the scratch buffer. 
Likely for macro expansions where token pasting is involved. Another case would 
be errors on the command line.

I'm not entirely sure how the SARIF spec would handle this case, it might 
require an extension.

A few ways that might work could be:

Using the [[ 
https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127692
 | logicalLocations ]] property to specify ([[ 
https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127910
 | logicalLocation object ]]), this might need an extension for kind: "macro", 
another case that might need extension is diagnostics about invalid command 
line flags which are also diagnostics without a valid

The parentIndex for these logical locations could be set to the physical 
location that produced them.

I think this definitely warrants some discussion since the spec doesn't provide 
a clear way to express these cases.

WDYT @aaron.ballman @cjdb @denik 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131632/new/

https://reviews.llvm.org/D131632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:71
+  // other infrastructure necessary when emitting more rich diagnostics.
+  if (!Info.getLocation().isValid()) { // TODO: What is this case? 
+// SARIFDiag->addDiagnosticWithoutLocation(

vaibhav.y wrote:
> The location maybe if the diagnostic's source is located in the scratch 
> buffer. Likely for macro expansions where token pasting is involved. Another 
> case would be errors on the command line.
> 
> I'm not entirely sure how the SARIF spec would handle this case, it might 
> require an extension.
> 
> A few ways that might work could be:
> 
> Using the [[ 
> https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127692
>  | logicalLocations ]] property to specify ([[ 
> https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127910
>  | logicalLocation object ]]), this might need an extension for kind: 
> "macro", another case that might need extension is diagnostics about invalid 
> command line flags which are also diagnostics without a valid
> 
> The parentIndex for these logical locations could be set to the physical 
> location that produced them.
> 
> I think this definitely warrants some discussion since the spec doesn't 
> provide a clear way to express these cases.
> 
> WDYT @aaron.ballman @cjdb @denik 
The spec does say for "kind":

> If none of those strings accurately describes the construct, kind MAY contain 
> any value specified by the analysis tool.

So an extension might not be necessary, but might be worth discussing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131632/new/

https://reviews.llvm.org/D131632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135987: [clangBasic] Refactor StaticAnalyzer to use `clang::SarifDocumentWriter`

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 475475.
vaibhav.y added a comment.

Update FileCheck tests to include now serialized file URIs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135987/new/

https://reviews.llvm.org/D135987

Files:
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  
clang/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
  
clang/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif
  clang/test/Frontend/sarif-diagnostics.cpp
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -292,7 +292,7 @@
 TEST_F(SarifDocumentWriterTest, checkSerializingArtifacts) {
   // GIVEN:
   const std::string ExpectedOutput =
-  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0,"uri":"file:///main.cpp"},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
 
   SarifDocumentWriter Writer{SourceMgr};
   const SarifRule &Rule =
@@ -332,7 +332,7 @@
 TEST_F(SarifDocumentWriterTest, checkSerializingCodeflows) {
   // GIVEN:
   const std::string ExpectedOutput =
-  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":27,"location":{"index":1,"uri":"file:///test-header-1.h"},"mimeType":"text/plain","roles":["resultFile"]},{"length":30,"location":{"index":2,"uri":"file:///test-header-2.h"},"mimeType":"text/plain","roles":["resultFile"]},{"length":28,"location":{"index":3,"uri":"file:///test-header-3.h"},"mimeType":"text/plain","roles":["resultFile"]},{"length":41,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"codeFlows":[{"threadFlows":[{"locations":[{"importance":"essential","location":{"message":{"text":"Message #1"},"physicalLocation":{"artifactLocation":{"index":1},"region":{"endColumn":8,"endLine":2,"startColumn":1,"startLine":1,{"importance":"important","location":{"message":{"text":"Message #2"},"physicalLocation":{"artifactLocation":{"index":2},"region":{"endColumn":8,"endLine":2,"startColumn":1,"startLine":1,{"importance":"unimportant","location":{"message":{"text":"Message #3"},"physicalLocation":{"artifactLocation":{"index":3},"region":{"endColumn":8,"endLine":2,"startColumn":1,"startLine":1]}]}],"level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":8,"endLine":2,"startColumn":5,"startLine":2}}}],"message":{"text":"Redefinition of 'foo'"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+   

[PATCH] D135987: [clangBasic] Refactor StaticAnalyzer to use `clang::SarifDocumentWriter`

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

In D135987#3906253 , @aaron.ballman 
wrote:

> Thank you for this refactoring, I really like the direction it's going! I've 
> not spotted anything concerning, but if someone can double-check conformance 
> to SARIF in terms of the changes to the tests, that would be appreciated.
>
> It looks like precommit CI found a relevant failure that should be addressed.

Thank you! It was the FileCheck tests for -Wsarif-format-unstable. I think I've 
updated those correctly, but would appreciate another look. I haven't had much 
experience with FileCheck so far.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135987/new/

https://reviews.llvm.org/D135987

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135987: [clangBasic] Refactor StaticAnalyzer to use `clang::SarifDocumentWriter`

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Great! Please use the following for patch attribution:

- Name: Vaibhav Yenamandra
- email address: vyenaman...@bloomberg.net


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135987/new/

https://reviews.llvm.org/D135987

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135987: [clangBasic] Refactor StaticAnalyzer to use `clang::SarifDocumentWriter`

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 476186.
vaibhav.y added a comment.

Rebase on upstream/main


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135987/new/

https://reviews.llvm.org/D135987

Files:
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  
clang/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
  
clang/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif
  clang/test/Frontend/sarif-diagnostics.cpp
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- clang/unittests/Basic/SarifTest.cpp
+++ clang/unittests/Basic/SarifTest.cpp
@@ -292,7 +292,7 @@
 TEST_F(SarifDocumentWriterTest, checkSerializingArtifacts) {
   // GIVEN:
   const std::string ExpectedOutput =
-  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0,"uri":"file:///main.cpp"},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
 
   SarifDocumentWriter Writer{SourceMgr};
   const SarifRule &Rule =
@@ -332,7 +332,7 @@
 TEST_F(SarifDocumentWriterTest, checkSerializingCodeflows) {
   // GIVEN:
   const std::string ExpectedOutput =
-  R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":27,"location":{"index":1,"uri":"file:///test-header-1.h"},"mimeType":"text/plain","roles":["resultFile"]},{"length":30,"location":{"index":2,"uri":"file:///test-header-2.h"},"mimeType":"text/plain","roles":["resultFile"]},{"length":28,"location":{"index":3,"uri":"file:///test-header-3.h"},"mimeType":"text/plain","roles":["resultFile"]},{"length":41,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"codeFlows":[{"threadFlows":[{"locations":[{"importance":"essential","location":{"message":{"text":"Message #1"},"physicalLocation":{"artifactLocation":{"index":1},"region":{"endColumn":8,"endLine":2,"startColumn":1,"startLine":1,{"importance":"important","location":{"message":{"text":"Message #2"},"physicalLocation":{"artifactLocation":{"index":2},"region":{"endColumn":8,"endLine":2,"startColumn":1,"startLine":1,{"importance":"unimportant","location":{"message":{"text":"Message #3"},"physicalLocation":{"artifactLocation":{"index":3},"region":{"endColumn":8,"endLine":2,"startColumn":1,"startLine":1]}]}],"level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":8,"endLine":2,"startColumn":5,"startLine":2}}}],"message":{"text":"Redefinition of 'foo'"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+  R"({"$schema":"https://docs.oasi

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 436030.
vaibhav.y added a comment.

Rebase on main branch from upstream git


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,315 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  FullSourceRange getFakeFullSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return FullSourceRange{FullSourceLoc{BeginLoc, SourceMgr},
+   FullSourceLoc{EndLoc, SourceMgr}};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TR

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 436110.
vaibhav.y marked 2 inline comments as done.
vaibhav.y added a comment.

Fix bug detected from ASAN run

Passes `-fsanitize=address`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,315 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  FullSourceRange getFakeFullSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return FullSourceRange{FullSourceLoc{BeginLoc, SourceMgr},
+   FullSourceLoc{EndLoc, SourceMgr}};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+ 

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y marked an inline comment as done.
vaibhav.y added a comment.

Thanks, will push changes to address the comments soon.

As I understood from our discussion the work @cjdb has planned would create a 
new `DiagnosticsConsumer`, it can be started in parallel but would need the 
changes in D109701  to complete. I have other 
work planned for SARIF as well, some notes here: 
https://gist.github.com/envp/6abc3230dcc5043416c86aefb3d24419 (@cjdb plans to 
address the "BRIDGE" component)

For validation, I think having a hybrid approach is best here. As much as 
possible we should try to be correct by construction, but we certainly need to 
have validation before we serialize on the data.

My plan is to iterate on the interface as follows:

1. To decompose `SarifDocumentWriter`, into: `SarifLog` & other builders, along 
with a `SarifLogSerializer` whose sole responsibility is to construct the 
`json::Value`. This should improve the number of properties that are correct by 
construction.
2. Try to use this newer interface with 
https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp,
 and incorporate any new features I think might be useful

Do you think it would it better to use the changes here in `libStaticAnalyzer` 
before they land & then iterate on it? I don't have a strong preference for 
either approach so I'd defer to your experience.

I'm also unfamiliar with the workflow for stacking patches in phabricator, is 
there documentation I can refer to for this? My guess is: I make a git branch 
whose base is that of `D109701`, and then `arc diff D109701..OTHER_BRANCH` to 
create the phabricator that would use these changes?




Comment at: clang/include/clang/Basic/Sarif.h:162
+public:
+  static ThreadFlow create() { return {}; }
+

aaron.ballman wrote:
> aaron.ballman wrote:
> > One question this raises for me is whether we should be enforcing 
> > invariants from the SARIF spec as part of this interface or not. Currently, 
> > you can create a thread flow that has no importance or a rule without a 
> > name/id, etc. That means it's pretty easy to create SARIF that won't 
> > validate properly. One possible way to alleviate this would be for the 
> > `create()` methods/ctors to require these be set when creating the objects. 
> > However, I can imagine there will be times when that is awkward due to 
> > following the builder pattern with the interface. Another option would be 
> > to have some validation methods on each of the interfaces and the whole 
> > tree gets validated after construction, but this could have performance 
> > impacts.
> > 
> > What are your thoughts?
> Are you still intending to add a `validate()` interface to take care of this, 
> or are you still thinking about how to enforce invariants during construction 
> (or some combination of the two)?
Definitely! I plan to add that in a follow-up patch. The goal would be to have 
as much as possible be correct by construction (through small builders having a 
limited field set), but we'll still need a `validate()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 439096.
vaibhav.y added a comment.

Discard top-level const specifier where target isn't pointee/ref


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,315 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  FullSourceRange getFakeFullSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return FullSourceRange{FullSourceLoc{BeginLoc, SourceMgr},
+   FullSourceLoc{EndLoc, SourceMgr}};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 439153.
vaibhav.y added a comment.

Fix formatting in Sarif.h


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,315 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  FullSourceRange getFakeFullSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return FullSourceRange{FullSourceLoc{BeginLoc, SourceMgr},
+   FullSourceLoc{EndLoc, SourceMgr}};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->get

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Thanks for your patience with the review as well!

Just noticed that I need to add a link to revision in the commit messages as 
well: (https://www.llvm.org/docs/Phabricator.html#committing-a-change)

I will update the commit messages, but I cannot commit to the github repo.

Regarding further work on validation and integration, I couldn't find any 
documentation on how to work with stacked changes. Could you please direct me 
to some documentation for that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 439507.
vaibhav.y added a comment.

Reword commit messages

- Prefix `[tag]` specifying which components are affected
- Append link to D109701  in commit message 
per commit message guidelines


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,315 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace clang;
+
+namespace {
+
+using LineCol = std::pair;
+
+static std::string serializeSarifDocument(llvm::json::Object &&Doc) {
+  std::string Output;
+  llvm::json::Value value(std::move(Doc));
+  llvm::raw_string_ostream OS{Output};
+  OS << llvm::formatv("{0}", value);
+  OS.flush();
+  return Output;
+}
+
+class SarifDocumentWriterTest : public ::testing::Test {
+protected:
+  SarifDocumentWriterTest()
+  : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
+FileMgr(FileSystemOptions(), InMemoryFileSystem),
+DiagID(new DiagnosticIDs()), DiagOpts(new DiagnosticOptions()),
+Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr) {}
+
+  IntrusiveRefCntPtr InMemoryFileSystem;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  IntrusiveRefCntPtr DiagOpts;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+
+  FileID registerSource(llvm::StringRef Name, const char *SourceText,
+bool IsMainFile = false) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+const FileEntry *SourceFile =
+FileMgr.getVirtualFile(Name, SourceBuf->getBufferSize(), 0);
+SourceMgr.overrideFileContents(SourceFile, std::move(SourceBuf));
+FileID FID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+if (IsMainFile)
+  SourceMgr.setMainFileID(FID);
+return FID;
+  }
+
+  FullSourceRange getFakeFullSourceRange(FileID FID, LineCol Begin,
+ LineCol End) {
+auto BeginLoc = SourceMgr.translateLineCol(FID, Begin.first, Begin.second);
+auto EndLoc = SourceMgr.translateLineCol(FID, End.first, End.second);
+return FullSourceRange{FullSourceLoc{BeginLoc, SourceMgr},
+   FullSourceLoc{EndLoc, SourceMgr}};
+  }
+};
+
+TEST_F(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+
+  // WHEN:
+  const llvm::json::Object &EmptyDoc = Writer.createDocument();
+  std::vector Keys(EmptyDoc.size());
+  std::transform(EmptyDoc.begin(), EmptyDoc.end(), Keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(Keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST_F(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter Writer{LangOpts};
+  const char *ShortName = "sariftest";
+  const char *LongName = "sarif writer test";
+
+  // WHEN:
+  Writer.createRun(ShortName, LongName);
+  Writer.endRun();
+  const llvm::json::Object &Doc = Writer.createDocument();
+  const llvm::json::Array *Runs = Doc.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(Runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(Runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const llvm::json::Object *driver =
+  Runs->begin()->getAsObject()->g

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

>> I will update the commit messages, but I cannot commit to the github repo.
>
> Ah, thank you for letting me know. I can land the changes on your behalf. 
> What name and email address would you like me to use for patch attribution? 
> (I probably won't land it until tomorrow at this point though.)

Please use:

- Name: Vaibhav Yenamandra
- email address: `vyenaman...@bloomberg.net`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Basic/Sarif.cpp:161
+Region["endColumn"] = adjustColumnPos(
+R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0),
+  R.getEnd().getManager(), LO));

aaron.ballman wrote:
> I didn't catch this during the review -- but this is a layering violation 
> that caused link errors on some of the build bots. Lexer can call into Basic, 
> but Basic cannot call into Lexer. So we'll need to find a different way to 
> handle this.
Would moving the code to Support, having it depend on Basic & Lex work?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 434802.
vaibhav.y added a comment.

Update tests to check serialization as well

- SARIF text generated is validated externally against [Microsoft's online 
validator][1]

[1]: https://sarifweb.azurewebsites.net/Validation


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,170 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/LangOptions.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest-matchers.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer{LangOptions{}};
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer{LangOptions{}};
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer{LangOptions{}};
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+  SarifResult &&emptyResult = SarifResult::create(ruleIdx);
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(emptyResult); }, ".*create a run first.*");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWithValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer{LangOptions{}};
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  const SarifResult &result = SarifResult::create(ruleIdx);
+
+  writer.appendResult(result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  A

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

@aaron.ballman Would it be possible that I add `::validate` through a follow-up 
PR?

I'm currently checking the JSON output from the writer using Microsoft's online 
validator , and it is passing. 
Though it tends to complain about things outside of the spec (e.g. the spec 
doesn't constrain the toolComponent version to be numeric but the tool requires 
it to be).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

https://discourse.llvm.org/t/rfc-improving-clang-s-diagnostics/62584/8

There is an ongoing RFC similar to the work here, worth noting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146654: [clang] replaces numeric SARIF ids with heirarchical names

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:51-52
+  Diag->getDiags()->getDiagnosticIDs()->getStableName(Diag->getID()).str();
+  std::replace(StableName.begin(), StableName.end(), '_', '.');
+  SarifRule Rule = SarifRule::create().setRuleId(StableName);
 

cjdb wrote:
> denik wrote:
> > §3.5.4 says that the hierarchical strings are separated by "/".
> > 
> > But more generally, are diagnostic names really fall under "hierarchical" 
> > definition?
> > Words between underscores should be treated as components. And $3.27.5 says 
> > that the leading components have to be the identifier of the rule.
> > In some cases they look like valid components, e.g. `err_access`, 
> > `err_access_dtor`, `err_access_dtor_exception`.
> > But in cases like `err_cannot_open_file` neither of the leading components 
> > exists.
> > 
> > Theoretically we could use groups as the leading component for warnings for 
> > example. For errors the leading components are probably not even necessary, 
> > since if I understood correctly they are needed to suppress subsets of 
> > violations on the SARIF consumer side.
> > Or we just could keep the names with underscores as is. WDYT?
> I think in light of what you've said, changing back to underscores is 
> probably best.
> But more generally, are diagnostic names really fall under "hierarchical" 
> definition?

I have the same concern, but this is okay for a first pass as a "flat 
hierarchy" :)

If we want a deeper structure, we'll need some extra metadata in 
`DiagnosticSemaKinds.td`'s `Error<...>`  to add cluster names as a follow up to 
this.

WDYT about something like `clang/visibility/err_access` or 
`clang/syntax/err_stmtexpr_file_scope`.

Alternatively: we could draw from the c++ standard structure: 
https://eel.is/c++draft/basic.def.odr#term.odr.use and say the error code for 
an ODR violation could be `clang/basic/def/odr`, again I'm unsure how well this 
meshes with clang's diagnostic model.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146654/new/

https://reviews.llvm.org/D146654

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145284: WIP [clang] adds capabilities for SARIF to be written to file

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Tooling/SarifLinker.cpp:21
+
+namespace llvm::sarif_linker {
+[[noreturn]] void ReportErrorAndExit(llvm::errc error_code, StringRef Message) 
{

Is this for link diagnostics reported as SARIF?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145284/new/

https://reviews.llvm.org/D145284

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

ping: Requesting review


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

ping: This is ready for review now.

Thanks for your patience with the review as well!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y created this revision.
vaibhav.y added a reviewer: lattner.
Herald added subscribers: dexonsmith, wenlei, mgorny.
vaibhav.y edited the summary of this revision.
vaibhav.y added a project: clang.
vaibhav.y added subscribers: aaron.ballman, lebedev.ri.
vaibhav.y added a comment.
vaibhav.y edited the summary of this revision.
vaibhav.y edited the summary of this revision.
vaibhav.y edited the summary of this revision.
vaibhav.y published this revision for review.
Herald added a subscriber: cfe-commits.

I've added Roman Lebedev and Aaron Ballman as subscribers from the original RFC 
thread




Comment at: clang/lib/Basic/Sarif.cpp:27
+
+StringRef getFileName(const FileEntry &FE) {
+  StringRef Filename = FE.tryGetRealPathName();

A lot of these are copied from [[ 
https://github.com/llvm/llvm-project/blob/181d18ef53db1e5810bf6b905fbafc91da9b5baa/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp#L64
 | SarifDiagnostics.cpp ]]


Create an interface for writing SARIF documents from within `clang`:

The primary intent of this change is to introduce the interface 
`clang::SarifDocumentWriter`, which allows incrementally adding diagnostic data 
to a JSON backed document. The proposed interface is not yet connected to the 
compiler internals, which will be covered in future work. As such this change 
will not change the input/output interface of `clang`.

This change also introduces the `clang::FullSourceRange` type that is modeled 
after `clang::SourceRange` + `clang::FullSourceLoc`, this is useful for 
packaging a pair of `clang::SourceLocation` objects with their corresponding 
`SourceManager`s.

Previous discussions:

- RFC for this change: 
https://lists.llvm.org/pipermail/cfe-dev/2021-March/067907.html
- https://lists.llvm.org/pipermail/cfe-dev/2021-July/068480.html

SARIF Standard (2.1.0):

- https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,153 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was no

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 372289.
vaibhav.y added a comment.

[clangBasic] Fixup header guard


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,153 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(ruleIdx, emptyResult); },
+   ".*create a run first.*");
+}
+
+// Test adding result for invalid ruleIdx causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWithoutRuleWillCrash) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  // But caller forgot to create a rule for this run:
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(0, emptyResult); },
+   "Trying to reference a rule that doesn't exist");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWIthValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifResult &result = SarifResult::create();
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  writer.appendResult(ruleIdx, result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->s

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/include/clang/Basic/Sarif.h:69
+/// Reference:
+/// 1. https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317427";>artifactLocation
 object
+/// 2. \ref SarifArtifact

I'm not sure how to deal with overlength links in docs directly, turning clang 
format off & on on comments also seems counter-productive. Would it be okay to 
add an alias in the doxyfile for the root page of SARIF docs?

E.g.:

```
ALIASES = 
sarifDocs="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html";
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.



> Btw, is the intent for this functionality to replace what's in 
> SarifDiagnostics.cpp 
> (https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp)
>  fairly immediately? Or are we going to carry both implementations?

This is intended to replace what's in `SarifDiagnostics.cpp` in the future, it 
copies quite a few functions over from there as well. There is more work 
planned on this, since we've decided that the best way to add this feature 
without too much disrpution would be as follows:

1. Add an interface to create sarif docs
2. Add an adapter that can translate a specific diagnostic type (e.g. 
`PathDiagnostic`) to sarif
3. Rewrite the existing code to use the adapter
4. Introduce the same to clang internals

I think ordering the changes like so will let keep the individual changesets 
narrow, while adding temporary bloat. For now we must keep both implementations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.



Comment at: clang/lib/Basic/Sarif.cpp:1
+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/LangOptions.h"

lattner wrote:
> THis nees the standard header boilerplate per the coding standards doc
Ack, didn't grok the "all source files" part.



Comment at: clang/lib/Basic/Sarif.cpp:27
+
+StringRef getFileName(const FileEntry &FE) {
+  StringRef Filename = FE.tryGetRealPathName();

lattner wrote:
> vaibhav.y wrote:
> > A lot of these are copied from [[ 
> > https://github.com/llvm/llvm-project/blob/181d18ef53db1e5810bf6b905fbafc91da9b5baa/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp#L64
> >  | SarifDiagnostics.cpp ]]
> Please use static methods instead of anonymous namespaces, per the coding 
> standards doc.
Ack.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 372542.
vaibhav.y added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,153 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(ruleIdx, emptyResult); },
+   ".*create a run first.*");
+}
+
+// Test adding result for invalid ruleIdx causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWithoutRuleWillCrash) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  // But caller forgot to create a rule for this run:
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(0, emptyResult); },
+   "Trying to reference a rule that doesn't exist");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWIthValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifResult &result = SarifResult::create();
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  writer.appendResult(ruleIdx, result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 372543.
vaibhav.y added a comment.

Fixup name of class defined in documentation


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,153 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(ruleIdx, emptyResult); },
+   ".*create a run first.*");
+}
+
+// Test adding result for invalid ruleIdx causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWithoutRuleWillCrash) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  // But caller forgot to create a rule for this run:
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(0, emptyResult); },
+   "Trying to reference a rule that doesn't exist");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWIthValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifResult &result = SarifResult::create();
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  writer.appendResult(ruleIdx, result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSE

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y updated this revision to Diff 372548.
vaibhav.y added a comment.

Format clang/lib/Basic/Sarif.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109701/new/

https://reviews.llvm.org/D109701

Files:
  clang/include/clang/Basic/Sarif.h
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Sarif.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/SarifTest.cpp

Index: clang/unittests/Basic/SarifTest.cpp
===
--- /dev/null
+++ clang/unittests/Basic/SarifTest.cpp
@@ -0,0 +1,153 @@
+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include 
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+ [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+  runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(ruleIdx, emptyResult); },
+   ".*create a run first.*");
+}
+
+// Test adding result for invalid ruleIdx causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWithoutRuleWillCrash) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  // But caller forgot to create a rule for this run:
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(0, emptyResult); },
+   "Trying to reference a rule that doesn't exist");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWIthValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifResult &result = SarifResult::create();
+  const SarifRule &rule =
+  SarifRule::create()
+  .setRuleId("clang.unittest")
+  .setDescription("Example rule created during unit tests")
+  .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  writer.appendResult(ruleIdx, result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->