On Wed, May 13, 2015 at 9:38 AM, Tom de Vries <tom_devr...@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
