steakhal added a comment.
In D158499#4608847 <https://reviews.llvm.org/D158499#4608847>, @danix800 wrote:
> We have `getDynamicExtentWithOffset` to use, which can handle more general
> dynamic memory based cases than this fix.
>
> I'll abandon this one.
>
> There are issues worth clarifying and fixing:
>
> 1). Debugging APIs like `clang_analyzer_dumpExtent` in `ExprInspection` might
> be expanded
> to use `getDynamicExtentWithOffset` if it's intended to be used on not only
> dynamic allocated
> regions but more general ones, and more testcases are needed to demonstrate
> the usage.
>
> 2). Fix type-inconsistency of several size-related `SVal`s, e.g.
>
> FAM fam;
> clang_analyzer_dump(clang_analyzer_getExtent(&fam));
> clang_analyzer_dump(clang_analyzer_getExtent(fam.data));
> // expected-warning@-2 {{4 S64b}} // U64b is more reasonable when used as
> an extent
> // expected-warning@-2 {{0 U64b}}
>
> `ArrayIndexType` might be misused here.
>
> Simple searching results listed here (might not be complete):
>
> 1. `getDynamicExtentWithOffset` return value
> 2. `getElementExtent` return value
> 3. `ExprEngineCallAndReturn.cpp` when calling `setDynamicExtent` the `Size`
> arg
I've also thought of these two. I think we should indeed fix both.
In D158499#4608974 <https://reviews.llvm.org/D158499#4608974>, @danix800 wrote:
> One of the observable issue with inconsistent size type is
>
> void clang_analyzer_eval(int);
>
> typedef unsigned long long size_t;
> void *malloc(unsigned long size);
> void free(void *);
>
> void symbolic_longlong_and_int0(long long len) {
> char *a = malloc(5);
> (void)a[len + 1]; // no-warning
> // len: [-1,3]
> clang_analyzer_eval(-1 <= len && len <= 3); // expected-warning {{TRUE}}
> clang_analyzer_eval(0 <= len); // expected-warning
> {{UNKNOWN}}
> clang_analyzer_eval(len <= 2); // expected-warning
> {{UNKNOWN}}
> free(a);
> }
>
> which is extracted from
> `clang/test/Analysis/array-bound-v2-constraint-check.c`,
> with `DynamicMemoryModeling` turned on,
> the second warning does not hold anymore: `clang_analyzer_eval(0 <= len);`
> will be reported as `TRUE` which is not expected.
>
> `DynamicMemoryModeling` will record the extent of allocated memory as `5ULL`,
> `ArrayBoundV2` will do `len + 1 < 5ULL` assumption, simplified to `len <
> 4ULL`,
> which casts `len` to unsigned, dropping `-1`, similar to
>
> void clang_analyzer_eval(int);
>
> void test(int len) {
> if (len >= -1 && len <= 4U) { // len is promoted into unsigned, thus can
> never be negative
> clang_analyzer_eval(0 <= len); // TRUE
> }
> }
I suspected that the wrong cast modeling and how we infer what value ranges are
calculated is susceptible to such APSInt signedness issues, but I haven't seen
a case in the wild for extents. Thus, I didn't think of fixing it either. But
yes, we should.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158499/new/
https://reviews.llvm.org/D158499
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
