On Fri, 25 Oct 2024, Jakub Jelinek wrote:
> Hi!
>
> We have currently 3 different definitions of gcc_assert macro, one used most
> of the time (unless --disable-checking) which evaluates the condition at
> runtime and also checks it at runtime, then one for --disable-checking GCC
> 4.5+
> which looks like
> ((void)(UNLIKELY (!(EXPR)) ? __builtin_unreachable (), 0 : 0))
> and a fallback one
> ((void)(0 && (EXPR)))
> Now, the last one actually doesn't evaluate any of the side-effects in the
> argument, just quiets up unused var/parameter warnings.
> I've tried to replace the middle definition with
> ({ [[assume (EXPR)]]; (void) 0; })
> for compilers which support assume attribute and statement expressions
> (surprisingly quite a few spots use gcc_assert inside of comma expressions),
> but ran into PR117287, so for now such a change isn't being proposed.
>
> The following patch attempts to move important side-effects from gcc_assert
> arguments.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux with normal
> --enable-checking=yes,rtl,extra, plus additionally I've attempted to do
> x86_64-linux bootstrap with --disable-checking and gcc_assert changed to the
> ((void)(0 && (EXPR)))
> version when --disable-checking. That version ran into spurious middle-end
> warnings
> ../../gcc/../include/libiberty.h:733:36: error: argument to ‘alloca’ is too
> large [-Werror=alloca-larger-than=]
> ../../gcc/tree-ssa-reassoc.cc:5659:20: note: in expansion of macro
> ‘XALLOCAVEC’
> int op_num = ops.length ();
> int op_normal_num = op_num;
> gcc_assert (op_num > 0);
> int stmt_num = op_num - 1;
> gimple **stmts = XALLOCAVEC (gimple *, stmt_num);
> where we have gcc_assert exactly to work-around middle-end warnings.
> Guess I'd need to also disable -Werror for this experiment, which actually
> isn't a problem with unmodified system.h, because even for
> --disable-checking we use the __builtin_unreachable at least in
> stage2/stage3 and so the warnings aren't emitted, and even if it used
> [[assume ()]]; it would work too because in stage2/stage3 we could again
> rely on assume and statement expression support.
>
> Ok for trunk?
OK.
Thanks,
Richard.
> 2024-10-25 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/117249
> * tree-ssa-structalias.cc (insert_vi_for_tree): Move put calls out of
> gcc_assert.
> * lto-cgraph.cc (lto_symtab_encoder_delete_node): Likewise.
> * gimple-ssa-strength-reduction.cc (get_alternative_base,
> add_cand_for_stmt): Likewise.
> * tree-eh.cc (add_stmt_to_eh_lp_fn): Likewise.
> * except.cc (duplicate_eh_regions_1): Likewise.
> * tree-ssa-reassoc.cc (insert_operand_rank): Likewise.
> * config/nvptx/nvptx.cc (nvptx_expand_call): Use == rather than = in
> gcc_assert.
> * opts-common.cc (jobserver_info::disconnect): Call close outside of
> gcc_assert and only check result in it.
> (jobserver_info::return_token): Call write outside of gcc_assert and
> only check result in it.
> * genautomata.cc (output_default_latencies): Move j++ side-effect
> outside of gcc_assert.
> * tree-ssa-loop-ivopts.cc (get_alias_ptr_type_for_ptr_address): Use
> == rather than = in gcc_assert.
> * cgraph.cc (symbol_table::create_edge): Move ++edges_max_uid
> side-effect outside of gcc_assert.
> * pair-fusion.cc (pair_fusion_bb_info::fuse_pair): Call
> restrict_movement outside of gcc_assert and only check result in it.
> (pair_fusion::try_promote_writeback): Likewise.
>
> --- gcc/tree-ssa-structalias.cc.jj 2024-10-02 13:30:14.982371644 +0200
> +++ gcc/tree-ssa-structalias.cc 2024-10-23 10:42:02.584056165 +0200
> @@ -2986,7 +2986,8 @@ static void
> insert_vi_for_tree (tree t, varinfo_t vi)
> {
> gcc_assert (vi);
> - gcc_assert (!vi_for_tree->put (t, vi));
> + bool existed = vi_for_tree->put (t, vi);
> + gcc_assert (!existed);
> }
>
> /* Find the variable info for tree T in VI_FOR_TREE. If T does not
> --- gcc/lto-cgraph.cc.jj 2024-09-24 11:31:48.729621966 +0200
> +++ gcc/lto-cgraph.cc 2024-10-23 10:43:11.013085410 +0200
> @@ -155,7 +155,8 @@ lto_symtab_encoder_delete_node (lto_symt
> last_node = encoder->nodes.pop ();
> if (last_node.node != node)
> {
> - gcc_assert (encoder->map->put (last_node.node, index + 1));
> + bool existed = encoder->map->put (last_node.node, index + 1);
> + gcc_assert (existed);
>
> /* Move the last element to the original spot of NODE. */
> encoder->nodes[index] = last_node;
> --- gcc/gimple-ssa-strength-reduction.cc.jj 2024-09-02 09:43:28.806148076
> +0200
> +++ gcc/gimple-ssa-strength-reduction.cc 2024-10-23 10:38:24.386151585
> +0200
> @@ -474,7 +474,8 @@ get_alternative_base (tree base)
> aff.offset = 0;
> expr = aff_combination_to_tree (&aff);
>
> - gcc_assert (!alt_base_map->put (base, base == expr ? NULL : expr));
> + bool existed = alt_base_map->put (base, base == expr ? NULL : expr);
> + gcc_assert (!existed);
>
> return expr == base ? NULL : expr;
> }
> @@ -792,7 +793,8 @@ base_cand_from_table (tree base_in)
> static void
> add_cand_for_stmt (gimple *gs, slsr_cand_t c)
> {
> - gcc_assert (!stmt_cand_map->put (gs, c));
> + bool existed = stmt_cand_map->put (gs, c);
> + gcc_assert (!existed);
> }
>
> /* Given PHI which contains a phi statement, determine whether it
> --- gcc/tree-eh.cc.jj 2024-07-19 17:22:59.474096182 +0200
> +++ gcc/tree-eh.cc 2024-10-23 10:41:17.686693092 +0200
> @@ -76,7 +76,8 @@ add_stmt_to_eh_lp_fn (struct function *i
> if (!get_eh_throw_stmt_table (ifun))
> set_eh_throw_stmt_table (ifun, hash_map<gimple *, int>::create_ggc (31));
>
> - gcc_assert (!get_eh_throw_stmt_table (ifun)->put (t, num));
> + bool existed = get_eh_throw_stmt_table (ifun)->put (t, num);
> + gcc_assert (!existed);
> }
>
> /* Add statement T in the current function (cfun) to EH landing pad NUM. */
> --- gcc/except.cc.jj 2024-08-14 18:19:52.678848514 +0200
> +++ gcc/except.cc 2024-10-23 10:35:03.365003334 +0200
> @@ -541,7 +541,8 @@ duplicate_eh_regions_1 (struct duplicate
> eh_region new_r;
>
> new_r = gen_eh_region (old_r->type, outer);
> - gcc_assert (!data->eh_map->put (old_r, new_r));
> + bool existed = data->eh_map->put (old_r, new_r);
> + gcc_assert (!existed);
>
> switch (old_r->type)
> {
> @@ -586,7 +587,8 @@ duplicate_eh_regions_1 (struct duplicate
> continue;
>
> new_lp = gen_eh_landing_pad (new_r);
> - gcc_assert (!data->eh_map->put (old_lp, new_lp));
> + bool existed = data->eh_map->put (old_lp, new_lp);
> + gcc_assert (!existed);
>
> new_lp->post_landing_pad
> = data->label_map (old_lp->post_landing_pad, data->label_map_data);
> --- gcc/tree-ssa-reassoc.cc.jj 2024-09-25 16:06:33.849422365 +0200
> +++ gcc/tree-ssa-reassoc.cc 2024-10-23 10:41:39.307386374 +0200
> @@ -404,7 +404,8 @@ static inline void
> insert_operand_rank (tree e, int64_t rank)
> {
> gcc_assert (rank > 0);
> - gcc_assert (!operand_rank->put (e, rank));
> + bool existed = operand_rank->put (e, rank);
> + gcc_assert (!existed);
> }
>
> /* Given an expression E, return the rank of the expression. */
> --- gcc/config/nvptx/nvptx.cc.jj 2024-10-24 10:29:13.535951981 +0200
> +++ gcc/config/nvptx/nvptx.cc 2024-10-24 11:10:13.592174203 +0200
> @@ -1901,7 +1901,7 @@ nvptx_expand_call (rtx retval, rtx addre
> if (varargs)
> XVECEXP (pat, 0, vec_pos++) = gen_rtx_USE (VOIDmode, varargs);
>
> - gcc_assert (vec_pos = XVECLEN (pat, 0));
> + gcc_assert (vec_pos == XVECLEN (pat, 0));
>
> nvptx_emit_forking (parallel, true);
> emit_call_insn (pat);
> --- gcc/opts-common.cc.jj 2024-10-24 10:29:14.009945200 +0200
> +++ gcc/opts-common.cc 2024-10-24 11:25:17.323552274 +0200
> @@ -2156,7 +2156,8 @@ jobserver_info::disconnect ()
> {
> if (!pipe_path.empty ())
> {
> - gcc_assert (close (pipefd) == 0);
> + int res = close (pipefd);
> + gcc_assert (res == 0);
> pipefd = -1;
> }
> }
> @@ -2181,5 +2182,6 @@ jobserver_info::return_token ()
> {
> int fd = pipe_path.empty () ? wfd : pipefd;
> char c = 'G';
> - gcc_assert (write (fd, &c, 1) == 1);
> + int res = write (fd, &c, 1);
> + gcc_assert (res == 1);
> }
> --- gcc/genautomata.cc.jj 2024-02-24 22:52:31.999449280 +0100
> +++ gcc/genautomata.cc 2024-10-24 11:07:21.316602951 +0200
> @@ -8348,7 +8348,8 @@ output_default_latencies (void)
> if ((col = (col+1) % 8) == 0)
> fputs ("\n ", output_file);
> decl = description->decls[i];
> - gcc_assert (j++ == DECL_INSN_RESERV (decl)->insn_num);
> + gcc_assert (j == DECL_INSN_RESERV (decl)->insn_num);
> + ++j;
> fprintf (output_file, "% 4d,",
> DECL_INSN_RESERV (decl)->default_latency);
> }
> --- gcc/tree-ssa-loop-ivopts.cc.jj 2024-10-14 19:39:48.494778534 +0200
> +++ gcc/tree-ssa-loop-ivopts.cc 2024-10-24 11:03:47.991610401 +0200
> @@ -7562,7 +7562,7 @@ get_alias_ptr_type_for_ptr_address (iv_u
> case IFN_MASK_LEN_LOAD:
> case IFN_MASK_LEN_STORE:
> /* The second argument contains the correct alias type. */
> - gcc_assert (use->op_p = gimple_call_arg_ptr (call, 0));
> + gcc_assert (use->op_p == gimple_call_arg_ptr (call, 0));
> return TREE_TYPE (gimple_call_arg (call, 1));
>
> default:
> --- gcc/cgraph.cc.jj 2024-10-24 10:29:13.211956616 +0200
> +++ gcc/cgraph.cc 2024-10-24 11:06:35.854243877 +0200
> @@ -894,7 +894,8 @@ symbol_table::create_edge (cgraph_node *
> edge->m_summary_id = -1;
> edges_count++;
>
> - gcc_assert (++edges_max_uid != 0);
> + ++edges_max_uid;
> + gcc_assert (edges_max_uid != 0);
> edge->m_uid = edges_max_uid;
> edge->aux = NULL;
> edge->caller = caller;
> --- gcc/pair-fusion.cc.jj 2024-10-22 17:09:09.372091098 +0200
> +++ gcc/pair-fusion.cc 2024-10-24 11:13:07.023744574 +0200
> @@ -1962,7 +1962,10 @@ pair_fusion_bb_info::fuse_pair (bool loa
>
> auto ignore = ignore_changing_insns (changes);
> for (unsigned i = 0; i < changes.length (); i++)
> - gcc_assert (rtl_ssa::restrict_movement (*changes[i], ignore));
> + {
> + bool ok = rtl_ssa::restrict_movement (*changes[i], ignore);
> + gcc_assert (ok);
> + }
>
> // Check the pair pattern is recog'd.
> if (!rtl_ssa::recog (attempt, *pair_change, ignore))
> @@ -3053,7 +3056,10 @@ pair_fusion::try_promote_writeback (insn
>
> auto ignore = ignore_changing_insns (changes);
> for (unsigned i = 0; i < ARRAY_SIZE (changes); i++)
> - gcc_assert (rtl_ssa::restrict_movement (*changes[i], ignore));
> + {
> + bool ok = rtl_ssa::restrict_movement (*changes[i], ignore);
> + gcc_assert (ok);
> + }
>
> if (!rtl_ssa::recog (attempt, pair_change, ignore))
> {
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
