[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: NoQ, steakhal, xazax.hun.
ASDenysPetrov added a project: clang.
Herald added subscribers: nullptr.cpp, martong, Charusso, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.
ASDenysPetrov requested review of this revision.
Herald added a subscriber: cfe-commits.

Move logic from CastRetrievedVal to evalCast and replace CastRetrievedVal with 
evalCast. Also we need to move guts from SimpleSValBuilder::dispatchCast inside 
evalCast. Now evalCast operates in two modes:

- without passing last parameter it works like previous dispatchCast
- with passing last parameter it stays as is.

This patch ideally shall not change any behavior. (Should we mark it as [NFC]?)

This is an intermediate revision for D89055 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96090

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp

Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -394,37 +394,6 @@
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
- ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-return V;
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) {
-  QualType sr = SR->getSymbol()->getType();
-  if (!hasSameUnqualifiedPointeeType(sr, castTy))
-  return loc::MemRegionVal(castRegion(SR, castTy));
-}
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -66,9 +66,7 @@
 //===--===//
 
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs() || Val.getAs());
-  return Val.getAs() ? evalCastFromLoc(Val.castAs(), CastTy)
-   : evalCastFromNonLoc(Val.castAs(), CastTy);
+  return evalCast(Val, CastTy);
 }
 
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -541,20 +541,29 @@
 // `evalCastKind` and `evalCastSubKind` are helpers
 //===--===//
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` a bit differently.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,
+   QualType OriginalTy /*= QualType{}*/) {
+  if (CastTy.isNull())
 return V;
 
-  // FIXME: Move this check to the most appropriate evalCastKind/evalCastSubKind
-  // function.
-  // For const casts, casts to void, just propagate the value.
-  if (!CastTy->isVariableArrayType() && !OriginalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(CastTy),
-Conte

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Please, consider removing D95799  from the 
parent revisions if that is not a hard requirement of this patch.
It would be awesome to land your patches on dealing with cast problems in the 
close future.
Dealing with floating-point values is a plus, but maybe not a necessity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D96090#2544477 , @steakhal wrote:

> Please, consider removing D95799  from the 
> parent revisions if that is not a hard requirement of this patch.

Actually D95799  is one of the steps to clean 
up `CastRetrievedVal` initially. It significantly reduce the next effort of 
replacing with `evalCast`. But if float cast fix blocks the patch stack, I can 
try to find a way to bypass it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 322422.
ASDenysPetrov added a comment.

Updated. Unlinked parent revision D95799  from 
the stack. Made corresponding changes in this patch.


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

https://reviews.llvm.org/D96090

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp

Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -394,48 +394,6 @@
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
- ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-return V;
-
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-SymbolRef Sym = V.getAsSymbol();
-if (Sym && !Sym->getType()->isFloatingType())
-  return UnknownVal();
-  }
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) {
-  QualType sr = SR->getSymbol()->getType();
-  if (!hasSameUnqualifiedPointeeType(sr, castTy))
-  return loc::MemRegionVal(castRegion(SR, castTy));
-}
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -66,9 +66,7 @@
 //===--===//
 
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs() || Val.getAs());
-  return Val.getAs() ? evalCastFromLoc(Val.castAs(), CastTy)
-   : evalCastFromNonLoc(Val.castAs(), CastTy);
+  return evalCast(Val, CastTy);
 }
 
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -536,20 +536,29 @@
 // `evalCastKind` and `evalCastSubKind` are helpers
 //===--===//
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` a bit differently.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,
+   QualType OriginalTy /*= QualType{}*/) {
+  if (CastTy.isNull())
 return V;
 
-  // FIXME: Move this check to the most appropriate evalCastKind/evalCastSubKind
-  // function.
-  // For const casts, casts to void, just propagate the value.
-  if (!CastTy->isVariableArrayType() && !OriginalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(CastTy),
-Context.getPointerType(OriginalTy)))
+  CastTy = Context.getCanonicalType(CastTy);
+
+  if (!OriginalTy.isNull()) {
+OriginalTy = Context.getCanonicalTyp

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96090#2551664 , @ASDenysPetrov 
wrote:

> Updated. Unlinked parent revision D95799  
> from the stack. Made corresponding changes in this patch.
> @steakhal I did it. The changes should be safer now.

In the upcoming days, I'm gonna schedule this on our CI. We will see the 
results.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> In the upcoming days, I'm gonna schedule this on our CI. We will see the 
> results.

Thank you. That would be great!


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Mikhail Ramalho via Phabricator via cfe-commits]

mikhail.ramalho added a comment.

I'm glad to see these patches, the SMT API will benefit greatly from them!


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

@ASDenysPetrov Could you please rebase this?
`git apply` does not seem to apply this patch on top of `llvm/main`.
If it depends on any parent revisions please document them as well.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

This patch preserves all previous reports as expected.
You can check it by yourself at 
https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=D96090&items-per-page=50.

However, I still have some minor concerns inline.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:101-103
   // FIXME: Make these protected again once RegionStoreManager correctly
   // handles loads from different bound value types.
   virtual SVal dispatchCast(SVal val, QualType castTy) = 0;

I'm not sure if this FIXME is still applicable.

I'm also confused about having two functions doing effectively the same thing.
`SimpleSValBuilder::dispatchCast` is a virtual function, which just invokes a 
non-virtual function `SValBuilder::evalCast`.

Why should it be virtual in the first place?



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:727-728
+  // pointers as well.
+  // FIXME: We really need a single good function to perform casts for us
+  // correctly every time we need it.
+  if (CastTy->isPointerType() && !CastTy->isVoidPointerType()) {

You are resolving exactly this FIXME in this patch if I'm correct.
Shouldn't you update this comment?



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:868-870
+  if (OriginalTy.isNull())
+// Pass to MemRegion function.
+return evalCastSubKind(loc::MemRegionVal(R), CastTy, OriginalTy);

Eh, I don't like comments preceding a single-statement block.
It might be a personal preference though.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D96090#2568431 , @steakhal wrote:

> This patch preserves all previous reports as expected.

That's great results!

> Could you please rebase this?

I'll update and rebase this patch soon.

> If it depends on any parent revisions please document them as well.

It does. You can see this in the stack. Do I need to mention this somewhere 
else?




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:101-103
   // FIXME: Make these protected again once RegionStoreManager correctly
   // handles loads from different bound value types.
   virtual SVal dispatchCast(SVal val, QualType castTy) = 0;

steakhal wrote:
> I'm not sure if this FIXME is still applicable.
> 
> I'm also confused about having two functions doing effectively the same thing.
> `SimpleSValBuilder::dispatchCast` is a virtual function, which just invokes a 
> non-virtual function `SValBuilder::evalCast`.
> 
> Why should it be virtual in the first place?
I want to make the patch smaller avoiding things that could be changed 
individually. We can remove `dispatchCast` in the next post-cleanup patch.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:727-728
+  // pointers as well.
+  // FIXME: We really need a single good function to perform casts for us
+  // correctly every time we need it.
+  if (CastTy->isPointerType() && !CastTy->isVoidPointerType()) {

steakhal wrote:
> You are resolving exactly this FIXME in this patch if I'm correct.
> Shouldn't you update this comment?
I am not actually sure that the function I've made is good enough and is the 
single one. =)



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:868-870
+  if (OriginalTy.isNull())
+// Pass to MemRegion function.
+return evalCastSubKind(loc::MemRegionVal(R), CastTy, OriginalTy);

steakhal wrote:
> Eh, I don't like comments preceding a single-statement block.
> It might be a personal preference though.
I am trying to highlight the redirection to another cast function. If you think 
it's redundant, I'll remove it.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96090#2572069 , @ASDenysPetrov 
wrote:

> In D96090#2568431 , @steakhal wrote:
>
>> Could you please rebase this?
>
> I'll update and rebase this patch soon.
>
>> If it depends on any parent revisions please document them as well.
>
> It does. You can see this in the stack. Do I need to mention this somewhere 
> else?

No, I probably missed that. My bad.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:101-103
   // FIXME: Make these protected again once RegionStoreManager correctly
   // handles loads from different bound value types.
   virtual SVal dispatchCast(SVal val, QualType castTy) = 0;

ASDenysPetrov wrote:
> steakhal wrote:
> > I'm not sure if this FIXME is still applicable.
> > 
> > I'm also confused about having two functions doing effectively the same 
> > thing.
> > `SimpleSValBuilder::dispatchCast` is a virtual function, which just invokes 
> > a non-virtual function `SValBuilder::evalCast`.
> > 
> > Why should it be virtual in the first place?
> I want to make the patch smaller avoiding things that could be changed 
> individually. We can remove `dispatchCast` in the next post-cleanup patch.
Seems reasonable to me.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 324964.
ASDenysPetrov edited the summary of this revision.
ASDenysPetrov added a comment.

Rebased revision on top of the main branch. Minor improvements.


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

https://reviews.llvm.org/D96090

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp

Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -394,48 +394,6 @@
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
- ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-return V;
-
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-SymbolRef Sym = V.getAsSymbol();
-if (Sym && !Sym->getType()->isFloatingType())
-  return UnknownVal();
-  }
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) {
-  QualType sr = SR->getSymbol()->getType();
-  if (!hasSameUnqualifiedPointeeType(sr, castTy))
-  return loc::MemRegionVal(castRegion(SR, castTy));
-}
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -65,12 +65,12 @@
 // Transfer function for Casts.
 //===--===//
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs() || Val.getAs());
-  return Val.getAs() ? evalCastFromLoc(Val.castAs(), CastTy)
-   : evalCastFromNonLoc(Val.castAs(), CastTy);
+  return evalCast(Val, CastTy);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
   bool isLocType = Loc::isLocType(castTy);
   if (val.getAs())
@@ -127,6 +127,7 @@
 return makeIntVal(i);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromLoc(Loc val, QualType castTy) {
 
   // Casts from pointers -> pointers, just return the lval.
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -536,22 +536,33 @@
 // `evalCastKind` and `evalCastSubKind` are helpers
 //===--===//
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` less accurately.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,
+   QualType OriginalTy /*= QualType{}*/) {
+  if (CastTy.isNull())
 return V;
 
-  // FIXME: Mov

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal wrote:

> This patch preserves all previous reports as expected.

If this patch is technically full-baked and does not bring any harm, can we 
approve this it as well to move forward?


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96090#2574526 , @ASDenysPetrov 
wrote:

> @steakhal wrote:
>
>> This patch preserves all previous reports as expected.

Even with or without Z3 refutation.

> If this patch is technically full-baked and does not bring any harm, can we 
> approve it as well to move forward?

I would like @NoQ or someone else to also review this.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> I would like @NoQ or someone else to also review this.

Oh, I would be very pleased if our colleagues pay attention to my recent 
patches. But seems you are the only who helps me with this stuff 🙂


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:539
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` less accurately.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,

This is one of the most important APIs of `SValBuilder`, alongside 
`evalBinOp()`. The newly added behavior is definitely obscure and needs 
explaining. So i think you should move this comment into the header, turn it 
into a doxygen comment, and elaborate what "less accurately" actually means 
here.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

So, like, I think it's a start. You introduced a single source of truth for 
casting `SVal`s and it's good. But i'm worried about our API surface.




Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:539
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` less accurately.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,

NoQ wrote:
> This is one of the most important APIs of `SValBuilder`, alongside 
> `evalBinOp()`. The newly added behavior is definitely obscure and needs 
> explaining. So i think you should move this comment into the header, turn it 
> into a doxygen comment, and elaborate what "less accurately" actually means 
> here.
Like, are you sure that the new behavior makes sense at all for call sites that 
aren't ex-`CastRetrievedVal`? When, and why, would a regular user use this 
functionality?

Maybe instead of obfuscating this functionality as a null pointer we should add 
an explicit flag and give it an understandable name? Even if it's `bool 
CastRetrievedValBackdoorHackDoNotUse = false` it's still much better than "umm 
we kind of expect it to work if you pass null here but we don't really know 
how".



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:564
   switch (V.getBaseKind()) {
+  default:
+llvm_unreachable("Unknown SVal kind");

No, don't add `default:`. All possible cases are currently handled. It breaks 
compiler warning about not all cases handled in the switch when new enum cases 
are added. Compile-time warnings are better than run-time warnings.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 328105.
ASDenysPetrov added a comment.

Rebased on main.


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

https://reviews.llvm.org/D96090

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp

Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -394,48 +394,6 @@
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
- ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-return V;
-
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-SymbolRef Sym = V.getAsSymbol();
-if (Sym && !Sym->getType()->isFloatingType())
-  return UnknownVal();
-  }
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) {
-  QualType sr = SR->getSymbol()->getType();
-  if (!hasSameUnqualifiedPointeeType(sr, castTy))
-  return loc::MemRegionVal(castRegion(SR, castTy));
-}
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -65,12 +65,12 @@
 // Transfer function for Casts.
 //===--===//
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs() || Val.getAs());
-  return Val.getAs() ? evalCastFromLoc(Val.castAs(), CastTy)
-   : evalCastFromNonLoc(Val.castAs(), CastTy);
+  return evalCast(Val, CastTy);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
   bool isLocType = Loc::isLocType(castTy);
   if (val.getAs())
@@ -127,6 +127,7 @@
 return makeIntVal(i);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromLoc(Loc val, QualType castTy) {
 
   // Casts from pointers -> pointers, just return the lval.
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -536,22 +536,33 @@
 // `evalCastKind` and `evalCastSubKind` are helpers
 //===--===//
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` less accurately.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,
+   QualType OriginalTy /*= QualType{}*/) {
+  if (CastTy.isNull())
 return V;
 
-  // FIXME: Move this check to the most appropriate evalCastKind/evalCastSubKind
-  // function.
-  // For const 

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ
Thanks, I could finally draw your attention :)

> When, and why, would a regular user use this functionality?

IMO there is no reason to use evalCast without an **original** type. But I'm 
not sure that you can get an **original** type in every case.

About `CastRetrievedValBackdoorHackDoNotUse` I need a strong advise or some 
discussion. Look.
Initially `evalCast` deduces a new `SVal` using **original `QualType`** and 
**cast `QualType`** in case when we've got a **state** and **expression** and 
are able to get the **original** type. E.g.:

  ...
  
  SVal OldV = Context.getSVal(Expression);
  QualType OriginalTy = Expr->getType();
  SVal NewV = SVB.evalCast(OldV, CastTy, Expr->getType());
  ...

But `dispatchCast`, `evalCastFromNonLoc`, `evalCastFromLoc` operate using 
**cast `QualType`** only without knowing the **original** one.

  SVal OldV = Context.getSVal(Expression);
  SVal NewV = SVB.evalCastFromLoc(OldV.castAs, CastTy);

Actually many cases don't need to know an exact **original** type. Some cases 
can extract the //type// from `SVal` (`SymbolVal`, `ConcreteInt`) Other cases 
need it from outside (`MemRegionVal`, `LocAsInteger`).
`dispatchCast`, `evalCastFromNonLoc`, `evalCastFromLoc` have a lot of similar 
code which is already in `evalCast`. They also calls inside `evalCast` 
function. I decided to move their functionality into a new splitted version of 
`evalCast` to decrease complexity of comprehension and try to substitute them 
in the future with the single `evalCast`. But substitusion needs to deal 
something with absent **original** type parameter. Then I decided to add 
support for`evalCast` to work in both modes. Practically, new `evaCast` has 
additional checks and casts when the **original** type is not null.

Now `evalCastFromNonLoc` and `evalCastFromLoc` are used in `SimpleSValBuilder` 
functions only. I think it would be better to add an explaination to the doc 
like `OriginalTy.isNull() is a CastRetrievedVal backdoor hack. Do not use it.` 
instead of new flags. I'll make a better description, but at first I'd try to 
investigate how to pass an **original** type to `evalCastFromNonLoc` and 
`evalCastFromLoc` in `SimpleSValBuilder` functions. Or maybe some other 
solution.

In sum I'll try to handle this and provide a better solution.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96090#2607387 , @ASDenysPetrov 
wrote:

> Actually many cases don't need to know an exact **original** type. Some cases 
> can extract the //type// from `SVal` (`SymbolVal`, `ConcreteInt`) Other cases 
> need it from outside (`MemRegionVal`, `LocAsInteger`).
> `dispatchCast`, `evalCastFromNonLoc`, `evalCastFromLoc` have a lot of similar 
> code which is already in `evalCast`. They also calls inside `evalCast` 
> function. I decided to move their functionality into a new splitted version 
> of `evalCast` to decrease complexity of comprehension and try to substitute 
> them in the future with the single `evalCast`. But substitusion needs to deal 
> something with absent **original** type parameter. Then I decided to add 
> support for`evalCast` to work in both modes. Practically, new `evaCast` has 
> additional checks and casts when the **original** type is not null.

Seems reasonable to me.

> Now `evalCastFromNonLoc` and `evalCastFromLoc` are used in 
> `SimpleSValBuilder` functions only. I think it would be better to add an 
> explaination to the doc like `OriginalTy.isNull() is a CastRetrievedVal 
> backdoor hack. Do not use it.` instead of new flags. I'll make a better 
> description, but at first I'd try to investigate how to pass an **original** 
> type to `evalCastFromNonLoc` and `evalCastFromLoc` in `SimpleSValBuilder` 
> functions. Or maybe some other solution.

What about not defaulting that parameter? That would better represent our 
expectation that it **requires** an original-type.
You could still pass a default constructed QualType at each callsite.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal thanks for the approval.




Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:564-565
   }
-
-  llvm_unreachable("Unknown SVal kind");
 }

steakhal wrote:
> You probably don't want to remove this. The same applies to similar ones.
Yep. I'd keep it here, beacuse of compiler warnings of no-return function.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks amazing, thanks a lot.

I have one final question: can we already remove `dispatchCast()`&Co.? I don't 
see any other call sites anymore. Looks like it becomes dead code after your 
patch.

Which is fairly shocking and mind blowing that we've organically developed two 
independent implementations of casting, one for RegionStore and one for 
everything else. I'm super happy that we're cleaning this up.




Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:564-565
   }
-
-  llvm_unreachable("Unknown SVal kind");
 }

ASDenysPetrov wrote:
> steakhal wrote:
> > You probably don't want to remove this. The same applies to similar ones.
> Yep. I'd keep it here, beacuse of compiler warnings of no-return function.
> compiler warnings of no-return function.

Aha ok, in this case it's much better to keep `llvm_unreachable` outside the 
switch, like here and unlike the one in `evalCastKind` functions. This way 
we're getting compile-time warnings for unhandled enum cases.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Which is fairly shocking and mind blowing that we've organically developed 
> two independent implementations of casting, one for RegionStore and one for 
> everything else.

Wait, no, nvm, please disregard this. It wasn't like this forever, i just 
happened to catch code in an intermediate state after D90157 
. Either way, it's definitely getting much 
better, and either way, i'm curious if `dispatchCast` can now be eliminated.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ 
Many thanks for your evaluation!

> Wait, no, nvm, please disregard this. It wasn't like this forever, i just 
> happened to catch code in an intermediate state after D90157 
> . Either way, it's definitely getting much 
> better, and either way, i'm curious if `dispatchCast` can now be eliminated.

Already approved a month ago D97277  :-) 
That's the next step.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

> Already approved a month ago D97277  :-) 
> That's the next step.

In addition to the above:




Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:68
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {

@NoQ 
I've mentioned the need in elimination.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7736b08c2872: [analyzer] Replace 
StoreManager::CastRetrievedVal with SValBuilder::evalCast (authored by 
ASDenysPetrov).

Changed prior to commit:
  https://reviews.llvm.org/D96090?vs=329982&id=337153#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96090

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp

Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -394,48 +394,6 @@
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
- ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-return V;
-
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-SymbolRef Sym = V.getAsSymbol();
-if (Sym && !Sym->getType()->isFloatingType())
-  return UnknownVal();
-  }
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) {
-  QualType sr = SR->getSymbol()->getType();
-  if (!hasSameUnqualifiedPointeeType(sr, castTy))
-  return loc::MemRegionVal(castRegion(SR, castTy));
-}
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -65,12 +65,12 @@
 // Transfer function for Casts.
 //===--===//
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs() || Val.getAs());
-  return Val.getAs() ? evalCastFromLoc(Val.castAs(), CastTy)
-   : evalCastFromNonLoc(Val.castAs(), CastTy);
+  return evalCast(Val, CastTy, QualType{});
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
   bool isLocType = Loc::isLocType(castTy);
   if (val.getAs())
@@ -127,6 +127,7 @@
 return makeIntVal(i);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromLoc(Loc val, QualType castTy) {
 
   // Casts from pointers -> pointers, just return the lval.
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -544,20 +544,39 @@
 // `evalCastKind` and `evalCastSubKind` are helpers
 //===--===//
 
+/// Cast a given SVal to another SVal using given QualType's.
+/// \param V -- SVal that should be casted.
+/// \param CastTy -- QualType that V should be casted according to.
+/// \param OriginalTy -- QualType which is associated to V. It provides
+/// additional information about what type the cast performs from.
+/// \returns the most appropriate casted SVal.
+/// Note: Ma

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 329699.
ASDenysPetrov edited the summary of this revision.

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

https://reviews.llvm.org/D96090

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp

Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -394,48 +394,6 @@
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
- ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-return V;
-
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-SymbolRef Sym = V.getAsSymbol();
-if (Sym && !Sym->getType()->isFloatingType())
-  return UnknownVal();
-  }
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) {
-  QualType sr = SR->getSymbol()->getType();
-  if (!hasSameUnqualifiedPointeeType(sr, castTy))
-  return loc::MemRegionVal(castRegion(SR, castTy));
-}
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -65,12 +65,12 @@
 // Transfer function for Casts.
 //===--===//
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs() || Val.getAs());
-  return Val.getAs() ? evalCastFromLoc(Val.castAs(), CastTy)
-   : evalCastFromNonLoc(Val.castAs(), CastTy);
+  return evalCast(Val, CastTy);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
   bool isLocType = Loc::isLocType(castTy);
   if (val.getAs())
@@ -127,6 +127,7 @@
 return makeIntVal(i);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromLoc(Loc val, QualType castTy) {
 
   // Casts from pointers -> pointers, just return the lval.
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -536,20 +536,40 @@
 // `evalCastKind` and `evalCastSubKind` are helpers
 //===--===//
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
-  CastTy = Context.getCanonicalType(CastTy);
-  OriginalTy = Context.getCanonicalType(OriginalTy);
-  if (CastTy == OriginalTy)
+/// Cast a given SVal to another SVal using given QualType's.
+/// \param V -- SVal that should be casted.
+/// \param CastTy -- QualType that V should be casted according to.
+/// \param OriginalTy -- QualType which is associated to V. It provides
+/// additional information about what type the cast performs from. This is a
+/// default param. \re

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D96090#261 , @steakhal wrote:

> You could still pass a default constructed QualType at each callsite.

We can try. At least it will be like this is a hack for the particular cases 
then, that will emphesys to pay attention to. Well, I'll present this vertion 
tomorrow.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Actually many cases don't need to know an exact original type.

In all cases we're supposed to have an original type, whether we need it or 
not. Simply because we're simulating a typed language. If we don't have it it's 
a bug.

> Some cases can extract the type from `SVal`

Which is generally impossible because integral casts aren't modeled correctly. 
Which, again, is a bug. I don't know whether we'll have a need to pass the type 
separately when all other parts of the static analyzer become type-correct; I 
suspect that we won't need it. But one way or another, my point is, one or more 
of the following is true: "we have the ability to provide the type in all 
cases", "we don't need to provide the type in any of the cases". None of these 
ideal situations leave room for an API that receives the type //optionally//. 
For this reason I suggest to either always have the caller supply the original 
type (even if means extra work on the caller side) or never have it supply the 
original type.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 329982.
ASDenysPetrov added a comment.

Updated.

In D96090#2618410 , @NoQ wrote:

> In all cases we're supposed to have an **original** type, whether we need it 
> or not. Simply because we're simulating a typed language. If we don't have it 
> it's a bug.

I agree. That's how `evalCastFromNonLoc` and `evalCastFromLoc` work now. 
Finally I want to replace them along with passing a correct **original** type. 
This is just one another step closer to this direction. I don't want patches be 
too comlicated.

>> Some cases can extract the type from `SVal`
>
> Which is generally impossible because integral casts aren't modeled correctly.

Maybe I'm missing smth, But how about this:

  QualType T = V.castAs().getSymbol()->getType();
  QualType T = 
cast(V.castAs().getAsRegion()).getSymbol()->getType();
  // Belowees are not exact types but well narrowed information which can be 
sufficient.
  V.castAs() <-> OriginalTy->isIntegralOrEnumerationType() 
  V.castAs() | V.castAs() | 
V.castAs() <-> Loc::isLocType(CastTy)
  V.castAs() <-> OriginalTy->isMemberPointerType() 
 (I guess)
  ...

> None of these ideal situations leave room for an API that receives the type 
> //optionally//.

I've returned back `OriginalTy` param from a **default** to a **regular** one 
as @steakhal advised in D96090#261 
.


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

https://reviews.llvm.org/D96090

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp

Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -394,48 +394,6 @@
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
- ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-return V;
-
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-SymbolRef Sym = V.getAsSymbol();
-if (Sym && !Sym->getType()->isFloatingType())
-  return UnknownVal();
-  }
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) {
-  QualType sr = SR->getSymbol()->getType();
-  if (!hasSameUnqualifiedPointeeType(sr, castTy))
-  return loc::MemRegionVal(castRegion(SR, castTy));
-}
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
 return Base;
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -65,12 +65,12 @@
 // Transfer function for Casts.
 //===--===//
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs() || Val.getAs());
-  return Val.getAs() ? evalCastFromLoc(Val.castAs(), CastTy)
-   : evalCastFromNonLoc(Val.castAs(), CastTy);
+  return evalCast(Val, CastTy, QualType{});
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Just a ping for you, guys, to let me move in this direction.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96090: [analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

I don't know. I think it's already way better than it was.
I think we can reiterate this later.

I want to get the `llvm_unreachable("Unknown SVal kind")` statements back. 
Besides those, I have no objections to this patch.
It preserves the old semantics, so it should be safe to land.
What do you think @NoQ?




Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:564-565
   }
-
-  llvm_unreachable("Unknown SVal kind");
 }

You probably don't want to remove this. The same applies to similar ones.


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

https://reviews.llvm.org/D96090

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits