Skip to content

Commit c019142

Browse files
committed
[analyzer][NFC] Split the main logic of NoStoreFuncVisitor to an abstract NoStateChangeVisitor class
Preceding discussion on cfe-dev: https://lists.llvm.org/pipermail/cfe-dev/2021-June/068450.html NoStoreFuncVisitor is a rather unique visitor. As VisitNode is invoked on most other visitors, they are looking for the point where something changed -- change on a value, some checker-specific GDM trait, a new constraint. NoStoreFuncVisitor, however, looks specifically for functions that *didn't* write to a MemRegion of interesting. Quoting from its comments: /// Put a diagnostic on return statement of all inlined functions /// for which the region of interest \p RegionOfInterest was passed into, /// but not written inside, and it has caused an undefined read or a null /// pointer dereference outside. It so happens that there are a number of other similar properties that are worth checking. For instance, if some memory leaks, it might be interesting why a function didn't take ownership of said memory: void sink(int *P) {} // no notes void f() { sink(new int(5)); // note: Memory is allocated // Well hold on, sink() was supposed to deal with // that, this must be a false positive... } // warning: Potential memory leak [cplusplus.NewDeleteLeaks] In here, the entity of interest isn't a MemRegion, but a symbol. The property that changed here isn't a change of value, but rather liveness and GDM traits managed by MalloChecker. This patch moves some of the logic of NoStoreFuncVisitor to a new abstract class, NoStateChangeFuncVisitor. This is mostly calculating and caching the stack frames in which the entity of interest wasn't changed. Descendants of this interface have to define 3 things: * What constitutes as a change to an entity (this is done by overriding wasModifiedBeforeCallExit) * What the diagnostic message should be (this is done by overriding maybeEmitNoteFor.*) * What constitutes as the entity of interest being passed into the function (this is also done by overriding maybeEmitNoteFor.*) Differential Revision: https://reviews.llvm.org/D105553
1 parent 2c5c06c commit c019142

File tree

2 files changed

+269
-168
lines changed

2 files changed

+269
-168
lines changed

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

+79
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "llvm/ADT/FoldingSet.h"
2222
#include "llvm/ADT/IntrusiveRefCntPtr.h"
2323
#include "llvm/ADT/STLExtras.h"
24+
#include "llvm/ADT/SmallPtrSet.h"
2425
#include "llvm/ADT/StringRef.h"
2526
#include <list>
2627
#include <memory>
@@ -622,6 +623,84 @@ class TagVisitor : public BugReporterVisitor {
622623
PathSensitiveBugReport &R) override;
623624
};
624625

626+
class ObjCMethodCall;
627+
class CXXConstructorCall;
628+
629+
/// Put a diagnostic on return statement (or on } in its absence) of all inlined
630+
/// functions for which some property remained unchanged.
631+
/// Resulting diagnostics may read such as "Returning without writing to X".
632+
///
633+
/// Descendants can define what a "state change is", like a change of value
634+
/// to a memory region, liveness, etc. For function calls where the state did
635+
/// not change as defined, a custom note may be constructed.
636+
class NoStateChangeFuncVisitor : public BugReporterVisitor {
637+
private:
638+
/// Frames modifying the state as defined in \c wasModifiedBeforeCallExit.
639+
/// This visitor generates a note only if a function does *not* change the
640+
/// state that way. This information is not immediately available
641+
/// by looking at the node associated with the exit from the function
642+
/// (usually the return statement). To avoid recomputing the same information
643+
/// many times (going up the path for each node and checking whether the
644+
/// region was written into) we instead lazily compute the stack frames
645+
/// along the path.
646+
llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifying;
647+
llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
648+
649+
/// Check and lazily calculate whether the state is modified in the stack
650+
/// frame to which \p CallExitBeginN belongs.
651+
/// The calculation is cached in FramesModifying.
652+
bool isModifiedInFrame(const ExplodedNode *CallExitBeginN);
653+
654+
/// Write to \c FramesModifying all stack frames along the path in the current
655+
/// stack frame which modifies the state.
656+
void findModifyingFrames(const ExplodedNode *const CallExitBeginN);
657+
658+
protected:
659+
bugreporter::TrackingKind TKind;
660+
661+
/// \return Whether the state was modified from the current node, \CurrN, to
662+
/// the end of the stack fram, at \p CallExitBeginN.
663+
virtual bool
664+
wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
665+
const ExplodedNode *CallExitBeginN) = 0;
666+
667+
/// Consume the information on the non-modifying stack frame in order to
668+
/// either emit a note or not. May suppress the report entirely.
669+
/// \return Diagnostics piece for the unmodified state in the current
670+
/// function, if it decides to emit one. A good description might start with
671+
/// "Returning without...".
672+
virtual PathDiagnosticPieceRef
673+
maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
674+
const ObjCMethodCall &Call,
675+
const ExplodedNode *N) = 0;
676+
677+
/// Consume the information on the non-modifying stack frame in order to
678+
/// either emit a note or not. May suppress the report entirely.
679+
/// \return Diagnostics piece for the unmodified state in the current
680+
/// function, if it decides to emit one. A good description might start with
681+
/// "Returning without...".
682+
virtual PathDiagnosticPieceRef
683+
maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
684+
const CXXConstructorCall &Call,
685+
const ExplodedNode *N) = 0;
686+
687+
/// Consume the information on the non-modifying stack frame in order to
688+
/// either emit a note or not. May suppress the report entirely.
689+
/// \return Diagnostics piece for the unmodified state in the current
690+
/// function, if it decides to emit one. A good description might start with
691+
/// "Returning without...".
692+
virtual PathDiagnosticPieceRef
693+
maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
694+
const ExplodedNode *N) = 0;
695+
696+
public:
697+
NoStateChangeFuncVisitor(bugreporter::TrackingKind TKind) : TKind(TKind) {}
698+
699+
PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
700+
BugReporterContext &BR,
701+
PathSensitiveBugReport &R) override final;
702+
};
703+
625704
} // namespace ento
626705

627706
} // namespace clang

0 commit comments

Comments
 (0)