| Message ID | 5552FF8D.2040604@mentor.com |
|---|---|
| State | New |
| Headers | show |
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
Gimplify va_arg ap based on do_deref 2015-05-13 Tom de Vries <tom@codesourcery.com> PR tree-optimization/66010 * gimplify.h (gimplify_va_arg_internal): Remove declaration. * gimplify.c (gimplify_va_arg_internal): Remove and inline into ... * tree-stdarg.c (expand_ifn_va_arg_1): ... here. Choose between lval and rval based on do_deref. --- gcc/gimplify.c | 26 -------------------------- gcc/gimplify.h | 1 - gcc/tree-stdarg.c | 9 ++++++++- 3 files changed, 8 insertions(+), 28 deletions(-) diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 322d0ba..4846478 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -9302,32 +9302,6 @@ dummy_object (tree type) return build2 (MEM_REF, type, t, t); } -/* Call the target expander for evaluating a va_arg call of VALIST - and TYPE. */ - -tree -gimplify_va_arg_internal (tree valist, tree type, 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) - { - gcc_assert (TREE_CODE (TREE_TYPE (valist)) != ARRAY_TYPE); - gimplify_expr (&valist, pre_p, post_p, is_gimple_val, fb_rvalue); - } - else - gimplify_expr (&valist, pre_p, post_p, is_gimple_min_lval, fb_lvalue); - - return targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p); -} - /* Gimplify __builtin_va_arg, aka VA_ARG_EXPR, which is not really a builtin function, but a very special sort of operator. */ diff --git a/gcc/gimplify.h b/gcc/gimplify.h index 83bf525..615925c 100644 --- a/gcc/gimplify.h +++ b/gcc/gimplify.h @@ -82,7 +82,6 @@ extern void gimplify_function_tree (tree); extern enum gimplify_status gimplify_va_arg_expr (tree *, gimple_seq *, gimple_seq *); gimple gimplify_assign (tree, tree, gimple_seq *); -extern tree gimplify_va_arg_internal (tree, tree, gimple_seq *, gimple_seq *); /* Return true if gimplify_one_sizepos doesn't need to gimplify expr (when in TYPE_SIZE{,_UNIT} and similar type/decl size/bitsize diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c index 3bede7e..f8ff70a 100644 --- a/gcc/tree-stdarg.c +++ b/gcc/tree-stdarg.c @@ -1059,7 +1059,14 @@ expand_ifn_va_arg_1 (function *fun) push_gimplify_context (false); - expr = gimplify_va_arg_internal (ap, type, &pre, &post); + /* Make it easier for the backends by protecting the valist argument + from multiple evaluations. */ + 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); + + expr = targetm.gimplify_va_arg_expr (ap, type, &pre, &post); lhs = gimple_call_lhs (stmt); if (lhs != NULL_TREE) -- 1.9.1
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? Thanks, - Tom