On Wed, Apr 30, 2025 at 9:03 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?
>
> 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.
I wonder if a general fallback for expanding a trap could be
label:
jmp label;
a nop in general wouldn't do (in this particular case it would, but then
not as expansion for __builtin_unreachable_trap ()).
But yeah, we should possibly force targets to implement a trap
instruction, but more thorougly document what should happen
(the program should stop [making progress]).
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
> > >
