On 27-04-15 09:45, Tom de Vries wrote:
> 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.
>

this patch marks the va_arg ap argument in the frontend as addressable, and 
removes the fixup.

Bootstrapped and reg-tested on x86_64.

OK for trunk?

Thanks,
- Tom