[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy, a.sidorin, szepet.
Herald added a project: clang.

As i mentioned in D58067#1393674 <https://reviews.llvm.org/D58067#1393674>, i dislike the way bug reporter visitors have to reverse-engineer the checker's behavior instead of asking the checker what happened directly.

The approach suggested here is to allow the checker to take notes of what happens as it adds transitions, and then when the report is thrown, reconstruct path messages from these notes.

Because for the sake of future-proofness such messages should probably be allowed to be expensive to construct (even though i'm not aware of any specific examples of expensive-to-construct messages), the checker notes are defined to be lambdas that simply capture the necessary information but don't process it until a report is actually emitted. As a useful side effect, this allows the message to depend on the `BugReport` object itself, which is completely essential because most checkers won't emit path messages for every state update they make. For instance, when MallocChecker diagnoses a use-after-free, it only emits the "memory is freed" path message for the symbol that was accessed after free, but not for other symbols that were allocated or deallocated along the path. With lambda notes, we can check if the symbol is marked as "interesting" in the BugReport before emitting the path message. In the future we may want to extend the BugReport object to carry arbitrary Checker-specific data that the checker can take advantage of within its note lambdas.

Checker notes from which path messages are constructed are implemented as `ProgramPointTags` of special sub-kind: `NoteTag`. Then a special visitor is added to every report in order to scan the report for those tags and invoke the lambdas.

Here's how it looks in the checker in my next patch that actually makes use of the new functionality:

  const NoteTag *T = C.getNoteTag([PVD]() -> std::string {
    SmallString<64> Str;
    llvm::raw_svector_ostream OS(Str);
    OS << "Deallocating object passed through parameter '" << PVD->getName() << '\'';
    return OS.str();
  });
  
  C.addTransition(C.getState()->set<ReleasedParameter>(true), T);

And there's no need to write all of this anymore:

  class MyVisitor: public BugReporterVisitor {
  /*Manual captures...*/
  public:
    MyVisitor(/*Manual captures...*/) { ... }
  
    void Profile(llvm::FoldingSetNodeID &ID) {
      // The usual static int business.
      // And mention all manual captures.
      // Or maybe almost all, depends.
    }
  
    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                 BugReporterContext &BRC,
                                                 BugReport &BR) override {
      // The usual GDM update reverse-engineering idiom.
      if (N->getState()->get<MyTrait>(...) != N->getFirstPred()->get<MyTrait>(...)) {
         // Then the same message generating code.
  
         // Then a bunch of boilerplate to generate the piece itself:
         const Stmt *S = PathDiagnosticLocation::getStmt(N);
         if (!S)
           return nullptr;
         PathDiagnosticLocation Loc = PathDiagnosticLocation::create(S);
         return std::make_shared<PathDiagnosticEventPiece>(Loc, OS.str());
      }
    }
  };
  
  ...
  
  BR.addVisitor(MyVisitor(/*Manual captures...*/));


Repository:
  rC Clang

https://reviews.llvm.org/D58367

Files:
  clang/include/clang/Analysis/ProgramPoint.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58367.187297.patch
Type: text/x-patch
Size: 7673 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190219/6890e756/attachment.bin>