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

[Kristóf Umann via Phabricator via cfe-commits]

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