On Wed, Apr 22, 2015 at 3:38 PM, Tom de Vries <tom_devr...@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_devr...@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
