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
<https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317850>,
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
<https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317647>,
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 unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
SarifDocumentWriter Writer{SourceMgr};
const SarifRule &Rule =
@@ -226,7 +225,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":[{"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":[{"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},"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":[{"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 =
@@ -252,8 +251,10 @@
DiagLocs.push_back(SourceCSR);
const SarifResult &Result =
- SarifResult::create(RuleIdx).setLocations(DiagLocs).setDiagnosticMessage(
- "expected ';' after top level declarator");
+ SarifResult::create(RuleIdx)
+ .setLocations(DiagLocs)
+ .setDiagnosticMessage("expected ';' after top level declarator")
+ .setDiagnosticLevel(SarifResultLevel::Error);
Writer.appendResult(Result);
std::string Output = serializeSarifDocument(Writer.createDocument());
@@ -264,7 +265,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}}}}]}]}],"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":[{"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":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":[{"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
const char *SourceText = "int foo = 0;\n"
"int foo = 1;\n"
@@ -309,10 +310,12 @@
// WHEN: A result containing code flows and diagnostic locations is added
Writer.createRun("sarif test", "sarif test runner", "1.0.0");
unsigned RuleIdx = Writer.createRule(Rule);
- const SarifResult &Result = SarifResult::create(RuleIdx)
- .setLocations({DiagLoc})
- .setDiagnosticMessage("Redefinition of 'foo'")
- .setThreadFlows(Threadflows);
+ const SarifResult &Result =
+ SarifResult::create(RuleIdx)
+ .setLocations({DiagLoc})
+ .setDiagnosticMessage("Redefinition of 'foo'")
+ .setThreadFlows(Threadflows)
+ .setDiagnosticLevel(SarifResultLevel::Warning);
Writer.appendResult(Result);
std::string Output = serializeSarifDocument(Writer.createDocument());
Index: clang/lib/Basic/Sarif.cpp
===================================================================
--- clang/lib/Basic/Sarif.cpp
+++ clang/lib/Basic/Sarif.cpp
@@ -22,6 +22,7 @@
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/Path.h"
@@ -180,6 +181,19 @@
llvm_unreachable("Fully covered switch is not so fully covered");
}
+static StringRef resultLevelToStr(SarifResultLevel R) {
+ switch (R) {
+ case SarifResultLevel::Note:
+ return "note";
+ case SarifResultLevel::Warning:
+ return "warning";
+ case SarifResultLevel::Error:
+ return "error";
+ }
+ llvm_unreachable("Potentially un-handled SarifResultLevel. "
+ "Is the switch not fully covered?");
+}
+
static json::Object
createThreadFlowLocation(json::Object &&Location,
const ThreadFlowImportance &Importance) {
@@ -370,6 +384,7 @@
}
if (!Result.ThreadFlows.empty())
Ret["codeFlows"] = json::Array{createCodeFlow(Result.ThreadFlows)};
+ Ret["level"] = resultLevelToStr(Result.Level);
json::Object &Run = getCurrentRun();
json::Array *Results = Run.getArray("results");
Results->emplace_back(std::move(Ret));
Index: clang/include/clang/Basic/Sarif.h
===================================================================
--- clang/include/clang/Basic/Sarif.h
+++ clang/include/clang/Basic/Sarif.h
@@ -145,6 +145,17 @@
enum class ThreadFlowImportance { Important, Essential, Unimportant };
+/// The level of severity associated with a \ref SarifResult.
+///
+/// This enum excludes the level \c None since that is typically used for
+/// proof-oriented tools. For clang's case the assumption is that if a
+/// dianostic is emitted it usually implies that some change in code is being
+/// requested.
+///
+/// Reference:
+/// 1. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317648";>level property</a>
+enum class SarifResultLevel { Note, Warning, Error };
+
/// A thread flow is a sequence of code locations that specify a possible path
/// through a single thread of execution.
/// A thread flow in SARIF is related to a code flow which describes
@@ -257,6 +268,7 @@
std::string DiagnosticMessage;
llvm::SmallVector<CharSourceRange, 8> Locations;
llvm::SmallVector<ThreadFlow, 8> ThreadFlows;
+ SarifResultLevel Level = SarifResultLevel::Warning;
SarifResult() = delete;
explicit SarifResult(uint32_t RuleIdx) : RuleIdx(RuleIdx) {}
@@ -293,6 +305,11 @@
ThreadFlows.assign(ThreadFlowResults.begin(), ThreadFlowResults.end());
return *this;
}
+
+ SarifResult setDiagnosticLevel(const SarifResultLevel &TheLevel) {
+ Level = TheLevel;
+ return *this;
+ }
};
/// This class handles creating a valid SARIF document given various input
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
