[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

[Ding Fei via Phabricator via cfe-commits]

danix800 added a comment.

In D158707#4614100 <https://reviews.llvm.org/D158707#4614100>, @steakhal wrote:

> As a general comment on requiring all extents to be of unsigned APSInts.
> Checkers, like the `MallocChecker`, binds the result of arbitrary expression's values (the argument of malloc, for example) as extents of some regions.
> Usually, that argument is of an `unsigned` static type, thus even if some other expression is there, and implicit cast will turn it into `unsigned`.
> However, in CSA, we don't respect such casts, thus if it was signed, we will bind a signed value as an extent.
>
> Here is an example <https://godbolt.org/z/jv5e8EoaM> demonstrating this:
>
>   int n = conjure();
>   clang_analyzer_dump(n);  // conj
>   clang_analyzer_value(n); // i32
>   
>   char *p = (char*)malloc(n);
>   clang_analyzer_dump(clang_analyzer_getExtent(p));  // conj
>   clang_analyzer_value(clang_analyzer_getExtent(p)); // i32
>
> Consequently, I'm not sure it's possible to turn extents "unsigned", without fixing casts.
> I was sort of expecting this, but I didn't know real world issues materialized by this in the domain of extents.
> Usually, they just happen to work, so I didn't really bother with it.
> This was actually what I intended to deliver with my comment at D158499#4609046 <https://reviews.llvm.org/D158499#4609046>.
>
> By this comment I'm not suggesting that this is a lost cause, or we shouldn't move in this direction. I think something like this would make sense, but I'd need to delve into the change to make sure.

Wow! Really another unexpectation to me!

I think either signedness is ok, the stored data would not be lost. How clients use these values are the actual problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158707