Hi,
On Thu, Sep 26 2019, Richard Biener wrote:
> 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.
No, I don't. As I wrote above, even though would be enough to know
which actual arguments are pointers and which are not, I'd rather avoid
that too if we can.
I am looking into doing something similar that inlining does, but I'd
very much like to avoid re-creating a variant of PR 70929 for IPA-SRA
too, so I am looking into that PR.
>
> 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)
Makes sense, like the patch below? It has passed bootstrap and testing
on an x86_64-linux.
>
> which you then would need to eventually re-gimplify of course
> (we could in theory refactor setup_one_parameter to work with
> GIMPLE...)
If we start with a gimple_val, the code above should at worst produce
something we can feed into gimple_assign_set_rhs_from_tree, which is not
ideal but is better than full re-gimplification?
Thanks,
Martin
2019-09-27 Martin Jambor <mjam...@suse.cz>
PR ipa/91853
* tree-inline.c (force_value_to_type): New function.
(setup_one_parameter): Use force_value_to_type to convert type.
* tree-inline.c (force_value_to_type): Declare.
* 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 | 11 ++++++--
gcc/testsuite/gcc.dg/ipa/pr91853.c | 30 ++++++++++++++++++++++
gcc/tree-inline.c | 41 +++++++++++++++++-------------
gcc/tree-inline.h | 1 +
4 files changed, 64 insertions(+), 19 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..bbf646726e2 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -651,8 +651,15 @@ 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 type mismatches in calls in invalid programs and make a
+ poor attempt to gracefully convert them so that we don't ICE. */
+ if (!POINTER_TYPE_P (TREE_TYPE (base)))
+ base = force_value_to_type (ptr_type_node, base);
+
+ 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);
+}
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index e4ae1b058fd..4c972f3ce60 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3333,6 +3333,29 @@ insert_init_stmt (copy_body_data *id, basic_block bb,
gimple *init_stmt)
}
}
+/* Deal with mismatched formal/actual parameters, in a rather brute-force way
+ if need be (which should only be necessary for invalid programs). Attempt
+ to convert VAL to TYPE and return the result if it is possible, just return
+ a zero constant of the given type if it fails. */
+
+tree
+force_value_to_type (tree type, tree value)
+{
+ /* If we can match up types by promotion/demotion do so. */
+ if (fold_convertible_p (type, value))
+ return fold_convert (type, value);
+
+ /* ??? 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 (type) == TYPE_SIZE (TREE_TYPE (value)))
+ return fold_build1 (VIEW_CONVERT_EXPR, type, value);
+ else
+ return build_zero_cst (type);
+}
+
/* Initialize parameter P with VALUE. If needed, produce init statement
at the end of BB. When BB is NULL, we return init statement to be
output later. */
@@ -3349,23 +3372,7 @@ setup_one_parameter (copy_body_data *id, tree p, tree
value, tree fn,
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));
- }
- }
+ rhs = force_value_to_type (TREE_TYPE (p), value);
/* Make an equivalent VAR_DECL. Note that we must NOT remap the type
here since the type of this decl must be visible to the calling
diff --git a/gcc/tree-inline.h b/gcc/tree-inline.h
index 87a149c357b..b226dc03833 100644
--- a/gcc/tree-inline.h
+++ b/gcc/tree-inline.h
@@ -250,6 +250,7 @@ extern tree copy_fn (tree, tree&, tree&);
extern const char *copy_forbidden (struct function *fun);
extern tree copy_decl_for_dup_finish (copy_body_data *id, tree decl, tree
copy);
extern tree copy_decl_to_var (tree, copy_body_data *);
+extern tree force_value_to_type (tree type, tree value);
/* This is in tree-inline.c since the routine uses
data structures from the inliner. */
--
2.23.0
