On Tue, Apr 29, 2025 at 4:25 PM Andrew Pinski <quic_apin...@quicinc.com> wrote:
>
> When we have an empty function, things can go wrong with
> cfi_startproc/cfi_endproc and a few other things like exceptions. So if
> the only thing the function does is a call to __builtin_unreachable,
> let's expand that to a __builtin_trap instead. For most targets that
> is one instruction wide so it won't hurt things that much and we get
> correct behavior for exceptions and some linkers will be better for it.
>
> The only thing I have a concern about is that some targets still
> don't define a trap instruction. I tried to emit a nop instead of
> an abort but that nop is removed during RTL DCE.
> Should we just push targets to define a trap instead?
> E.g. BPF, avr and sh are the 3 semi active targets which still don't
> have a trap defined.
Do any of those targets have the cfi_startproc/cfi_endproc issue
or exceptions are relevant on those?
I'd say guard this with targetm.have_trap (), there's the chance that
say on avr the expansion to abort() might fail to link in a
freestanding environment.
As for the nop, if you mark it volatile does it prevail?
> The QOI idea for basic block reorder is recorded as PR 120004.
>
> Changes since v1:
> * v2: Move to final gimple cfg cleanup instead of expand and use
> BUILT_IN_UNREACHABLE_TRAP.
>
> Bootstrapped and tested on x86_64-linux-gnu.
>
> PR middle-end/109267
>
> gcc/ChangeLog:
>
> * tree-cfgcleanup.cc (execute_cleanup_cfg_post_optimizing): If the
> first
> non debug statement in the first (and only) basic block is a call
> to __builtin_unreachable change it to a call to __builtin_trap.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/pr109267-1.c: New test.
> * gcc.dg/pr109267-2.c: New test.
>
> Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> ---
> gcc/testsuite/gcc.dg/pr109267-1.c | 14 ++++++++++++++
> gcc/testsuite/gcc.dg/pr109267-2.c | 14 ++++++++++++++
> gcc/tree-cfgcleanup.cc | 14 ++++++++++++++
> 3 files changed, 42 insertions(+)
> create mode 100644 gcc/testsuite/gcc.dg/pr109267-1.c
> create mode 100644 gcc/testsuite/gcc.dg/pr109267-2.c
>
> diff --git a/gcc/testsuite/gcc.dg/pr109267-1.c
> b/gcc/testsuite/gcc.dg/pr109267-1.c
> new file mode 100644
> index 00000000000..d6df2c3b49a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr109267-1.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +/* PR middle-end/109267 */
> +
> +int f(void)
> +{
> + __builtin_unreachable();
> +}
> +
> +/* This unreachable should be changed to be a trap. */
> +
> +/* { dg-final { scan-tree-dump-times "__builtin_unreachable trap \\\(" 1
> "optimized"} } */
> +/* { dg-final { scan-tree-dump-not "__builtin_unreachable \\\(" "optimized"}
> } */
> diff --git a/gcc/testsuite/gcc.dg/pr109267-2.c
> b/gcc/testsuite/gcc.dg/pr109267-2.c
> new file mode 100644
> index 00000000000..6cd1419a1e3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr109267-2.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +/* PR middle-end/109267 */
> +void g(void);
> +int f(int *t)
> +{
> + g();
> + __builtin_unreachable();
> +}
> +
> +/* The unreachable should stay a unreachable. */
> +/* { dg-final { scan-tree-dump-not "__builtin_unreachable trap \\\("
> "optimized"} } */
> +/* { dg-final { scan-tree-dump-times "__builtin_unreachable \\\(" 1
> "optimized"} } */
> diff --git a/gcc/tree-cfgcleanup.cc b/gcc/tree-cfgcleanup.cc
> index 9a8a668e12b..38a62499f93 100644
> --- a/gcc/tree-cfgcleanup.cc
> +++ b/gcc/tree-cfgcleanup.cc
> @@ -46,6 +46,7 @@ along with GCC; see the file COPYING3. If not see
> #include "cgraph.h"
> #include "tree-into-ssa.h"
> #include "tree-cfgcleanup.h"
> +#include "target.h"
>
>
> /* The set of blocks in that at least one of the following changes happened:
> @@ -1530,6 +1531,19 @@ execute_cleanup_cfg_post_optimizing (void)
> cleanup_dead_labels ();
> if (group_case_labels ())
> todo |= TODO_cleanup_cfg;
> +
> + basic_block bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
> + gimple_stmt_iterator gsi = gsi_start_nondebug_after_labels_bb (bb);
> + /* If the first (and only) bb and the only non debug
> + statement is __builtin_unreachable call, then replace it with a trap
> + so the function is at least one instruction in size. */
> + if (!gsi_end_p (gsi)
> + && gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_UNREACHABLE))
> + {
> + gimple_call_set_fndecl (gsi_stmt (gsi), builtin_decl_implicit
> (BUILT_IN_UNREACHABLE_TRAP));
> + update_stmt (gsi_stmt (gsi));
> + }
> +
> if ((flag_compare_debug_opt || flag_compare_debug)
> && flag_dump_final_insns)
> {
> --
> 2.43.0
>
