Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Apr 22, 2015 at 3:38 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 22-04-15 10:06, Richard Biener wrote:
>>
>> On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> this patch fixes PR65823.
>>>
>
> <SNIP>
>
>>>
>>> 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.
>
>
> Committed.
>
>> But I wonder if we can't fix things to not require that
>> odd extra copy.
>
>
> Agreed, that would be good.
>
>> In fact that we introduce ap.1 looks completely bogus to me
>
>
> AFAICT, it's introduced by gimplify_arg ('&argp') because argp (a PARM_DECL)
> is not addressable.
>
>> (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"?
>>
>
> [ Originally mentioned at https://gcc.gnu.org/ml/gcc/2015-02/msg00011.html .
> ]
>
> I've committed gcc.target/x86_64/abi/callabi/vaarg-6.c specifically as a
> minimal version of this problem.
>
> If we remove the ap_copy fixup, at original we have:
> ...
> ;; Function do_cpy2 (null)
> {
>   char * e;
>
>     char * e;
>   e = VA_ARG_EXPR <argp>;
>   e = VA_ARG_EXPR <argp>;
>   if (e != b)
>     {
>       abort ();
>     }
> }
> ...
>
> and after gimplify we have:
> ...
> do_cpy2 (char * argp)
> {
>   char * argp.1;
>   char * argp.2;
>   char * b.3;
>   char * e;
>
>   argp.1 = argp;
>   e = VA_ARG (&argp.1, 0B);
>   argp.2 = argp;
>   e = VA_ARG (&argp.2, 0B);
>   b.3 = b;
>   if (e != b.3) goto <D.1373>; else goto <D.1374>;
>   <D.1373>:
>   abort ();
>   <D.1374>:
> }
> ...
>
> The second VA_ARG uses &argp.2, which is a copy of argp, which is unmodified
> by the first VA_ARG.
>
>
> Using attached _proof-of-concept_ patch, I get callabi.exp working without
> the ap_copy, still at the cost of one 'argp.1 = argp' copy though:
> ...
> do_cpy2 (char * argp)
> {
>   char * argp.1;
>   char * b.2;
>   char * e;
>
>   argp.1 = argp;
>   e = VA_ARG (&argp.1, 0B);
>   e = VA_ARG (&argp.1, 0B);
>   b.2 = b;
>   if (e != b.2) goto <D.1372>; else goto <D.1373>;
>   <D.1372>:
>   abort ();
>   <D.1373>:
> }
> ...
>
> But perhaps there's an easier way?

Hum, simply

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c      (revision 222320)
+++ gcc/gimplify.c      (working copy)
@@ -9419,6 +9419,7 @@ gimplify_va_arg_expr (tree *expr_p, gimp
     }

   /* Transform a VA_ARG_EXPR into an VA_ARG internal function.  */
+  mark_addressable (valist);
   ap = build_fold_addr_expr_loc (loc, valist);
   tag = build_int_cst (build_pointer_type (type), 0);
   *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, ap, tag);

pre-approved with removing the kludge.

Thanks,
Richard.

> Thanks,
> - Tom