From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id C950C385E00A for ; Tue, 30 Aug 2022 07:30:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C950C385E00A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 3885B1F8B8 for ; Tue, 30 Aug 2022 07:30:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1661844633; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Nc+MS7dvf/0l4aEMcUSb/XBuiyvjJvFhvUe3twfSa/I=; b=BZk8wG1B2kS74Asde4iHlxF7zmr5MQam/IO1TfVES+39oyxJg6V+5MXl6ibRi04tspD4li LJQT4RVUHPQxupHkcschAH/VdZdHng52JXszhs/gRfT/D+uVuGWPJ1AmoA6Qql+8ahXx7m YwkmZE2ei/5Y1m+e7cs1WdgcQzHIEeE= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1661844633; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Nc+MS7dvf/0l4aEMcUSb/XBuiyvjJvFhvUe3twfSa/I=; b=QOunBD2MAvTn43+yeHdF4+vrTnw0jv+AhXxRbrEsb/kf5hwQ8ufl9FmM3Jg1AUO0k04s4E aOc8Vd1MBkx6DFCA== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (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 32C4C2C141 for ; Tue, 30 Aug 2022 07:30:32 +0000 (UTC) Date: Tue, 30 Aug 2022 07:30:32 +0000 (UTC) From: Richard Biener To: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Make uninit PHI processing more consistent In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 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,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 List-Id: On Mon, 29 Aug 2022, Richard Biener wrote: > On Mon, 29 Aug 2022, Richard Biener wrote: > > [...] > > > The patch correctly diagnoses an uninitalized use of 'regnum' > > in store_bit_field_1 but also diagnoses an uninitialized use of > > best_match::m_best_candidate_len which I've chosen to silence > > without analyzing it in detail (I'm doing that right now). > > To followup myself this is > > cpp_hashnode *best_macro = bmm.get_best_meaningful_candidate (); > /* If a macro is the closest so far to NAME, use it, creating an > identifier tree node for it. */ > if (best_macro) > { > const char *id = (const char *)best_macro->ident.str; > tree macro_as_identifier > = get_identifier_with_length (id, best_macro->ident.len); > bm.set_best_so_far (macro_as_identifier, > bmm.get_best_distance (), > bmm.get_best_candidate_length ()); > > and > > edit_distance_t get_cutoff (size_t candidate_len) const > { > return ::get_edit_distance_cutoff (m_goal_len, candidate_len); > } > > candidate_t get_best_meaningful_candidate () const > { > /* If the edit distance is too high, the suggestion is likely to be > meaningless. */ > if (m_best_candidate) > { > edit_distance_t cutoff = get_cutoff (m_best_candidate_len); > if (m_best_distance > cutoff) > return NULL; > } > > where the connection between m_best_candidate_len being initialized > when m_best_candidate is not NULL is not visible. I think it's > OK to initialize the member together with m_best_candidate here. > > I'm reducing a testcase, but not sure where that will go. I've done that an the testcase shows the patch exactly working as intended, uncovering this previous false negative. False negative as far as GCC can see which doesn't get to see the connection of m_best_candidate != NULL meaning m_best_candidate_len being initialized. I've pushed the following with an added testcase and omitting the meanwhile fixed expmed.cc hunk. Richard. >From da1558c957799b4a46ddc2e7496150bf7a9926b5 Mon Sep 17 00:00:00 2001 From: Richard Biener Date: Mon, 29 Aug 2022 12:20:10 +0200 Subject: [PATCH] Make uninit PHI processing more consistent To: gcc-patches@gcc.gnu.org Currently the main working of the maybe-uninit pass is to scan over all PHIs with possibly undefined arguments, diagnosing whether there's a direct not guarded use. For not guarded uses in PHIs those are queued for later processing and to make the uninit analysis PHI def handling work, mark the PHI def as possibly uninitialized. But this happens only for those PHI uses that happen to be seen before a direct not guarded use and whether all arguments of a PHI node which are defined by a PHI are properly marked as maybe uninitialized depends on the processing order. The following changes the uninit pass to perform an RPO walk over the function, ensuring that PHI argument defs are visited before the PHI node (besides backedge uses which we ignore already), getting rid of the worklist. It also makes sure to process all PHI uses, but recording those that are properly guarded so they are not treated as maybe undefined when processing the PHI use later. Overall this should make behavior more consistent, avoid some false negative because of the previous early out and order issue, and avoid some false positive because of the missed recording of guarded PHI uses. The patch correctly diagnoses an uninitalized use of 'regnum' in store_bit_field_1 but also diagnoses an uninitialized use of best_match::m_best_candidate_len which I've chosen to silence without analyzing it in detail. * gimple-predicate-analysis.h (uninit_analysis::operator()): Remove. * gimple-predicate-analysis.cc (uninit_analysis::collect_phi_def_edges): Use phi_arg_set, simplify a bit. * tree-ssa-uninit.cc (defined_args): New global. (compute_uninit_opnds_pos): Mask with the recorded set of guarded maybe-uninitialized uses. (uninit_undef_val_t::operator()): Remove. (find_uninit_use): Process all PHI uses, recording the guarded ones and marking the PHI result as uninitialized consistently. (warn_uninitialized_phi): Adjust. (execute_late_warn_uninitialized): Get rid of the PHI worklist and instead walk the function in RPO order. * expmed.cc (store_bit_field_1): Initialize regnum. * spellcheck.h (best_match::m_best_candidate_len): Initialize. --- gcc/expmed.cc | 2 +- gcc/gimple-predicate-analysis.cc | 49 +++++----- gcc/gimple-predicate-analysis.h | 2 - gcc/spellcheck.h | 3 +- gcc/tree-ssa-uninit.cc | 149 +++++++++++++------------------ 5 files changed, 83 insertions(+), 122 deletions(-) diff --git a/gcc/expmed.cc b/gcc/expmed.cc index 8d7418be418..cdc0adb3892 100644 --- a/gcc/expmed.cc +++ b/gcc/expmed.cc @@ -794,7 +794,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum, words or to cope with mode punning between equal-sized modes. In the latter case, use subreg on the rhs side, not lhs. */ rtx sub; - HOST_WIDE_INT regnum; + HOST_WIDE_INT regnum = 0; poly_uint64 regsize = REGMODE_NATURAL_SIZE (GET_MODE (op0)); if (known_eq (bitnum, 0U) && known_eq (bitsize, GET_MODE_BITSIZE (GET_MODE (op0)))) diff --git a/gcc/gimple-predicate-analysis.cc b/gcc/gimple-predicate-analysis.cc index e388bb37685..3f9b80d9128 100644 --- a/gcc/gimple-predicate-analysis.cc +++ b/gcc/gimple-predicate-analysis.cc @@ -543,35 +543,13 @@ uninit_analysis::collect_phi_def_edges (gphi *phi, basic_block cd_root, return; unsigned n = gimple_phi_num_args (phi); + unsigned opnds_arg_phi = m_eval.phi_arg_set (phi); for (unsigned i = 0; i < n; i++) { - edge opnd_edge = gimple_phi_arg_edge (phi, i); - tree opnd = gimple_phi_arg_def (phi, i); - - if (TREE_CODE (opnd) == SSA_NAME) - { - gimple *def = SSA_NAME_DEF_STMT (opnd); - - if (!m_eval (opnd)) - { - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, - "\tFound def edge %i -> %i for cd_root %i " - "and operand %u of: ", - opnd_edge->src->index, opnd_edge->dest->index, - cd_root->index, i); - print_gimple_stmt (dump_file, phi, 0); - } - edges->safe_push (opnd_edge); - } - else if (gimple_code (def) == GIMPLE_PHI - && dominated_by_p (CDI_DOMINATORS, gimple_bb (def), cd_root)) - collect_phi_def_edges (as_a (def), cd_root, edges, - visited); - } - else + if (!MASK_TEST_BIT (opnds_arg_phi, i)) { + /* Add the edge for a not maybe-undefined edge value. */ + edge opnd_edge = gimple_phi_arg_edge (phi, i); if (dump_file && (dump_flags & TDF_DETAILS)) { fprintf (dump_file, @@ -581,9 +559,22 @@ uninit_analysis::collect_phi_def_edges (gphi *phi, basic_block cd_root, cd_root->index, i); print_gimple_stmt (dump_file, phi, 0); } - - if (!m_eval (opnd)) - edges->safe_push (opnd_edge); + edges->safe_push (opnd_edge); + continue; + } + else + { + tree opnd = gimple_phi_arg_def (phi, i); + if (TREE_CODE (opnd) == SSA_NAME) + { + gimple *def = SSA_NAME_DEF_STMT (opnd); + if (gimple_code (def) == GIMPLE_PHI + && dominated_by_p (CDI_DOMINATORS, gimple_bb (def), cd_root)) + /* Process PHI defs of maybe-undefined edge values + recursively. */ + collect_phi_def_edges (as_a (def), cd_root, edges, + visited); + } } } } diff --git a/gcc/gimple-predicate-analysis.h b/gcc/gimple-predicate-analysis.h index 1ca6ab1f7e4..48da8f70efd 100644 --- a/gcc/gimple-predicate-analysis.h +++ b/gcc/gimple-predicate-analysis.h @@ -104,8 +104,6 @@ class uninit_analysis { typedef unsigned phi_arg_set_t; - /* Return true if the argument is an expression of interest. */ - virtual bool operator()(tree) = 0; /* Return a bitset of PHI arguments of interest. By default returns bitset with a bit set for each argument. Should be called in the overriden function first and, if nonzero, the result then diff --git a/gcc/spellcheck.h b/gcc/spellcheck.h index 3706de38a9d..1b76a54b678 100644 --- a/gcc/spellcheck.h +++ b/gcc/spellcheck.h @@ -95,7 +95,8 @@ class best_match : m_goal (goal_traits::get_string (goal)), m_goal_len (goal_traits::get_length (goal)), m_best_candidate (NULL), - m_best_distance (best_distance_so_far) + m_best_distance (best_distance_so_far), + m_best_candidate_len (0) {} /* Compare the edit distance between CANDIDATE and m_goal, diff --git a/gcc/tree-ssa-uninit.cc b/gcc/tree-ssa-uninit.cc index 450ff0fd062..3e5816f1ebb 100644 --- a/gcc/tree-ssa-uninit.cc +++ b/gcc/tree-ssa-uninit.cc @@ -56,7 +56,8 @@ along with GCC; see the file COPYING3. If not see /* Pointer set of potentially undefined ssa names, i.e., ssa names that are defined by phi with operands that are not defined or potentially undefined. */ -static hash_set *possibly_undefined_names = 0; +static hash_set *possibly_undefined_names; +static hash_map *defined_args; /* Returns the first bit position (starting from LSB) in mask that is non zero. Returns -1 if the mask is empty. */ @@ -1131,6 +1132,9 @@ compute_uninit_opnds_pos (gphi *phi) MASK_SET_BIT (uninit_opnds, i); } } + /* If we have recorded guarded uses of may-uninit values mask those. */ + if (auto *def_mask = defined_args->get (phi)) + uninit_opnds &= ~*def_mask; return uninit_opnds; } @@ -1139,21 +1143,9 @@ compute_uninit_opnds_pos (gphi *phi) struct uninit_undef_val_t: public uninit_analysis::func_t { - virtual bool operator()(tree) override; virtual unsigned phi_arg_set (gphi *) override; }; -/* Return true if the argument is an expression of interest. */ - -bool -uninit_undef_val_t::operator()(tree val) -{ - if (TREE_CODE (val) == SSA_NAME) - return uninit_undefined_value_p (val); - - return false; -} - /* Return a bitset of PHI arguments of interest. */ unsigned @@ -1166,14 +1158,10 @@ uninit_undef_val_t::phi_arg_set (gphi *phi) uninitialized variable defined by PHI and returns a use statement if the use is not properly guarded. It returns NULL if all uses are guarded. UNINIT_OPNDS is a bitvector - holding the position(s) of uninit PHI operands. WORKLIST - is the vector of candidate phis that may be updated by this - function. ADDED_TO_WORKLIST is the pointer set tracking - if the new phi is already in the worklist. */ + holding the position(s) of uninit PHI operands. */ static gimple * -find_uninit_use (gphi *phi, unsigned uninit_opnds, - vec *worklist, hash_set *added_to_worklist) +find_uninit_use (gphi *phi, unsigned uninit_opnds) { /* The Boolean predicate guarding the PHI definition. Initialized lazily from PHI in the first call to is_use_guarded() and cached @@ -1184,6 +1172,7 @@ find_uninit_use (gphi *phi, unsigned uninit_opnds, use_operand_p use_p; imm_use_iterator iter; tree phi_result = gimple_phi_result (phi); + gimple *uninit_use = NULL; FOR_EACH_IMM_USE_FAST (use_p, iter, phi_result) { gimple *use_stmt = USE_STMT (use_p); @@ -1202,11 +1191,32 @@ find_uninit_use (gphi *phi, unsigned uninit_opnds, if (e->flags & EDGE_DFS_BACK) continue; } + else if (uninit_use) + /* Only process the first real uninitialized use, but continue + looking for unguarded uses in PHIs. */ + continue; else use_bb = gimple_bb (use_stmt); if (def_preds.is_use_guarded (use_stmt, use_bb, phi, uninit_opnds)) - continue; + { + /* For a guarded use in a PHI record the PHI argument as + initialized. */ + if (gphi *use_phi = dyn_cast (use_stmt)) + { + unsigned idx = PHI_ARG_INDEX_FROM_USE (use_p); + if (idx < uninit_analysis::func_t::max_phi_args) + { + bool existed_p; + auto &def_mask + = defined_args->get_or_insert (use_phi, &existed_p); + if (!existed_p) + def_mask = 0; + MASK_SET_BIT (def_mask, idx); + } + } + continue; + } if (dump_file && (dump_flags & TDF_DETAILS)) { @@ -1214,59 +1224,35 @@ find_uninit_use (gphi *phi, unsigned uninit_opnds, use_bb->index); print_gimple_stmt (dump_file, use_stmt, 0); } - /* Found one real use, return. */ - if (gimple_code (use_stmt) != GIMPLE_PHI) - return use_stmt; - - /* Found a phi use that is not guarded, - add the phi to the worklist. */ - if (!added_to_worklist->add (as_a (use_stmt))) - { - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, "[WORKLIST]: Update worklist with phi: "); - print_gimple_stmt (dump_file, use_stmt, 0); - } - - worklist->safe_push (as_a (use_stmt)); - possibly_undefined_names->add (phi_result); - } + /* Found a phi use that is not guarded, mark the phi_result as + possibly undefined. */ + if (is_a (use_stmt)) + possibly_undefined_names->add (phi_result); + else + uninit_use = use_stmt; } - return NULL; + return uninit_use; } /* Look for inputs to PHI that are SSA_NAMEs that have empty definitions and gives warning if there exists a runtime path from the entry to a use of the PHI def that does not contain a definition. In other words, the warning is on the real use. The more dead paths that can be pruned - by the compiler, the fewer false positives the warning is. WORKLIST - is a vector of candidate phis to be examined. ADDED_TO_WORKLIST is - a pointer set tracking if the new phi is added to the worklist or not. */ + by the compiler, the fewer false positives the warning is. */ static void -warn_uninitialized_phi (gphi *phi, vec *worklist, - hash_set *added_to_worklist) +warn_uninitialized_phi (gphi *phi, unsigned uninit_opnds) { - /* Don't look at virtual operands. */ - if (virtual_operand_p (gimple_phi_result (phi))) - return; - - unsigned uninit_opnds = compute_uninit_opnds_pos (phi); - if (MASK_EMPTY (uninit_opnds)) - return; - if (dump_file && (dump_flags & TDF_DETAILS)) { fprintf (dump_file, "Examining phi: "); print_gimple_stmt (dump_file, phi, 0); } - gimple *uninit_use_stmt = find_uninit_use (phi, uninit_opnds, - worklist, added_to_worklist); + gimple *uninit_use_stmt = find_uninit_use (phi, uninit_opnds); - /* All uses are properly guarded but a new PHI may have been added - to WORKLIST. */ + /* All uses are properly guarded. */ if (!uninit_use_stmt) return; @@ -1341,10 +1327,6 @@ public: static void execute_late_warn_uninitialized (function *fun) { - basic_block bb; - gphi_iterator gsi; - vec worklist = vNULL; - calculate_dominance_info (CDI_DOMINATORS); calculate_dominance_info (CDI_POST_DOMINATORS); @@ -1352,6 +1334,8 @@ execute_late_warn_uninitialized (function *fun) unreachable blocks. */ set_all_edges_as_executable (fun); mark_dfs_back_edges (fun); + int *rpo = XNEWVEC (int, n_basic_blocks_for_fn (fun)); + int n = pre_and_rev_post_order_compute_fn (fun, NULL, rpo, false); /* Re-do the plain uninitialized variable check, as optimization may have straightened control flow. Do this first so that we don't accidentally @@ -1361,11 +1345,15 @@ execute_late_warn_uninitialized (function *fun) timevar_push (TV_TREE_UNINIT); possibly_undefined_names = new hash_set; - hash_set added_to_worklist; - - /* Initialize worklist */ - FOR_EACH_BB_FN (bb, fun) - for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi)) + defined_args = new hash_map; + + /* Walk the CFG in RPO order so we visit PHIs with defs that are + possibly uninitialized from other PHIs after those. The uninit + predicate analysis will then expand the PHIs predicate with + the predicates of the edges from such PHI defs. */ + for (int i = 0; i < n; ++i) + for (auto gsi = gsi_start_phis (BASIC_BLOCK_FOR_FN (fun, rpo[i])); + !gsi_end_p (gsi); gsi_next (&gsi)) { gphi *phi = gsi.phi (); @@ -1373,35 +1361,18 @@ execute_late_warn_uninitialized (function *fun) if (virtual_operand_p (gimple_phi_result (phi))) continue; - unsigned n = gimple_phi_num_args (phi); - for (unsigned i = 0; i < n; ++i) - { - tree op = gimple_phi_arg_def (phi, i); - if (TREE_CODE (op) == SSA_NAME && uninit_undefined_value_p (op)) - { - worklist.safe_push (phi); - added_to_worklist.add (phi); - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, "[WORKLIST]: add to initial list " - "for operand %u of: ", i); - print_gimple_stmt (dump_file, phi, 0); - } - break; - } - } - } + unsigned uninit_opnds = compute_uninit_opnds_pos (phi); + if (MASK_EMPTY (uninit_opnds)) + continue; - while (worklist.length () != 0) - { - gphi *cur_phi = 0; - cur_phi = worklist.pop (); - warn_uninitialized_phi (cur_phi, &worklist, &added_to_worklist); - } + warn_uninitialized_phi (phi, uninit_opnds); + } - worklist.release (); + free (rpo); delete possibly_undefined_names; possibly_undefined_names = NULL; + delete defined_args; + defined_args = NULL; free_dominance_info (CDI_POST_DOMINATORS); timevar_pop (TV_TREE_UNINIT); } -- 2.35.3