[PATCH][PR65823] Fix va_arg ap_copy nop detection

[Richard Biener]

On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> this patch fixes PR65823.
>
> The problem is a verify_gimple ICE during compilation of
> gcc.c-torture/execute/stdarg-2.c for arm at -O0/-O1:
> ...
> In function 'f3':
> src/gcc/testsuite/gcc.c-torture/execute/stdarg-2.c:61:1:
> error: incorrect sharing of tree nodes
> aps[4]
> # .MEM_5 = VDEF <.MEM_11>
> aps[4] = aps[4];
> ...
>
>
> Before gimplification, f3 looks like this in the original dump:
> ...
>   struct va_list aps[10];
>
>     struct va_list aps[10];
>   __builtin_va_start ((struct  &) (struct  *) &aps[4], i);
>   x = VA_ARG_EXPR <aps[4]>;
>   __builtin_va_end ((struct  &) (struct  *) &aps[4]);
> ...
>
> After gimplification, it looks like:
> ...
> f3 (int i)
> {
>   long intD.5 x.0D.4231;
>   struct va_listD.4222 apsD.4227[10];
>
>   try
>     {
>       # USE = anything
>       # CLB = anything
>       __builtin_va_startD.1052 (&apsD.4227[4], 0);
>       # USE = anything
>       # CLB = anything
>       x.0D.4231 = VA_ARG (&apsD.4227[4], 0B);
>       apsD.4227[4] = apsD.4227[4];
>       xD.4223 = x.0D.4231;
>       # USE = anything
>       # CLB = anything
>       __builtin_va_endD.1051 (&apsD.4227[4]);
>     }
>   finally
>     {
>       apsD.4227 = {CLOBBER};
>     }
> }
> ...
>
> The nop 'apsD.4227[4] = apsD.4227[4]' introduced during gimplification is
> not meant to be there.
>
>
> There is already a test 'TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))'
> in gimplify_modify_expr to prevent this nop:
> ...
>   /* When gimplifying the &ap argument of va_arg, we might end up with
>        ap.1 = ap
>        va_arg (&ap.1, 0B)
>      We need to assign ap.1 back to ap, otherwise va_arg has no effect on
>      ap.  */
>   if (ap != NULL_TREE
>       && TREE_CODE (ap) == ADDR_EXPR
>       && TREE_CODE (ap_copy) == ADDR_EXPR
>       && TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))
>     gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0),
> pre_p);
> ...
>
> But the test is a pointer equality test, and it fails in this case.
>
> The patches fixes the problem by using operand_equal_p to do the equality
> test.
>
>
> Bootstrapped and reg-tested on x86_64.
>
> Did minimal non-bootstrap build on arm and reg-tested.
>
> OK for trunk?

Hmm, ok for now.  But I wonder if we can't fix things to not require that
odd extra copy.  In fact that we introduce ap.1 looks completely bogus to me
(and we don't in this case for arm).  Note that the pointer compare obviously
fails because we unshare the expression.

So ... what breaks if we simply remove this odd "fixup"?

Thanks,
Richard.

> Thanks,
> - Tom