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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+    if (CI->getValue().isUnsigned())
+      Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+
----------------
That one is interesting. Some of the checkers / SValBuilder(?) generate unsigned integers which should not happen, I believe. May we need a FIXME and an assertion about signedness. What do you think?


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:698
+      DefinedOrUnknownSVal Size = UnknownVal();
+      if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+        Size = State->getSVal(SizeExpr, LCtx).castAs<DefinedOrUnknownSVal>();
----------------
NoQ wrote:
> Same. I guess we should add a test for both cases, given that nothing failed when we screwed up the extent.
Well, it was equally wrong everywhere, so that it works... I have noticed it like 5 months ago, but I was lazy to fix.


================
Comment at: clang/test/Analysis/explain-svals.cpp:52
   // Sic! What gets computed is the extent of the element-region.
-  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re{{{{^signed 32-bit integer '4'$}}}}
+  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re{{{{^\(argument 'ext'\) \* 4$}}}}
   delete[] x;
----------------
Yea, that is the fact: The size is the size of the parameter, which is unknown.


================
Comment at: clang/test/Analysis/memory-model.cpp:108
+  free(c);
+}
----------------
Here I wanted to put more, but I am not that cool with other MemRegions. Some wise words about the followups of this test file:
> Some day
- The 11 years old comment in ExprEngine.cpp: https://github.com/llvm/llvm-project/blame/master/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp#L237


================
Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+        tmp2[x] = am; // expected-warning \
+        {{Access out-of-bound array element (buffer overflow)}}
       }
----------------
That is the single regression which I do not get.


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

https://reviews.llvm.org/D69726