Szelethus added a comment.
While this patch spans across a lot of files, it is actually rather
straightforward. I'm stunned to see that we never really had a proper dynamic
size support, especially that this patch has been out there for a good long
time :) Oh well, better learn that now than never.
There sure is some rebasing nightmare waiting for you in `VLASizeChecker`, and
I honestly lack the confidence to accept a patch with so much SVal-smithing
going on. With that said, I do like the overall direction.
================
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);
----------------
`getSuperRegionIfElementRegion`?
================
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;
+
----------------
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.
================
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());*/
----------------
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?
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:779-780
bool IsStandardGlobalOpNewFunction =
FD->isReplaceableGlobalAllocationFunction();
----------------
I wonder what the rationale was here. Why do we explicitly avoid alignment new?
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:799-800
+ } else {
symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(),
blockCount);
+ }
----------------
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()`).
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
