[PATCH] D136603: [analyzer] Model cast after LValueToRValueBitCasts

[Gabor Marton via Phabricator via cfe-commits]

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