[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

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