Hello,
I would like to ping the patch below.
Martin
On Mon, Nov 29 2021, Martin Jambor wrote:
> 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
