[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ

> Even though NFC commits don't require tests, this doesn't mean they shouldn't 
> add them!

While developing this, I wasn't able to reproduce any regression or unpassed 
cases on the projects from CodeChecker list (//Bitcoin_v0.20.1, 
Curl_curl-7_66_0, Memchached_1.6.8, OpenSSL_openssl-3.0.0-alpha7, etc.//) due 
to my platform. I use Windows and those projects either don't compile or need 
hard effort to set them up.
But anyway I was able to detect some different diagnostics in existing tests 
`casts.cpp` using Z3 refutation option. That actually helped to make 
corrections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I'm catching up and these changes look great.

In D90157#2433657 , @steakhal wrote:

> I've run the baseline and your patch as well on 15 projects, both with and 
> without Z3 refutation enabled.
> (...)
> All in all, I'm still in favor of this change, but I'm really curious why did 
> we observe such changes. Debugging the cause seems really tricky to me.

If testing on a large codebase uncovered changes in behavior that weren't 
caught by our LIT test suite it often make sense to update our LIT test suite 
to include those tests in order to avoid similar regressions in the future. 
Regressions like this are easy to auto-reduce with tools like `creduce` because 
there's no functional change intended in the patch so there's a 100% formal 
reduction criterion "any change in diagnostics is observed" which can be easily 
fed to `creduce`. Even though NFC commits don't require tests, this doesn't 
mean they shouldn't add them!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG13f4448ae7db: [analyzer] Rework SValBuilder::evalCast 
function into maintainable and clear way (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90157

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

Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -530,108 +530,197 @@
   return evalCast(val, castTy, originalTy);
 }
 
-// FIXME: should rewrite according to the cast kind.
-SVal SValBuilder::evalCast(SVal val, QualType castTy, QualType originalTy) {
-  castTy = Context.getCanonicalType(castTy);
-  originalTy = Context.getCanonicalType(originalTy);
-  if (val.isUnknownOrUndef() || castTy == originalTy)
-return val;
+//===--===//
+// Cast methods.
+// `evalCast` is the main method
+// `evalCastKind` and `evalCastSubKind` are helpers
+//===--===//
 
-  if (castTy->isBooleanType()) {
-if (val.isUnknownOrUndef())
-  return val;
-if (val.isConstant())
-  return makeTruthVal(!val.isZeroConstant(), castTy);
-if (!Loc::isLocType(originalTy) &&
-!originalTy->isIntegralOrEnumerationType() &&
-!originalTy->isMemberPointerType())
-  return UnknownVal();
-if (SymbolRef Sym = val.getAsSymbol(true)) {
-  BasicValueFactory  = getBasicValueFactory();
-  // FIXME: If we had a state here, we could see if the symbol is known to
-  // be zero, but we don't.
-  return makeNonLoc(Sym, BO_NE, BVF.getValue(0, Sym->getType()), castTy);
-}
-// Loc values are not always true, they could be weakly linked functions.
-if (Optional L = val.getAs())
-  return evalCastFromLoc(*L, castTy);
+SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+  CastTy = Context.getCanonicalType(CastTy);
+  OriginalTy = Context.getCanonicalType(OriginalTy);
+  if (CastTy == OriginalTy)
+return V;
 
-Loc L = val.castAs().getLoc();
-return evalCastFromLoc(L, castTy);
+  // 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)))
+  return V;
+
+  // Cast SVal according to kinds.
+  switch (V.getBaseKind()) {
+  case SVal::UndefinedValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::UnknownValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::LocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::NonLocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // For const casts, casts to void, just propagate the value.
-  if (!castTy->isVariableArrayType() && !originalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(castTy),
- Context.getPointerType(originalTy)))
-  return val;
+SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
 
-  // Check for casts from pointers to integers.
-  if (castTy->isIntegralOrEnumerationType() && Loc::isLocType(originalTy))
-return evalCastFromLoc(val.castAs(), castTy);
-
-  // Check for casts from integers to pointers.
-  if (Loc::isLocType(castTy) && originalTy->isIntegralOrEnumerationType()) {
-if (Optional LV = val.getAs()) {
-  if (const MemRegion *R = LV->getLoc().getAsRegion()) {
-StoreManager  = StateMgr.getStoreManager();
-R = storeMgr.castRegion(R, castTy);
-return R ? SVal(loc::MemRegionVal(R)) : UnknownVal();
-  }
-  return LV->getLoc();
-}
-return dispatchCast(val, castTy);
+SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
+
+SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
+  switch (V.getSubKind()) {
+  case loc::ConcreteIntKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::GotoLabelKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::MemRegionValKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
 

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Hi, guys!
Does anyone can review this item, except @steakhal (thanks him)? Please, look.


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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

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

steakhal added a comment.

In D90157#2519800 , @ASDenysPetrov 
wrote:

> Updated. Naming fixes. Member access changes.

Looks great!

IMO we are sort-of late to commit this into clang-12 with confidence.
We should land it after we tag that release.
What do you think @xazax.hun?


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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

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

Updated. Naming fixes. Member access changes.


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

https://reviews.llvm.org/D90157

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

Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -530,108 +530,197 @@
   return evalCast(val, castTy, originalTy);
 }
 
-// FIXME: should rewrite according to the cast kind.
-SVal SValBuilder::evalCast(SVal val, QualType castTy, QualType originalTy) {
-  castTy = Context.getCanonicalType(castTy);
-  originalTy = Context.getCanonicalType(originalTy);
-  if (val.isUnknownOrUndef() || castTy == originalTy)
-return val;
+//===--===//
+// Cast methods.
+// `evalCast` is the main method
+// `evalCastKind` and `evalCastSubKind` are helpers
+//===--===//
 
-  if (castTy->isBooleanType()) {
-if (val.isUnknownOrUndef())
-  return val;
-if (val.isConstant())
-  return makeTruthVal(!val.isZeroConstant(), castTy);
-if (!Loc::isLocType(originalTy) &&
-!originalTy->isIntegralOrEnumerationType() &&
-!originalTy->isMemberPointerType())
-  return UnknownVal();
-if (SymbolRef Sym = val.getAsSymbol(true)) {
-  BasicValueFactory  = getBasicValueFactory();
-  // FIXME: If we had a state here, we could see if the symbol is known to
-  // be zero, but we don't.
-  return makeNonLoc(Sym, BO_NE, BVF.getValue(0, Sym->getType()), castTy);
-}
-// Loc values are not always true, they could be weakly linked functions.
-if (Optional L = val.getAs())
-  return evalCastFromLoc(*L, castTy);
+SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+  CastTy = Context.getCanonicalType(CastTy);
+  OriginalTy = Context.getCanonicalType(OriginalTy);
+  if (CastTy == OriginalTy)
+return V;
 
-Loc L = val.castAs().getLoc();
-return evalCastFromLoc(L, castTy);
+  // 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)))
+  return V;
+
+  // Cast SVal according to kinds.
+  switch (V.getBaseKind()) {
+  case SVal::UndefinedValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::UnknownValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::LocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::NonLocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // For const casts, casts to void, just propagate the value.
-  if (!castTy->isVariableArrayType() && !originalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(castTy),
- Context.getPointerType(originalTy)))
-  return val;
+SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
 
-  // Check for casts from pointers to integers.
-  if (castTy->isIntegralOrEnumerationType() && Loc::isLocType(originalTy))
-return evalCastFromLoc(val.castAs(), castTy);
-
-  // Check for casts from integers to pointers.
-  if (Loc::isLocType(castTy) && originalTy->isIntegralOrEnumerationType()) {
-if (Optional LV = val.getAs()) {
-  if (const MemRegion *R = LV->getLoc().getAsRegion()) {
-StoreManager  = StateMgr.getStoreManager();
-R = storeMgr.castRegion(R, castTy);
-return R ? SVal(loc::MemRegionVal(R)) : UnknownVal();
-  }
-  return LV->getLoc();
-}
-return dispatchCast(val, castTy);
+SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
+
+SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
+  switch (V.getSubKind()) {
+  case loc::ConcreteIntKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::GotoLabelKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::MemRegionValKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // Just pass through function and block pointers.
-  if (originalTy->isBlockPointerType() || 

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

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

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:647-650
+  // Array to pointer.
+  if (isa(OriginalTy))
+if (CastTy->isPointerType() || CastTy->isReferenceType())
   return UnknownVal();

ASDenysPetrov wrote:
> steakhal wrote:
> > Arrays decay to a pointer to the first element, but in this case, you 
> > return `Unknown`.
> > Why?
> This is just how `evalCast` worked before the patch. I didn't think of why. 
> I've just tried to replicate previous cast logic. If I'd change anything, 
> you'd catch a bunch of differences in CodeChecker. That's what I didn't want 
> the most.
> 
I agree, you should not change any behavior in this patch.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:765-769
+  auto castedValue = [V, CastTy, this]() {
+llvm::APSInt value = V.getValue();
+BasicVals.getAPSIntType(CastTy).apply(value);
+return value;
+  };

ASDenysPetrov wrote:
> steakhal wrote:
> > Just call immediately that lambda and assign that value to a `const 
> > llvm::APSInt CastedValue`.
> I just wanted to make this lazy. Otherwise this set of calls will be invoked 
> unnecessarily in some cases. I'd prefer to leave it as it is.
Maybe rename it then. What about `GetCastedValue(x)`?


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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> Why don't you use the `SValVisitor` instead?

I simply didn't know of its exsistence. We can try to transform this patch 
using `SValVisitor` in the next revision to make the review easier and avoid 
additional complexity.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:105-126
+  SVal evalCast(SVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(UndefinedVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(UnknownVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(Loc V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(NonLoc V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastSubKind(loc::ConcreteInt V, QualType CastTy,
+   QualType OriginalTy);

steakhal wrote:
> Why are all of these `public`?
> I would expect only the first overload to be public.
Great catch. I'll do it.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:647-650
+  // Array to pointer.
+  if (isa(OriginalTy))
+if (CastTy->isPointerType() || CastTy->isReferenceType())
   return UnknownVal();

steakhal wrote:
> Arrays decay to a pointer to the first element, but in this case, you return 
> `Unknown`.
> Why?
This is just how `evalCast` worked before the patch. I didn't think of why. 
I've just tried to replicate previous cast logic. If I'd change anything, you'd 
catch a bunch of differences in CodeChecker. That's what I didn't want the most.




Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:680-681
+
+  // Try to cast to array
+  const auto *arrayT = dyn_cast(OriginalTy.getCanonicalType());
+

steakhal wrote:
> nit
Thanks! I'll check all for the naming rules.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:765-769
+  auto castedValue = [V, CastTy, this]() {
+llvm::APSInt value = V.getValue();
+BasicVals.getAPSIntType(CastTy).apply(value);
+return value;
+  };

steakhal wrote:
> Just call immediately that lambda and assign that value to a `const 
> llvm::APSInt CastedValue`.
I just wanted to make this lazy. Otherwise this set of calls will be invoked 
unnecessarily in some cases. I'd prefer to leave it as it is.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:844-847
+  // Symbol to bool.
+  if (CastTy->isBooleanType()) {
+// Non-float(presumably) to bool.
+if (Loc::isLocType(OriginalTy) ||

steakhal wrote:
> Presumably what?
We assume that this condition presents a type which is opposite to `float` (aka 
`non-float`). But it may happen that it is also opposite to some other types. 
So naming it `non-float` could be a bit incomplete and we are dealing with some 
other type. Yes, I feel this embarrassing. I'll remove `(presumably)`.


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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

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

steakhal added a comment.

What you are basically doing is a visitation on the kind of the `SVal`.
Why don't you use the `SValVisitor` instead? That way you could focus on the 
leaf nodes in the hierarchy, leaving you an even cleaner solution.

  class CastEvaluator : public SValVisitor {
  public:
CastEvaluator(QualType CastTy, QualType OriginalTy /*etc.*/)
SVal VisitPointerToMember(nonloc::PointerToMember V) { return V; }
SVal VisitGotoLabel(loc::GotoLabel V) { /*impl.*/ }
/* etc. */
  };

Comments should be punctuated and the variables should start with an uppercase 
letter in general.
About the correctness: I think it still needs a bit more confidence. I'm gonna 
run this on LLVM itself and see if anything changes.

I really like where this is going. Keep up the good work.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:105-126
+  SVal evalCast(SVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(UndefinedVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(UnknownVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(Loc V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(NonLoc V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastSubKind(loc::ConcreteInt V, QualType CastTy,
+   QualType OriginalTy);

Why are all of these `public`?
I would expect only the first overload to be public.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:647-650
+  // Array to pointer.
+  if (isa(OriginalTy))
+if (CastTy->isPointerType() || CastTy->isReferenceType())
   return UnknownVal();

Arrays decay to a pointer to the first element, but in this case, you return 
`Unknown`.
Why?



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:680-681
+
+  // Try to cast to array
+  const auto *arrayT = dyn_cast(OriginalTy.getCanonicalType());
+

nit



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:765-769
+  auto castedValue = [V, CastTy, this]() {
+llvm::APSInt value = V.getValue();
+BasicVals.getAPSIntType(CastTy).apply(value);
+return value;
+  };

Just call immediately that lambda and assign that value to a `const 
llvm::APSInt CastedValue`.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:844-847
+  // Symbol to bool.
+  if (CastTy->isBooleanType()) {
+// Non-float(presumably) to bool.
+if (Loc::isLocType(OriginalTy) ||

Presumably what?


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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

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

@steakhal 
Oh, sure. Updated.


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

https://reviews.llvm.org/D90157

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

Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -530,108 +530,197 @@
   return evalCast(val, castTy, originalTy);
 }
 
-// FIXME: should rewrite according to the cast kind.
-SVal SValBuilder::evalCast(SVal val, QualType castTy, QualType originalTy) {
-  castTy = Context.getCanonicalType(castTy);
-  originalTy = Context.getCanonicalType(originalTy);
-  if (val.isUnknownOrUndef() || castTy == originalTy)
-return val;
+//===--===//
+// Cast methods.
+// `evalCast` is the main method
+// `evalCastKind` and `evalCastSubKind` are helpers
+//===--===//
 
-  if (castTy->isBooleanType()) {
-if (val.isUnknownOrUndef())
-  return val;
-if (val.isConstant())
-  return makeTruthVal(!val.isZeroConstant(), castTy);
-if (!Loc::isLocType(originalTy) &&
-!originalTy->isIntegralOrEnumerationType() &&
-!originalTy->isMemberPointerType())
-  return UnknownVal();
-if (SymbolRef Sym = val.getAsSymbol(true)) {
-  BasicValueFactory  = getBasicValueFactory();
-  // FIXME: If we had a state here, we could see if the symbol is known to
-  // be zero, but we don't.
-  return makeNonLoc(Sym, BO_NE, BVF.getValue(0, Sym->getType()), castTy);
-}
-// Loc values are not always true, they could be weakly linked functions.
-if (Optional L = val.getAs())
-  return evalCastFromLoc(*L, castTy);
+SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+  CastTy = Context.getCanonicalType(CastTy);
+  OriginalTy = Context.getCanonicalType(OriginalTy);
+  if (CastTy == OriginalTy)
+return V;
 
-Loc L = val.castAs().getLoc();
-return evalCastFromLoc(L, castTy);
+  // 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)))
+  return V;
+
+  // Cast SVal according to kinds.
+  switch (V.getBaseKind()) {
+  case SVal::UndefinedValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::UnknownValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::LocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::NonLocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // For const casts, casts to void, just propagate the value.
-  if (!castTy->isVariableArrayType() && !originalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(castTy),
- Context.getPointerType(originalTy)))
-  return val;
+SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
 
-  // Check for casts from pointers to integers.
-  if (castTy->isIntegralOrEnumerationType() && Loc::isLocType(originalTy))
-return evalCastFromLoc(val.castAs(), castTy);
-
-  // Check for casts from integers to pointers.
-  if (Loc::isLocType(castTy) && originalTy->isIntegralOrEnumerationType()) {
-if (Optional LV = val.getAs()) {
-  if (const MemRegion *R = LV->getLoc().getAsRegion()) {
-StoreManager  = StateMgr.getStoreManager();
-R = storeMgr.castRegion(R, castTy);
-return R ? SVal(loc::MemRegionVal(R)) : UnknownVal();
-  }
-  return LV->getLoc();
-}
-return dispatchCast(val, castTy);
+SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
+
+SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
+  switch (V.getSubKind()) {
+  case loc::ConcreteIntKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::GotoLabelKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::MemRegionValKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // Just pass through function and block pointers.
-  if (originalTy->isBlockPointerType() || originalTy->isFunctionPointerType()) 

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

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

steakhal added a comment.

Please @ASDenysPetrov, give full context with the option `-U` when you 
export the diff from git.
I would like to quickly swipe through the code before I accept this.


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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

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

Unfortunately I wasn't able to reproduce the singe difference in PostgreSQL 
project. So I revised the patch in a part of the difference nature.
This update passed all of CodeChecker's tests without any difference.
So for now the patch looks like ready to be accepted.
Please, welcome for review.


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

https://reviews.llvm.org/D90157

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

Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -530,108 +530,197 @@
   return evalCast(val, castTy, originalTy);
 }
 
-// FIXME: should rewrite according to the cast kind.
-SVal SValBuilder::evalCast(SVal val, QualType castTy, QualType originalTy) {
-  castTy = Context.getCanonicalType(castTy);
-  originalTy = Context.getCanonicalType(originalTy);
-  if (val.isUnknownOrUndef() || castTy == originalTy)
-return val;
+//===--===//
+// Cast methods.
+// `evalCast` is the main method
+// `evalCastKind` and `evalCastSubKind` are helpers
+//===--===//
 
-  if (castTy->isBooleanType()) {
-if (val.isUnknownOrUndef())
-  return val;
-if (val.isConstant())
-  return makeTruthVal(!val.isZeroConstant(), castTy);
-if (!Loc::isLocType(originalTy) &&
-!originalTy->isIntegralOrEnumerationType() &&
-!originalTy->isMemberPointerType())
-  return UnknownVal();
-if (SymbolRef Sym = val.getAsSymbol(true)) {
-  BasicValueFactory  = getBasicValueFactory();
-  // FIXME: If we had a state here, we could see if the symbol is known to
-  // be zero, but we don't.
-  return makeNonLoc(Sym, BO_NE, BVF.getValue(0, Sym->getType()), castTy);
-}
-// Loc values are not always true, they could be weakly linked functions.
-if (Optional L = val.getAs())
-  return evalCastFromLoc(*L, castTy);
+SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+  CastTy = Context.getCanonicalType(CastTy);
+  OriginalTy = Context.getCanonicalType(OriginalTy);
+  if (CastTy == OriginalTy)
+return V;
 
-Loc L = val.castAs().getLoc();
-return evalCastFromLoc(L, castTy);
+  // 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)))
+  return V;
+
+  // Cast SVal according to kinds.
+  switch (V.getBaseKind()) {
+  case SVal::UndefinedValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::UnknownValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::LocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::NonLocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // For const casts, casts to void, just propagate the value.
-  if (!castTy->isVariableArrayType() && !originalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(castTy),
- Context.getPointerType(originalTy)))
-  return val;
+SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
 
-  // Check for casts from pointers to integers.
-  if (castTy->isIntegralOrEnumerationType() && Loc::isLocType(originalTy))
-return evalCastFromLoc(val.castAs(), castTy);
-
-  // Check for casts from integers to pointers.
-  if (Loc::isLocType(castTy) && originalTy->isIntegralOrEnumerationType()) {
-if (Optional LV = val.getAs()) {
-  if (const MemRegion *R = LV->getLoc().getAsRegion()) {
-StoreManager  = StateMgr.getStoreManager();
-R = storeMgr.castRegion(R, castTy);
-return R ? SVal(loc::MemRegionVal(R)) : UnknownVal();
-  }
-  return LV->getLoc();
-}
-return dispatchCast(val, castTy);
+SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
+
+SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
+  switch (V.getSubKind()) {
+  case loc::ConcreteIntKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::GotoLabelKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case 

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Thank to @steakhal for the new tests.
//All// previous result differences have **gone** in `FFMPEG` and `twin` but a 
new //one// **appeared** in `PostgreSQL`.

  /src/common/d2s.c  Line 895 Memory copy function overflows the destination 
buffer alpha.unix.cstring.OutOfBounds
   
   

Here is an original link to the report: 
https://codechecker-demo.eastus.cloudapp.azure.com/Default?run=D90157=100
 (login: demo, pass: demo)

I'm keep going to work on the patch to remove this single difference.


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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

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

@steakhal 
I've precisely inspected your reports and find the bug. I've fixed it. I also 
verified all the rest and find some other vulnerabilities and fixed them as 
well.
Thank you for your testing, and I would ask you to run these tests again on my 
fix.


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

https://reviews.llvm.org/D90157

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

Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -530,108 +530,184 @@
   return evalCast(val, castTy, originalTy);
 }
 
-// FIXME: should rewrite according to the cast kind.
-SVal SValBuilder::evalCast(SVal val, QualType castTy, QualType originalTy) {
-  castTy = Context.getCanonicalType(castTy);
-  originalTy = Context.getCanonicalType(originalTy);
-  if (val.isUnknownOrUndef() || castTy == originalTy)
-return val;
+//===--===//
+// Cast methods.
+// `evalCast` is the main method
+// `evalCastKind` and `evalCastSubKind` are helpers
+//===--===//
 
-  if (castTy->isBooleanType()) {
-if (val.isUnknownOrUndef())
-  return val;
-if (val.isConstant())
-  return makeTruthVal(!val.isZeroConstant(), castTy);
-if (!Loc::isLocType(originalTy) &&
-!originalTy->isIntegralOrEnumerationType() &&
-!originalTy->isMemberPointerType())
-  return UnknownVal();
-if (SymbolRef Sym = val.getAsSymbol(true)) {
-  BasicValueFactory  = getBasicValueFactory();
-  // FIXME: If we had a state here, we could see if the symbol is known to
-  // be zero, but we don't.
-  return makeNonLoc(Sym, BO_NE, BVF.getValue(0, Sym->getType()), castTy);
-}
-// Loc values are not always true, they could be weakly linked functions.
-if (Optional L = val.getAs())
-  return evalCastFromLoc(*L, castTy);
+SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+  CastTy = Context.getCanonicalType(CastTy);
+  OriginalTy = Context.getCanonicalType(OriginalTy);
+  if (CastTy == OriginalTy)
+return V;
 
-Loc L = val.castAs().getLoc();
-return evalCastFromLoc(L, castTy);
+  // 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)))
+  return V;
+
+  // Cast SVal according to kinds.
+  switch (V.getBaseKind()) {
+  case SVal::UndefinedValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::UnknownValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::LocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::NonLocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // For const casts, casts to void, just propagate the value.
-  if (!castTy->isVariableArrayType() && !originalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(castTy),
- Context.getPointerType(originalTy)))
-  return val;
+SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
 
-  // Check for casts from pointers to integers.
-  if (castTy->isIntegralOrEnumerationType() && Loc::isLocType(originalTy))
-return evalCastFromLoc(val.castAs(), castTy);
-
-  // Check for casts from integers to pointers.
-  if (Loc::isLocType(castTy) && originalTy->isIntegralOrEnumerationType()) {
-if (Optional LV = val.getAs()) {
-  if (const MemRegion *R = LV->getLoc().getAsRegion()) {
-StoreManager  = StateMgr.getStoreManager();
-R = storeMgr.castRegion(R, castTy);
-return R ? SVal(loc::MemRegionVal(R)) : UnknownVal();
-  }
-  return LV->getLoc();
-}
-return dispatchCast(val, castTy);
+SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
+
+SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
+  switch (V.getSubKind()) {
+  case loc::ConcreteIntKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::GotoLabelKind:
+return evalCastSubKind(V.castAs(), CastTy, OriginalTy);
+  case loc::MemRegionValKind:
+return 

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal
Thank you for your invaluable analysis!

> What do you think @ASDenysPetrov ?

I think if there is any difference then I have to inspect the changes deeper. I 
tryed to make this changes in a way of not changing any behaviour.
Let me back to this patch a bit later. Currently have another important task.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

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

steakhal added a comment.

In D90157#2361773 , @ASDenysPetrov 
wrote:

> Who can confirm if this is correct or somewhere it needs fixes? Here is a 
> generated result of `evalCast` from the origin branch(before the patch):
>
>   void foo(int* x,  // {reg_$0}
>int** y, // {reg_$1}
>int***z) // {reg_$2}
>   {
>(void*)x;// {reg_$0}
>(void*)y;// {reg_$1}
>(void*)z;// {reg_$2}
>(void**)x;   // {SymRegion{reg_$0},0 S64b,void *}
>(void**)y;   // {reg_$1}
>(void**)z;   // {reg_$2}
>(void***)x;  // {SymRegion{reg_$0},0 S64b,void **}
>(void***)y;  // {SymRegion{reg_$1},0 S64b,void **}
>(void***)z;  // {reg_$2}
>(void)x; // {SymRegion{reg_$0},0 S64b,void ***}
>(void)y; // {SymRegion{reg_$1},0 S64b,void ***}
>(void)z; // {SymRegion{reg_$2},0 S64b,void ***}
>   }

AFAIK, Element regions represent the reinterpret casts, and static-casts 
doesn't require anything to do as the loading from the region would be cast to 
the appropriate type.
So these dumps seem to be correct.

BTW, this dispatching logic should be done with an SValVisitor IMO. That way 
you could make it even more cleaner. @ASDenysPetrov

> @steakhal
>
>> By the same token, I think you should run the test-suite using the Z3 
>> refutation and/or Z3 constraint solver to check if this introduces any 
>> regression.
>
> Never worked with Z3 before. I'll try.



---

I've run the baseline and your patch as well on 15 projects, both with and 
without Z3 refutation enabled.

The reports for these 13 projects did not change at all:

  Bitcoin_v0.20.1
  Curl_curl-7_66_0
  Memchached_1.6.8
  OpenSSL_openssl-3.0.0-alpha7
  PostgreSQL_REL_13_0
  Redis_5.0.6
  SQLite_version-3.33.0
  TinyXML2_8.0.0
  Vim_v8.2.1920
  Xerces_v3.2.3
  libWebM_libwebm-1.0.0.27
  protobuf_v3.13.0
  tmux_3.0

However, for `twin`, it introduced a new unique report:

  server/socketalien.h @ Line 64Access out-of-bound array element 
(buffer overflow) alpha.security.ArrayBound

And for the `FFMPEG`, 2 unique reports were lost without Z3 refutation:

  vf_edgedetect.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  mpc8.cAccess out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound

3 unique reports were lost with Z3 refutation:

  utvideodec.c  7th function call argument is an uninitialized value
core.CallAndMessage
  utvideodec.c  The left operand of '-' is a garbage value  
core.UndefinedBinaryOperatorResult
  xan.c Arguments must not be overlapping buffers   
alpha.unix.cstring.BufferOverlap

And introduced 35 unique new ones without Z3 refutation, only 34 with 
refutation:

  lpc.c Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound   (Z3 refutation filters this)
  on2avc.c  Memory copy function accesses out-of-bound array element
alpha.unix.cstring.OutOfBounds
  bytestream.h  Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  ws-snd1.c Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer overflow) 
alpha.security.ArrayBound
  h264qpel_template.c   Access out-of-bound array element (buffer 

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Who can confirm if this is correct or somewhere it needs fixes? Here is a 
generated result of `evalCast` from the origin branch(before the patch):

  void foo(int* x,  // {reg_$0}
   int** y, // {reg_$0}
   int***z) // {reg_$0}
  {
   (void*)x;// {reg_$0}
   (void*)y;// {reg_$0}
   (void*)z;// {reg_$0}
   (void**)x;   // {SymRegion{reg_$0},0 S64b,void *}
   (void**)y;   // {reg_$0}
   (void**)z;   // {reg_$2}
   (void***)x;  // {SymRegion{reg_$0},0 S64b,void **}
   (void***)y;  // {SymRegion{reg_$1},0 S64b,void **}
   (void***)z;  // {reg_$2}
   (void)x; // {SymRegion{reg_$0},0 S64b,void ***}
   (void)y; // {SymRegion{reg_$1},0 S64b,void ***}
   (void)z; // {SymRegion{reg_$2},0 S64b,void ***}
  }

@martong

> https://en.cppreference.com/w/cpp/language/explicit_cast
> https://en.cppreference.com/w/cpp/language/implicit_conversion

Thanks for the links.
@steakhal

> By the same token, I think you should run the test-suite using the Z3 
> refutation and/or Z3 constraint solver to check if this introduces any 
> regression.

Never worked with Z3 before. I'll try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

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

steakhal added a comment.

In D90157#2356170 , @martong wrote:

>> Now I'm going to make unittests for SValBuilder::evalCast. Let me ask you to 
>> offer me some examples of casts to check. As many cases we can collect as 
>> robust the test would be. E.g.
>
> This is not going to be easy. I guess we should cover both implicit and 
> explicit type conversions. C++ is ridiculously complex in these domains. For 
> maximal precision I'd suggest to systematically go through the below pages 
> and try to cover each major case:
> https://en.cppreference.com/w/cpp/language/explicit_cast
> https://en.cppreference.com/w/cpp/language/implicit_conversion

@martong  Keep in mind that we try hard not to emit casts - even if that would 
require. Check D85528 , exactly touching this 
topic.

---

By the same token, I think you should run the test-suite using the Z3 
refutation and/or Z3 constraint solver to check if this introduces any 
regression.
You can do that by installing Z3, adding the cmake options: 
`-DLLVM_ENABLE_Z3_SOLVER=ON`, `-DLLVM_Z3_INSTALL_DIR=/path/to/z3`
After this, you should be able to run the lit-test:
`./bin/llvm-lit -sv --show-unsupported --show-xfail 
/home/elnbbea/git/llvm-project/clang/test/Analysis --param USE_Z3_SOLVER=1`
Run it before and after your fix and you should expect no //new// 
failures/crashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Now I'm going to make unittests for SValBuilder::evalCast. Let me ask you to 
> offer me some examples of casts to check. As many cases we can collect as 
> robust the test would be. E.g.

This is not going to be easy. I guess we should cover both implicit and 
explicit type conversions. C++ is ridiculously complex in these domains. For 
maximal precision I'd suggest to systematically go through the below pages and 
try to cover each major case:
https://en.cppreference.com/w/cpp/language/explicit_cast
https://en.cppreference.com/w/cpp/language/implicit_conversion




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:105
 
-  SVal evalCast(SVal val, QualType castTy, QualType originalType);
+  SVal evalCast(SVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(UndefinedVal V, QualType CastTy, QualType OriginalTy);

+1 for this kind of splitting, makes the code cleaner for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90157

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

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

[Denys Petrov via Phabricator via cfe-commits]

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

Refactor `SValBuilder::evalCast` function. Make the function clear and get rid 
of redundant and repetitive code. Unite `SValBuilder::evalCast`, 
`SimpleSValBuilder::dispatchCast`, `SimpleSValBuilder::evalCastFromNonLoc` and 
`SimpleSValBuilder::evalCastFromLoc` functions into single 
`SValBuilder::evalCast`.

P.S. Inspired by D89055 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90157

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

Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -530,108 +530,178 @@
   return evalCast(val, castTy, originalTy);
 }
 
-// FIXME: should rewrite according to the cast kind.
-SVal SValBuilder::evalCast(SVal val, QualType castTy, QualType originalTy) {
-  castTy = Context.getCanonicalType(castTy);
-  originalTy = Context.getCanonicalType(originalTy);
-  if (val.isUnknownOrUndef() || castTy == originalTy)
-return val;
-
-  if (castTy->isBooleanType()) {
-if (val.isUnknownOrUndef())
-  return val;
-if (val.isConstant())
-  return makeTruthVal(!val.isZeroConstant(), castTy);
-if (!Loc::isLocType(originalTy) &&
-!originalTy->isIntegralOrEnumerationType() &&
-!originalTy->isMemberPointerType())
-  return UnknownVal();
-if (SymbolRef Sym = val.getAsSymbol(true)) {
-  BasicValueFactory  = getBasicValueFactory();
-  // FIXME: If we had a state here, we could see if the symbol is known to
-  // be zero, but we don't.
-  return makeNonLoc(Sym, BO_NE, BVF.getValue(0, Sym->getType()), castTy);
-}
-// Loc values are not always true, they could be weakly linked functions.
-if (Optional L = val.getAs())
-  return evalCastFromLoc(*L, castTy);
+//===--===//
+// Cast methods.
+// `evalCast` is the main method
+// `evalCastKind` and `evalCastSubKind` are helpers
+//===--===//
 
-Loc L = val.castAs().getLoc();
-return evalCastFromLoc(L, castTy);
+SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+  // 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)))
+  return V;
+
+  // Cast SVal according to kinds.
+  switch (V.getBaseKind()) {
+  case SVal::UndefinedValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::UnknownValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::LocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::NonLocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // For const casts, casts to void, just propagate the value.
-  if (!castTy->isVariableArrayType() && !originalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(castTy),
- Context.getPointerType(originalTy)))
-  return val;
+SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
 
-  // Check for casts from pointers to integers.
-  if (castTy->isIntegralOrEnumerationType() && Loc::isLocType(originalTy))
-return evalCastFromLoc(val.castAs(), castTy);
-
-  // Check for casts from integers to pointers.
-  if (Loc::isLocType(castTy) && originalTy->isIntegralOrEnumerationType()) {
-if (Optional LV = val.getAs()) {
-  if (const MemRegion *R = LV->getLoc().getAsRegion()) {
-StoreManager  = StateMgr.getStoreManager();
-R = storeMgr.castRegion(R, castTy);
-return R ? SVal(loc::MemRegionVal(R)) : UnknownVal();
-  }
-  return LV->getLoc();
-}
-return dispatchCast(val, castTy);
+SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
+
+SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
+  switch (V.getSubKind()) {
+  case loc::ConcreteIntKind:
+