[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
This revision was automatically updated to reflect the committed changes.
Closed by commit rG827ba67e3833: [Sema] Validate calls to GetExprRange.
(authored by Mordante).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85601/new/
https://reviews.llvm.org/D85601
Files:
clang/lib/Sema/SemaChecking.cpp
clang/test/SemaCXX/conditional-expr.cpp
Index: clang/test/SemaCXX/conditional-expr.cpp
===
--- clang/test/SemaCXX/conditional-expr.cpp
+++ clang/test/SemaCXX/conditional-expr.cpp
@@ -409,3 +409,20 @@
D d = b ? D{B()} : D{C()};
}
}
+
+namespace PR46484 {
+// expected-error@+4{{expected ':'}}
+// expected-note@+3{{to match this '?'}}
+// expected-warning@+2{{variable 'b' is uninitialized}}
+// expected-error@+1 2 {{expected ';' after top level declarator}}
+int a long b = a = b ? throw 0 1
+
+void g() {
+ extern int a;
+ extern long b;
+ long c = a = b ? throw 0 : 1;
+ long d = a = b ? 1 : throw 0;
+ // expected-error@+1 {{assigning to 'int' from incompatible type 'void'}}
+ long e = a = b ? throw 0 : throw 1;
+}
+} // namespace PR46484
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10368,10 +10368,16 @@
MaxWidth, InConstantContext);
// Otherwise, conservatively merge.
-IntRange L =
-GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext);
-IntRange R =
-GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext);
+// GetExprRange requires an integer expression, but a throw expression
+// results in a void type.
+Expr *E = CO->getTrueExpr();
+IntRange L = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
+E = CO->getFalseExpr();
+IntRange R = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
return IntRange::join(L, R);
}
Index: clang/test/SemaCXX/conditional-expr.cpp
===
--- clang/test/SemaCXX/conditional-expr.cpp
+++ clang/test/SemaCXX/conditional-expr.cpp
@@ -409,3 +409,20 @@
D d = b ? D{B()} : D{C()};
}
}
+
+namespace PR46484 {
+// expected-error@+4{{expected ':'}}
+// expected-note@+3{{to match this '?'}}
+// expected-warning@+2{{variable 'b' is uninitialized}}
+// expected-error@+1 2 {{expected ';' after top level declarator}}
+int a long b = a = b ? throw 0 1
+
+void g() {
+ extern int a;
+ extern long b;
+ long c = a = b ? throw 0 : 1;
+ long d = a = b ? 1 : throw 0;
+ // expected-error@+1 {{assigning to 'int' from incompatible type 'void'}}
+ long e = a = b ? throw 0 : throw 1;
+}
+} // namespace PR46484
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10368,10 +10368,16 @@
MaxWidth, InConstantContext);
// Otherwise, conservatively merge.
-IntRange L =
-GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext);
-IntRange R =
-GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext);
+// GetExprRange requires an integer expression, but a throw expression
+// results in a void type.
+Expr *E = CO->getTrueExpr();
+IntRange L = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
+E = CO->getFalseExpr();
+IntRange R = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
return IntRange::join(L, R);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85601/new/ https://reviews.llvm.org/D85601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
Mordante updated this revision to Diff 285704.
Mordante marked an inline comment as done.
Mordante added a comment.
Addresses review comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85601/new/
https://reviews.llvm.org/D85601
Files:
clang/lib/Sema/SemaChecking.cpp
clang/test/SemaCXX/conditional-expr.cpp
Index: clang/test/SemaCXX/conditional-expr.cpp
===
--- clang/test/SemaCXX/conditional-expr.cpp
+++ clang/test/SemaCXX/conditional-expr.cpp
@@ -409,3 +409,20 @@
D d = b ? D{B()} : D{C()};
}
}
+
+namespace PR46484 {
+// expected-error@+4{{expected ':'}}
+// expected-note@+3{{to match this '?'}}
+// expected-warning@+2{{variable 'b' is uninitialized}}
+// expected-error@+1 2 {{expected ';' after top level declarator}}
+int a long b = a = b ? throw 0 1
+
+void g() {
+ extern int a;
+ extern long b;
+ long c = a = b ? throw 0 : 1;
+ long d = a = b ? 1 : throw 0;
+ // expected-error@+1 {{assigning to 'int' from incompatible type 'void'}}
+ long e = a = b ? throw 0 : throw 1;
+}
+} // namespace PR46484
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10308,10 +10308,16 @@
MaxWidth, InConstantContext);
// Otherwise, conservatively merge.
-IntRange L =
-GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext);
-IntRange R =
-GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext);
+// GetExprRange requires an integer expression, but a throw expression
+// results in a void type.
+Expr *E = CO->getTrueExpr();
+IntRange L = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
+E = CO->getFalseExpr();
+IntRange R = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
return IntRange::join(L, R);
}
Index: clang/test/SemaCXX/conditional-expr.cpp
===
--- clang/test/SemaCXX/conditional-expr.cpp
+++ clang/test/SemaCXX/conditional-expr.cpp
@@ -409,3 +409,20 @@
D d = b ? D{B()} : D{C()};
}
}
+
+namespace PR46484 {
+// expected-error@+4{{expected ':'}}
+// expected-note@+3{{to match this '?'}}
+// expected-warning@+2{{variable 'b' is uninitialized}}
+// expected-error@+1 2 {{expected ';' after top level declarator}}
+int a long b = a = b ? throw 0 1
+
+void g() {
+ extern int a;
+ extern long b;
+ long c = a = b ? throw 0 : 1;
+ long d = a = b ? 1 : throw 0;
+ // expected-error@+1 {{assigning to 'int' from incompatible type 'void'}}
+ long e = a = b ? throw 0 : throw 1;
+}
+} // namespace PR46484
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10308,10 +10308,16 @@
MaxWidth, InConstantContext);
// Otherwise, conservatively merge.
-IntRange L =
-GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext);
-IntRange R =
-GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext);
+// GetExprRange requires an integer expression, but a throw expression
+// results in a void type.
+Expr *E = CO->getTrueExpr();
+IntRange L = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
+E = CO->getFalseExpr();
+IntRange R = E->getType()->isVoidType()
+ ? IntRange{0, true}
+ : GetExprRange(C, E, MaxWidth, InConstantContext);
return IntRange::join(L, R);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
Mordante marked 3 inline comments as done. Mordante added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:10317-10320 IntRange L = GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext); IntRange R = GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext); rsmith wrote: > It seems to me that the bug is here. `GetExprRange` is documented as > "Pseudo-evaluate the given **integer** expression", but the true and false > expressions here might not be integer expressions -- specifically, one of > them could be of type `void` if it's a //throw-expression//. In that case, we > should only call `GetExprRange` on the other expression. The expression range > of the `void`-typed expression is irrelevant in this case, because that > expression never returns. Thanks for the hint I'll look into this solution. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85601/new/ https://reviews.llvm.org/D85601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
rsmith added inline comments.
Comment at: clang/lib/Sema/SemaChecking.cpp:10164
const BuiltinType *BT = cast(T);
-assert(BT->isInteger());
+if (!BT->isInteger()) {
+ // This can happen in a conditional expression with a throw statement
Mordante wrote:
> rsmith wrote:
> > Can we handle this in code that's specific to conditional expressions
> > instead? Presumably somewhere higher up in the call graph, some code is
> > assuming that it can recurse from a conditional expression to its
> > subexpressions, and that assumption is wrong.
> I can take a look at it if you want. However I feel this fix is better. If
> the conditional doesn't throw it can properly evaluate the required IntRange.
> If it throws the range doesn't matter, therefore I didn't want to increment
> the required range.
> Do you agree?
> Should I add more comment to clarify the design decission?
In order to get here, we need to have called functions that are documented as
taking only integer types but passing them a `void` type. So the bug has
already occurred before we got here.
Comment at: clang/lib/Sema/SemaChecking.cpp:10317-10320
IntRange L =
GetExprRange(C, CO->getTrueExpr(), MaxWidth, InConstantContext);
IntRange R =
GetExprRange(C, CO->getFalseExpr(), MaxWidth, InConstantContext);
It seems to me that the bug is here. `GetExprRange` is documented as
"Pseudo-evaluate the given **integer** expression", but the true and false
expressions here might not be integer expressions -- specifically, one of them
could be of type `void` if it's a //throw-expression//. In that case, we should
only call `GetExprRange` on the other expression. The expression range of the
`void`-typed expression is irrelevant in this case, because that expression
never returns.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85601/new/
https://reviews.llvm.org/D85601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
Mordante marked an inline comment as done.
Mordante added a comment.
I added `void g()` since that's valid code which also caused an assertion
failure. So the issue isn't in the error recovery but in determining the
required IntRange. It seems the code doesn't take
http://eel.is/c++draft/expr.cond#2.1 into account.
Comment at: clang/lib/Sema/SemaChecking.cpp:10164
const BuiltinType *BT = cast(T);
-assert(BT->isInteger());
+if (!BT->isInteger()) {
+ // This can happen in a conditional expression with a throw statement
rsmith wrote:
> Can we handle this in code that's specific to conditional expressions
> instead? Presumably somewhere higher up in the call graph, some code is
> assuming that it can recurse from a conditional expression to its
> subexpressions, and that assumption is wrong.
I can take a look at it if you want. However I feel this fix is better. If the
conditional doesn't throw it can properly evaluate the required IntRange. If it
throws the range doesn't matter, therefore I didn't want to increment the
required range.
Do you agree?
Should I add more comment to clarify the design decission?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85601/new/
https://reviews.llvm.org/D85601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
rsmith added a comment.
Did this only crash during error recovery before, or also for your `void g()`
example? If we were only crashing in error recovery, that would suggest that
error recovery was producing a bad AST, and perhaps we should be fixing this
elsewhere.
Comment at: clang/lib/Sema/SemaChecking.cpp:10164
const BuiltinType *BT = cast(T);
-assert(BT->isInteger());
+if (!BT->isInteger()) {
+ // This can happen in a conditional expression with a throw statement
Can we handle this in code that's specific to conditional expressions instead?
Presumably somewhere higher up in the call graph, some code is assuming that it
can recurse from a conditional expression to its subexpressions, and that
assumption is wrong.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85601/new/
https://reviews.llvm.org/D85601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D85601: Fixes an assertion when IntRange processes a throw expression
Mordante created this revision.
Mordante added reviewers: rsmith, rjmccall.
Mordante added a project: clang.
Mordante requested review of this revision.
Fixes PR46484: Clang crash in clang/lib/Sema/SemaChecking.cpp:10028
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D85601
Files:
clang/lib/Sema/SemaChecking.cpp
clang/test/SemaCXX/conditional-expr.cpp
Index: clang/test/SemaCXX/conditional-expr.cpp
===
--- clang/test/SemaCXX/conditional-expr.cpp
+++ clang/test/SemaCXX/conditional-expr.cpp
@@ -409,3 +409,17 @@
D d = b ? D{B()} : D{C()};
}
}
+
+namespace PR46484 {
+// expected-error@+4{{expected ':'}}
+// expected-note@+3{{to match this '?'}}
+// expected-warning@+2{{variable 'b' is uninitialized}}
+// expected-error@+1 2 {{expected ';' after top level declarator}}
+int a long b = a = b ? throw 0 1
+
+void g() {
+ extern int a;
+ extern long b;
+ long c = a = b ? throw 0 : 1;
+}
+} // namespace PR46484
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10161,7 +10161,13 @@
return IntRange(EIT->getNumBits(), EIT->isUnsigned());
const BuiltinType *BT = cast(T);
-assert(BT->isInteger());
+if (!BT->isInteger()) {
+ // This can happen in a conditional expression with a throw statement
+ // C++11 [expr.cond]p2
+ // If either the second or the third operand has type (cv) void, ...
+ assert(BT->isVoidType());
+ IntRange(1, true /*NonNegative*/);
+}
return IntRange(C.getIntWidth(QualType(T, 0)), BT->isUnsignedInteger());
}
Index: clang/test/SemaCXX/conditional-expr.cpp
===
--- clang/test/SemaCXX/conditional-expr.cpp
+++ clang/test/SemaCXX/conditional-expr.cpp
@@ -409,3 +409,17 @@
D d = b ? D{B()} : D{C()};
}
}
+
+namespace PR46484 {
+// expected-error@+4{{expected ':'}}
+// expected-note@+3{{to match this '?'}}
+// expected-warning@+2{{variable 'b' is uninitialized}}
+// expected-error@+1 2 {{expected ';' after top level declarator}}
+int a long b = a = b ? throw 0 1
+
+void g() {
+ extern int a;
+ extern long b;
+ long c = a = b ? throw 0 : 1;
+}
+} // namespace PR46484
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10161,7 +10161,13 @@
return IntRange(EIT->getNumBits(), EIT->isUnsigned());
const BuiltinType *BT = cast(T);
-assert(BT->isInteger());
+if (!BT->isInteger()) {
+ // This can happen in a conditional expression with a throw statement
+ // C++11 [expr.cond]p2
+ // If either the second or the third operand has type (cv) void, ...
+ assert(BT->isVoidType());
+ IntRange(1, true /*NonNegative*/);
+}
return IntRange(C.getIntWidth(QualType(T, 0)), BT->isUnsignedInteger());
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
