Charusso added a reviewer: vsavchenko. Charusso added a comment. Hey, I am back.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44 +/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR. +ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR, + const CXXNewExpr *NE, + const LocationContext *LCtx, SValBuilder &SVB); ---------------- Charusso wrote: > NoQ wrote: > > This function is probably going to be used exactly once in the whole code. > > There's no need to turn it into a public API. > It is being used 3 times already, so I believe it is a cool API. Let us remove the `setDynamicSize(CXXNewExpr)` API from now on (was not that cool). This function will reside inside `ExprEngine::bindReturnValue()` which is the second phase of evaluating operator new: https://reviews.llvm.org/D40560#961694 ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311 + + QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext()) + : CE->getArg(0)->IgnoreParenImpCasts()->getType(); + ---------------- Charusso wrote: > NoQ wrote: > > How is this better than `getValueType()`? Are you sure you're not getting a > > pointer type instead in the `!TVR` case? I.e., can you test this on a > > non-heap `SymRegion`? > > How is this better than getValueType()? > Consistency. We get the static ~~size~~ extent by getting the desugared type > which most likely just an extra overhead. > > > Are you sure you're not getting a pointer type instead in the !TVR case? > > I.e., can you test this on a non-heap SymRegion? > The issue was the `var_region_simple_ptr` test: we need to use regex for > checking after the `}}` token where `/ 4` is the correct result. I did not > really get why the test pass. Added a conjured `SymbolicRegion` test called `user_defined_new` and an assertion. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1388 - ->castAs<SubRegion>() - ->StripCasts() - ->castAs<SubRegion>(); ---------------- NoQ wrote: > What happened to this `StripCasts()`? I don't see it in the after-code and I > vaguely remember having to be very careful with it. I have reimplemented it by a mistake with the `getSuperRegion(MR)` helper routine. From now I will use `StripCasts()` instead. Nice catch! ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:29 +/// Helper to bypass the top-level ElementRegion of \p MR. +static const MemRegion *getSuperRegion(const MemRegion *MR) { + assert(MR); ---------------- Szelethus wrote: > `getSuperRegionIfElementRegion`? `getSuperRegion(MR)` was just a plain helper routine not restricted to return iff `MR` is an `ElementRegion`. It obtains the immediate region which we can measure in size, but it turns out the same idea as `MemRegion::StripCasts()`, so I have removed it. ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:36-44 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR, SValBuilder &SVB) { + MR = getSuperRegion(MR); + + if (const DefinedOrUnknownSVal *Size = State->get<DynamicSizeMap>(MR)) + return *Size; + ---------------- Szelethus wrote: > Well hold on for a second. Does this mean that we legit resolved > `getDynamicSize` to `getStaticSize`??? That's crazy! I've been looking around > in the code for how does your patch interact with our current dynamic size > modeling, but I guess I never really realized that there is no such a thing. > Odd to not even see a FIXME around. I wanted to upstream both of the patches, but this one became dead: [analyzer] DynamicSize: Remove 'getExtent()' from regions Oct 29 2019, 01:43 [analyzer] DynamicSize: Store the dynamic size Nov 1 2019, 19:57 Sorry for the inconvenience. ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:80-82 + /* FIXME: Make this work. + if (const auto CI = Size.getAs<nonloc::ConcreteInt>()) + assert(CI->getValue().isUnsigned());*/ ---------------- Szelethus wrote: > Make this work as is "make sure that checkers actually obey this > precondition" or does the assert just not work as-is? Could you specify this > comment? This assertion was about the signedness, because I have seen something like `unsigned(42) == signed(42)` did not evaluate to true, and I really wanted to make the size equality to be true in such value-equality cases. I have added a test (signedness_equality) instead, which is working fine now (I hope, because I am not sure what was the original behavior). ================ Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:779-780 bool IsStandardGlobalOpNewFunction = FD->isReplaceableGlobalAllocationFunction(); ---------------- Szelethus wrote: > I wonder what the rationale was here. Why do we explicitly avoid alignment > new? We avoid to argue about non-standard memory allocation/alignment and extent measurement, because we are unsure about the resulting memory block. (May this is not the best way to measure standardity, but I leave it as is.) ================ Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:799-800 + } else { symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(), blockCount); + } ---------------- Szelethus wrote: > Regardless of whether this is heap allocated or not, we can set a dynamic > size here, correct? Like, all the align new expressions are standard, but > they aren't replaceable (as documented in > `isReplaceableGlobalAllocationFunction()`). May the replacability is not the same as being standard operator new, which is a problem, but I leave this area as is, because I have realized the `CXXNewExpr` evaluation is the last phase of evaluating operator new [1], so we do not need to create the dynamic size again. (Also as I have tested out the `symVal` is unknown iff we could not model the operator new so far (being last phase [1]), which means it is non-standard). [1] https://reviews.llvm.org/D40560#961694 ================ Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:698 + DefinedOrUnknownSVal Size = UnknownVal(); + if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr)) + Size = State->getSVal(SizeExpr, LCtx).castAs<DefinedOrUnknownSVal>(); ---------------- Charusso wrote: > NoQ wrote: > > Same. I guess we should add a test for both cases, given that nothing > > failed when we screwed up the extent. > Well, it was equally wrong everywhere, so that it works... I have noticed it > like 5 months ago, but I was lazy to fix. More tests are added. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69726/new/ https://reviews.llvm.org/D69726 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits