Re: [PATCH] c++, v2: Fix ICE with __builtin_bit_cast [PR98469]
On 1/5/21 10:26 AM, Jakub Jelinek wrote:
On Mon, Jan 04, 2021 at 04:01:25PM -0500, Jason Merrill via Gcc-patches wrote:
On 1/4/21 3:48 PM, Jakub Jelinek wrote:
On Mon, Jan 04, 2021 at 03:44:46PM -0500, Jason Merrill wrote:
This change is OK, but part of the problem is that we're trying to do
overload resolution for an S copy/move constructor, which we shouldn't be
because bit_cast is a prvalue, so in C++17 and up we should use it to
directly initialize the target without any implied constructor call.
It seems we're mishandling this because the code in
build_special_member_call specifically looks for TARGET_EXPR or CONSTRUCTOR,
and BIT_CAST_EXPR is neither of those.
Wrapping a BIT_CAST_EXPR of aggregate type in a TARGET_EXPR would address
this, and any other places that expect a class prvalue to come in the form
of a TARGET_EXPR.
I can try that tomorrow. Won't that cause copying through extra temporary
in some cases though, or is that guaranteed to be optimized?
It won't cause any extra copying when it's used to initialize another object
(like the return value of std::bit_cast). Class prvalues are always
expressed with a TARGET_EXPR in the front end; the TARGET_EXPR melts away
when used as an initializer, it only creates a temporary when it's used in
another way.
Ok, this version wraps it into a TARGET_EXPR then, it alone fixes the bug,
but I've kept the constexpr.c change too.
This patch corrects this and one other place to not be as dependent on
TARGET_EXPR, but I think I'm going to save it for stage 1.
Jason
commit 0d732b8c7fb3f8378dc1c894358bb5d766e6be5d
Author: Jason Merrill
Date: Mon Jan 4 16:11:08 2021 -0500
c++: Tweak prvalue test [PR98469]
Discussing the 98469 patch and class prvalues with Jakub also inspired me to
change the place that was mishandling BIT_CAST_EXPR and one other to use the
lvalue_kind machinery to decide whether something is a prvalue, instead of
looking specifically for a TARGET_EXPR.
gcc/cp/ChangeLog:
* call.c (build_special_member_call): Use !glvalue_p rather
than specific tree codes to test for prvalue.
(conv_is_prvalue): Likewise.
(implicit_conversion): Check CLASS_TYPE_P first.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 218157088ef..e2d2b23e449 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -2118,8 +2118,8 @@ implicit_conversion (tree to, tree from, tree expr, bool c_cast_p,
flags, complain);
if (!conv || conv->bad_p)
return conv;
- if (conv_is_prvalue (conv)
- && CLASS_TYPE_P (conv->type)
+ if (CLASS_TYPE_P (conv->type)
+ && conv_is_prvalue (conv)
&& CLASSTYPE_PURE_VIRTUALS (conv->type))
conv->bad_p = true;
return conv;
@@ -8500,8 +8500,7 @@ conv_is_prvalue (conversion *c)
return true;
if (c->kind == ck_user && !TYPE_REF_P (c->type))
return true;
- if (c->kind == ck_identity && c->u.expr
- && TREE_CODE (c->u.expr) == TARGET_EXPR)
+ if (c->kind == ck_identity && c->u.expr && !glvalue_p (c->u.expr))
return true;
return false;
@@ -9950,8 +9949,7 @@ build_special_member_call (tree instance, tree name, vec **args,
&& CONSTRUCTOR_NELTS (arg) == 1)
arg = CONSTRUCTOR_ELT (arg, 0)->value;
- if ((TREE_CODE (arg) == TARGET_EXPR
- || TREE_CODE (arg) == CONSTRUCTOR)
+ if (!glvalue_p (arg)
&& (same_type_ignoring_top_level_qualifiers_p
(class_type, TREE_TYPE (arg
{
Re: [PATCH] c++, v2: Fix ICE with __builtin_bit_cast [PR98469]
On 1/5/21 10:26 AM, Jakub Jelinek wrote:
On Mon, Jan 04, 2021 at 04:01:25PM -0500, Jason Merrill via Gcc-patches wrote:
On 1/4/21 3:48 PM, Jakub Jelinek wrote:
On Mon, Jan 04, 2021 at 03:44:46PM -0500, Jason Merrill wrote:
This change is OK, but part of the problem is that we're trying to do
overload resolution for an S copy/move constructor, which we shouldn't be
because bit_cast is a prvalue, so in C++17 and up we should use it to
directly initialize the target without any implied constructor call.
It seems we're mishandling this because the code in
build_special_member_call specifically looks for TARGET_EXPR or CONSTRUCTOR,
and BIT_CAST_EXPR is neither of those.
Wrapping a BIT_CAST_EXPR of aggregate type in a TARGET_EXPR would address
this, and any other places that expect a class prvalue to come in the form
of a TARGET_EXPR.
I can try that tomorrow. Won't that cause copying through extra temporary
in some cases though, or is that guaranteed to be optimized?
It won't cause any extra copying when it's used to initialize another object
(like the return value of std::bit_cast). Class prvalues are always
expressed with a TARGET_EXPR in the front end; the TARGET_EXPR melts away
when used as an initializer, it only creates a temporary when it's used in
another way.
Ok, this version wraps it into a TARGET_EXPR then, it alone fixes the bug,
but I've kept the constexpr.c change too.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
2021-01-05 Jakub Jelinek
PR c++/98469
* constexpr.c (cxx_eval_constant_expression) :
Punt if lval is true.
* semantics.c (cp_build_bit_cast): Call get_target_expr_sfinae on
the result if it has a class type.
* g++.dg/cpp2a/bit-cast8.C: New test.
* g++.dg/cpp2a/bit-cast9.C: New test.
--- gcc/cp/constexpr.c.jj 2021-01-04 10:25:48.750121531 +0100
+++ gcc/cp/constexpr.c 2021-01-05 11:41:38.315032636 +0100
@@ -6900,6 +6900,15 @@ cxx_eval_constant_expression (const cons
return t;
case BIT_CAST_EXPR:
+ if (lval)
+ {
+ if (!ctx->quiet)
+ error_at (EXPR_LOCATION (t),
+ "address of a call to %qs is not a constant expression",
+ "__builtin_bit_cast");
+ *non_constant_p = true;
+ return t;
+ }
r = cxx_eval_bit_cast (ctx, t, non_constant_p, overflow_p);
break;
--- gcc/cp/semantics.c.jj 2021-01-04 10:25:48.489124486 +0100
+++ gcc/cp/semantics.c 2021-01-05 11:27:49.327372582 +0100
@@ -10761,6 +10761,10 @@ cp_build_bit_cast (location_t loc, tree
tree ret = build_min (BIT_CAST_EXPR, type, arg);
SET_EXPR_LOCATION (ret, loc);
+
+ if (!processing_template_decl && CLASS_TYPE_P (type))
+ret = get_target_expr_sfinae (ret, complain);
+
return ret;
}
--- gcc/testsuite/g++.dg/cpp2a/bit-cast8.C.jj 2021-01-05 11:41:38.315032636 +0100
+++ gcc/testsuite/g++.dg/cpp2a/bit-cast8.C 2021-01-05 11:41:38.315032636
+0100
@@ -0,0 +1,11 @@
+// PR c++/98469
+// { dg-do compile { target c++20 } }
+// { dg-options "-Wall" }
+
+struct S { int s; };
+
+S
+foo ()
+{
+ return __builtin_bit_cast (S, 0);
+}
--- gcc/testsuite/g++.dg/cpp2a/bit-cast9.C.jj 2021-01-05 11:41:38.315032636
+0100
+++ gcc/testsuite/g++.dg/cpp2a/bit-cast9.C 2021-01-05 11:41:38.315032636
+0100
@@ -0,0 +1,15 @@
+// PR c++/98469
+// { dg-do compile { target c++20 } }
+// { dg-options "-Wall" }
+
+template
+constexpr T
+bit_cast (const F ) noexcept
+{
+ return __builtin_bit_cast (T, f);
+}
+struct S { int s; };
+constexpr int foo (const S ) { return x.s; }
+constexpr int bar () { return foo (bit_cast (0)); }
+constexpr int x = bar ();
+static_assert (!x);
Jakub
[PATCH] c++, v2: Fix ICE with __builtin_bit_cast [PR98469]
On Mon, Jan 04, 2021 at 04:01:25PM -0500, Jason Merrill via Gcc-patches wrote:
> On 1/4/21 3:48 PM, Jakub Jelinek wrote:
> > On Mon, Jan 04, 2021 at 03:44:46PM -0500, Jason Merrill wrote:
> > > This change is OK, but part of the problem is that we're trying to do
> > > overload resolution for an S copy/move constructor, which we shouldn't be
> > > because bit_cast is a prvalue, so in C++17 and up we should use it to
> > > directly initialize the target without any implied constructor call.
> > >
> > > It seems we're mishandling this because the code in
> > > build_special_member_call specifically looks for TARGET_EXPR or
> > > CONSTRUCTOR,
> > > and BIT_CAST_EXPR is neither of those.
> > >
> > > Wrapping a BIT_CAST_EXPR of aggregate type in a TARGET_EXPR would address
> > > this, and any other places that expect a class prvalue to come in the form
> > > of a TARGET_EXPR.
> >
> > I can try that tomorrow. Won't that cause copying through extra temporary
> > in some cases though, or is that guaranteed to be optimized?
>
> It won't cause any extra copying when it's used to initialize another object
> (like the return value of std::bit_cast). Class prvalues are always
> expressed with a TARGET_EXPR in the front end; the TARGET_EXPR melts away
> when used as an initializer, it only creates a temporary when it's used in
> another way.
Ok, this version wraps it into a TARGET_EXPR then, it alone fixes the bug,
but I've kept the constexpr.c change too.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2021-01-05 Jakub Jelinek
PR c++/98469
* constexpr.c (cxx_eval_constant_expression) :
Punt if lval is true.
* semantics.c (cp_build_bit_cast): Call get_target_expr_sfinae on
the result if it has a class type.
* g++.dg/cpp2a/bit-cast8.C: New test.
* g++.dg/cpp2a/bit-cast9.C: New test.
--- gcc/cp/constexpr.c.jj 2021-01-04 10:25:48.750121531 +0100
+++ gcc/cp/constexpr.c 2021-01-05 11:41:38.315032636 +0100
@@ -6900,6 +6900,15 @@ cxx_eval_constant_expression (const cons
return t;
case BIT_CAST_EXPR:
+ if (lval)
+ {
+ if (!ctx->quiet)
+ error_at (EXPR_LOCATION (t),
+ "address of a call to %qs is not a constant expression",
+ "__builtin_bit_cast");
+ *non_constant_p = true;
+ return t;
+ }
r = cxx_eval_bit_cast (ctx, t, non_constant_p, overflow_p);
break;
--- gcc/cp/semantics.c.jj 2021-01-04 10:25:48.489124486 +0100
+++ gcc/cp/semantics.c 2021-01-05 11:27:49.327372582 +0100
@@ -10761,6 +10761,10 @@ cp_build_bit_cast (location_t loc, tree
tree ret = build_min (BIT_CAST_EXPR, type, arg);
SET_EXPR_LOCATION (ret, loc);
+
+ if (!processing_template_decl && CLASS_TYPE_P (type))
+ret = get_target_expr_sfinae (ret, complain);
+
return ret;
}
--- gcc/testsuite/g++.dg/cpp2a/bit-cast8.C.jj 2021-01-05 11:41:38.315032636
+0100
+++ gcc/testsuite/g++.dg/cpp2a/bit-cast8.C 2021-01-05 11:41:38.315032636
+0100
@@ -0,0 +1,11 @@
+// PR c++/98469
+// { dg-do compile { target c++20 } }
+// { dg-options "-Wall" }
+
+struct S { int s; };
+
+S
+foo ()
+{
+ return __builtin_bit_cast (S, 0);
+}
--- gcc/testsuite/g++.dg/cpp2a/bit-cast9.C.jj 2021-01-05 11:41:38.315032636
+0100
+++ gcc/testsuite/g++.dg/cpp2a/bit-cast9.C 2021-01-05 11:41:38.315032636
+0100
@@ -0,0 +1,15 @@
+// PR c++/98469
+// { dg-do compile { target c++20 } }
+// { dg-options "-Wall" }
+
+template
+constexpr T
+bit_cast (const F ) noexcept
+{
+ return __builtin_bit_cast (T, f);
+}
+struct S { int s; };
+constexpr int foo (const S ) { return x.s; }
+constexpr int bar () { return foo (bit_cast (0)); }
+constexpr int x = bar ();
+static_assert (!x);
Jakub
