> 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....
> +/* 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
> @@ -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
> + 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.
> @@ -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...
Also with the flags, I think you want to modify ipa-cp so NULL pointers
gets propagated acroess the ancestor jump functions.
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.
Honza
> default:
> @@ -4758,6 +4764,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 +4890,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 42842d9466a..8f0039d1ef8 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.c
> b/gcc/testsuite/gcc.dg/ipa/pr103083.c
> new file mode 100644
> index 00000000000..e2fbb45d3cc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr103083.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;
> +}
> --
> 2.33.1
>
