From patchwork Fri Nov 26 14:00:24 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 48191 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A161B3857C7F for ; Fri, 26 Nov 2021 14:00:53 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id B22B63857C56 for ; Fri, 26 Nov 2021 14:00:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B22B63857C56 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-out2.suse.de (Postfix) with ESMTP id 1AD231FD38; Fri, 26 Nov 2021 14:00:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1637935225; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type; bh=Evbmw3OSzAUpI8k7Q/80ErZDb0/23lu8w7C0oNT4k0g=; b=JnsjGGsH1SjTRJfM7yZMWwUleQzw0m4cp6yMSKATOCrYjmhn5FgtazI7WVycE5Oo/bOuyG PHHC775uPINcH0jhrGHaGfARqPjFtzKUdO1vrzw36KRuWaL4DEuuUCMP+Dky+Slf8sYDsS jY8p9CoLmK5HcUP39fL9dDCCWz9htu0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1637935225; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type; bh=Evbmw3OSzAUpI8k7Q/80ErZDb0/23lu8w7C0oNT4k0g=; b=JURrCNRTFF/yHqEaH/HneuDICbH87hJnDAhmJPLlMyc7XEIaYx/drhYrA2MZLgY2bOTeiO eJSGiAr5u+q9E7BA== 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 055F0A3B83; Fri, 26 Nov 2021 14:00:25 +0000 (UTC) From: Martin Jambor To: GCC Patches Subject: [PATCH] ipa: Careful processing ANCESTOR jump functions and NULL pointers (PR 103083) User-Agent: Notmuch/0.34.1 (https://notmuchmail.org) Emacs/27.2 (x86_64-suse-linux-gnu) Date: Fri, 26 Nov 2021 15:00:24 +0100 Message-ID: MIME-Version: 1.0 X-Spam-Status: No, score=-11.2 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: , Cc: Jan Hubicka Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" 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. Bootstrapped, LTO-bootstrapped and tested on x86_64-linux. OK for master? Thanks, Martin gcc/ChangeLog: 2021-11-25 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. (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. gcc/testsuite/ChangeLog: 2021-11-25 Martin Jambor * gcc.dg/ipa/pr103083.c: New test. --- gcc/ipa-cp.c | 39 ++++++++++++++++++++--------- gcc/ipa-prop.c | 19 ++++++++++---- gcc/ipa-prop.h | 13 ++++++++++ gcc/testsuite/gcc.dg/ipa/pr103083.c | 28 +++++++++++++++++++++ 4 files changed, 82 insertions(+), 17 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083.c diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 703541d15cc..7e94bd78b98 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::eq_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0); +} + /* Convert operand to value, mask form. */ void @@ -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; + 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 (); + } 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 e85df0971fc..7d19e30e3f5 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 @@ -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; } 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; +}