On Wed, 25 Sep 2019, Martin Jambor wrote:
> Hi,
>
> PR 91853 and its duplicate PR 91894 show that IPA-SRA can stumble when
> presented with code with mismatched types, whether because it is a K&R C
> or happening through an originally indirect call (or probably also
> because of LTO).
>
> The problem is that we try to work with a register value - in this case
> an integer constant - like if it was a pointer to a structure and try to
> dereference it in the caller, leading to expressions like ADDR_EXPR of a
> constant zero. Old IPA-SRA dealt with these simply by checking type
> compatibility which is difficult in an LTO-capable IPA pass, basically
> we would at least have to remember and stream a bitmap for each call
> telling which arguments are pointers which looks a bit excessive given
> that we just don't want to ICE.
>
> So this patch attempts to deal with the situation rather than avoid it.
> When an integer is used instead of a pointer, there is some chance that
> it actually contains the pointer value and so I create a NOP_EXPR to
> convert it to a pointer (which in the testcase is actually a widening
> conversion). For other register types, I don't bother and simply pull
> an undefined pointer default definition SSA name and use that. I wonder
> whether I should somehow warn as well. Hopefully there is no code doing
> that that can conceivably work - maybe someone coding for x86_16 and
> passing a vector of integers as a segment and offset pointer? :-)
>
> What do people think? In any event, this patch passed bootstrap and
> testing and deals with the issue, so if it is OK, I'd like to commit it
> to trunk.
Humm... while I believe this "mostly" matches what we do in inlining
(but that also has some type verification disabling inlining for really
odd cases IIRC) I think the appropriate fix is in the IPA-SRA
decision stage (at WPA) where we should see that we cannot modify
a call in this way. That of course requires streaming of the actual
call stmt (or at least it's "ABI signature"), not sure if you already
do that.
So in inlining we do
static gimple *
setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn,
basic_block bb, tree *vars)
{
...
if (value
&& value != error_mark_node
&& !useless_type_conversion_p (TREE_TYPE (p), TREE_TYPE (value)))
{
/* If we can match up types by promotion/demotion do so. */
if (fold_convertible_p (TREE_TYPE (p), value))
rhs = fold_convert (TREE_TYPE (p), value);
else
{
/* ??? For valid programs we should not end up here.
Still if we end up with truly mismatched types here, fall
back
to using a VIEW_CONVERT_EXPR or a literal zero to not leak
invalid
GIMPLE to the following passes. */
if (!is_gimple_reg_type (TREE_TYPE (value))
|| TYPE_SIZE (TREE_TYPE (p)) == TYPE_SIZE (TREE_TYPE
(value)))
rhs = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (p), value);
else
rhs = build_zero_cst (TREE_TYPE (p));
}
}
I suggest that if we go a similar way that we copy this behavior
rather than inventing sth similar but slightly different. Maybe
split it out as
tree
force_value_to_type (tree type, tree val)
which you then would need to eventually re-gimplify of course
(we could in theory refactor setup_one_parameter to work with
GIMPLE...)
Richard.
> Martin
>
>
>
> 2019-09-23 Martin Jambor <mjam...@suse.cz>
>
> PR ipa/91853
> * ipa-param-manipulation.c (ipa_param_adjustments::modify_call): Deal
> with register type mismatches.
>
> testsuite/
> * gcc.dg/ipa/pr91853.c: New test.
> ---
> gcc/ipa-param-manipulation.c | 22 ++++++++++++++++++++--
> gcc/testsuite/gcc.dg/ipa/pr91853.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 50 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr91853.c
>
> diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
> index 913b96fefa4..bc175a5541a 100644
> --- a/gcc/ipa-param-manipulation.c
> +++ b/gcc/ipa-param-manipulation.c
> @@ -651,8 +651,26 @@ ipa_param_adjustments::modify_call (gcall *stmt,
> bool deref_base = false;
> unsigned int deref_align = 0;
> if (TREE_CODE (base) != ADDR_EXPR
> - && POINTER_TYPE_P (TREE_TYPE (base)))
> - off = build_int_cst (apm->alias_ptr_type, apm->unit_offset);
> + && is_gimple_reg_type (TREE_TYPE (base)))
> + {
> + /* Detect (gimple register) type mismatches in calls so that we don't
> + ICE. Make a poor attempt to gracefully treat integers passed in
> + place of pointers, for everything else create a proper undefined
> + value which it is. */
> + if (INTEGRAL_TYPE_P (TREE_TYPE (base)))
> + {
> + tree tmp = make_ssa_name (ptr_type_node);
> + gassign *convert = gimple_build_assign (tmp, NOP_EXPR, base);
> + gsi_insert_before (&gsi, convert, GSI_SAME_STMT);
> + base = tmp;
> + }
> + else if (!POINTER_TYPE_P (TREE_TYPE (base)))
> + {
> + tree tmp = create_tmp_var (ptr_type_node);
> + base = get_or_create_ssa_default_def (cfun, tmp);
> + }
> + off = build_int_cst (apm->alias_ptr_type, apm->unit_offset);
> + }
> else
> {
> bool addrof;
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr91853.c
> b/gcc/testsuite/gcc.dg/ipa/pr91853.c
> new file mode 100644
> index 00000000000..4bad7803751
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr91853.c
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "--param ipa-cp-value-list-size=0 -Os -fno-inline" } */
> +
> +struct _wincore
> +{
> + int y;
> + int width;
> +};
> +int a;
> +void fn2 (void);
> +static int fn1 (dpy, winInfo) struct _XDisplay *dpy;
> +struct _wincore *winInfo;
> +{
> + a = winInfo->width;
> + fn2 ();
> +}
> +
> +void fn4 (int, int, int);
> +static int fn3 (dpy, winInfo, visrgn) struct _XDisplay *dpy;
> +int winInfo, visrgn;
> +{
> + int b = fn1 (0, winInfo);
> + fn4 (0, 0, visrgn);
> +}
> +
> +int
> +fn5 (event) struct _XEvent *event;
> +{
> + fn3 (0, 0, 0);
> +}
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 247165 (AG München)
