[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the review! I am not sure why but after your review I always see the most appropriate design immediately.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1253-1255
+  MemRegionManager(ASTContext &c, llvm::BumpPtrAllocator &a, SValBuilder &SVB,
+                   SymbolManager &SymMgr)
+      : Ctx(c), A(a), SVB(SVB), SymMgr(SymMgr) {}
----------------
NoQ wrote:
> This looks like a layering violation to me. It's not super important, but i'd rather not have `MemRegion` depend on `SValBuilder`.
> 
> Can we have `getStaticSize()` be a method on `SValBuilder` instead? Or simply a standalone static function in `DynamicSize.cpp`? 'Cause ideally it shouldn't be called directly.
Hm, in my game-dev world every manager knows about every manager, so I felt that it needs to work. I like the idea behind the directness and hiding the implementation, but I believe the `MemRegionManager` should manage its stuff. Also we are lucky, because the `SValBuilder` is available everywhere with a tiny stress on the API.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp:95-99
+    DefinedOrUnknownSVal DynSize = getDynamicSize(state, R);
+
+    DefinedOrUnknownSVal DynSizeMatchesSizeArg =
+        svalBuilder.evalEQ(state, DynSize, Size.castAs<DefinedOrUnknownSVal>());
+    state = state->assume(DynSizeMatchesSizeArg, true);
----------------
NoQ wrote:
> As the next obvious step for the next patch, i suggest replacing `evalEQ()` with some sort of `setDynamicSize()` here.
Okay, good idea, thanks! I want to eliminate `getSizeInElements` as well.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:158
               // symbols to use, only content metadata.
-              return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+              return FTR->getMemRegionManager().getStaticSize(FTR);
 
----------------
NoQ wrote:
> Charusso wrote:
> > That is the breaking test's code, which is super wonky. I cannot understand what is the rational behind this concept.
> Your new code would return a concrete integer here:
> ```lang=c++
>   case MemRegion::FunctionCodeRegionKind: {
>     QualType Ty = cast<TypedRegion>(SR)->getDesugaredLocationType(Ctx);
>     return getTypeSize(Ty, Ctx, SVB);
>   }
> ```
> Previously it was a symbol.
> 
> That said, the original code looks like a super gross hack: they used an extent symbol not because they actually needed an extent, but because they didn't have a better symbol to use :/ I guess you should just keep the extent symbol for now :/
I see, that was really missing, whoops. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69540/new/

https://reviews.llvm.org/D69540