On Tue, Apr 29, 2025 at 11:49 PM Richard Biener <richard.guent...@gmail.com> wrote: > > 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?
What is interesting is we are having the same discussion 25 years later about targets not having a trap being defined. https://gcc.gnu.org/legacy-ml/gcc-patches/2000-01/msg00323.html I think it is time to stop kicking the bucket down the road and start requiring targets to define a trap. Thanks, Andrew > > > 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 > >
