On 22-04-15 10:06, Richard Biener wrote:
On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <tom_devr...@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?
Thanks,
- Tom
Add copy for va_list parameter
---
gcc/function.c | 29 +++++++++++++++++++++++++++++
gcc/gimplify.c | 16 ----------------
2 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/gcc/function.c b/gcc/function.c
index 7d4df92..2ebfec4 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -3855,6 +3855,24 @@ gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
return NULL;
}
+static inline bool
+is_va_list_type (tree type)
+{
+ tree id = TYPE_IDENTIFIER (type);
+ if (id == NULL_TREE)
+ return false;
+ const char *s = IDENTIFIER_POINTER (id);
+ if (s == NULL)
+ return false;
+ if (strcmp (s, "va_list") == 0)
+ return true;
+ if (strcmp (s, "__builtin_sysv_va_list") == 0)
+ return true;
+ if (strcmp (s, "__builtin_ms_va_list") == 0)
+ return true;
+ return false;
+}
+
/* Gimplify the parameter list for current_function_decl. This involves
evaluating SAVE_EXPRs of variable sized parameters and generating code
to implement callee-copies reference parameters. Returns a sequence of
@@ -3953,6 +3971,17 @@ gimplify_parameters (void)
DECL_HAS_VALUE_EXPR_P (parm) = 1;
}
}
+ else if (is_va_list_type (TREE_TYPE (parm)))
+ {
+ tree cp = create_tmp_reg (data.nominal_type, get_name (parm));
+ DECL_IGNORED_P (cp) = 0;
+ TREE_ADDRESSABLE (cp) = 1;
+ tree t = build2 (MODIFY_EXPR, TREE_TYPE (cp), cp, parm);
+ gimplify_and_add (t, &stmts);
+
+ SET_DECL_VALUE_EXPR (parm, cp);
+ DECL_HAS_VALUE_EXPR_P (parm) = 1;
+ }
}
fnargs.release ();
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 5f1dd1a..c922dc7 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4569,7 +4569,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
gimple assign;
location_t loc = EXPR_LOCATION (*expr_p);
gimple_stmt_iterator gsi;
- tree ap = NULL_TREE, ap_copy = NULL_TREE;
gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
|| TREE_CODE (*expr_p) == INIT_EXPR);
@@ -4730,16 +4729,12 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
enum internal_fn ifn = CALL_EXPR_IFN (*from_p);
auto_vec<tree> vargs (nargs);
- if (ifn == IFN_VA_ARG)
- ap = unshare_expr (CALL_EXPR_ARG (*from_p, 0));
for (i = 0; i < nargs; i++)
{
gimplify_arg (&CALL_EXPR_ARG (*from_p, i), pre_p,
EXPR_LOCATION (*from_p));
vargs.quick_push (CALL_EXPR_ARG (*from_p, i));
}
- if (ifn == IFN_VA_ARG)
- ap_copy = CALL_EXPR_ARG (*from_p, 0);
call_stmt = gimple_build_call_internal_vec (ifn, vargs);
gimple_set_location (call_stmt, EXPR_LOCATION (*expr_p));
}
@@ -4784,17 +4779,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
gsi = gsi_last (*pre_p);
maybe_fold_stmt (&gsi);
- /* When gimplifying the &ap argument of va_arg, we might end up with
- ap.1 = ap
- va_arg (&ap.1, 0B)
- We need to assign ap.1 back to ap, otherwise va_arg has no effect on
- ap. */
- if (ap != NULL_TREE
- && TREE_CODE (ap) == ADDR_EXPR
- && TREE_CODE (ap_copy) == ADDR_EXPR
- && !operand_equal_p (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), 0))
- gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p);
-
if (want_value)
{
*expr_p = TREE_THIS_VOLATILE (*to_p) ? *from_p : unshare_expr (*to_p);
--
1.9.1
