[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
NoQ added a comment. > Functional change here is accidental - by communicating array destructor > situation properly, we're able to fix an old FIXME. Minor temporary insanity. This test was "fixed" because in `mayInlineCall()` for destructors i started to look for the flag that i never set for destructors :/ Repository: rL LLVM https://reviews.llvm.org/D42457 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
This revision was automatically updated to reflect the committed changes. Closed by commit rL324018: [analyzer] Dont communicate evaluation failures through memregion hierarchy. (authored by dergachev, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D42457?vs=132450=132473#toc Repository: rL LLVM https://reviews.llvm.org/D42457 Files: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp cfe/trunk/test/Analysis/new.cpp Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h === --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -567,14 +567,23 @@ const LocationContext *LCtx, ProgramStateRef State); + struct EvalCallOptions { +bool IsConstructorWithImproperlyModeledTargetRegion = false; +bool IsArrayConstructorOrDestructor = false; + +EvalCallOptions() {} + }; + /// Evaluate a call, running pre- and post-call checks and allowing checkers /// to be responsible for handling the evaluation of the call itself. void evalCall(ExplodedNodeSet , ExplodedNode *Pred, const CallEvent ); /// \brief Default implementation of call evaluation. void defaultEvalCall(NodeBuilder , ExplodedNode *Pred, - const CallEvent ); + const CallEvent , + const EvalCallOptions = {}); + private: void evalLoadCommon(ExplodedNodeSet , const Expr *NodeEx, /* Eventually will be a CFGStmt */ @@ -598,9 +607,23 @@ void examineStackFrames(const Decl *D, const LocationContext *LCtx, bool , unsigned ); + enum CallInlinePolicy { +CIP_Allowed, +CIP_DisallowedOnce, +CIP_DisallowedAlways + }; + + /// \brief See if a particular call should be inlined, by only looking + /// at the call event and the current state of analysis. + CallInlinePolicy mayInlineCallKind(const CallEvent , + const ExplodedNode *Pred, + AnalyzerOptions , + const EvalCallOptions ); + /// Checks our policies and decides weither the given call should be inlined. bool shouldInlineCall(const CallEvent , const Decl *D, -const ExplodedNode *Pred); +const ExplodedNode *Pred, +const EvalCallOptions = {}); bool inlineCall(const CallEvent , const Decl *D, NodeBuilder , ExplodedNode *Pred, ProgramStateRef State); @@ -650,11 +673,11 @@ /// For a given constructor, look forward in the current CFG block to /// determine the region into which an object will be constructed by \p CE. - /// Returns either a field or local variable region if the object will be - /// directly constructed in an existing region or a temporary object region - /// if not. + /// When the lookahead fails, a temporary region is returned, and the + /// IsConstructorWithImproperlyModeledTargetRegion flag is set in \p CallOpts. const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE, - ExplodedNode *Pred); + ExplodedNode *Pred, + EvalCallOptions ); /// Store the region returned by operator new() so that the constructor /// that follows it knew what location to initialize. The value should be Index: cfe/trunk/test/Analysis/new.cpp === --- cfe/trunk/test/Analysis/new.cpp +++ cfe/trunk/test/Analysis/new.cpp @@ -311,8 +311,7 @@ void testArrayDestr() { NoReturnDtor *p = new NoReturnDtor[2]; delete[] p; // Calls the base destructor which aborts, checked below - //TODO: clang_analyzer_eval should not be called - clang_analyzer_eval(true); // expected-warning{{TRUE}} + clang_analyzer_eval(true); // no-warning } // Invalidate Region even in case of default destructor Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp === --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -85,28 +85,32 @@ /// Returns a region representing the first element of a (possibly -/// multi-dimensional) array. +/// multi-dimensional) array, for the purposes of element construction or +/// destruction. /// /// On return, \p Ty will be set to the base type of the array. /// /// If the type is not an array type
[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
NoQ updated this revision to Diff 132450. NoQ added a comment. Switched to plain bools and in-class initializers. https://reviews.llvm.org/D42457 Files: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h lib/StaticAnalyzer/Core/ExprEngineCXX.cpp lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp test/Analysis/new.cpp Index: test/Analysis/new.cpp === --- test/Analysis/new.cpp +++ test/Analysis/new.cpp @@ -311,8 +311,7 @@ void testArrayDestr() { NoReturnDtor *p = new NoReturnDtor[2]; delete[] p; // Calls the base destructor which aborts, checked below - //TODO: clang_analyzer_eval should not be called - clang_analyzer_eval(true); // expected-warning{{TRUE}} + clang_analyzer_eval(true); // no-warning } // Invalidate Region even in case of default destructor Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp === --- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -616,15 +616,10 @@ Bldr.generateNode(Call.getProgramPoint(), State, Pred); } -enum CallInlinePolicy { - CIP_Allowed, - CIP_DisallowedOnce, - CIP_DisallowedAlways -}; - -static CallInlinePolicy mayInlineCallKind(const CallEvent , - const ExplodedNode *Pred, - AnalyzerOptions ) { +ExprEngine::CallInlinePolicy +ExprEngine::mayInlineCallKind(const CallEvent , const ExplodedNode *Pred, + AnalyzerOptions , + const ExprEngine::EvalCallOptions ) { const LocationContext *CurLC = Pred->getLocationContext(); const StackFrameContext *CallerSFC = CurLC->getCurrentStackFrame(); switch (Call.getKind()) { @@ -658,18 +653,8 @@ // initializers for array fields in default move/copy constructors. // We still allow construction into ElementRegion targets when they don't // represent array elements. -const MemRegion *Target = Ctor.getCXXThisVal().getAsRegion(); -if (Target && isa(Target)) { - if (ParentExpr) -if (const CXXNewExpr *NewExpr = dyn_cast(ParentExpr)) - if (NewExpr->isArray()) -return CIP_DisallowedOnce; - - if (const TypedValueRegion *TR = dyn_cast( - cast(Target)->getSuperRegion())) -if (TR->getValueType()->isArrayType()) - return CIP_DisallowedOnce; -} +if (CallOpts.IsArrayConstructorOrDestructor) + return CIP_DisallowedOnce; // Inlining constructors requires including initializers in the CFG. const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext(); @@ -688,7 +673,7 @@ // FIXME: This is a hack. We don't handle temporary destructors // right now, so we shouldn't inline their constructors. if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete) - if (!Target || isa(Target)) + if (CallOpts.IsConstructorWithImproperlyModeledTargetRegion) return CIP_DisallowedOnce; break; @@ -702,11 +687,8 @@ assert(ADC->getCFGBuildOptions().AddImplicitDtors && "No CFG destructors"); (void)ADC; -const CXXDestructorCall = cast(Call); - // FIXME: We don't handle constructors or destructors for arrays properly. -const MemRegion *Target = Dtor.getCXXThisVal().getAsRegion(); -if (Target && isa(Target)) +if (CallOpts.IsArrayConstructorOrDestructor) return CIP_DisallowedOnce; break; @@ -847,7 +829,8 @@ } bool ExprEngine::shouldInlineCall(const CallEvent , const Decl *D, - const ExplodedNode *Pred) { + const ExplodedNode *Pred, + const EvalCallOptions ) { if (!D) return false; @@ -894,7 +877,7 @@ // FIXME: this checks both static and dynamic properties of the call, which // means we're redoing a bit of work that could be cached in the function // summary. - CallInlinePolicy CIP = mayInlineCallKind(Call, Pred, Opts); + CallInlinePolicy CIP = mayInlineCallKind(Call, Pred, Opts, CallOpts); if (CIP != CIP_Allowed) { if (CIP == CIP_DisallowedAlways) { assert(!MayInline.hasValue() || MayInline.getValue()); @@ -946,7 +929,8 @@ } void ExprEngine::defaultEvalCall(NodeBuilder , ExplodedNode *Pred, - const CallEvent ) { + const CallEvent , + const EvalCallOptions ) { // Make sure we have the most recent state attached to the call. ProgramStateRef State = Pred->getState(); CallEventRef<> Call = CallTemplate.cloneWithState(State); @@ -969,7 +953,7 @@ } else { RuntimeDefinition RD = Call->getRuntimeDefinition(); const Decl *D = RD.getDecl(); -if (shouldInlineCall(*Call, D, Pred)) { +if (shouldInlineCall(*Call, D, Pred,
[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. However, I do think you should take George's suggestion to have makeZeroElementRegion() have a boolean out parameter rather than a EvalCallOptions out parameter. This avoids unnecessarily coupling of `makeZeroElementRegion()` and `EvalallOptions`. You will need to make the boolean fields not bitfields (since we can't take a bitfield's address in C++). But EvalCallOptions is only on the stack (and we can pass it by const reference) so I don't think the increased size is something we should worry about. https://reviews.llvm.org/D42457 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:572 +bool IsConstructorWithImproperlyModeledTargetRegion : 1; +bool IsArrayConstructorOrDestructor : 1; + }; george.karpenkov wrote: > OK my C++ knowledge is weak here. > What happens if you don't initialize those at the callsite and then read? > Wouldn't be safer to set them both to false in the declaration? Yeah, i guess i'd add a constructor. Unfortunately inline initialization for bitfields is not available until `C++20`. So i hope we'd be able to get rid of this construct before we switch to C++20 =) Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:661 + /// When the lookahead fails, a temporary region is returned, and a flag is + /// set in \p Flags. const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE, george.karpenkov wrote: > Which flag? The //respective// flag. To be exact, the `IntentionallyLongTemporaryFlagNameThatNobodyWouldEverBotherToReadCorrectly` one :) Fxd thx :) https://reviews.llvm.org/D42457 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
NoQ updated this revision to Diff 131321. NoQ marked 8 inline comments as done. NoQ added a comment. Address comments. https://reviews.llvm.org/D42457 Files: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h lib/StaticAnalyzer/Core/ExprEngineCXX.cpp lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp test/Analysis/new.cpp Index: test/Analysis/new.cpp === --- test/Analysis/new.cpp +++ test/Analysis/new.cpp @@ -311,8 +311,7 @@ void testArrayDestr() { NoReturnDtor *p = new NoReturnDtor[2]; delete[] p; // Calls the base destructor which aborts, checked below - //TODO: clang_analyzer_eval should not be called - clang_analyzer_eval(true); // expected-warning{{TRUE}} + clang_analyzer_eval(true); // no-warning } // Invalidate Region even in case of default destructor Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp === --- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -622,9 +622,9 @@ CIP_DisallowedAlways }; -static CallInlinePolicy mayInlineCallKind(const CallEvent , - const ExplodedNode *Pred, - AnalyzerOptions ) { +static CallInlinePolicy +mayInlineCallKind(const CallEvent , const ExplodedNode *Pred, + AnalyzerOptions , ExprEngine::EvalCallOptions CallOpts) { const LocationContext *CurLC = Pred->getLocationContext(); const StackFrameContext *CallerSFC = CurLC->getCurrentStackFrame(); switch (Call.getKind()) { @@ -658,18 +658,8 @@ // initializers for array fields in default move/copy constructors. // We still allow construction into ElementRegion targets when they don't // represent array elements. -const MemRegion *Target = Ctor.getCXXThisVal().getAsRegion(); -if (Target && isa(Target)) { - if (ParentExpr) -if (const CXXNewExpr *NewExpr = dyn_cast(ParentExpr)) - if (NewExpr->isArray()) -return CIP_DisallowedOnce; - - if (const TypedValueRegion *TR = dyn_cast( - cast(Target)->getSuperRegion())) -if (TR->getValueType()->isArrayType()) - return CIP_DisallowedOnce; -} +if (CallOpts.IsArrayConstructorOrDestructor) + return CIP_DisallowedOnce; // Inlining constructors requires including initializers in the CFG. const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext(); @@ -688,7 +678,7 @@ // FIXME: This is a hack. We don't handle temporary destructors // right now, so we shouldn't inline their constructors. if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete) - if (!Target || isa(Target)) + if (CallOpts.IsConstructorWithImproperlyModeledTargetRegion) return CIP_DisallowedOnce; break; @@ -702,11 +692,8 @@ assert(ADC->getCFGBuildOptions().AddImplicitDtors && "No CFG destructors"); (void)ADC; -const CXXDestructorCall = cast(Call); - // FIXME: We don't handle constructors or destructors for arrays properly. -const MemRegion *Target = Dtor.getCXXThisVal().getAsRegion(); -if (Target && isa(Target)) +if (CallOpts.IsArrayConstructorOrDestructor) return CIP_DisallowedOnce; break; @@ -847,7 +834,8 @@ } bool ExprEngine::shouldInlineCall(const CallEvent , const Decl *D, - const ExplodedNode *Pred) { + const ExplodedNode *Pred, + EvalCallOptions CallOpts) { if (!D) return false; @@ -894,7 +882,7 @@ // FIXME: this checks both static and dynamic properties of the call, which // means we're redoing a bit of work that could be cached in the function // summary. - CallInlinePolicy CIP = mayInlineCallKind(Call, Pred, Opts); + CallInlinePolicy CIP = mayInlineCallKind(Call, Pred, Opts, CallOpts); if (CIP != CIP_Allowed) { if (CIP == CIP_DisallowedAlways) { assert(!MayInline.hasValue() || MayInline.getValue()); @@ -946,7 +934,8 @@ } void ExprEngine::defaultEvalCall(NodeBuilder , ExplodedNode *Pred, - const CallEvent ) { + const CallEvent , + EvalCallOptions CallOpts) { // Make sure we have the most recent state attached to the call. ProgramStateRef State = Pred->getState(); CallEventRef<> Call = CallTemplate.cloneWithState(State); @@ -969,7 +958,7 @@ } else { RuntimeDefinition RD = Call->getRuntimeDefinition(); const Decl *D = RD.getDecl(); -if (shouldInlineCall(*Call, D, Pred)) { +if (shouldInlineCall(*Call, D, Pred, CallOpts)) { if (RD.mayHaveOtherDefinitions()) { AnalyzerOptions = getAnalysisManager().options; Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
george.karpenkov added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:570 + struct EvalCallFlags { +bool IsConstructorWithImproperlyModeledTargetRegion : 1; Those are not really flags. Perhaps options? Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:572 +bool IsConstructorWithImproperlyModeledTargetRegion : 1; +bool IsArrayConstructorOrDestructor : 1; + }; OK my C++ knowledge is weak here. What happens if you don't initialize those at the callsite and then read? Wouldn't be safer to set them both to false in the declaration? Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:661 + /// When the lookahead fails, a temporary region is returned, and a flag is + /// set in \p Flags. const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE, Which flag? Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:92 /// /// If the type is not an array type at all, the original value is returned. static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue, Doxygen comment for out-parameter? E.g. it's non-obvious that it only sets the flag to true and does not touch it otherwise. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:94 static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue, - QualType ) { + QualType , bool ) { SValBuilder = State->getStateManager().getSValBuilder(); Perhaps `IsArray` will also do? Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:142 +bool IsArray = false; +LValue = makeZeroElementRegion(State, LValue, Ty, IsArray); +if (IsArray) Or I suppose you could do `makeZeroElementRegion(..., )` to express the intent Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:171 + bool IsArray = false; + FieldVal = makeZeroElementRegion(State, FieldVal, Ty, IsArray); + if (IsArray) Same as previous comment, I suppose you could write into `IsArrayConstructorOrDestructor` directly. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:451 + if (IsArray) +Flags.IsArrayConstructorOrDestructor = true; + Again perhaps write into `Flags.Is...` directly? Repository: rC Clang https://reviews.llvm.org/D42457 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.
NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs. - If we are unable to determine a good target region for a C++ constructor call, we come up with a temporary region and construct into that. - If we suddenly notice that our constructor is part of an C++ array construction (operator new[] or an array-typed local variable), we come up with the element-zero region and construct into that, same for destructors. Both are bad not only because we fail to model stuff correctly, but also because we use region structure to communicate the fact that we failed to `ExprEngine::evalCall`, which would later decide if we should inline the call or not by looking at the region structure. The former is particularly bad because this prevents us from supporting the actual construction into temporaries - even if we produce a correct temporary region, we won't inline the constructor because temporary region is an (incorrect) indication of failure. In order to unblock the progress in this area, this patch adds an auxiliary flag for `evalCall`'s clients to communicate potential problems with the call. Then `evalCall` would decide if it should still inline the call. Apparently, the current decision process is non-trivial in this case: for instance, we may still inline the constructor even if the target region is incorrect aka temporary, when the class's destructor is trivial. In any case, it's good to have all the when-to-inline heuristics in one place. For now `ExprEngine::evalCall` itself doesn't have a flag - only `ExprEngine::defaultEvalCall` does. Because for now it's only relevant to constructor/destructor calls, but checker-side `evalCall` only works for simple call expressions - that's a separate issue. Ideally the flag should go there as well, and `defaultEvalCall` would only be called from `ExprEngine::evalCall`. Functional change here is accidental - by communicating array destructor situation properly, we're able to fix an old FIXME. Repository: rC Clang https://reviews.llvm.org/D42457 Files: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h lib/StaticAnalyzer/Core/ExprEngineCXX.cpp lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp test/Analysis/new.cpp Index: test/Analysis/new.cpp === --- test/Analysis/new.cpp +++ test/Analysis/new.cpp @@ -311,8 +311,7 @@ void testArrayDestr() { NoReturnDtor *p = new NoReturnDtor[2]; delete[] p; // Calls the base destructor which aborts, checked below - //TODO: clang_analyzer_eval should not be called - clang_analyzer_eval(true); // expected-warning{{TRUE}} + clang_analyzer_eval(true); // no-warning } // Invalidate Region even in case of default destructor Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp === --- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -624,7 +624,8 @@ static CallInlinePolicy mayInlineCallKind(const CallEvent , const ExplodedNode *Pred, - AnalyzerOptions ) { + AnalyzerOptions , + ExprEngine::EvalCallFlags Flags) { const LocationContext *CurLC = Pred->getLocationContext(); const StackFrameContext *CallerSFC = CurLC->getCurrentStackFrame(); switch (Call.getKind()) { @@ -658,18 +659,8 @@ // initializers for array fields in default move/copy constructors. // We still allow construction into ElementRegion targets when they don't // represent array elements. -const MemRegion *Target = Ctor.getCXXThisVal().getAsRegion(); -if (Target && isa(Target)) { - if (ParentExpr) -if (const CXXNewExpr *NewExpr = dyn_cast(ParentExpr)) - if (NewExpr->isArray()) -return CIP_DisallowedOnce; - - if (const TypedValueRegion *TR = dyn_cast( - cast(Target)->getSuperRegion())) -if (TR->getValueType()->isArrayType()) - return CIP_DisallowedOnce; -} +if (Flags.IsArrayConstructorOrDestructor) + return CIP_DisallowedOnce; // Inlining constructors requires including initializers in the CFG. const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext(); @@ -688,7 +679,7 @@ // FIXME: This is a hack. We don't handle temporary destructors // right now, so we shouldn't inline their constructors. if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete) - if (!Target || isa(Target)) + if (Flags.IsConstructorWithImproperlyModeledTargetRegion) return CIP_DisallowedOnce; break; @@ -702,11 +693,8 @@ assert(ADC->getCFGBuildOptions().AddImplicitDtors && "No CFG destructors");