On September 28, 2019 12:24:40 AM GMT+02:00, Martin Jambor <mjam...@suse.cz>
wrote:
>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?
Yeah, that should work.
Patch is OK.
Thanks,
Richard.
>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. */
