[PATCH] D68725: [analyzer] MemoryBlockRegion: Generalize AllocaRegion

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> This patch generalizes the `AllocaRegion` to store metadata about the expression of the allocation of a memory block.

Mmm, but you can easily obtain that expression from the existing `AllocaRegion`. I.e., you can obtain the expression on which the memory was allocated, which is an invocation of `alloca()`. You can always inspect this `CallExpr` to extract the size argument or the callee declaration, there's no need to store them separately.

Generally, there's no need to add more information to `MemRegion` and `SymExpr` objects unless you want to discriminate between different regions/symbols that are currently treated as equal regions/symbols (cf. D21978 <https://reviews.llvm.org/D21978>). If the information is not part of region's/symbol's identity, it shouldn't be their field. You can attach this information to them in a state trait if necessary.

Extent is definitely an example of such information. It's currently implemented via range constraints over `SymbolExtent`, but i'd much rather have extents implemented as a `REGISTER_MAP_WITH_PROGRAMSTATE(Extent, const MemRegion *, SVal)` and scrap the virtual methods.

> This information could be used to apply fix-its to the size expression of the allocation when the buffer would overflow.

Even if `AllocaRegion` didn't store the expression, any path-sensitive checker for which such region is "interesting" would have to implement a bug visitor to display the allocation site. Such visitor automatically knows the location of the `alloca()` expression and can emit the necessary fixits.



================
Comment at: clang/test/Analysis/pr22954.c:628
   clang_analyzer_eval(m29[j].s3[k] == 1); // expected-warning{{TRUE}}
+  // expected-warning at -1 {{Potential leak of memory pointed to by field 's4'}}
   clang_analyzer_eval(l29->s1[m] == 2); // expected-warning{{UNKNOWN}}
----------------
See the other part of the FIXME: `But not on the previous line`! The symbol is not yet dead on line 628; i specifically left this comment around because this test often gets "fixed" incorrectly.


Repository:
  rC Clang

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

https://reviews.llvm.org/D68725