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?
Yes, the sh target is the one which can run fully Linux even. There is an open bug about sh not having trap pattern implemented yet; https://gcc.gnu.org/PR70216; been open for 9 years now too. > > 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. I was thinking of that even (I even accidently left in the include for target.h :) ) > > As for the nop, if you mark it volatile does it prevail? I don't even know how to mark the rtl insn as volatile. the volatil field for INSN is listed as being if it was deleted: 1 in an INSN, CALL_INSN, JUMP_INSN, CODE_LABEL, BARRIER, or NOTE if it has been deleted. So that won't help. Now we could use the `used` field for this marking. I have not looked at what it could take to make sure it does not get deleted though. 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 > >
