[PATCH] D103644: [analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbebf38b736a: [analyzer] Refactor StoreSiteFinder and
extract DefaultStoreHandler (authored by vsavchenko).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103644/new/
https://reviews.llvm.org/D103644
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1232,12 +1232,7 @@
SVal V;
bool Satisfied = false;
- /// If the visitor is tracking the value directly responsible for the
- /// bug, we are going to employ false positive suppression.
- bool EnableNullFPSuppression;
-
- using TrackingKind = bugreporter::TrackingKind;
- TrackingKind TKind;
+ TrackingOptions Options;
const StackFrameContext *OriginSFC;
public:
@@ -1252,11 +1247,9 @@
///this visitor can prevent that without polluting the bugpath too
///much.
StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
- const MemRegion *R, bool InEnableNullFPSuppression,
- TrackingKind TKind,
+ const MemRegion *R, TrackingOptions Options,
const StackFrameContext *OriginSFC = nullptr)
- : TrackingBugReporterVisitor(ParentTracker), R(R), V(V),
-EnableNullFPSuppression(InEnableNullFPSuppression), TKind(TKind),
+ : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), Options(Options),
OriginSFC(OriginSFC) {
assert(R);
}
@@ -1273,8 +1266,8 @@
ID.AddPointer();
ID.AddPointer(R);
ID.Add(V);
- ID.AddInteger(static_cast(TKind));
- ID.AddBoolean(EnableNullFPSuppression);
+ ID.AddInteger(static_cast(Options.Kind));
+ ID.AddBoolean(Options.EnableNullFPSuppression);
}
/// Returns true if \p N represents the DeclStmt declaring and initializing
@@ -1533,8 +1526,7 @@
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
-getParentTracker().track(InitE, StoreSite,
- {TKind, EnableNullFPSuppression});
+getParentTracker().track(InitE, StoreSite, Options);
}
// Let's try to find the region where the value came from.
@@ -1605,7 +1597,7 @@
}
}
- if (TKind == TrackingKind::Condition &&
+ if (Options.Kind == TrackingKind::Condition && OriginSFC &&
!OriginSFC->isParentOf(StoreSite->getStackFrame()))
return nullptr;
@@ -1613,60 +1605,41 @@
SmallString<256> sbuf;
llvm::raw_svector_ostream os(sbuf);
+ StoreInfo SI = {StoreInfo::Assignment, // default kind
+ StoreSite,
+ InitE,
+ V,
+ R,
+ OldRegion};
+
if (Optional PS = StoreSite->getLocationAs()) {
const Stmt *S = PS->getStmt();
-const char *action = nullptr;
const auto *DS = dyn_cast(S);
const auto *VR = dyn_cast(R);
if (DS) {
- action = R->canPrintPretty() ? "initialized to " :
- "Initializing to ";
+ SI.StoreKind = StoreInfo::Initialization;
} else if (isa(S)) {
- action = R->canPrintPretty() ? "captured by block as " :
- "Captured by block as ";
+ SI.StoreKind = StoreInfo::BlockCapture;
if (VR) {
// See if we can get the BlockVarRegion.
ProgramStateRef State = StoreSite->getState();
SVal V = StoreSite->getSVal(S);
if (const auto *BDR =
- dyn_cast_or_null(V.getAsRegion())) {
+dyn_cast_or_null(V.getAsRegion())) {
if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
-if (auto KV = State->getSVal(OriginalR).getAs())
- getParentTracker().track(
- *KV, OriginalR, {TKind, EnableNullFPSuppression}, OriginSFC);
+getParentTracker().track(State->getSVal(OriginalR), OriginalR,
+ Options, OriginSFC);
}
}
}
}
-if (action)
- showBRDiagnostics(action, os, R, V, OldRegion, DS);
-
- } else if (StoreSite->getLocation().getAs()) {
-if (const auto *VR = dyn_cast(R))
- showBRParamDiagnostics(os, VR, V, OldRegion);
+ } else if (SI.StoreSite->getLocation().getAs() &&
+ isa(SI.Dest)) {
+SI.StoreKind = StoreInfo::CallArgument;
}
- if (os.str().empty())
-showBRDefaultDiagnostics(os, R, V, OldRegion);
-
- if (TKind == bugreporter::TrackingKind::Condition)
-os << WillBeUsedForACondition;
-
- // Construct a new PathDiagnosticPiece.
- ProgramPoint P = StoreSite->getLocation();
- PathDiagnosticLocation L;
- if (P.getAs() && InitE)
-L =
[PATCH] D103644: [analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Great, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103644/new/ https://reviews.llvm.org/D103644 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103644: [analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler
vsavchenko updated this revision to Diff 351512.
vsavchenko marked 2 inline comments as done.
vsavchenko added a comment.
Fix review remarks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103644/new/
https://reviews.llvm.org/D103644
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1232,12 +1232,7 @@
SVal V;
bool Satisfied = false;
- /// If the visitor is tracking the value directly responsible for the
- /// bug, we are going to employ false positive suppression.
- bool EnableNullFPSuppression;
-
- using TrackingKind = bugreporter::TrackingKind;
- TrackingKind TKind;
+ TrackingOptions Options;
const StackFrameContext *OriginSFC;
public:
@@ -1252,11 +1247,9 @@
///this visitor can prevent that without polluting the bugpath too
///much.
StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
- const MemRegion *R, bool InEnableNullFPSuppression,
- TrackingKind TKind,
+ const MemRegion *R, TrackingOptions Options,
const StackFrameContext *OriginSFC = nullptr)
- : TrackingBugReporterVisitor(ParentTracker), R(R), V(V),
-EnableNullFPSuppression(InEnableNullFPSuppression), TKind(TKind),
+ : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), Options(Options),
OriginSFC(OriginSFC) {
assert(R);
}
@@ -1273,8 +1266,8 @@
ID.AddPointer();
ID.AddPointer(R);
ID.Add(V);
- ID.AddInteger(static_cast(TKind));
- ID.AddBoolean(EnableNullFPSuppression);
+ ID.AddInteger(static_cast(Options.Kind));
+ ID.AddBoolean(Options.EnableNullFPSuppression);
}
/// Returns true if \p N represents the DeclStmt declaring and initializing
@@ -1533,8 +1526,7 @@
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
-getParentTracker().track(InitE, StoreSite,
- {TKind, EnableNullFPSuppression});
+getParentTracker().track(InitE, StoreSite, Options);
}
// Let's try to find the region where the value came from.
@@ -1605,7 +1597,7 @@
}
}
- if (TKind == TrackingKind::Condition &&
+ if (Options.Kind == TrackingKind::Condition && OriginSFC &&
!OriginSFC->isParentOf(StoreSite->getStackFrame()))
return nullptr;
@@ -1613,60 +1605,41 @@
SmallString<256> sbuf;
llvm::raw_svector_ostream os(sbuf);
+ StoreInfo SI = {StoreInfo::Assignment, // default kind
+ StoreSite,
+ InitE,
+ V,
+ R,
+ OldRegion};
+
if (Optional PS = StoreSite->getLocationAs()) {
const Stmt *S = PS->getStmt();
-const char *action = nullptr;
const auto *DS = dyn_cast(S);
const auto *VR = dyn_cast(R);
if (DS) {
- action = R->canPrintPretty() ? "initialized to " :
- "Initializing to ";
+ SI.StoreKind = StoreInfo::Initialization;
} else if (isa(S)) {
- action = R->canPrintPretty() ? "captured by block as " :
- "Captured by block as ";
+ SI.StoreKind = StoreInfo::BlockCapture;
if (VR) {
// See if we can get the BlockVarRegion.
ProgramStateRef State = StoreSite->getState();
SVal V = StoreSite->getSVal(S);
if (const auto *BDR =
- dyn_cast_or_null(V.getAsRegion())) {
+dyn_cast_or_null(V.getAsRegion())) {
if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
-if (auto KV = State->getSVal(OriginalR).getAs())
- getParentTracker().track(
- *KV, OriginalR, {TKind, EnableNullFPSuppression}, OriginSFC);
+getParentTracker().track(State->getSVal(OriginalR), OriginalR,
+ Options, OriginSFC);
}
}
}
}
-if (action)
- showBRDiagnostics(action, os, R, V, OldRegion, DS);
-
- } else if (StoreSite->getLocation().getAs()) {
-if (const auto *VR = dyn_cast(R))
- showBRParamDiagnostics(os, VR, V, OldRegion);
+ } else if (SI.StoreSite->getLocation().getAs() &&
+ isa(SI.Dest)) {
+SI.StoreKind = StoreInfo::CallArgument;
}
- if (os.str().empty())
-showBRDefaultDiagnostics(os, R, V, OldRegion);
-
- if (TKind == bugreporter::TrackingKind::Condition)
-os << WillBeUsedForACondition;
-
- // Construct a new PathDiagnosticPiece.
- ProgramPoint P = StoreSite->getLocation();
- PathDiagnosticLocation L;
- if (P.getAs() && InitE)
-L = PathDiagnosticLocation(InitE, BRC.getSourceManager(),
-
[PATCH] D103644: [analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler
vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2047 + +// TODO: get rid of it. +const DeclStmt *DS = nullptr; NoQ wrote: > WDYM? > > Also maybe downscope it? I meant that the only client for it is undef value printer and we can probably get rid of it altogether, or at least what I did in the very last patch from the stack. Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2063 + : "Captured by block as "; + showBRDiagnostics(Action, OS, SI.Dest, SI.Value, SI.Origin, DS); + break; NoQ wrote: > Isn't `DS` always null here? Duh! Yes! Good catch, I'll remove DS declaration! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103644/new/ https://reviews.llvm.org/D103644 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103644: [analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler
NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2047 + +// TODO: get rid of it. +const DeclStmt *DS = nullptr; WDYM? Also maybe downscope it? Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2063 + : "Captured by block as "; + showBRDiagnostics(Action, OS, SI.Dest, SI.Value, SI.Origin, DS); + break; Isn't `DS` always null here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103644/new/ https://reviews.llvm.org/D103644 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103644: [analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler
vsavchenko updated this revision to Diff 351145.
vsavchenko added a comment.
Rebase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103644/new/
https://reviews.llvm.org/D103644
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1232,12 +1232,7 @@
SVal V;
bool Satisfied = false;
- /// If the visitor is tracking the value directly responsible for the
- /// bug, we are going to employ false positive suppression.
- bool EnableNullFPSuppression;
-
- using TrackingKind = bugreporter::TrackingKind;
- TrackingKind TKind;
+ TrackingOptions Options;
const StackFrameContext *OriginSFC;
public:
@@ -1252,11 +1247,9 @@
///this visitor can prevent that without polluting the bugpath too
///much.
StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
- const MemRegion *R, bool InEnableNullFPSuppression,
- TrackingKind TKind,
+ const MemRegion *R, TrackingOptions Options,
const StackFrameContext *OriginSFC = nullptr)
- : TrackingBugReporterVisitor(ParentTracker), R(R), V(V),
-EnableNullFPSuppression(InEnableNullFPSuppression), TKind(TKind),
+ : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), Options(Options),
OriginSFC(OriginSFC) {
assert(R);
}
@@ -1273,8 +1266,8 @@
ID.AddPointer();
ID.AddPointer(R);
ID.Add(V);
- ID.AddInteger(static_cast(TKind));
- ID.AddBoolean(EnableNullFPSuppression);
+ ID.AddInteger(static_cast(Options.Kind));
+ ID.AddBoolean(Options.EnableNullFPSuppression);
}
/// Returns true if \p N represents the DeclStmt declaring and initializing
@@ -1533,8 +1526,7 @@
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
-getParentTracker().track(InitE, StoreSite,
- {TKind, EnableNullFPSuppression});
+getParentTracker().track(InitE, StoreSite, Options);
}
// Let's try to find the region where the value came from.
@@ -1605,7 +1597,7 @@
}
}
- if (TKind == TrackingKind::Condition &&
+ if (Options.Kind == TrackingKind::Condition && OriginSFC &&
!OriginSFC->isParentOf(StoreSite->getStackFrame()))
return nullptr;
@@ -1613,60 +1605,41 @@
SmallString<256> sbuf;
llvm::raw_svector_ostream os(sbuf);
+ StoreInfo SI = {StoreInfo::Assignment, // default kind
+ StoreSite,
+ InitE,
+ V,
+ R,
+ OldRegion};
+
if (Optional PS = StoreSite->getLocationAs()) {
const Stmt *S = PS->getStmt();
-const char *action = nullptr;
const auto *DS = dyn_cast(S);
const auto *VR = dyn_cast(R);
if (DS) {
- action = R->canPrintPretty() ? "initialized to " :
- "Initializing to ";
+ SI.StoreKind = StoreInfo::Initialization;
} else if (isa(S)) {
- action = R->canPrintPretty() ? "captured by block as " :
- "Captured by block as ";
+ SI.StoreKind = StoreInfo::BlockCapture;
if (VR) {
// See if we can get the BlockVarRegion.
ProgramStateRef State = StoreSite->getState();
SVal V = StoreSite->getSVal(S);
if (const auto *BDR =
- dyn_cast_or_null(V.getAsRegion())) {
+dyn_cast_or_null(V.getAsRegion())) {
if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
-if (auto KV = State->getSVal(OriginalR).getAs())
- getParentTracker().track(
- *KV, OriginalR, {TKind, EnableNullFPSuppression}, OriginSFC);
+getParentTracker().track(State->getSVal(OriginalR), OriginalR,
+ Options, OriginSFC);
}
}
}
}
-if (action)
- showBRDiagnostics(action, os, R, V, OldRegion, DS);
-
- } else if (StoreSite->getLocation().getAs()) {
-if (const auto *VR = dyn_cast(R))
- showBRParamDiagnostics(os, VR, V, OldRegion);
+ } else if (SI.StoreSite->getLocation().getAs() &&
+ isa(SI.Dest)) {
+SI.StoreKind = StoreInfo::CallArgument;
}
- if (os.str().empty())
-showBRDefaultDiagnostics(os, R, V, OldRegion);
-
- if (TKind == bugreporter::TrackingKind::Condition)
-os << WillBeUsedForACondition;
-
- // Construct a new PathDiagnosticPiece.
- ProgramPoint P = StoreSite->getLocation();
- PathDiagnosticLocation L;
- if (P.getAs() && InitE)
-L = PathDiagnosticLocation(InitE, BRC.getSourceManager(),
- P.getLocationContext());
-
- if (!L.isValid() ||
[PATCH] D103644: [analyzer] Refactor StoreSiteFinder and extract DefaultStoreHandler
vsavchenko created this revision.
vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus,
manas, RedDocMD.
Herald added subscribers: ASDenysPetrov, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
After this patch, custom StoreHandlers will also work as expected.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D103644
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1232,12 +1232,7 @@
SVal V;
bool Satisfied = false;
- /// If the visitor is tracking the value directly responsible for the
- /// bug, we are going to employ false positive suppression.
- bool EnableNullFPSuppression;
-
- using TrackingKind = bugreporter::TrackingKind;
- TrackingKind TKind;
+ TrackingOptions Options;
const StackFrameContext *OriginSFC;
public:
@@ -1252,11 +1247,9 @@
///this visitor can prevent that without polluting the bugpath too
///much.
StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
- const MemRegion *R, bool InEnableNullFPSuppression,
- TrackingKind TKind,
+ const MemRegion *R, TrackingOptions Options,
const StackFrameContext *OriginSFC = nullptr)
- : TrackingBugReporterVisitor(ParentTracker), R(R), V(V),
-EnableNullFPSuppression(InEnableNullFPSuppression), TKind(TKind),
+ : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), Options(Options),
OriginSFC(OriginSFC) {
assert(R);
}
@@ -1273,8 +1266,8 @@
ID.AddPointer();
ID.AddPointer(R);
ID.Add(V);
- ID.AddInteger(static_cast(TKind));
- ID.AddBoolean(EnableNullFPSuppression);
+ ID.AddInteger(static_cast(Options.Kind));
+ ID.AddBoolean(Options.EnableNullFPSuppression);
}
/// Returns true if \p N represents the DeclStmt declaring and initializing
@@ -1533,8 +1526,7 @@
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
-getParentTracker().track(InitE, StoreSite,
- {TKind, EnableNullFPSuppression});
+getParentTracker().track(InitE, StoreSite, Options);
}
// Let's try to find the region where the value came from.
@@ -1605,7 +1597,7 @@
}
}
- if (TKind == TrackingKind::Condition &&
+ if (Options.Kind == TrackingKind::Condition && OriginSFC &&
!OriginSFC->isParentOf(StoreSite->getStackFrame()))
return nullptr;
@@ -1613,60 +1605,41 @@
SmallString<256> sbuf;
llvm::raw_svector_ostream os(sbuf);
+ StoreInfo SI = {StoreInfo::Assignment, // default kind
+ StoreSite,
+ InitE,
+ V,
+ R,
+ OldRegion};
+
if (Optional PS = StoreSite->getLocationAs()) {
const Stmt *S = PS->getStmt();
-const char *action = nullptr;
const auto *DS = dyn_cast(S);
const auto *VR = dyn_cast(R);
if (DS) {
- action = R->canPrintPretty() ? "initialized to " :
- "Initializing to ";
+ SI.StoreKind = StoreInfo::Initialization;
} else if (isa(S)) {
- action = R->canPrintPretty() ? "captured by block as " :
- "Captured by block as ";
+ SI.StoreKind = StoreInfo::BlockCapture;
if (VR) {
// See if we can get the BlockVarRegion.
ProgramStateRef State = StoreSite->getState();
SVal V = StoreSite->getSVal(S);
if (const auto *BDR =
- dyn_cast_or_null(V.getAsRegion())) {
+dyn_cast_or_null(V.getAsRegion())) {
if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
if (auto KV = State->getSVal(OriginalR).getAs())
- getParentTracker().track(
- *KV, OriginalR, {TKind, EnableNullFPSuppression}, OriginSFC);
+ getParentTracker().track(*KV, OriginalR, Options, OriginSFC);
}
}
}
}
-if (action)
- showBRDiagnostics(action, os, R, V, OldRegion, DS);
-
- } else if (StoreSite->getLocation().getAs()) {
-if (const auto *VR = dyn_cast(R))
- showBRParamDiagnostics(os, VR, V, OldRegion);
+ } else if (SI.StoreSite->getLocation().getAs() &&
+ isa(SI.Dest)) {
+SI.StoreKind = StoreInfo::CallArgument;
}
- if (os.str().empty())
-showBRDefaultDiagnostics(os, R, V, OldRegion);
-
- if (TKind == bugreporter::TrackingKind::Condition)
-os << WillBeUsedForACondition;
-
- // Construct a new PathDiagnosticPiece.
- ProgramPoint P =
