[PR65823] Fix va_arg ap_copy nop detection
diff mbox

Message ID CAFiYyc1bJ51ynbWH_=M25wt_5pw7LGO_gys9D5s7pvqXt7XL=A@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener April 22, 2015, 1:50 p.m. UTC
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


pre-approved with removing the kludge.

Thanks,
Richard.

> Thanks,
> - Tom

Comments

Tom de Vries April 27, 2015, 7:45 a.m. UTC | #1
On 22-04-15 15:50, Richard Biener wrote:
> 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);
>

That sort of works, but causes other problems. Filed PR65887 - 'remove va_arg ap 
copies' to track this issue.

Thanks,
- Tom

Patch
diff mbox

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);