Hi,
On Sat, Nov 27 2021, Jan Hubicka wrote:
>> Hi,
>>
>> IPA_JF_ANCESTOR jump functions are constructed also when the formal
>> parameter of the caller is first checked whether it is NULL and left
>> as it is if it is NULL, to accommodate C++ casts to an ancestor class.
>>
>> The jump function type was invented for devirtualization and IPA-CP
>> propagation of tree constants is also careful to apply it only to
>> existing DECLs(*) but as PR 103083 shows, the part propagating "known
>> bits" was not careful about this, which can lead to miscompilations.
>>
>> This patch introduces a flag to the ancestor jump functions which
>> tells whether a NULL-check was elided when creating it and makes the
>> bits propagation behave accordingly.
>>
>> (*) There still may remain problems when a DECL resides on address
>> zero (with -fno-delete-null-pointer-checks ...I hope it cannot happen
>> otherwise). I am looking into that now but I think it will be easier
>> for everyone if I do so in a follow-up patch.
>
> I guess so. Thinking about it bit more after our discussion yesterday
> I think it really matters where the ADDR_EXPR was created (since in
> funciton with -fno-delete-null-pointer-checks it might be equal to 0)
> and we need to somehow keep track of it.
>
> One observation is that it is unsafe to constant propagate
> ADDR_EXPR with -fno-delete-null-pointer-checks to function with
> -fdelete-null-pointer-checks, so we may just simply stop propagating
> known ADDR_EXPRs on when caller is -fno... and call is -f....
Makes sense.
>> +/* Return true if any of the known bits are non-zero. */
>> +bool
>> +ipcp_bits_lattice::known_nonzero_p () const
>> +{
>> + if (!constant_p ())
>> + return false;
>> + return !wi::eq_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0);
> There is also ne_p
Changed.
>> @@ -2374,6 +2384,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs,
>> int idx,
>> tree operand = NULL_TREE;
>> enum tree_code code;
>> unsigned src_idx;
>> + bool only_for_nonzero = false;
>>
>> if (jfunc->type == IPA_JF_PASS_THROUGH)
>> {
>> @@ -2386,7 +2397,9 @@ propagate_bits_across_jump_function (cgraph_edge *cs,
>> int idx,
>> {
>> code = POINTER_PLUS_EXPR;
>> src_idx = ipa_get_jf_ancestor_formal_id (jfunc);
>> - unsigned HOST_WIDE_INT offset = ipa_get_jf_ancestor_offset (jfunc) /
>> BITS_PER_UNIT;
>> + unsigned HOST_WIDE_INT offset
>> + = ipa_get_jf_ancestor_offset (jfunc) / BITS_PER_UNIT;
> I still think the offset should be in bytes (and probably poly_int64).
> There is get_addr_base_and_unit_offset
I agree about bytes, not sure about poly_ints, but could be persuaded.
But probably not as a part of this patch?
>
>> + only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset);
>> operand = build_int_cstu (size_type_node, offset);
>> }
>>
>> @@ -2404,16 +2417,18 @@ propagate_bits_across_jump_function (cgraph_edge
>> *cs, int idx,
>> and we store it in jump function during analysis stage. */
>>
>> if (src_lats->bits_lattice.bottom_p ()
>> - && jfunc->bits)
>> - return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
>> - precision);
>> + || (only_for_nonzero && !src_lats->bits_lattice.known_nonzero_p ()))
>> + {
>> + if (jfunc->bits)
>> + return dest_lattice->meet_with (jfunc->bits->value,
>> + jfunc->bits->mask, precision);
>> + else
>> + return dest_lattice->set_to_bottom ();
>> + }
> I do not think you need to set to bottom here. For pointers, we
> primarily track alignment and NULL is aligned - all you need to do is to
> make sure that we do not believe that some bits are 1.
I am not sure I understand, the testcase you wrote has all bits as zeros
and still miscompiles? It is primarily used for alignment but not only
for that.
>> @@ -3327,7 +3332,8 @@ update_jump_functions_after_inlining (struct
>> cgraph_edge *cs,
>> ipa_set_ancestor_jf (dst,
>> ipa_get_jf_ancestor_offset (src),
>> ipa_get_jf_ancestor_formal_id (src),
>> - agg_p);
>> + agg_p,
>> + ipa_get_jf_ancestor_keep_null (src));
>> break;
> Somewhere here you need to consider the case where you have two accessor
> jump functions to combine together and merge the keep_null flags...
Oops, I missed that, fixed.
>
> Also with the flags, I think you want to modify ipa-cp so NULL pointers
> gets propagated acroess the ancestor jump functions.
OK, added, along with a new testcase.
> Moreover ipa-modref is using ancestor jf to update its summary. Here I
> think with -fno-delete-null-pointer-checks it is safe to pdate also with
> the flag set, since we know the memory access is invalid otherwise.
I think it currently is, but only because for:
int array_at_addr_zero[2];
void __attribute__((noinline))
bar(int *p)
{
clobber(p ? &p[1] : p);
}
/* ... */
bar(array_at_addr_zero);
we do not produce an ancestor jump function (or any jump function
whatsoever). In the IL it is not represented as an ADDR_EXPR of a
MEM_REF but as a POINTER_PLUS.
Anyway, I guess I'll need to amend it to avoid the set_to_bottom you
claim is not necessary but currently I have the following.
Thanks,
Martin
gcc/ChangeLog:
2021-11-29 Martin Jambor <mjam...@suse.cz>
PR ipa/103083
* ipa-prop.h (ipa_ancestor_jf_data): New flag keep_null;
(ipa_get_jf_ancestor_keep_null): New function.
* ipa-prop.c (ipa_set_ancestor_jf): Initialize keep_null field of the
ancestor function.
(compute_complex_assign_jump_func): Pass false to keep_null
parameter of ipa_set_ancestor_jf.
(compute_complex_ancestor_jump_func): Pass true to keep_null
parameter of ipa_set_ancestor_jf.
(update_jump_functions_after_inlining): Carry over keep_null from the
original ancestor jump-function or merge them.
(ipa_write_jump_function): Stream keep_null flag.
(ipa_read_jump_function): Likewise.
(ipa_print_node_jump_functions_for_edge): Print the new flag.
* ipa-cp.c (class ipcp_bits_lattice): Make various getters const. New
member function known_nonzero_p.
(ipcp_bits_lattice::known_nonzero_p): New.
(propagate_bits_across_jump_function): Only process ancestor functions
when safe. Remove extraneous condition handling ancestor jump
functions.
(propagate_aggs_across_jump_function): Take care of keep_null
flag.
(ipa_get_jf_ancestor_result): Propagate NULL accross keep_null
jump functions.
gcc/testsuite/ChangeLog:
2021-11-25 Martin Jambor <mjam...@suse.cz>
* gcc.dg/ipa/pr103083-1.c: New test.
* gcc.dg/ipa/pr103083-2.c: Likewise.
---
gcc/ipa-cp.c | 42 +++++++++++++++++++--------
gcc/ipa-prop.c | 20 +++++++++----
gcc/ipa-prop.h | 13 +++++++++
gcc/testsuite/gcc.dg/ipa/pr103083-1.c | 28 ++++++++++++++++++
gcc/testsuite/gcc.dg/ipa/pr103083-2.c | 30 +++++++++++++++++++
5 files changed, 116 insertions(+), 17 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-1.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-2.c
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 5d9bb974d6b..c19fe9cf2ea 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -306,14 +306,15 @@ public:
class ipcp_bits_lattice
{
public:
- bool bottom_p () { return m_lattice_val == IPA_BITS_VARYING; }
- bool top_p () { return m_lattice_val == IPA_BITS_UNDEFINED; }
- bool constant_p () { return m_lattice_val == IPA_BITS_CONSTANT; }
+ bool bottom_p () const { return m_lattice_val == IPA_BITS_VARYING; }
+ bool top_p () const { return m_lattice_val == IPA_BITS_UNDEFINED; }
+ bool constant_p () const { return m_lattice_val == IPA_BITS_CONSTANT; }
bool set_to_bottom ();
bool set_to_constant (widest_int, widest_int);
+ bool known_nonzero_p () const;
- widest_int get_value () { return m_value; }
- widest_int get_mask () { return m_mask; }
+ widest_int get_value () const { return m_value; }
+ widest_int get_mask () const { return m_mask; }
bool meet_with (ipcp_bits_lattice& other, unsigned, signop,
enum tree_code, tree);
@@ -1081,6 +1082,15 @@ ipcp_bits_lattice::set_to_constant (widest_int value,
widest_int mask)
return true;
}
+/* Return true if any of the known bits are non-zero. */
+bool
+ipcp_bits_lattice::known_nonzero_p () const
+{
+ if (!constant_p ())
+ return false;
+ return wi::ne_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0);
+}
+
/* Convert operand to value, mask form. */
void
@@ -1477,6 +1487,9 @@ ipa_get_jf_ancestor_result (struct ipa_jump_func *jfunc,
tree input)
fold_build2 (MEM_REF, TREE_TYPE (TREE_TYPE (input)), input,
build_int_cst (ptr_type_node, byte_offset)));
}
+ else if (ipa_get_jf_ancestor_keep_null (jfunc)
+ && zerop (input))
+ return input;
else
return NULL_TREE;
}
@@ -2373,6 +2386,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int
idx,
tree operand = NULL_TREE;
enum tree_code code;
unsigned src_idx;
+ bool only_for_nonzero = false;
if (jfunc->type == IPA_JF_PASS_THROUGH)
{
@@ -2385,7 +2399,9 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int
idx,
{
code = POINTER_PLUS_EXPR;
src_idx = ipa_get_jf_ancestor_formal_id (jfunc);
- unsigned HOST_WIDE_INT offset = ipa_get_jf_ancestor_offset (jfunc) /
BITS_PER_UNIT;
+ unsigned HOST_WIDE_INT offset
+ = ipa_get_jf_ancestor_offset (jfunc) / BITS_PER_UNIT;
+ only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset);
operand = build_int_cstu (size_type_node, offset);
}
@@ -2403,16 +2419,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs,
int idx,
and we store it in jump function during analysis stage. */
if (src_lats->bits_lattice.bottom_p ()
- && jfunc->bits)
- return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
- precision);
+ || (only_for_nonzero && !src_lats->bits_lattice.known_nonzero_p ()))
+ {
+ if (jfunc->bits)
+ return dest_lattice->meet_with (jfunc->bits->value,
+ jfunc->bits->mask, precision);
+ else
+ return dest_lattice->set_to_bottom ();
+ }
else
return dest_lattice->meet_with (src_lats->bits_lattice, precision, sgn,
code, operand);
}
-
- else if (jfunc->type == IPA_JF_ANCESTOR)
- return dest_lattice->set_to_bottom ();
else if (jfunc->bits)
return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
precision);
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index bc5643206b9..ef9e353764c 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -357,6 +357,8 @@ ipa_print_node_jump_functions_for_edge (FILE *f, struct
cgraph_edge *cs)
jump_func->value.ancestor.offset);
if (jump_func->value.ancestor.agg_preserved)
fprintf (f, ", agg_preserved");
+ if (jump_func->value.ancestor.keep_null)
+ fprintf (f, ", keep_null");
fprintf (f, "\n");
}
@@ -601,12 +603,13 @@ ipa_set_jf_arith_pass_through (struct ipa_jump_func
*jfunc, int formal_id,
static void
ipa_set_ancestor_jf (struct ipa_jump_func *jfunc, HOST_WIDE_INT offset,
- int formal_id, bool agg_preserved)
+ int formal_id, bool agg_preserved, bool keep_null)
{
jfunc->type = IPA_JF_ANCESTOR;
jfunc->value.ancestor.formal_id = formal_id;
jfunc->value.ancestor.offset = offset;
jfunc->value.ancestor.agg_preserved = agg_preserved;
+ jfunc->value.ancestor.keep_null = keep_null;
}
/* Get IPA BB information about the given BB. FBI is the context of analyzis
@@ -1438,7 +1441,8 @@ compute_complex_assign_jump_func (struct
ipa_func_body_info *fbi,
index = ipa_get_param_decl_index (info, SSA_NAME_VAR (ssa));
if (index >= 0 && param_type && POINTER_TYPE_P (param_type))
ipa_set_ancestor_jf (jfunc, offset, index,
- parm_ref_data_pass_through_p (fbi, index, call, ssa));
+ parm_ref_data_pass_through_p (fbi, index, call, ssa),
+ false);
}
/* Extract the base, offset and MEM_REF expression from a statement ASSIGN if
@@ -1564,7 +1568,8 @@ compute_complex_ancestor_jump_func (struct
ipa_func_body_info *fbi,
}
ipa_set_ancestor_jf (jfunc, offset, index,
- parm_ref_data_pass_through_p (fbi, index, call, parm));
+ parm_ref_data_pass_through_p (fbi, index, call, parm),
+ true);
}
/* Inspect the given TYPE and return true iff it has the same structure (the
@@ -3250,6 +3255,7 @@ update_jump_functions_after_inlining (struct cgraph_edge
*cs,
dst->value.ancestor.offset += src->value.ancestor.offset;
dst->value.ancestor.agg_preserved &=
src->value.ancestor.agg_preserved;
+ dst->value.ancestor.keep_null |= src->value.ancestor.keep_null;
}
else
ipa_set_jf_unknown (dst);
@@ -3327,7 +3333,8 @@ update_jump_functions_after_inlining (struct cgraph_edge
*cs,
ipa_set_ancestor_jf (dst,
ipa_get_jf_ancestor_offset (src),
ipa_get_jf_ancestor_formal_id (src),
- agg_p);
+ agg_p,
+ ipa_get_jf_ancestor_keep_null (src));
break;
}
default:
@@ -4758,6 +4765,7 @@ ipa_write_jump_function (struct output_block *ob,
streamer_write_uhwi (ob, jump_func->value.ancestor.formal_id);
bp = bitpack_create (ob->main_stream);
bp_pack_value (&bp, jump_func->value.ancestor.agg_preserved, 1);
+ bp_pack_value (&bp, jump_func->value.ancestor.keep_null, 1);
streamer_write_bitpack (&bp);
break;
default:
@@ -4883,7 +4891,9 @@ ipa_read_jump_function (class lto_input_block *ib,
int formal_id = streamer_read_uhwi (ib);
struct bitpack_d bp = streamer_read_bitpack (ib);
bool agg_preserved = bp_unpack_value (&bp, 1);
- ipa_set_ancestor_jf (jump_func, offset, formal_id, agg_preserved);
+ bool keep_null = bp_unpack_value (&bp, 1);
+ ipa_set_ancestor_jf (jump_func, offset, formal_id, agg_preserved,
+ keep_null);
break;
}
default:
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index ba49843a510..4e243bcfe19 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -143,6 +143,8 @@ struct GTY(()) ipa_ancestor_jf_data
int formal_id;
/* Flag with the same meaning like agg_preserve in ipa_pass_through_data. */
unsigned agg_preserved : 1;
+ /* When set, the operation should not have any effect on NULL pointers. */
+ unsigned keep_null : 1;
};
/* A jump function for an aggregate part at a given offset, which describes how
@@ -438,6 +440,17 @@ ipa_get_jf_ancestor_type_preserved (struct ipa_jump_func
*jfunc)
return jfunc->value.ancestor.agg_preserved;
}
+/* Return if jfunc represents an operation whether we first check the formal
+ parameter for non-NULLness unless it does not matter because the offset is
+ zero anyway. */
+
+static inline bool
+ipa_get_jf_ancestor_keep_null (struct ipa_jump_func *jfunc)
+{
+ gcc_checking_assert (jfunc->type == IPA_JF_ANCESTOR);
+ return jfunc->value.ancestor.keep_null;
+}
+
/* Class for allocating a bundle of various potentially known properties about
actual arguments of a particular call on stack for the usual case and on
heap only if there are unusually many arguments. The data is deallocated
diff --git a/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
b/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
new file mode 100644
index 00000000000..e2fbb45d3cc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -Wno-pointer-to-int-cast" } */
+
+struct b {int b;};
+struct a {int a; struct b b;};
+
+long i;
+
+__attribute__ ((noinline))
+static void test2 (struct b *b)
+{
+ if (((int)b)&4)
+ __builtin_abort ();
+}
+
+__attribute__ ((noinline))
+static void
+test (struct a *a)
+{
+ test2(a? &a->b : 0);
+}
+
+int
+main()
+{
+ test(0);
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
b/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
new file mode 100644
index 00000000000..ae1b905af81
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-ipa-bit-cp -fdump-tree-optimized" } */
+
+struct b {int b;};
+struct a {int a; struct b b;};
+
+void remove_any_mention (void);
+
+__attribute__ ((noinline))
+static void test2 (struct b *b)
+{
+ if (b)
+ remove_any_mention ();
+}
+
+__attribute__ ((noinline))
+static void
+test (struct a *a)
+{
+ test2(a? &a->b : 0);
+}
+
+int
+foo()
+{
+ test(0);
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "remove_any_mention" "optimized" } } */
--
2.33.1
