On Thu, May 1, 2025 at 11:18 PM Richard Biener <richard.guent...@gmail.com> wrote: > > On Fri, May 2, 2025 at 12:56 AM Andrew Pinski <pins...@gmail.com> wrote: > > > > 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. > > I tried to check for SH and could not find a 'trap' instruction. So I think > we need to suggest somewhat common idioms that targets can use > as replacement? Like div by zero (when that traps), volatile store > to NULL (when that reliably traps), jump to not properly aligned address, > etc. - and the question then is whether such thing always will exist, > and if not, whether "halting" is as good as trapping then or if a > call to abort() is required. > > But yes, I kind-of agree. > > Are there bugzillas for all the targets that do not define a trap?
Only one for sh, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216 . I will file one for the other targets tomorrow. Thanks, Andrew > > Richard. > > > > > 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 > > > >
