[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
DonatNagyE wrote:
Thanks!
https://github.com/llvm/llvm-project/pull/70837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
https://github.com/steakhal updated
https://github.com/llvm/llvm-project/pull/70837
>From 2de19fc8e14319674ce87c18771ba1b8ba22f79b Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 23 Oct 2023 18:10:29 +0200
Subject: [PATCH 1/3] [analyzer] Fix assertion failure in
CXXInstanceCall::getCXXThisVal
Workaround the case when the `this` pointer is actually a `NonLoc`, by
returning `Unknown` instead.
The solution isn't ideal, as `this` should be really a `Loc`, but due to
how casts work, I feel this is our easiest and best option.
I've considered using `SVB.evalCast(ThisVal, Base->getType(), QualType())`,
but it doesn't work as `EvalCastVisitor::VisitNonLocSymbolVal()` only
evaluates casts that happen from NonLoc to NonLocs.
When I tried to actually implement that case, I figured:
1) Create a SymbolicRegion from that nonloc::SymbolVal; but SymbolRegion
ctor expects a pointer type for the symbol.
2) Okay, just have a SymbolCast, getting us the pointer type; but
SymbolRegion expects SymbolData symbols, not generic SymExprs, as stated:
> // Because pointer arithmetic is represented by ElementRegion layers,
> // the base symbol here should not contain any arithmetic.
3) We can't use ElementRegions to perform this cast because to have an
ElementRegion, you already have to have a SubRegion that you want to
cast, but the point is that we don't have that.
At this point, I gave up, and just returned `Unknown` xD
IMO this is still better than having a crash.
Fixes #69922
---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 5 ++---
clang/test/Analysis/builtin_bitcast.cpp | 21 +
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index ad5bb66c4fff3c8..20bc64dc2631cec 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -715,10 +715,9 @@ void CXXInstanceCall::getExtraInvalidatedValues(
SVal CXXInstanceCall::getCXXThisVal() const {
const Expr *Base = getCXXThisExpr();
// FIXME: This doesn't handle an overloaded ->* operator.
- if (!Base)
+ SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
+ if (isa(ThisVal))
return UnknownVal();
-
- SVal ThisVal = getSVal(Base);
assert(ThisVal.isUnknownOrUndef() || isa(ThisVal));
return ThisVal;
}
diff --git a/clang/test/Analysis/builtin_bitcast.cpp
b/clang/test/Analysis/builtin_bitcast.cpp
index 396e7caa45f6acd..13475694d287a22 100644
--- a/clang/test/Analysis/builtin_bitcast.cpp
+++ b/clang/test/Analysis/builtin_bitcast.cpp
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
+void gh_69922(ptr_size p) {
+ // expected-warning-re@+1 {{(reg_${{[0-9]+}}) & 1U}}
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1));
+
+ __builtin_bit_cast(A*, p & 1)->set(2); // no-crash
+ // However, since the `this` pointer is expected to be a Loc, but we have
+ // NonLoc there, we simply give up and resolve it as `Unknown`.
+ // Then, inside the `set()` member function call we can't evaluate the
+ // store to the member variable `n`.
+
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1)->n); // Ideally, this
should print "2".
+ // expected-warning-re@-1 {{(reg_${{[0-9]+}}) & 1U}}
+}
>From bcb048c09dcc7bb2728d46afc0ff9a09cf71f663 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Tue, 31 Oct 2023 19:20:16 +0100
Subject: [PATCH 2/3] Add fixme and add the ineffective evalCast
---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 9 +++--
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp | 5 +
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 20bc64dc2631cec..76fb7481f65194b 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -716,8 +716,13 @@ SVal CXXInstanceCall::getCXXThisVal() const {
const Expr *Base = getCXXThisExpr();
// FIXME: This doesn't handle an overloaded ->* operator.
SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
- if (isa(ThisVal))
-return UnknownVal();
+
+ if (isa(ThisVal)) {
+SValBuilder = getState()->getStateManager().getSValBuilder();
+QualType OriginalTy = ThisVal.getType(SVB.getContext());
+return SVB.evalCast(ThisVal, Base->getType(), OriginalTy);
+ }
+
assert(ThisVal.isUnknownOrUndef() || isa(ThisVal));
return ThisVal;
}
diff --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index d89d82626f72694..9375f39b2d71dd4 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -980,6 +980,11 @@ class EvalCastVisitor : public
SValVisitor {
return
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
steakhal wrote:
Fixed. Now using `size_t`.
https://github.com/llvm/llvm-project/pull/70837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
+void gh_69922(ptr_size p) {
+ // expected-warning-re@+1 {{(reg_${{[0-9]+}}) & 1U}}
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1));
steakhal wrote:
Yes, that is to form a `SymExpr`, which is a `SymbolVal` (NonLoc) wrapping a
`BO_And` `SymIntExpr` I think.
https://github.com/llvm/llvm-project/pull/70837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
https://github.com/DonatNagyE commented: Basically LGTM, thanks for fixing this crash. I don't give a formal approval because I didn't independently verify the correctness of the details, but I like the direction of the change. https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
+void gh_69922(ptr_size p) {
+ // expected-warning-re@+1 {{(reg_${{[0-9]+}}) & 1U}}
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1));
DonatNagyE wrote:
What is the role of the `&` operator here? Is it just an arbitrary numerical
operation that's not handled by the analyzer?
(No action needed, I'm just curious.)
https://github.com/llvm/llvm-project/pull/70837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
https://github.com/DonatNagyE edited https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
DonatNagyE wrote:
Do I see it correctly that `ptr_size` just an alias of `size_t`? If yes, then
why don't you use the standard name?
https://github.com/llvm/llvm-project/pull/70837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
https://github.com/Xazax-hun approved this pull request. https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
steakhal wrote: > > WDYT? > > I like this! I hope we do not add too much redundant work, but at least we > make it clear what is the plan to fix this in the future. Please approve the PR again, so that I could merge this after I give some time for others to look at this. https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
Xazax-hun wrote: > WDYT? I like this! I hope we do not add too much redundant work, but at least we make it clear what is the plan to fix this in the future. https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (PR #70837)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal (PR #70837)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal (PR #70837)
steakhal wrote: > Hmm, I wonder if we should leave a FIXME comment, but it looks good to me. I was thinking where to put the FIXME, and as I explored that should be within the CastVisitor. After that, I argued, that then I should still have the (ineffective) `SVB.evalCast()` to actually exercise that path. It should be a fancy way of returning `Unknown` now, but arguably for the better reason xD @Xazax-hun WDYT? https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal (PR #70837)
https://github.com/steakhal updated
https://github.com/llvm/llvm-project/pull/70837
>From a7f64815f4986fad597b9cb2d1acce2de9ac20bf Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 23 Oct 2023 18:10:29 +0200
Subject: [PATCH 1/2] [analyzer] Fix assertion failure in
CXXInstanceCall::getCXXThisVal
Workaround the case when the `this` pointer is actually a `NonLoc`, by
returning `Unknown` instead.
The solution isn't ideal, as `this` should be really a `Loc`, but due to
how casts work, I feel this is our easiest and best option.
I've considered using `SVB.evalCast(ThisVal, Base->getType(), QualType())`,
but it doesn't work as `EvalCastVisitor::VisitNonLocSymbolVal()` only
evaluates casts that happen from NonLoc to NonLocs.
When I tried to actually implement that case, I figured:
1) Create a SymbolicRegion from that nonloc::SymbolVal; but SymbolRegion
ctor expects a pointer type for the symbol.
2) Okay, just have a SymbolCast, getting us the pointer type; but
SymbolRegion expects SymbolData symbols, not generic SymExprs, as stated:
> // Because pointer arithmetic is represented by ElementRegion layers,
> // the base symbol here should not contain any arithmetic.
3) We can't use ElementRegions to perform this cast because to have an
ElementRegion, you already have to have a SubRegion that you want to
cast, but the point is that we don't have that.
At this point, I gave up, and just returned `Unknown` xD
IMO this is still better than having a crash.
Fixes #69922
---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 5 ++---
clang/test/Analysis/builtin_bitcast.cpp | 21 +
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index ad5bb66c4fff3c8..20bc64dc2631cec 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -715,10 +715,9 @@ void CXXInstanceCall::getExtraInvalidatedValues(
SVal CXXInstanceCall::getCXXThisVal() const {
const Expr *Base = getCXXThisExpr();
// FIXME: This doesn't handle an overloaded ->* operator.
- if (!Base)
+ SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
+ if (isa(ThisVal))
return UnknownVal();
-
- SVal ThisVal = getSVal(Base);
assert(ThisVal.isUnknownOrUndef() || isa(ThisVal));
return ThisVal;
}
diff --git a/clang/test/Analysis/builtin_bitcast.cpp
b/clang/test/Analysis/builtin_bitcast.cpp
index 396e7caa45f6acd..13475694d287a22 100644
--- a/clang/test/Analysis/builtin_bitcast.cpp
+++ b/clang/test/Analysis/builtin_bitcast.cpp
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
+void gh_69922(ptr_size p) {
+ // expected-warning-re@+1 {{(reg_${{[0-9]+}}) & 1U}}
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1));
+
+ __builtin_bit_cast(A*, p & 1)->set(2); // no-crash
+ // However, since the `this` pointer is expected to be a Loc, but we have
+ // NonLoc there, we simply give up and resolve it as `Unknown`.
+ // Then, inside the `set()` member function call we can't evaluate the
+ // store to the member variable `n`.
+
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1)->n); // Ideally, this
should print "2".
+ // expected-warning-re@-1 {{(reg_${{[0-9]+}}) & 1U}}
+}
>From 27d2b84e14d73dcc2385f202a204993f25235de4 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Tue, 31 Oct 2023 19:20:16 +0100
Subject: [PATCH 2/2] Add fixme and add the ineffective evalCast
---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 9 +++--
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp | 5 +
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 20bc64dc2631cec..76fb7481f65194b 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -716,8 +716,13 @@ SVal CXXInstanceCall::getCXXThisVal() const {
const Expr *Base = getCXXThisExpr();
// FIXME: This doesn't handle an overloaded ->* operator.
SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
- if (isa(ThisVal))
-return UnknownVal();
+
+ if (isa(ThisVal)) {
+SValBuilder = getState()->getStateManager().getSValBuilder();
+QualType OriginalTy = ThisVal.getType(SVB.getContext());
+return SVB.evalCast(ThisVal, Base->getType(), OriginalTy);
+ }
+
assert(ThisVal.isUnknownOrUndef() || isa(ThisVal));
return ThisVal;
}
diff --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index d89d82626f72694..9375f39b2d71dd4 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -980,6 +980,11 @@ class EvalCastVisitor : public
SValVisitor {
return
[clang] [analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal (PR #70837)
Xazax-hun wrote: Hmm, I wonder if we should leave a FIXME comment, but it looks good to me. https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal (PR #70837)
https://github.com/Xazax-hun approved this pull request. https://github.com/llvm/llvm-project/pull/70837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal (PR #70837)
llvmbot wrote:
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Balazs Benics (steakhal)
Changes
Workaround the case when the `this` pointer is actually a `NonLoc`, by
returning `Unknown` instead.
The solution isn't ideal, as `this` should be really a `Loc`, but due to how
casts work, I feel this is our easiest and best option.
I've considered using `SVB.evalCast(ThisVal, Base-getType(), QualType())`,
but it doesn't work as `EvalCastVisitor::VisitNonLocSymbolVal()` only evaluates
casts that happen from NonLoc to NonLocs.
When I tried to actually implement that case, I figured:
1) Create a SymbolicRegion from that nonloc::SymbolVal; but SymbolRegion ctor
expects a pointer type for the symbol.
2) Okay, just have a SymbolCast, getting us the pointer type; but SymbolRegion
expects SymbolData symbols, not generic SymExprs, as stated:
// Because pointer arithmetic is represented by ElementRegion layers,
// the base symbol here should not contain any arithmetic.
3) We can't use ElementRegions to perform this cast because to have an
ElementRegion, you already have to have a SubRegion that you want to cast, but
the point is that we don't have that.
At this point, I gave up, and just returned `Unknown` xD IMO this is still
better than having a crash.
Fixes #69922
---
Full diff: https://github.com/llvm/llvm-project/pull/70837.diff
2 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Core/CallEvent.cpp (+2-3)
- (modified) clang/test/Analysis/builtin_bitcast.cpp (+21)
``diff
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index ad5bb66c4fff3c8..20bc64dc2631cec 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -715,10 +715,9 @@ void CXXInstanceCall::getExtraInvalidatedValues(
SVal CXXInstanceCall::getCXXThisVal() const {
const Expr *Base = getCXXThisExpr();
// FIXME: This doesn't handle an overloaded ->* operator.
- if (!Base)
+ SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
+ if (isa(ThisVal))
return UnknownVal();
-
- SVal ThisVal = getSVal(Base);
assert(ThisVal.isUnknownOrUndef() || isa(ThisVal));
return ThisVal;
}
diff --git a/clang/test/Analysis/builtin_bitcast.cpp
b/clang/test/Analysis/builtin_bitcast.cpp
index 396e7caa45f6acd..13475694d287a22 100644
--- a/clang/test/Analysis/builtin_bitcast.cpp
+++ b/clang/test/Analysis/builtin_bitcast.cpp
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
+void gh_69922(ptr_size p) {
+ // expected-warning-re@+1 {{(reg_${{[0-9]+}}) & 1U}}
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1));
+
+ __builtin_bit_cast(A*, p & 1)->set(2); // no-crash
+ // However, since the `this` pointer is expected to be a Loc, but we have
+ // NonLoc there, we simply give up and resolve it as `Unknown`.
+ // Then, inside the `set()` member function call we can't evaluate the
+ // store to the member variable `n`.
+
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1)->n); // Ideally, this
should print "2".
+ // expected-warning-re@-1 {{(reg_${{[0-9]+}}) & 1U}}
+}
``
https://github.com/llvm/llvm-project/pull/70837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal (PR #70837)
https://github.com/steakhal created
https://github.com/llvm/llvm-project/pull/70837
Workaround the case when the `this` pointer is actually a `NonLoc`, by
returning `Unknown` instead.
The solution isn't ideal, as `this` should be really a `Loc`, but due to how
casts work, I feel this is our easiest and best option.
I've considered using `SVB.evalCast(ThisVal, Base->getType(), QualType())`, but
it doesn't work as `EvalCastVisitor::VisitNonLocSymbolVal()` only evaluates
casts that happen from NonLoc to NonLocs.
When I tried to actually implement that case, I figured:
1) Create a SymbolicRegion from that nonloc::SymbolVal; but SymbolRegion ctor
expects a pointer type for the symbol.
2) Okay, just have a SymbolCast, getting us the pointer type; but SymbolRegion
expects SymbolData symbols, not generic SymExprs, as stated:
> // Because pointer arithmetic is represented by ElementRegion layers,
> // the base symbol here should not contain any arithmetic.
3) We can't use ElementRegions to perform this cast because to have an
ElementRegion, you already have to have a SubRegion that you want to cast, but
the point is that we don't have that.
At this point, I gave up, and just returned `Unknown` xD IMO this is still
better than having a crash.
Fixes #69922
>From a7f64815f4986fad597b9cb2d1acce2de9ac20bf Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 23 Oct 2023 18:10:29 +0200
Subject: [PATCH] [analyzer] Fix assertion failure in
CXXInstanceCall::getCXXThisVal
Workaround the case when the `this` pointer is actually a `NonLoc`, by
returning `Unknown` instead.
The solution isn't ideal, as `this` should be really a `Loc`, but due to
how casts work, I feel this is our easiest and best option.
I've considered using `SVB.evalCast(ThisVal, Base->getType(), QualType())`,
but it doesn't work as `EvalCastVisitor::VisitNonLocSymbolVal()` only
evaluates casts that happen from NonLoc to NonLocs.
When I tried to actually implement that case, I figured:
1) Create a SymbolicRegion from that nonloc::SymbolVal; but SymbolRegion
ctor expects a pointer type for the symbol.
2) Okay, just have a SymbolCast, getting us the pointer type; but
SymbolRegion expects SymbolData symbols, not generic SymExprs, as stated:
> // Because pointer arithmetic is represented by ElementRegion layers,
> // the base symbol here should not contain any arithmetic.
3) We can't use ElementRegions to perform this cast because to have an
ElementRegion, you already have to have a SubRegion that you want to
cast, but the point is that we don't have that.
At this point, I gave up, and just returned `Unknown` xD
IMO this is still better than having a crash.
Fixes #69922
---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 5 ++---
clang/test/Analysis/builtin_bitcast.cpp | 21 +
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index ad5bb66c4fff3c8..20bc64dc2631cec 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -715,10 +715,9 @@ void CXXInstanceCall::getExtraInvalidatedValues(
SVal CXXInstanceCall::getCXXThisVal() const {
const Expr *Base = getCXXThisExpr();
// FIXME: This doesn't handle an overloaded ->* operator.
- if (!Base)
+ SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
+ if (isa(ThisVal))
return UnknownVal();
-
- SVal ThisVal = getSVal(Base);
assert(ThisVal.isUnknownOrUndef() || isa(ThisVal));
return ThisVal;
}
diff --git a/clang/test/Analysis/builtin_bitcast.cpp
b/clang/test/Analysis/builtin_bitcast.cpp
index 396e7caa45f6acd..13475694d287a22 100644
--- a/clang/test/Analysis/builtin_bitcast.cpp
+++ b/clang/test/Analysis/builtin_bitcast.cpp
@@ -30,3 +30,24 @@ void test(int i) {
clang_analyzer_dump(g4);
// expected-warning@-1 {{ [as 64 bit integer]}}
}
+
+struct A {
+ int n;
+ void set(int x) {
+n = x;
+ }
+};
+using ptr_size = decltype(sizeof(void *));
+void gh_69922(ptr_size p) {
+ // expected-warning-re@+1 {{(reg_${{[0-9]+}}) & 1U}}
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1));
+
+ __builtin_bit_cast(A*, p & 1)->set(2); // no-crash
+ // However, since the `this` pointer is expected to be a Loc, but we have
+ // NonLoc there, we simply give up and resolve it as `Unknown`.
+ // Then, inside the `set()` member function call we can't evaluate the
+ // store to the member variable `n`.
+
+ clang_analyzer_dump(__builtin_bit_cast(A*, p & 1)->n); // Ideally, this
should print "2".
+ // expected-warning-re@-1 {{(reg_${{[0-9]+}}) & 1U}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
