| Message ID | CAFiYyc1bJ51ynbWH_=M25wt_5pw7LGO_gys9D5s7pvqXt7XL=A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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);