[Bug tree-optimization/65887] remove va_arg ap copies

["rguenth at gcc dot gnu.org" <gcc-bugzilla at gcc dot gnu dot org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65887

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to vries from comment #2)
> I.
> After removing the copyback using attached patch, and marking the va_arg
> first argument as addressable as suggested here (
> https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01314.html ) using this patch
> (nr 1):
> ...
> @@ -9408,6 +9458,23 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p,
>      }
>  
>    /* 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);
> ... 
> 
> we get the desired:
> ...
>        e = VA_ARG (&argp, 0B);
>        e = VA_ARG (&argp, 0B);
> ...
> 
> 
> II.
> However, we subsequently run into a verify_gimple_call failure in
> gcc.c-torture/execute/va-arg-10.c, for the second argument of this va_copy:
> ...
>             __builtin_va_copy (&apc, ap);
>             ...
>             D.2056 = VA_ARG (&ap, 0B);
> ...
> 
> Presumably because ap is not marked as addressable when gimplifying the
> va_copy, but ap is later marked as addressable when gimplifying VA_ARG_EXPR.
> 
> With this patch (nr 2), we mark the second va_copy argument as addressable
> when gimplifying va_copy:
> ...
> @@ -2339,6 +2340,55 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p,
> bool want_value)
>        && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
>      switch (DECL_FUNCTION_CODE (fndecl))
>        {
> +      case BUILT_IN_VA_COPY:
> +       mark_addressable (CALL_EXPR_ARG (*expr_p, 1));
> +       break;
>        case BUILT_IN_VA_START:
>          {
>           builtin_va_start_p = TRUE;
> ...
> 
> That indeed prevents the verify_gimple_call error. But the code now contains
> a copy:
> ...
>             ap.0 = ap;
>             __builtin_va_copy (&apc, ap.0);
>             ...
>             D.2057 = VA_ARG (&ap, 0B);
> ...
> The copy in itself does not look incorrect, but we'd rather not have it.
> 
> 
> III.
> Furthermore, patch nr 1 triggers a verify_gimple_call error on
> gcc.c-torture/execute/va-arg-14.c for the first argument of a va_copy:
> ...
>             __builtin_va_copy (param, &local);
>             ...
>             D.1845 = VA_ARG (&param, 0B);
> ...
> 
> Using this patch (nr 3), we also mark the first argument of the copy as
> addressable:
> ...
> @@ -2341,6 +2341,7 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p,
> bool want_value)
>        {
>        case BUILT_IN_VA_COPY:
>         mark_addressable (CALL_EXPR_ARG (*expr_p, 1));
> +       mark_addressable (CALL_EXPR_ARG (*expr_p, 0));
>         break;
>        case BUILT_IN_VA_START:
>          {
> ...
> 
> That indeed prevents the verify_gimple_call failure. But it results in this
> code:
> ...
>         param.0 = param; 
>         __builtin_va_copy (param.0, &local);
>         ...
>         D.1846 = VA_ARG (&param, 0B);
> ...
> which doesn't look correct: param is unmodified by the va_copy.

Well, you only get the "copy" if param is of register type (thus a pointer).
So the code is correct I belive.

Rather than marking the va_list arg addressable in all the cases above
you should probably simply ensure the frontend marks it so from the
point it creates a variable with va_list type.  This is because even

 va_list a1, a2;
 a1 = a2;
 __builtin_va_arg (a1, ...);

might go wrong when gimplifying a1 = a2.

> OTOH, the obvious tests (execute.exp=va-arg*.c, execute.exp=stdarg*.c,
> callabi.exp) are passing, probably because va_list is a pointer type, and
> va_copy modifies what param points to.