From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 6B035385840B for ; Mon, 29 Nov 2021 18:45:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6B035385840B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 4822C21763; Mon, 29 Nov 2021 18:45:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1638211531; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=N2gPlx9khaSXGzrqFoHCCYKvXwaCVQMqwDbGzP2FKUA=; b=y1omk9LThROlm4yeZ2mRkCYzcjKVvpOi9R1a3V74WrMuf6U7YZ9RBj0watdDoUj9EXJPz2 88QO6JF1Z8vGfME+xnrSf6Je2AtduQl2E/qj1ISPD0fwGb4I/hEKmC+l3RIdpkoNI8PXBH uZn9g9QcmZcqHOK+5gMcGbD3D1WX9+8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1638211531; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=N2gPlx9khaSXGzrqFoHCCYKvXwaCVQMqwDbGzP2FKUA=; b=lwa/wtiVxg6Q2cYABYRqke8jKQ7wYmwwSEy4xfJRCmj5ANL3CyWWtwf+K3OuaifL2Z96As PVfRiZ2v4DlH8TDw== Received: from suse.cz (virgil.suse.cz [10.100.13.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 34ABBA3B87; Mon, 29 Nov 2021 18:45:31 +0000 (UTC) From: Martin Jambor To: Jan Hubicka Cc: GCC Patches Subject: Re: [PATCH] ipa: Careful processing ANCESTOR jump functions and NULL pointers (PR 103083) In-Reply-To: <20211127133845.GA25962@kam.mff.cuni.cz> References: <20211127133845.GA25962@kam.mff.cuni.cz> User-Agent: Notmuch/0.34.1 (https://notmuchmail.org) Emacs/27.2 (x86_64-suse-linux-gnu) Date: Mon, 29 Nov 2021 19:45:31 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Nov 2021 18:45:35 -0000 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 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 * 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