NoQ updated this revision to Diff 319513. NoQ marked 22 inline comments as done. NoQ added a comment.
Address review comments! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94476/new/ https://reviews.llvm.org/D94476 Files: clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h clang/lib/Analysis/CMakeLists.txt clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp clang/unittests/Analysis/CMakeLists.txt clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp
Index: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp =================================================================== --- /dev/null +++ clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp @@ -0,0 +1,246 @@ +//===- PathDiagnosticConverterDiagnosticConsumer.cpp ------------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// This file provides unittests for PathDiagnosticConverterDiagnosticConsumer. +/// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Tooling/Tooling.h" +#include "gtest/gtest.h" + +using namespace clang; +using namespace ento; +using namespace llvm; + +struct ExpectedLocationTy { + unsigned Line; + unsigned Column; + + void testEquality(SourceLocation L, SourceManager &SM) const { + EXPECT_EQ(SM.getSpellingLineNumber(L), Line); + EXPECT_EQ(SM.getSpellingColumnNumber(L), Column); + } +}; + +struct ExpectedRangeTy { + ExpectedLocationTy Begin; + ExpectedLocationTy End; + + void testEquality(SourceRange R, SourceManager &SM) const { + Begin.testEquality(R.getBegin(), SM); + End.testEquality(R.getEnd(), SM); + } +}; + +struct ExpectedPieceTy { + ExpectedLocationTy Loc; + std::string Text; + std::vector<ExpectedRangeTy> Ranges; + + void testEquality(const PathDiagnosticPiece &Piece, SourceManager &SM) { + Loc.testEquality(Piece.getLocation().asLocation(), SM); + EXPECT_EQ(Piece.getString(), Text); + EXPECT_EQ(Ranges.size(), Piece.getRanges().size()); + for (const auto &RangeItem : llvm::enumerate(Piece.getRanges())) + Ranges[RangeItem.index()].testEquality(RangeItem.value(), SM); + } +}; + +struct ExpectedDiagTy { + ExpectedLocationTy Loc; + std::string VerboseDescription; + std::string ShortDescription; + std::string CheckerName; + std::string BugType; + std::string Category; + std::vector<ExpectedPieceTy> Path; + + void testEquality(const PathDiagnostic &Diag, SourceManager &SM) { + Loc.testEquality(Diag.getLocation().asLocation(), SM); + EXPECT_EQ(Diag.getVerboseDescription(), VerboseDescription); + EXPECT_EQ(Diag.getShortDescription(), ShortDescription); + EXPECT_EQ(Diag.getCheckerName(), CheckerName); + EXPECT_EQ(Diag.getBugType(), BugType); + EXPECT_EQ(Diag.getCategory(), Category); + + EXPECT_EQ(Path.size(), Diag.path.size()); + for (const auto &PieceItem : llvm::enumerate(Diag.path)) + Path[PieceItem.index()].testEquality(*PieceItem.value(), SM); + } +}; + +using ExpectedDiagsTy = std::vector<ExpectedDiagTy>; + +namespace { +class TestPathDiagnosticConsumer : public PathDiagnosticConsumer { + ExpectedDiagsTy ExpectedDiags; + SourceManager &SM; + +public: + TestPathDiagnosticConsumer(ExpectedDiagsTy &&ExpectedDiags, SourceManager &SM) + : ExpectedDiags(std::move(ExpectedDiags)), SM(SM) {} + + StringRef getName() const override { return "test"; } + + void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags, + FilesMade *filesMade) override { + EXPECT_EQ(Diags.size(), ExpectedDiags.size()); + for (const auto &Item : llvm::enumerate(Diags)) + ExpectedDiags[Item.index()].testEquality(*Item.value(), SM); + } +}; + +PathDiagnosticConverterDiagnosticConsumer::BugTypeInfo +bugTypeInfoProvider(const Diagnostic &) { + return PathDiagnosticConverterDiagnosticConsumer::BugTypeInfo{ + "test check name", "test bug type", "test bug category"}; +} + +class TestASTConsumer : public ASTConsumer { + ExpectedDiagsTy ExpectedDiags; + ASTContext &ACtx; + + void performTest(const Decl *D) { + if (!D->hasBody()) + return; + + assert(!ExpectedDiags.empty() && "Consumer invoked more than once!"); + TestPathDiagnosticConsumer TestPDC{std::move(ExpectedDiags), + ACtx.getSourceManager()}; + // Sanitize storage after move so that the above assertion actually worked. + ExpectedDiags.clear(); + + PathDiagnosticConsumers PathConsumers = {&TestPDC}; + + llvm::IntrusiveRefCntPtr<DiagnosticsEngine> DiagEngine = + new DiagnosticsEngine(new DiagnosticIDs, new DiagnosticOptions); + PathDiagnosticConverterDiagnosticConsumer DiagConsumer( + PathConsumers, bugTypeInfoProvider, + /*ShouldDisplayNotesAsEvents=*/true, /*ShouldTrimCheckerName=*/false); + DiagEngine->setClient(&DiagConsumer, /*ShouldOwnClient=*/false); + DiagEngine->setSourceManager(&ACtx.getSourceManager()); + + unsigned WarningDiagID = DiagEngine->getCustomDiagID( + DiagnosticsEngine::Warning, "bar is called"); + unsigned NoteDiagID = DiagEngine->getCustomDiagID( + DiagnosticsEngine::Note, "with argument at least %0"); + + const CompoundStmt *Body = cast<CompoundStmt>(D->getBody()); + for (const Stmt *S : Body->children()) { + const auto *CE = cast<CallExpr>(S); + DiagEngine->Report(CE->getBeginLoc(), WarningDiagID) + << CE->getSourceRange(); + + const auto *Arg = cast<IntegerLiteral>(CE->getArg(0)); + const llvm::APSInt &IArg = *Arg->getIntegerConstantExpr(ACtx); + for (uint64_t I = 0; I < IArg.getZExtValue(); ++I) + DiagEngine->Report(Arg->getBeginLoc(), NoteDiagID) + << (int)(I + 1) << Arg->getSourceRange(); + } + + // The last diagnostic must be flushed manually! + DiagConsumer.finish(); + + PathDiagnosticConsumer::FilesMade FM; + TestPDC.FlushDiagnostics(&FM); + } + +public: + TestASTConsumer(CompilerInstance &CI, ExpectedDiagsTy &&ExpectedDiags) + : ExpectedDiags(std::move(ExpectedDiags)), ACtx(CI.getASTContext()) {} + + bool HandleTopLevelDecl(DeclGroupRef DG) override { + for (auto *D : DG) + performTest(D); + + return true; + } +}; + +class TestAction : public ASTFrontendAction { + ExpectedDiagsTy ExpectedDiags; + +public: + TestAction(ExpectedDiagsTy &&ExpectedDiags) + : ExpectedDiags(std::move(ExpectedDiags)) {} + + std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI, + StringRef File) override { + return std::make_unique<TestASTConsumer>(CI, std::move(ExpectedDiags)); + } +}; + +TEST(PathDiagnosticConverter, ConsumeWarningsAndNotes) { + // The consumer would emit a warning against every call of 'bar()' + // and attach as many notes to it as the argument of 'bar()' says. + // Expected results are listed and should match actual results + // for the test to pass. + EXPECT_TRUE(tooling::runToolOnCode( + std::make_unique<TestAction>(ExpectedDiagsTy{ + {{3, 3}, + "Bar is called", + "Bar is called", + "test check name", + "test bug type", + "test bug category", + {{{3, 3}, "Bar is called", {{{3, 3}, {3, 8}}}}}}, + {{4, 3}, + "Bar is called", + "Bar is called", + "test check name", + "test bug type", + "test bug category", + {{{4, 3}, "Bar is called", {{{4, 3}, {4, 8}}}}}}, + {{5, 3}, + "Bar is called", + "Bar is called", + "test check name", + "test bug type", + "test bug category", + {{{5, 3}, "Bar is called", {{{5, 3}, {5, 8}}}}}}}), + "void bar(int);\nvoid foo() {\n bar(0);\n bar(0);\n bar(0);\n}\n", + "input.c")); + + EXPECT_TRUE(tooling::runToolOnCode( + std::make_unique<TestAction>(ExpectedDiagsTy{ + {{3, 3}, + "Bar is called", + "Bar is called", + "test check name", + "test bug type", + "test bug category", + {{{3, 3}, "Bar is called", {{{3, 3}, {3, 8}}}}, + {{3, 7}, "With argument at least 1", {{{3, 7}, {3, 7}}}}, + {{3, 7}, "With argument at least 2", {{{3, 7}, {3, 7}}}}}}}), + "void bar(int);\nvoid foo() {\n bar(2);\n}\n", "input.c")); + + EXPECT_TRUE(tooling::runToolOnCode( + std::make_unique<TestAction>(ExpectedDiagsTy{ + {{3, 3}, + "Bar is called", + "Bar is called", + "test check name", + "test bug type", + "test bug category", + {{{3, 3}, "Bar is called", {{{3, 3}, {3, 8}}}}, + {{3, 7}, "With argument at least 1", {{{3, 7}, {3, 7}}}}, + {{3, 7}, "With argument at least 2", {{{3, 7}, {3, 7}}}}}}, + {{4, 3}, + "Bar is called", + "Bar is called", + "test check name", + "test bug type", + "test bug category", + {{{4, 3}, "Bar is called", {{{4, 3}, {4, 8}}}}, + {{4, 7}, "With argument at least 1", {{{4, 7}, {4, 7}}}}}}}), + "void bar(int);\nvoid foo() {\n bar(2);\n bar(1);\n}\n", "input.c")); +} +} // namespace Index: clang/unittests/Analysis/CMakeLists.txt =================================================================== --- clang/unittests/Analysis/CMakeLists.txt +++ clang/unittests/Analysis/CMakeLists.txt @@ -8,6 +8,7 @@ CFGTest.cpp CloneDetectionTest.cpp ExprMutationAnalyzerTest.cpp + PathDiagnosticConverterDiagnosticConsumer.cpp ) clang_target_link_libraries(ClangAnalysisTests Index: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp =================================================================== --- /dev/null +++ clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp @@ -0,0 +1,112 @@ +//===--- PathDiagnosticConverterDiagnosticConsumer.cpp ----------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// +// +// This file defines the PathDiagnosticConverterDiagnosticConsumer object +// which converts regular Clang diagnostics into PathDiagnostics. +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h" + +using namespace clang; +using namespace ento; + +void PathDiagnosticConverterDiagnosticConsumer::flushPartialDiagnostic() { + if (PartialPDs.empty()) + return; + + assert(PartialPDs.size() == PathConsumers.size() && + "Path diagnostics were constructed for not all consumers"); + + for (PathDiagnosticConsumer *Consumer : PathConsumers) + Consumer->HandlePathDiagnostic(std::move(PartialPDs[Consumer])); + + PartialPDs.clear(); +} + +static void convertDiagnosticData(const Diagnostic &Info, + PathDiagnosticPiece &Piece) { + for (const auto &R : Info.getRanges()) + Piece.addRange(R.getAsRange()); + + // FIXME: Convert more data, eg. fix-it hints. +} + +void PathDiagnosticConverterDiagnosticConsumer::HandleDiagnostic( + DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) { + // Count warnings/errors. + DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info); + + llvm::SmallString<128> Msg; + Info.FormatDiagnostic(Msg); + + // Auto-capitalize. Path diagnostics are traditionally treated as + // full sentences whereas Clang diagnostics are traditionally lowercase. + assert(Msg.size() > 0 && "Empty diagnostic message"); + Msg[0] = std::toupper(Msg[0]); + + // Optionally try to remove checker name. + StringRef CleanMsg; // PathDiagnosticPiece will copy it. + if (ShouldTrimCheckerName) { + size_t I = Msg.str().rfind('['); + assert(I != std::string::npos && "Checker name not detected in diagnostic"); + assert(I > 0 && "Diagnostic message contains only checker name"); + CleanMsg = Msg.str().substr(0, I - 1); + } else { + CleanMsg = Msg; + } + + SourceLocation Loc = Info.getLocation(); + const SourceManager &SM = Info.getSourceManager(); + + PathDiagnosticLocation PDLoc(Loc, SM); + + // For now we assume that every diagnostic is either a warning or a note + // logically attached to a previous warning. + // Notes do not come in actually attached to their respective warnings + // but are fed to us independently. The good news is they always follow their + // respective warnings and come in in the right order so we can + // automatically re-attach them. + if (DiagLevel == DiagnosticsEngine::Note) { + for (PathDiagnosticConsumer *Consumer : PathConsumers) { + PathDiagnosticPieceRef Piece; + if (ShouldDisplayNotesAsEvents) { + Piece = std::make_shared<PathDiagnosticEventPiece>(PDLoc, CleanMsg); + } else { + Piece = std::make_shared<PathDiagnosticNotePiece>(PDLoc, CleanMsg); + } + + convertDiagnosticData(Info, *Piece); + + PartialPDs[Consumer]->getMutablePieces().push_back(Piece); + } + } else { + assert((DiagLevel == DiagnosticsEngine::Warning || + DiagLevel == DiagnosticsEngine::Error) && + "Unsupported diagnostic level"); + + // No more incoming notes. The current path diagnostic is complete. + flushPartialDiagnostic(); + + BugTypeInfo BTI = BTIProvider(Info); + + for (PathDiagnosticConsumer *Consumer : PathConsumers) { + auto PD = std::make_unique<PathDiagnostic>( + BTI.CheckName, /*DeclWithIssue=*/nullptr, BTI.BugType, + CleanMsg, CleanMsg, BTI.BugCategory, PDLoc, /*DeclToUnique=*/nullptr, + std::make_unique<FilesToLineNumsMap>()); + + PathDiagnosticPieceRef Piece = + std::make_shared<PathDiagnosticEventPiece>(PDLoc, CleanMsg); + convertDiagnosticData(Info, *Piece); + PD->setEndOfPath(std::move(Piece)); + + PartialPDs[Consumer] = std::move(PD); + } + } +} Index: clang/lib/Analysis/CMakeLists.txt =================================================================== --- clang/lib/Analysis/CMakeLists.txt +++ clang/lib/Analysis/CMakeLists.txt @@ -23,6 +23,7 @@ LiveVariables.cpp ObjCNoReturn.cpp PathDiagnostic.cpp + PathDiagnosticConverterDiagnosticConsumer.cpp PlistPathDiagnosticConsumer.cpp PlistHTMLPathDiagnosticConsumer.cpp PostOrderCFGView.cpp Index: clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h =================================================================== --- /dev/null +++ clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h @@ -0,0 +1,90 @@ +//===--- PathDiagnosticConverterDiagnosticConsumer.h ------------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// This file defines a Clang DiagnosticConsumer that converts Clang +/// diagnostics into PathDiagnostics. +/// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_ANALYSIS_PATHDIAGNOSTICCONVERTERDIAGNOSTICCONSUMER_H +#define LLVM_CLANG_ANALYSIS_PATHDIAGNOSTICCONVERTERDIAGNOSTICCONSUMER_H + +#include "clang/Analysis/PathDiagnostic.h" +#include "clang/Analysis/PathDiagnosticConsumers.h" + +namespace clang { +namespace ento { + +/// \brief A Clang diagnostic consumer that converts Clang diagnostics into +/// "path diagnostics" commonly used by the Static Analyzer. +/// +/// PathDiagnostics are immediately pushed into PathDiagnosticConsumers +/// that can display clang diagnostics in a number of different ways, +/// either simply for the visuals or possibly for achieving consistency +/// with other tools that already use path diagnostics. +/// If an existing tool already uses a PathDiagnosticConsumer that converts +/// path diagnostics into clang diagnostics (eg., TextPathDiagnosticConsumer), +/// putting such diagnostics back into PathDiagnosticConverterDiagnosticConsumer +/// is undesirable because the original conversion to Clang diagnostics +/// is not lossless. Instead such tool is supposed to keep accessing +/// PathDiagnosticConsumers directly. PathDiagnosticConsumers are expected +/// to be constructed and managed separately. +class PathDiagnosticConverterDiagnosticConsumer : public DiagnosticConsumer { +public: + struct BugTypeInfo { + StringRef CheckName; + StringRef BugType; + StringRef BugCategory; + }; + + using BugTypeInfoProvider = std::function<BugTypeInfo(const Diagnostic &)>; + +private: + PathDiagnosticConsumers &PathConsumers; + BugTypeInfoProvider BTIProvider; + bool ShouldDisplayNotesAsEvents; + bool ShouldTrimCheckerName; + + llvm::DenseMap<PathDiagnosticConsumer *, std::unique_ptr<PathDiagnostic>> + PartialPDs; + + void flushPartialDiagnostic(); + +public: + /// Construct a PathDiagnosticConverterDiagnosticConsumer with existing + /// PathDiagnosticConsumers and the necessary configuration. + /// \param PathConsumers The list of PathDiagnosticConsumers to work with. + /// \param BTIProvider A callback that provides additional metadata + /// about each diagnostic that's necessary for path diagnostics + /// but typically not present in regular Clang diagnostics. + /// \param ShouldDisplayNotesAsEvents Whether display note diagnostics as path + // events or non-path notes. The latter is always the intended + // behavior as long as the client actually understands non-path notes. + /// \param ShouldTrimCheckerName Whether incoming diagnostics are suffixed + // with [checker name] which we can automatically trim. + PathDiagnosticConverterDiagnosticConsumer( + PathDiagnosticConsumers &PathConsumers, BugTypeInfoProvider &&BTIProvider, + bool ShouldDisplayNotesAsEvents, bool ShouldTrimCheckerName) + : PathConsumers(PathConsumers), BTIProvider(std::move(BTIProvider)), + ShouldDisplayNotesAsEvents(ShouldDisplayNotesAsEvents), + ShouldTrimCheckerName(ShouldTrimCheckerName) {} + + void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, + const Diagnostic &Info) override; + + /// Inform the consumer that the last diagnostic has been sent. This is + /// necessary because the consumer does not know whether more notes are + /// going to be attached to the last warning. + void finish() override { flushPartialDiagnostic(); } +}; + +} // end 'ento' namespace +} // end 'clang' namespace + +#endif
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
