Re: [PATCH] Fix va_arg gimplification error for s390

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, May 13, 2015 at 9:38 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> this patch fixes a gimplification error of the va_list argument of va_arg
> for target s390. The error was introduced by r223054, the fix for PR66010.
>
>
> I.
>
> consider test-case:
> ...
> #include <stdarg.h>
>
> int
> f1 (int i, ...)
> {
>   int res;
>   va_list ap;
>
>   va_start (ap, i);
>   res = va_arg (ap, int);
>   va_end (ap);
>
>   return res;
> }
> ...
>
> For target s390, we're running into a gimplification error for valist '&ap'
> in gimplify_va_arg_internal when calling:
> ...
> 9326        gimplify_expr (&valist, pre_p, post_p, is_gimple_min_lval,
> fb_lvalue);
> ...
>
> Before committing r223054, we were calling instead:
> ...
> 9331          gimplify_expr (&valist, pre_p, post_p, is_gimple_val,
> fb_rvalue);
> ...
> with valist '(struct  *) &ap', and the gimplification went fine.
>
>
> II.
>
> The successful gimplification was triggered using the following path,
> entering with valist 'ap':
> ...
> gimplify_va_arg_internal (tree valist, tree type, location_t loc,
>                           gimple_seq *pre_p, gimple_seq *post_p)
> {
>   tree have_va_type = TREE_TYPE (valist);
>   tree cano_type = targetm.canonical_va_list_type (have_va_type);
>
>   if (cano_type != NULL_TREE)
>     have_va_type = cano_type;
>
>   /* Make it easier for the backends by protecting the valist argument
>      from multiple evaluations.  */
>   if (TREE_CODE (have_va_type) == ARRAY_TYPE)
>     {
>       /* For this case, the backends will be expecting a pointer to
>          TREE_TYPE (abi), but it's possible we've
>          actually been given an array (an actual TARGET_FN_ABI_VA_LIST).
>          So fix it.  */
>       if (TREE_CODE (TREE_TYPE (valist)) == ARRAY_TYPE)
>         {
>           tree p1 = build_pointer_type (TREE_TYPE (have_va_type));
>           valist = fold_convert_loc (loc, p1,
>                                      build_fold_addr_expr_loc (loc,
> valist));
>         }
>
>       gimplify_expr (&valist, pre_p, post_p, is_gimple_val, fb_rvalue);
>     }
> ...
>
> In r223054, we've moved the fixup of adding the addr_expr to
> gimplify_va_arg_expr. Consequently, we're now entering with valist '&ap'.
> The have_va_type has changed to 'struct [1] *', and cano_type is NULL_TREE.
> So have_va_type is no longer an array, and the other gimplification path is
> triggered, which causes the gimplification error.
>
> [ For x86_64, the type of '&ap' is not 'struct [1] *' but 'struct *', and
> the
>   cano_type is not NULL_TREE, so we didn't encounter this problem there. ]
>
>
> III.
>
> The patch fixes this error by inlining gimplify_va_arg_internal into
> expand_ifn_va_arg_1, and using the information present there to determine
> which gimplification path to choose:
> ...
>         if (do_deref == integer_one_node)
>           gimplify_expr (&ap, &pre, &post, is_gimple_min_lval, fb_lvalue);
>         else
>           gimplify_expr (&ap, &pre, &post, is_gimple_val, fb_rvalue);
> ...
>
> So for an va_list ap given as argument of va_arg:
> - in case the canonical va_list is an array type, we take the address in
>   gimplify_va_arg_expr, set do_deref to zero, meaning we pass '&ap' to
>   targetm.gimplify_va_arg_expr. The fact that it's an address means we can
>   expect it to be an rvalue, but not an lvalue.
> - in case the canonical va_list is not an array type, we take the address in
>   gimplify_va_arg_expr, but set do_deref to one, meaning we pass 'ap' to
>   targetm.gimplify_va_arg_expr. 'ap' may be modified by va_arg, so it needs
> to
>   be an lvalue.
>
>
> IV.
>
> Bootstrapped and reg-tested on x86_64.
>
> Noted to fix s390 bootstrap:
> https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01176.html .
>
> Noted to fix ppc build:
> https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01162.html .
>
> OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> - Tom