From: Kristof Umann Date: Wed, 21 Aug 2019 21:59:22 +0000 (+0000) Subject: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=00d62d777089f5b1d122e9dd1e1c1c3f6bfa3917;p=clang [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field Exactly what it says on the tin! Note that we're talking about interestingness in general, hence this isn't a control-dependency-tracking specific patch. Differential Revision: https://reviews.llvm.org/D65724 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@369589 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index f607494e1e..1d8167ad4f 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -211,8 +211,10 @@ public: /// Visitor that tries to report interesting diagnostics from conditions. class ConditionBRVisitor final : public BugReporterVisitor { // FIXME: constexpr initialization isn't supported by MSVC2013. - static const char *const GenericTrueMessage; - static const char *const GenericFalseMessage; + constexpr static llvm::StringLiteral GenericTrueMessage = + "Assuming the condition is true"; + constexpr static llvm::StringLiteral GenericFalseMessage = + "Assuming the condition is false"; public: void Profile(llvm::FoldingSetNodeID &ID) const override { diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 92e440ae2c..ccd147081d 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -180,21 +180,44 @@ static bool hasVisibleUpdate(const ExplodedNode *LeftNode, SVal LeftVal, RLCV->getStore() == RightNode->getState()->getStore(); } -static Optional -getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) { +static Optional getSValForVar(const Expr *CondVarExpr, + const ExplodedNode *N) { ProgramStateRef State = N->getState(); const LocationContext *LCtx = N->getLocationContext(); + assert(CondVarExpr); + CondVarExpr = CondVarExpr->IgnoreImpCasts(); + // The declaration of the value may rely on a pointer so take its l-value. - if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) { - if (const auto *VD = dyn_cast_or_null(DRE->getDecl())) { - SVal DeclSVal = State->getSVal(State->getLValue(VD, LCtx)); - if (auto DeclCI = DeclSVal.getAs()) - return &DeclCI->getValue(); - } - } + // FIXME: As seen in VisitCommonDeclRefExpr, sometimes DeclRefExpr may + // evaluate to a FieldRegion when it refers to a declaration of a lambda + // capture variable. We most likely need to duplicate that logic here. + if (const auto *DRE = dyn_cast(CondVarExpr)) + if (const auto *VD = dyn_cast(DRE->getDecl())) + return State->getSVal(State->getLValue(VD, LCtx)); + + if (const auto *ME = dyn_cast(CondVarExpr)) + if (const auto *FD = dyn_cast(ME->getMemberDecl())) + if (auto FieldL = State->getSVal(ME, LCtx).getAs()) + return State->getRawSVal(*FieldL, FD->getType()); - return {}; + return None; +} + +static Optional +getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) { + + if (Optional V = getSValForVar(CondVarExpr, N)) + if (auto CI = V->getAs()) + return &CI->getValue(); + return None; +} + +static bool isInterestingExpr(const Expr *E, const ExplodedNode *N, + const BugReport *B) { + if (Optional V = getSValForVar(E, N)) + return B->getInterestingnessKind(*V).hasValue(); + return false; } /// \return name of the macro inside the location \p Loc. @@ -2475,17 +2498,11 @@ PathDiagnosticPieceRef ConditionBRVisitor::VisitConditionVariable( const LocationContext *LCtx = N->getLocationContext(); PathDiagnosticLocation Loc(CondVarExpr, BRC.getSourceManager(), LCtx); + auto event = std::make_shared(Loc, Out.str()); - if (const auto *DR = dyn_cast(CondVarExpr)) { - if (const auto *VD = dyn_cast(DR->getDecl())) { - const ProgramState *state = N->getState().get(); - if (const MemRegion *R = state->getLValue(VD, LCtx).getAsRegion()) { - if (report.isInteresting(R)) - event->setPrunable(false); - } - } - } + if (isInterestingExpr(CondVarExpr, N, &report)) + event->setPrunable(false); return event; } @@ -2515,16 +2532,10 @@ PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest( PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx); auto event = std::make_shared(Loc, Out.str()); - const ProgramState *state = N->getState().get(); - if (const MemRegion *R = state->getLValue(VD, LCtx).getAsRegion()) { - if (report.isInteresting(R)) - event->setPrunable(false); - else { - SVal V = state->getSVal(R); - if (report.isInteresting(V)) - event->setPrunable(false); - } - } + + if (isInterestingExpr(DRE, N, &report)) + event->setPrunable(false); + return std::move(event); } @@ -2555,7 +2566,10 @@ PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest( if (!IsAssuming) return std::make_shared(Loc, Out.str()); - return std::make_shared(Loc, Out.str()); + auto event = std::make_shared(Loc, Out.str()); + if (isInterestingExpr(ME, N, &report)) + event->setPrunable(false); + return event; } bool ConditionBRVisitor::printValue(const Expr *CondVarExpr, raw_ostream &Out, @@ -2595,10 +2609,8 @@ bool ConditionBRVisitor::printValue(const Expr *CondVarExpr, raw_ostream &Out, return true; } -const char *const ConditionBRVisitor::GenericTrueMessage = - "Assuming the condition is true"; -const char *const ConditionBRVisitor::GenericFalseMessage = - "Assuming the condition is false"; +constexpr llvm::StringLiteral ConditionBRVisitor::GenericTrueMessage; +constexpr llvm::StringLiteral ConditionBRVisitor::GenericFalseMessage; bool ConditionBRVisitor::isPieceMessageGeneric( const PathDiagnosticPiece *Piece) { diff --git a/test/Analysis/track-control-dependency-conditions.cpp b/test/Analysis/track-control-dependency-conditions.cpp index 5f089b213d..02c53418a2 100644 --- a/test/Analysis/track-control-dependency-conditions.cpp +++ b/test/Analysis/track-control-dependency-conditions.cpp @@ -407,6 +407,91 @@ void f() { } } // end of namespace condition_written_in_nested_stackframe_before_assignment +namespace condition_lambda_capture_by_reference_last_write { +int getInt(); + +[[noreturn]] void halt(); + +void f(int flag) { + int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}} + + auto lambda = [&flag]() { + flag = getInt(); // tracking-note-re{{{{^}}Value assigned to 'flag', which participates in a condition later{{$}}}} + }; + + lambda(); // tracking-note-re{{{{^}}Calling 'operator()'{{$}}}} + // tracking-note-re@-1{{{{^}}Returning from 'operator()'{{$}}}} + + if (flag) // expected-note-re{{{{^}}Assuming 'flag' is not equal to 0{{$}}}} + // expected-note-re@-1{{{{^}}Taking true branch{{$}}}} + // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}} + *x = 5; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} +} +} // end of namespace condition_lambda_capture_by_reference_last_write + +namespace condition_lambda_capture_by_value_assumption { +int getInt(); + +[[noreturn]] void halt(); + +void bar(int &flag) { + flag = getInt(); // tracking-note-re{{{{^}}Value assigned to 'flag', which participates in a condition later{{$}}}} +} + +void f(int flag) { + int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}} + + auto lambda = [flag]() { + if (!flag) // tracking-note-re{{{{^}}Assuming 'flag' is not equal to 0{{$}}}} + // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}} + halt(); + }; + + bar(flag); // tracking-note-re{{{{^}}Calling 'bar'{{$}}}} + // tracking-note-re@-1{{{{^}}Returning from 'bar'{{$}}}} + lambda(); // tracking-note-re{{{{^}}Calling 'operator()'{{$}}}} + // tracking-note-re@-1{{{{^}}Returning from 'operator()'{{$}}}} + + if (flag) // expected-note-re{{{{^}}Assuming 'flag' is not equal to 0{{$}}}} + // expected-note-re@-1{{{{^}}Taking true branch{{$}}}} + // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}} + *x = 5; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} +} +} // end of namespace condition_lambda_capture_by_value_assumption + +namespace condition_lambda_capture_by_reference_assumption { +int getInt(); + +[[noreturn]] void halt(); + +void bar(int &flag) { + flag = getInt(); // tracking-note-re{{{{^}}Value assigned to 'flag', which participates in a condition later{{$}}}} +} + +void f(int flag) { + int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}} + + auto lambda = [&flag]() { + if (!flag) // tracking-note-re{{{{^}}Assuming 'flag' is not equal to 0{{$}}}} + // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}} + halt(); + }; + + bar(flag); // tracking-note-re{{{{^}}Calling 'bar'{{$}}}} + // tracking-note-re@-1{{{{^}}Returning from 'bar'{{$}}}} + lambda(); // tracking-note-re{{{{^}}Calling 'operator()'{{$}}}} + // tracking-note-re@-1{{{{^}}Returning from 'operator()'{{$}}}} + + if (flag) // expected-note-re{{{{^}}'flag' is not equal to 0{{$}}}} + // expected-note-re@-1{{{{^}}Taking true branch{{$}}}} + // debug-note-re@-2{{{{^}}Tracking condition 'flag'}} + *x = 5; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} +} +} // end of namespace condition_lambda_capture_by_reference_assumption + namespace collapse_point_not_in_condition { [[noreturn]] void halt(); @@ -471,6 +556,35 @@ void f6(int x) { } // end of namespace dont_crash_on_nonlogical_binary_operator +namespace collapse_point_not_in_condition_as_field { + +[[noreturn]] void halt(); +struct IntWrapper { + int b; + IntWrapper(); + + void check() { + if (!b) // tracking-note-re{{{{^}}Assuming field 'b' is not equal to 0{{$}}}} + // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}} + halt(); + return; + } +}; + +void f(IntWrapper i) { + int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}} + + i.check(); // tracking-note-re{{{{^}}Calling 'IntWrapper::check'{{$}}}} + // tracking-note-re@-1{{{{^}}Returning from 'IntWrapper::check'{{$}}}} + if (i.b) // expected-note-re{{{{^}}Field 'b' is not equal to 0{{$}}}} + // expected-note-re@-1{{{{^}}Taking true branch{{$}}}} + // debug-note-re@-2{{{{^}}Tracking condition 'i.b'{{$}}}} + *x = 5; // expected-warning{{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} +} + +} // end of namespace collapse_point_not_in_condition_as_field + namespace dont_track_assertlike_conditions { extern void __assert_fail(__const char *__assertion, __const char *__file,