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?
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
> > >
