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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
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) {}
----------------
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.


================
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);
----------------
As the next obvious step for the next patch, i suggest replacing `evalEQ()` with some sort of `setDynamicSize()` here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1074
     std::tie(StateWholeReg, StateNotWholeReg) =
-        State->assume(svalBuilder.evalEQ(State, Extent, *SizeNL));
+        State->assume(svalBuilder.evalEQ(State, SizeDV, *SizeNL));
 
----------------
And here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1413
         svalBuilder.getArrayIndexType());
-    DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(
-        State, Extent, SizeInBytes.castAs<DefinedOrUnknownSVal>());
-    State = State->assume(extentMatchesSize, true);
+    DefinedOrUnknownSVal DynSizeMatchesSize = svalBuilder.evalEQ(
+        State, DynSize, SizeInBytes.castAs<DefinedOrUnknownSVal>());
----------------
And here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1547
+    DefinedOrUnknownSVal DynSizeMatchesSize =
+        svalBuilder.evalEQ(State, DynSize, *DefinedSize);
 
----------------
And here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:176
   DefinedOrUnknownSVal sizeIsKnown =
-    svalBuilder.evalEQ(state, Extent, ArraySize);
+      svalBuilder.evalEQ(state, DynSize, ArraySize);
   state = state->assume(sizeIsKnown, true);
----------------
And here.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:29-30
+                                    const MemRegion *MR) {
+  if (const DefinedOrUnknownSVal *Size = State->get<DynamicSizeMap>(MR))
+    return *Size;
+
----------------
For now the map is always empty, right? Maybe we should remove the map until we actually add a setter.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:692-693
+  case MemRegion::FunctionCodeRegionKind: {
+    QualType Ty = cast<TypedRegion>(SR)->getDesugaredLocationType(Ctx);
+    return getTypeSize(Ty, Ctx, SVB);
+  }
----------------
This code doesn't make much sense; it'll return a pointer size, which has nothing to do with the size of the region.


================
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);
 
----------------
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 :/


Repository:
  rC Clang

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

https://reviews.llvm.org/D69540