[PATCH] D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions

[Balázs Benics via Phabricator via cfe-commits]

steakhal planned changes to this revision.
steakhal added a comment.

In D99658#2661452 <https://reviews.llvm.org/D99658#2661452>, @martong wrote:

> Ah, I see you need `nonloc::SymbolVal` in your next patch, and `getDynamicSizeWithOffset` returns an Unknown if the extend is symbolic.
>
> Anyway, I still feel misleading that `clang_analyzer_getExtent` does not handle the offset. Could we change `getDynamicSizeWithOffset` to return with a symbolic offset instead unknown without regression?

I'm gonna investigate. Thank you all for the snappy review!

BTW I really don't like the naming of these. Both 'dynamic' and 'size' are somewhat overloaded. I very much prefer //extent// to any of these.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:258
+  DefinedOrUnknownSVal Size =
+      getDynamicSize(State, MR->getBaseRegion(), C.getSValBuilder());
 
----------------
martong wrote:
> martong wrote:
> > Wait a bit, I think it should consider the offset too. So, `getDynamicSizeWithOffset` should be used IMHO.
> Or we should have a `clang_analyzer_getExtentOfBaseRegion` to be precise.
I was thinking exactly the same!
I would rather choose `getDynamicSizeWithOffset()` instead of creating another one.


================
Comment at: clang/test/Analysis/explain-svals.cpp:56
   clang_analyzer_explain(x); // expected-warning-re{{{{^pointer to element of type 'int' with index 0 of heap segment that starts at symbol of type 'int \*' conjured at statement 'new int \[ext\]'$}}}}
   // Sic! What gets computed is the extent of the element-region.
+  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re{{{{^extent of heap segment that starts at symbol of type 'int \*' conjured at statement 'new int \[ext\]'$}}}}
----------------
vsavchenko wrote:
> I guess this should go now, right?
uh I missed it :D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99658