From patchwork Thu May 12 01:46:04 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexandre Oliva X-Patchwork-Id: 53828 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 CB6703856257 for ; Thu, 12 May 2022 01:46:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CB6703856257 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1652320015; bh=PH9iiEMsvHKBHBdRq2Y24i7LuBk5OP+rYMZvpTtYu+c=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=IHUc0PNNGV0ihVjySLH4KyX0g2cvDDXaCNmSm57fOHtk97SBcEAohHyjKCSmGEKi9 6u+tWQbra+/V3DM19Pw/6cI5m2R1W7sr90giU4jkEuGT/TS9ZwfqPoYIkhmIMQpvo/ V+aZyIqN6JKEs6BmG+CZOuvkjUMZsJVnIi04nSwY= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTPS id C56423856245 for ; Thu, 12 May 2022 01:46:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C56423856245 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id D0D21116A06; Wed, 11 May 2022 21:46:15 -0400 (EDT) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id QQFNHg8Gf5sa; Wed, 11 May 2022 21:46:15 -0400 (EDT) Received: from free.home (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id 211EF116809; Wed, 11 May 2022 21:46:14 -0400 (EDT) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 24C1k4Vs106012 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 11 May 2022 22:46:05 -0300 To: gcc-patches@gcc.gnu.org Subject: [PATCH] Avoid visiting newly-created blocks in harden-conditionals Organization: Free thinker, does not speak for AdaCore Date: Wed, 11 May 2022 22:46:04 -0300 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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-Patchwork-Original-From: Alexandre Oliva via Gcc-patches From: Alexandre Oliva Reply-To: Alexandre Oliva Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Reverse iteration over blocks, in gimple-harden-conditionals.cc, was supposed to avoid visiting blocks introduced by hardening and introducing further reversed conditionals and traps for them, but newly-created blocks may be inserted before the current block, as shown by the PR105455 testcase. New blocks use and increment last block number, so test the block index against the initial last block number to skip new blocks. Regstrapped on x86_64-linux-gnu. Ok to install? for gcc/ChangeLog * gimple-harden-conditionals.cc (pass_harden_conditional_branches::execute): Skip new blocks. (pass_harden_compares::execute): Likewise. --- gcc/gimple-harden-conditionals.cc | 401 +++++++++++++++++++------------------ 1 file changed, 211 insertions(+), 190 deletions(-) diff --git a/gcc/gimple-harden-conditionals.cc b/gcc/gimple-harden-conditionals.cc index c7e5e077a74f6..28c4810f0a78e 100644 --- a/gcc/gimple-harden-conditionals.cc +++ b/gcc/gimple-harden-conditionals.cc @@ -301,9 +301,18 @@ insert_edge_check_and_trap (location_t loc, edge e, unsigned int pass_harden_conditional_branches::execute (function *fun) { + int orig_last_block = last_basic_block_for_fn (fun); + basic_block bb; FOR_EACH_BB_REVERSE_FN (bb, fun) { + /* Despite our backwards iteration on basic blocks, sometimes + split_edge will insert the new block before the block we're + hardening, and then we'd harden the hardening block. Skip + newly-created blocks to avoid that. */ + if (bb->index >= orig_last_block) + continue; + gimple_stmt_iterator gsi = gsi_last_bb (bb); if (gsi_end_p (gsi)) @@ -383,6 +392,8 @@ non_eh_succ_edge (basic_block bb, edge *ehp = NULL) unsigned int pass_harden_compares::execute (function *fun) { + int orig_last_block = last_basic_block_for_fn (fun); + basic_block bb; /* Go backwards over BBs and stmts, so that, even if we split the block multiple times to insert a cond_expr after each compare we @@ -390,198 +401,208 @@ pass_harden_compares::execute (function *fun) stmt exactly once, and not visiting newly-added blocks or stmts. */ FOR_EACH_BB_REVERSE_FN (bb, fun) - for (gimple_stmt_iterator gsi = gsi_last_bb (bb); - !gsi_end_p (gsi); gsi_prev (&gsi)) - { - gassign *asgn = dyn_cast (gsi_stmt (gsi)); - if (!asgn) - continue; - - /* Turn: - - z = x op y; - - into: - - z = x op y; - z' = x' cop y'; - if (z == z') __builtin_trap (); - - where cop is a complementary boolean operation to op; and x' - and y' hold the same value as x and y, but in a way that does - not enable the compiler to optimize the redundant compare - away. - */ - - enum tree_code op = gimple_assign_rhs_code (asgn); - - enum tree_code cop; - - switch (op) - { - case EQ_EXPR: - case NE_EXPR: - case GT_EXPR: - case GE_EXPR: - case LT_EXPR: - case LE_EXPR: - case LTGT_EXPR: - case UNEQ_EXPR: - case UNGT_EXPR: - case UNGE_EXPR: - case UNLT_EXPR: - case UNLE_EXPR: - case ORDERED_EXPR: - case UNORDERED_EXPR: - cop = invert_tree_comparison (op, - HONOR_NANS - (gimple_assign_rhs1 (asgn))); - - if (cop == ERROR_MARK) - /* ??? Can we do better? */ - continue; + { + /* Despite our backwards iteration on basic blocks, sometimes + split_edge will insert the new block before the block we're + hardening, and then we'd harden the hardening block. Skip + newly-created blocks to avoid that. */ + if (bb->index >= orig_last_block) + continue; - break; - - /* ??? Maybe handle these too? */ - case TRUTH_NOT_EXPR: - /* ??? The code below assumes binary ops, it would have to - be adjusted for TRUTH_NOT_EXPR, since it's unary. */ - case TRUTH_ANDIF_EXPR: - case TRUTH_ORIF_EXPR: - case TRUTH_AND_EXPR: - case TRUTH_OR_EXPR: - case TRUTH_XOR_EXPR: - default: + for (gimple_stmt_iterator gsi = gsi_last_bb (bb); + !gsi_end_p (gsi); gsi_prev (&gsi)) + { + gassign *asgn = dyn_cast (gsi_stmt (gsi)); + if (!asgn) + continue; + + /* Turn: + + z = x op y; + + into: + + z = x op y; + z' = x' cop y'; + if (z == z') __builtin_trap (); + + where cop is a complementary boolean operation to op; and x' + and y' hold the same value as x and y, but in a way that does + not enable the compiler to optimize the redundant compare + away. + */ + + enum tree_code op = gimple_assign_rhs_code (asgn); + + enum tree_code cop; + + switch (op) + { + case EQ_EXPR: + case NE_EXPR: + case GT_EXPR: + case GE_EXPR: + case LT_EXPR: + case LE_EXPR: + case LTGT_EXPR: + case UNEQ_EXPR: + case UNGT_EXPR: + case UNGE_EXPR: + case UNLT_EXPR: + case UNLE_EXPR: + case ORDERED_EXPR: + case UNORDERED_EXPR: + cop = invert_tree_comparison (op, + HONOR_NANS + (gimple_assign_rhs1 (asgn))); + + if (cop == ERROR_MARK) + /* ??? Can we do better? */ + continue; + + break; + + /* ??? Maybe handle these too? */ + case TRUTH_NOT_EXPR: + /* ??? The code below assumes binary ops, it would have to + be adjusted for TRUTH_NOT_EXPR, since it's unary. */ + case TRUTH_ANDIF_EXPR: + case TRUTH_ORIF_EXPR: + case TRUTH_AND_EXPR: + case TRUTH_OR_EXPR: + case TRUTH_XOR_EXPR: + default: + continue; + } + + /* These are the operands for the verification. */ + tree lhs = gimple_assign_lhs (asgn); + tree op1 = gimple_assign_rhs1 (asgn); + tree op2 = gimple_assign_rhs2 (asgn); + location_t loc = gimple_location (asgn); + + /* Vector booleans can't be used in conditional branches. ??? + Can we do better? How to reduce compare and + reversed-compare result vectors to a single boolean? */ + if (VECTOR_TYPE_P (TREE_TYPE (op1))) continue; - } - - /* These are the operands for the verification. */ - tree lhs = gimple_assign_lhs (asgn); - tree op1 = gimple_assign_rhs1 (asgn); - tree op2 = gimple_assign_rhs2 (asgn); - location_t loc = gimple_location (asgn); - - /* Vector booleans can't be used in conditional branches. ??? - Can we do better? How to reduce compare and - reversed-compare result vectors to a single boolean? */ - if (VECTOR_TYPE_P (TREE_TYPE (op1))) - continue; - - /* useless_type_conversion_p enables conversions from 1-bit - integer types to boolean to be discarded. */ - gcc_checking_assert (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE - || (INTEGRAL_TYPE_P (TREE_TYPE (lhs)) - && TYPE_PRECISION (TREE_TYPE (lhs)) == 1)); - - tree rhs = copy_ssa_name (lhs); - - gimple_stmt_iterator gsi_split = gsi; - /* Don't separate the original assignment from debug stmts - that might be associated with it, and arrange to split the - block after debug stmts, so as to make sure the split block - won't be debug stmts only. */ - gsi_next_nondebug (&gsi_split); - - bool throwing_compare_p = stmt_ends_bb_p (asgn); - if (throwing_compare_p) - { - basic_block nbb = split_edge (non_eh_succ_edge - (gimple_bb (asgn))); - gsi_split = gsi_start_bb (nbb); - - if (dump_file) - fprintf (dump_file, - "Splitting non-EH edge from block %i into %i" - " after a throwing compare\n", - gimple_bb (asgn)->index, nbb->index); - } - - bool same_p = (op1 == op2); - op1 = detach_value (loc, &gsi_split, op1); - op2 = same_p ? op1 : detach_value (loc, &gsi_split, op2); - - gassign *asgnck = gimple_build_assign (rhs, cop, op1, op2); - gimple_set_location (asgnck, loc); - gsi_insert_before (&gsi_split, asgnck, GSI_SAME_STMT); - - /* We wish to insert a cond_expr after the compare, so arrange - for it to be at the end of a block if it isn't, and for it - to have a single successor in case there's more than - one, as in PR104975. */ - if (!gsi_end_p (gsi_split) - || !single_succ_p (gsi_bb (gsi_split))) - { - if (!gsi_end_p (gsi_split)) - gsi_prev (&gsi_split); - else - gsi_split = gsi_last_bb (gsi_bb (gsi_split)); - basic_block obb = gsi_bb (gsi_split); - basic_block nbb = split_block (obb, gsi_stmt (gsi_split))->dest; - gsi_next (&gsi_split); - gcc_checking_assert (gsi_end_p (gsi_split)); - - single_succ_edge (bb)->goto_locus = loc; - - if (dump_file) - fprintf (dump_file, - "Splitting block %i into %i" - " before the conditional trap branch\n", - obb->index, nbb->index); - } - - /* If the check assignment must end a basic block, we can't - insert the conditional branch in the same block, so split - the block again, and prepare to insert the conditional - branch in the new block. - - Also assign an EH region to the compare. Even though it's - unlikely that the hardening compare will throw after the - original compare didn't, the compiler won't even know that - it's the same compare operands, so add the EH edge anyway. */ - if (throwing_compare_p) - { - add_stmt_to_eh_lp (asgnck, lookup_stmt_eh_lp (asgn)); - make_eh_edges (asgnck); - - edge ckeh; - basic_block nbb = split_edge (non_eh_succ_edge - (gimple_bb (asgnck), &ckeh)); - gsi_split = gsi_start_bb (nbb); - - if (dump_file) - fprintf (dump_file, - "Splitting non-EH edge from block %i into %i after" - " the newly-inserted reversed throwing compare\n", - gimple_bb (asgnck)->index, nbb->index); - - if (!gimple_seq_empty_p (phi_nodes (ckeh->dest))) - { - edge aseh; - non_eh_succ_edge (gimple_bb (asgn), &aseh); - - gcc_checking_assert (aseh->dest == ckeh->dest); - - for (gphi_iterator psi = gsi_start_phis (ckeh->dest); - !gsi_end_p (psi); gsi_next (&psi)) - { - gphi *phi = psi.phi (); - add_phi_arg (phi, PHI_ARG_DEF_FROM_EDGE (phi, aseh), ckeh, - gimple_phi_arg_location_from_edge (phi, aseh)); - } - - if (dump_file) - fprintf (dump_file, - "Copying PHI args in EH block %i from %i to %i\n", - aseh->dest->index, aseh->src->index, ckeh->src->index); - } - } - - gcc_checking_assert (single_succ_p (gsi_bb (gsi_split))); - - insert_check_and_trap (loc, &gsi_split, EDGE_TRUE_VALUE, - EQ_EXPR, lhs, rhs); - } + + /* useless_type_conversion_p enables conversions from 1-bit + integer types to boolean to be discarded. */ + gcc_checking_assert (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE + || (INTEGRAL_TYPE_P (TREE_TYPE (lhs)) + && TYPE_PRECISION (TREE_TYPE (lhs)) == 1)); + + tree rhs = copy_ssa_name (lhs); + + gimple_stmt_iterator gsi_split = gsi; + /* Don't separate the original assignment from debug stmts + that might be associated with it, and arrange to split the + block after debug stmts, so as to make sure the split block + won't be debug stmts only. */ + gsi_next_nondebug (&gsi_split); + + bool throwing_compare_p = stmt_ends_bb_p (asgn); + if (throwing_compare_p) + { + basic_block nbb = split_edge (non_eh_succ_edge + (gimple_bb (asgn))); + gsi_split = gsi_start_bb (nbb); + + if (dump_file) + fprintf (dump_file, + "Splitting non-EH edge from block %i into %i" + " after a throwing compare\n", + gimple_bb (asgn)->index, nbb->index); + } + + bool same_p = (op1 == op2); + op1 = detach_value (loc, &gsi_split, op1); + op2 = same_p ? op1 : detach_value (loc, &gsi_split, op2); + + gassign *asgnck = gimple_build_assign (rhs, cop, op1, op2); + gimple_set_location (asgnck, loc); + gsi_insert_before (&gsi_split, asgnck, GSI_SAME_STMT); + + /* We wish to insert a cond_expr after the compare, so arrange + for it to be at the end of a block if it isn't, and for it + to have a single successor in case there's more than + one, as in PR104975. */ + if (!gsi_end_p (gsi_split) + || !single_succ_p (gsi_bb (gsi_split))) + { + if (!gsi_end_p (gsi_split)) + gsi_prev (&gsi_split); + else + gsi_split = gsi_last_bb (gsi_bb (gsi_split)); + basic_block obb = gsi_bb (gsi_split); + basic_block nbb = split_block (obb, gsi_stmt (gsi_split))->dest; + gsi_next (&gsi_split); + gcc_checking_assert (gsi_end_p (gsi_split)); + + single_succ_edge (bb)->goto_locus = loc; + + if (dump_file) + fprintf (dump_file, + "Splitting block %i into %i" + " before the conditional trap branch\n", + obb->index, nbb->index); + } + + /* If the check assignment must end a basic block, we can't + insert the conditional branch in the same block, so split + the block again, and prepare to insert the conditional + branch in the new block. + + Also assign an EH region to the compare. Even though it's + unlikely that the hardening compare will throw after the + original compare didn't, the compiler won't even know that + it's the same compare operands, so add the EH edge anyway. */ + if (throwing_compare_p) + { + add_stmt_to_eh_lp (asgnck, lookup_stmt_eh_lp (asgn)); + make_eh_edges (asgnck); + + edge ckeh; + basic_block nbb = split_edge (non_eh_succ_edge + (gimple_bb (asgnck), &ckeh)); + gsi_split = gsi_start_bb (nbb); + + if (dump_file) + fprintf (dump_file, + "Splitting non-EH edge from block %i into %i after" + " the newly-inserted reversed throwing compare\n", + gimple_bb (asgnck)->index, nbb->index); + + if (!gimple_seq_empty_p (phi_nodes (ckeh->dest))) + { + edge aseh; + non_eh_succ_edge (gimple_bb (asgn), &aseh); + + gcc_checking_assert (aseh->dest == ckeh->dest); + + for (gphi_iterator psi = gsi_start_phis (ckeh->dest); + !gsi_end_p (psi); gsi_next (&psi)) + { + gphi *phi = psi.phi (); + add_phi_arg (phi, PHI_ARG_DEF_FROM_EDGE (phi, aseh), ckeh, + gimple_phi_arg_location_from_edge (phi, aseh)); + } + + if (dump_file) + fprintf (dump_file, + "Copying PHI args in EH block %i from %i to %i\n", + aseh->dest->index, aseh->src->index, + ckeh->src->index); + } + } + + gcc_checking_assert (single_succ_p (gsi_bb (gsi_split))); + + insert_check_and_trap (loc, &gsi_split, EDGE_TRUE_VALUE, + EQ_EXPR, lhs, rhs); + } + } return 0; }