| Message ID | 20190507204318.15031-1-jason@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR c++/86485 - -Wmaybe-unused with empty class ?: | expand |
On Tue, May 7, 2019 at 4:43 PM Jason Merrill <jason@redhat.com> wrote: > > * typeck.c (build_static_cast_1): Use cp_build_addr_expr. > > For GCC 9 I fixed this bug with a patch to gimplify_cond_expr, but this > function was also doing the wrong thing. > > Using build_address does not push the ADDR_EXPR down into the arms of a > COND_EXPR, which we need for proper handling of conversion of an lvalue ?: > to another reference type. Yet another tweak that would have fixed this bug: we should treat INIT_EXPR and MODIFY_EXPR differently for determining whether this is a simple empty class copy, since a TARGET_EXPR on the RHS is direct initialization if INIT_EXPR but copy if MODIFY_EXPR. * cp-gimplify.c (simple_empty_class_p): Also true for MODIFY_EXPR. commit 4ee9e054cba31bd532061bd357477e757b5b284a Author: Jason Merrill <jason@redhat.com> Date: Sat Mar 2 01:07:41 2019 -0500 PR c++/86485 - simple_empty_class_p Yet another tweak that would have fixed this bug: we should treat INIT_EXPR and MODIFY_EXPR differently for determining whether this is a simple empty class copy, since a TARGET_EXPR on the RHS is direct initialization if INIT_EXPR but copy if MODIFY_EXPR. * cp-gimplify.c (simple_empty_class_p): Also true for MODIFY_EXPR. diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index e8c22c071c2..30937b1a1a3 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -594,19 +594,20 @@ gimplify_must_not_throw_expr (tree *expr_p, gimple_seq *pre_p) return slot optimization alone because it isn't a copy. */ static bool -simple_empty_class_p (tree type, tree op) +simple_empty_class_p (tree type, tree op, tree_code code) { + if (TREE_CODE (op) == COMPOUND_EXPR) + return simple_empty_class_p (type, TREE_OPERAND (op, 1), code); return - ((TREE_CODE (op) == COMPOUND_EXPR - && simple_empty_class_p (type, TREE_OPERAND (op, 1))) - || TREE_CODE (op) == EMPTY_CLASS_EXPR + (TREE_CODE (op) == EMPTY_CLASS_EXPR + || code == MODIFY_EXPR || is_gimple_lvalue (op) || INDIRECT_REF_P (op) || (TREE_CODE (op) == CONSTRUCTOR - && CONSTRUCTOR_NELTS (op) == 0 - && !TREE_CLOBBER_P (op)) + && CONSTRUCTOR_NELTS (op) == 0) || (TREE_CODE (op) == CALL_EXPR && !CALL_EXPR_RETURN_SLOT_OPT (op))) + && !TREE_CLOBBER_P (op) && is_really_empty_class (type, /*ignore_vptr*/true); } @@ -715,7 +716,7 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p) TREE_OPERAND (*expr_p, 1) = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (op0), op1); - else if (simple_empty_class_p (TREE_TYPE (op0), op1)) + else if (simple_empty_class_p (TREE_TYPE (op0), op1, code)) { /* Remove any copies of empty classes. Also drop volatile variables on the RHS to avoid infinite recursion from
On Tue, May 7, 2019 at 4:43 PM Jason Merrill <jason@redhat.com> wrote: > > * typeck.c (build_static_cast_1): Use cp_build_addr_expr. > > For GCC 9 I fixed this bug with a patch to gimplify_cond_expr, but this > function was also doing the wrong thing. > > Using build_address does not push the ADDR_EXPR down into the arms of a > COND_EXPR, which we need for proper handling of conversion of an lvalue ?: > to another reference type. And that allows the gimplifier to assert that we should never see a COND_EXPR of addressable type. Tested x86_64-pc-linux-gnu, applying to trunk. commit c4f1e37204aaea7efb2aa7dc234d5c8ebeba1089 Author: Jason Merrill <jason@redhat.com> Date: Mon Mar 4 14:09:57 2019 -0500 * gimplify.c (gimplify_cond_expr): Don't check TREE_ADDRESSABLE. The front end shouldn't produce a GENERIC COND_EXPR of TREE_ADDRESSABLE type. diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 5bacb255ba7..6905165ad33 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -3990,10 +3990,12 @@ gimplify_cond_expr (tree *expr_p, gimple_seq *pre_p, fallback_t fallback) tree result; /* If either an rvalue is ok or we do not require an lvalue, create the - temporary. But we cannot do that if the type is addressable. */ + temporary. We cannot do that if the type is addressable, but + that should have been avoided before we got here. */ if (((fallback & fb_rvalue) || !(fallback & fb_lvalue)) - && !TREE_ADDRESSABLE (type)) + && (flag_checking || !TREE_ADDRESSABLE (type))) { + gcc_assert (!TREE_ADDRESSABLE (type)); if (gimplify_ctxp->allow_rhs_cond_expr /* If either branch has side effects or could trap, it can't be evaluated unconditionally. */
On Wed, May 22, 2019 at 05:59:56PM -0400, Jason Merrill wrote: > On Tue, May 7, 2019 at 4:43 PM Jason Merrill <jason@redhat.com> wrote: > > > > * typeck.c (build_static_cast_1): Use cp_build_addr_expr. > > > > For GCC 9 I fixed this bug with a patch to gimplify_cond_expr, but this > > function was also doing the wrong thing. > > > > Using build_address does not push the ADDR_EXPR down into the arms of a > > COND_EXPR, which we need for proper handling of conversion of an lvalue ?: > > to another reference type. > > And that allows the gimplifier to assert that we should never see a > COND_EXPR of addressable type. > > Tested x86_64-pc-linux-gnu, applying to trunk. > commit c4f1e37204aaea7efb2aa7dc234d5c8ebeba1089 > Author: Jason Merrill <jason@redhat.com> > Date: Mon Mar 4 14:09:57 2019 -0500 > > * gimplify.c (gimplify_cond_expr): Don't check TREE_ADDRESSABLE. > > The front end shouldn't produce a GENERIC COND_EXPR of TREE_ADDRESSABLE > type. I think this broke a lot in the D testsuite: +FAIL: gdc.test/runnable/link15017.d (internal compiler error) +UNRESOLVED: gdc.test/runnable/link15017.d compilation failed to produce executable +FAIL: gdc.test/runnable/sdtor.d -O2 (internal compiler error) +UNRESOLVED: gdc.test/runnable/sdtor.d -O2 compilation failed to produce executable +FAIL: gdc.test/runnable/testaa.d (internal compiler error) +UNRESOLVED: gdc.test/runnable/testaa.d compilation failed to produce executable +FAIL: libphobos.druntime/rt/lifetime.d (test for excess errors) +UNRESOLVED: libphobos.druntime/rt/lifetime.d compilation failed to produce executable +FAIL: libphobos.druntime_shared/rt/lifetime.d (test for excess errors) +UNRESOLVED: libphobos.druntime_shared/rt/lifetime.d compilation failed to produce executable +FAIL: libphobos.phobos/std/uni.d (test for excess errors) +UNRESOLVED: libphobos.phobos/std/uni.d compilation failed to produce executable +FAIL: libphobos.phobos_shared/std/uni.d (test for excess errors) +UNRESOLVED: libphobos.phobos_shared/std/uni.d compilation failed to produce executable (the list is reduced, so that for each failing tests there aren't many lines). Iain, is that something that can be fixed on the D FE side? > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -3990,10 +3990,12 @@ gimplify_cond_expr (tree *expr_p, gimple_seq *pre_p, fallback_t fallback) > tree result; > > /* If either an rvalue is ok or we do not require an lvalue, create the > - temporary. But we cannot do that if the type is addressable. */ > + temporary. We cannot do that if the type is addressable, but > + that should have been avoided before we got here. */ > if (((fallback & fb_rvalue) || !(fallback & fb_lvalue)) > - && !TREE_ADDRESSABLE (type)) > + && (flag_checking || !TREE_ADDRESSABLE (type))) > { > + gcc_assert (!TREE_ADDRESSABLE (type)); > if (gimplify_ctxp->allow_rhs_cond_expr > /* If either branch has side effects or could trap, it can't be > evaluated unconditionally. */ Jakub
On Fri, May 24, 2019 at 10:13 AM Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, May 22, 2019 at 05:59:56PM -0400, Jason Merrill wrote: > > On Tue, May 7, 2019 at 4:43 PM Jason Merrill <jason@redhat.com> wrote: > > > > > > * typeck.c (build_static_cast_1): Use cp_build_addr_expr. > > > > > > For GCC 9 I fixed this bug with a patch to gimplify_cond_expr, but this > > > function was also doing the wrong thing. > > > > > > Using build_address does not push the ADDR_EXPR down into the arms of a > > > COND_EXPR, which we need for proper handling of conversion of an lvalue ?: > > > to another reference type. > > > > And that allows the gimplifier to assert that we should never see a > > COND_EXPR of addressable type. > > > > Tested x86_64-pc-linux-gnu, applying to trunk. > > > commit c4f1e37204aaea7efb2aa7dc234d5c8ebeba1089 > > Author: Jason Merrill <jason@redhat.com> > > Date: Mon Mar 4 14:09:57 2019 -0500 > > > > * gimplify.c (gimplify_cond_expr): Don't check TREE_ADDRESSABLE. > > > > The front end shouldn't produce a GENERIC COND_EXPR of TREE_ADDRESSABLE > > type. > > I think this broke a lot in the D testsuite: Thanks, reverted. Jason
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index c107a321949..f039a3b3eb0 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -5916,7 +5916,8 @@ condition_conversion (tree expr) } /* Returns the address of T. This function will fold away - ADDR_EXPR of INDIRECT_REF. */ + ADDR_EXPR of INDIRECT_REF. This is only for low-level usage; + most places should use cp_build_addr_expr instead. */ tree build_address (tree t) @@ -7114,7 +7115,7 @@ build_static_cast_1 (tree type, tree expr, bool c_cast_p, base = lookup_base (TREE_TYPE (type), intype, c_cast_p ? ba_unique : ba_check, NULL, complain); - expr = build_address (expr); + expr = cp_build_addr_expr (expr, complain); if (sanitize_flags_p (SANITIZE_VPTR)) { diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog index bd0914b8ffa..d90cc099767 100644 --- a/gcc/cp/ChangeLog +++ b/gcc/cp/ChangeLog @@ -1,5 +1,8 @@ 2019-05-07 Jason Merrill <jason@redhat.com> + PR c++/86485 - -Wmaybe-unused with empty class ?: + * typeck.c (build_static_cast_1): Use cp_build_addr_expr. + * pt.c (type_dependent_expression_p): A non-type template parm with a placeholder type is type-dependent.