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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
