martong added a comment.
In D136603#3888065 <https://reviews.llvm.org/D136603#3888065>, @steakhal wrote:
> Previously, even in the `RegionStoreManager::getBinding()` even if `T` was
> non-null, we replaced it with `TVR->getValueType()` in case the `MR` was
> `TypedValueRegion`.
Yeah, that means, we actually evaded a cast, am I right?
> IMO we shouldn't overwrite the type unless we actually need to auto-detect
> the binding type.
I agree.
> This was particularly wrong when we reinterpret-cast some typed memory region
> (such as a stack-local variable's address) to something else while operating
> on the store.
You mean like when we loaded a value of reinterprec-cast<T1>(t2) with
`evalLoad`?
> I haven't done any measurements yet, but I'm still curious about what you
> think about this.
I think this is a good approach and the change is at the right place. But, any
change that relates to casts are very fragile unfortunately. So, I agree, it
would be great to see measurements and that we don't have new assertion
failures.
================
Comment at: clang/test/Analysis/svalbuilder-float-cast.c:18-19
*p += 1;
// This should NOT be (float)$x + 1. Symbol $x was never casted to float.
- // FIXME: Ideally, this should be $x + 1.
- clang_analyzer_express(*p); // expected-warning{{Not a symbol}}
+ clang_analyzer_express(*p); // expected-warning{{$x + 1}}
}
----------------
I think it would make sense to have another RUN line with
`support-symbolic-integer-casts`. In that case I guess we should see
`(int)(float)x` (?).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136603/new/
https://reviews.llvm.org/D136603
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
