Re: [C++ PATCH] Fix ICE with inline asm and MODIFY_EXPR/preinc/predec in output operand (PR c++/84961, take 2)

[Jason Merrill <jason at redhat dot com>]

OK.

On Wed, Mar 21, 2018 at 1:47 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 21, 2018 at 01:01:38PM -0400, Jason Merrill wrote:
>> >> Hmm, it would be nice to share this with the similar patterns in
>> >> unary_complex_lvalue and cp_build_modify_expr.
>>
>> > You mean just outline the
>> >       if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
>> >         lhs = build2 (TREE_CODE (lhs), TREE_TYPE (lhs),
>> >                       cp_stabilize_reference (TREE_OPERAND (lhs, 0)),
>> >                       TREE_OPERAND (lhs, 1));
>> >       lhs = build2 (COMPOUND_EXPR, lhstype, lhs, TREE_OPERAND (lhs, 0));
>> > into lhs = some_function (lhs); and use that in finish_asm_stmt,
>> > unary_complex_lvalue and cp_build_modify_expr in these spots?
>>
>> > I really have no idea how to commonize anything else, e.g. the COMPOUND_EXPR
>> > handling is substantially different between the 3 functions.
>>
>> > Any suggestion for the some_function name if you want that?
>>
>> Sure, that's something.  How about genericize_compound_lvalue?
>
> So like this (if it passes bootstrap/regtest)?
>
> 2018-03-21  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/84961
>         * cp-tree.h (genericize_compound_lvalue): Declare.
>         * typeck.c (genericize_compound_lvalue): New function.
>         (unary_complex_lvalue, cp_build_modify_expr): Use it.
>         * semantics.c (finish_asm_stmt): Replace MODIFY_EXPR, PREINCREMENT_EXPR
>         and PREDECREMENT_EXPR in output and "m" constrained input operands with
>         COMPOUND_EXPR.  Call cxx_mark_addressable on the rightmost
>         COMPOUND_EXPR operand.
>
>         * c-c++-common/pr43690.c: Don't expect errors on "m" (--x) and
>         "m" (++x) in C++.
>         * g++.dg/torture/pr84961-1.C: New test.
>         * g++.dg/torture/pr84961-2.C: New test.
>
> --- gcc/cp/cp-tree.h.jj 2018-03-20 22:05:57.053431471 +0100
> +++ gcc/cp/cp-tree.h    2018-03-21 18:41:42.301838677 +0100
> @@ -7145,6 +7145,7 @@ extern tree cp_build_addressof                    (locati
>  extern tree cp_build_addr_expr                 (tree, tsubst_flags_t);
>  extern tree cp_build_unary_op                   (enum tree_code, tree, bool,
>                                                   tsubst_flags_t);
> +extern tree genericize_compound_lvalue         (tree);
>  extern tree unary_complex_lvalue               (enum tree_code, tree);
>  extern tree build_x_conditional_expr           (location_t, tree, tree, tree,
>                                                   tsubst_flags_t);
> --- gcc/cp/typeck.c.jj  2018-03-06 08:01:37.827883402 +0100
> +++ gcc/cp/typeck.c     2018-03-21 18:40:23.350862956 +0100
> @@ -6357,6 +6357,25 @@ build_unary_op (location_t /*location*/,
>    return cp_build_unary_op (code, xarg, noconvert, tf_warning_or_error);
>  }
>
> +/* Adjust LVALUE, an MODIFY_EXPR, PREINCREMENT_EXPR or PREDECREMENT_EXPR,
> +   so that it is a valid lvalue even for GENERIC by replacing
> +   (lhs = rhs) with ((lhs = rhs), lhs)
> +   (--lhs) with ((--lhs), lhs)
> +   (++lhs) with ((++lhs), lhs)
> +   and if lhs has side-effects, calling cp_stabilize_reference on it, so
> +   that it can be evaluated multiple times.  */
> +
> +tree
> +genericize_compound_lvalue (tree lvalue)
> +{
> +  if (TREE_SIDE_EFFECTS (TREE_OPERAND (lvalue, 0)))
> +    lvalue = build2 (TREE_CODE (lvalue), TREE_TYPE (lvalue),
> +                    cp_stabilize_reference (TREE_OPERAND (lvalue, 0)),
> +                    TREE_OPERAND (lvalue, 1));
> +  return build2 (COMPOUND_EXPR, TREE_TYPE (TREE_OPERAND (lvalue, 0)),
> +                lvalue, TREE_OPERAND (lvalue, 0));
> +}
> +
>  /* Apply unary lvalue-demanding operator CODE to the expression ARG
>     for certain kinds of expressions which are not really lvalues
>     but which we can accept as lvalues.
> @@ -6391,17 +6410,7 @@ unary_complex_lvalue (enum tree_code cod
>    if (TREE_CODE (arg) == MODIFY_EXPR
>        || TREE_CODE (arg) == PREINCREMENT_EXPR
>        || TREE_CODE (arg) == PREDECREMENT_EXPR)
> -    {
> -      tree lvalue = TREE_OPERAND (arg, 0);
> -      if (TREE_SIDE_EFFECTS (lvalue))
> -       {
> -         lvalue = cp_stabilize_reference (lvalue);
> -         arg = build2 (TREE_CODE (arg), TREE_TYPE (arg),
> -                       lvalue, TREE_OPERAND (arg, 1));
> -       }
> -      return unary_complex_lvalue
> -       (code, build2 (COMPOUND_EXPR, TREE_TYPE (lvalue), arg, lvalue));
> -    }
> +    return unary_complex_lvalue (code, genericize_compound_lvalue (arg));
>
>    if (code != ADDR_EXPR)
>      return NULL_TREE;
> @@ -7887,11 +7896,7 @@ cp_build_modify_expr (location_t loc, tr
>      case PREINCREMENT_EXPR:
>        if (compound_side_effects_p)
>         newrhs = rhs = stabilize_expr (rhs, &preeval);
> -      if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
> -       lhs = build2 (TREE_CODE (lhs), TREE_TYPE (lhs),
> -                     cp_stabilize_reference (TREE_OPERAND (lhs, 0)),
> -                     TREE_OPERAND (lhs, 1));
> -      lhs = build2 (COMPOUND_EXPR, lhstype, lhs, TREE_OPERAND (lhs, 0));
> +      lhs = genericize_compound_lvalue (lhs);
>      maybe_add_compound:
>        /* If we had (bar, --foo) = 5; or (bar, (baz, --foo)) = 5;
>          and looked through the COMPOUND_EXPRs, readd them now around
> @@ -7914,11 +7919,7 @@ cp_build_modify_expr (location_t loc, tr
>      case MODIFY_EXPR:
>        if (compound_side_effects_p)
>         newrhs = rhs = stabilize_expr (rhs, &preeval);
> -      if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
> -       lhs = build2 (TREE_CODE (lhs), TREE_TYPE (lhs),
> -                     cp_stabilize_reference (TREE_OPERAND (lhs, 0)),
> -                     TREE_OPERAND (lhs, 1));
> -      lhs = build2 (COMPOUND_EXPR, lhstype, lhs, TREE_OPERAND (lhs, 0));
> +      lhs = genericize_compound_lvalue (lhs);
>        goto maybe_add_compound;
>
>      case MIN_EXPR:
> --- gcc/cp/semantics.c.jj       2018-03-20 22:05:54.385430766 +0100
> +++ gcc/cp/semantics.c  2018-03-21 18:42:50.084817830 +0100
> @@ -1512,6 +1512,21 @@ finish_asm_stmt (int volatile_p, tree st
>                       && C_TYPE_FIELDS_READONLY (TREE_TYPE (operand)))))
>             cxx_readonly_error (operand, lv_asm);
>
> +         tree *op = &operand;
> +         while (TREE_CODE (*op) == COMPOUND_EXPR)
> +           op = &TREE_OPERAND (*op, 1);
> +         switch (TREE_CODE (*op))
> +           {
> +           case PREINCREMENT_EXPR:
> +           case PREDECREMENT_EXPR:
> +           case MODIFY_EXPR:
> +             *op = genericize_compound_lvalue (*op);
> +             op = &TREE_OPERAND (*op, 1);
> +             break;
> +           default:
> +             break;
> +           }
> +
>           constraint = TREE_STRING_POINTER (TREE_VALUE (TREE_PURPOSE (t)));
>           oconstraints[i] = constraint;
>
> @@ -1520,7 +1535,7 @@ finish_asm_stmt (int volatile_p, tree st
>             {
>               /* If the operand is going to end up in memory,
>                  mark it addressable.  */
> -             if (!allows_reg && !cxx_mark_addressable (operand))
> +             if (!allows_reg && !cxx_mark_addressable (*op))
>                 operand = error_mark_node;
>             }
>           else
> @@ -1562,7 +1577,23 @@ finish_asm_stmt (int volatile_p, tree st
>                   /* Strip the nops as we allow this case.  FIXME, this really
>                      should be rejected or made deprecated.  */
>                   STRIP_NOPS (operand);
> -                 if (!cxx_mark_addressable (operand))
> +
> +                 tree *op = &operand;
> +                 while (TREE_CODE (*op) == COMPOUND_EXPR)
> +                   op = &TREE_OPERAND (*op, 1);
> +                 switch (TREE_CODE (*op))
> +                   {
> +                   case PREINCREMENT_EXPR:
> +                   case PREDECREMENT_EXPR:
> +                   case MODIFY_EXPR:
> +                     *op = genericize_compound_lvalue (*op);
> +                     op = &TREE_OPERAND (*op, 1);
> +                     break;
> +                   default:
> +                     break;
> +                   }
> +
> +                 if (!cxx_mark_addressable (*op))
>                     operand = error_mark_node;
>                 }
>               else if (!allows_reg && !allows_mem)
> --- gcc/testsuite/c-c++-common/pr43690.c.jj     2018-03-20 22:05:54.237430727 +0100
> +++ gcc/testsuite/c-c++-common/pr43690.c        2018-03-21 18:26:40.348908281 +0100
> @@ -6,8 +6,8 @@ void
>  foo (char *x)
>  {
>    asm ("" : : "m" (x++));      /* { dg-error "is not directly addressable" } */
> -  asm ("" : : "m" (++x));      /* { dg-error "is not directly addressable" } */
> +  asm ("" : : "m" (++x));      /* { dg-error "is not directly addressable" "" { target c } } */
>    asm ("" : : "m" (x--));      /* { dg-error "is not directly addressable" } */
> -  asm ("" : : "m" (--x));      /* { dg-error "is not directly addressable" } */
> +  asm ("" : : "m" (--x));      /* { dg-error "is not directly addressable" "" { target c } } */
>    asm ("" : : "m" (x + 1));    /* { dg-error "is not directly addressable" } */
>  }
> --- gcc/testsuite/g++.dg/torture/pr84961-1.C.jj 2018-03-21 18:26:40.349908281 +0100
> +++ gcc/testsuite/g++.dg/torture/pr84961-1.C    2018-03-21 18:26:40.349908281 +0100
> @@ -0,0 +1,24 @@
> +// PR c++/84961
> +// { dg-do compile }
> +
> +short a;
> +volatile int b;
> +int c, d;
> +
> +void
> +foo ()
> +{
> +  asm volatile ("" : "=r" (b = a));
> +}
> +
> +void
> +bar ()
> +{
> +  asm volatile ("" : "=r" (++c, ++d, b = a));
> +}
> +
> +void
> +baz ()
> +{
> +  asm volatile ("" : "=r" (--b));
> +}
> --- gcc/testsuite/g++.dg/torture/pr84961-2.C.jj 2018-03-21 18:26:40.349908281 +0100
> +++ gcc/testsuite/g++.dg/torture/pr84961-2.C    2018-03-21 18:26:40.349908281 +0100
> @@ -0,0 +1,24 @@
> +// PR c++/84961
> +// { dg-do compile }
> +
> +short a;
> +volatile int b;
> +int c, d;
> +
> +void
> +foo ()
> +{
> +  asm volatile ("" : : "m" (b = a));
> +}
> +
> +void
> +bar ()
> +{
> +  asm volatile ("" : : "m" (++c, ++d, b = a));
> +}
> +
> +void
> +baz ()
> +{
> +  asm volatile ("" : : "m" (--b));
> +}
>
>
>         Jakub