On Thu, Apr 3, 2025 at 9:04 AM Andrew Pinski <pins...@gmail.com> wrote:
>
> On Wed, Apr 2, 2025 at 11:52 PM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Thu, Apr 3, 2025 at 7:10 AM Andrew Pinski <quic_apin...@quicinc.com>
> > wrote:
> > >
> > > builtin_unreachable_bb_p exacts empty basic blocks to have only one
> > > successor edge but
> > > in the case after inliing a noreturn function that actually returns, we
> > > end up with an
> > > empty basic block with no successor edge. fixup_cfg fixes this up by
> > > placing a call to
> > > __builtin_unreachable but we don't do that before getting the function
> > > summary
> > > (after einiline). So the fix is to have the inliner insert the call
> > > to __builtin_unreachable (or the trap version) and not depend on the
> > > fixup if the return
> > > block is reachable after inlining a noreturn function.
> > >
> > > A small optimization is done not to insert the call if the block is NOT
> > > simplely reachable.
> > > This should prevent inserting the call just to remove it during
> > > cleanupcfg.
> > >
> > > Note this shows up only at -O0 with always_inline because when
> > > optimizations are turned on
> > > returns are turned into __builtin_unreachable inside noreturn functions.
> >
> > Where's that done? IMO we should do this at -O0 as well (possibly
> > defaulting to
> > use the trapping variant there).
>
> It is done inside pass_warn_function_return::execute and that was
> added by r8-3988-g356fcc67fba52b .
> Since it already uses gimple_build_builtin_unreachable,
> BUILT_IN_UNREACHABLE_TRAP is already used when flag_unreachable_traps
> is on (which defaults being on for -O0 and -gg).
> So should we just change that to be always on instead of being blocked
> for !optimized?
Yes, I think so.
Richard.
>
> Thanks,
> Andrew Pinski
>
> >
> > >
> > > Bootstrapped and tested on x86_64-linux-gnu.
> > >
> > > PR ipa/119599
> > >
> > > gcc/ChangeLog:
> > >
> > > * tree-inline.cc (expand_call_inline): Add a
> > > __builtin_unreachable call
> > > to the return block if it is still reachable and the call that
> > > was inlined
> > > was a noreturn function.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * gcc.dg/torture/pr119599-1.c: New test.
> > >
> > > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > > ---
> > > gcc/testsuite/gcc.dg/torture/pr119599-1.c | 27 +++++++++++++++++++++++
> > > gcc/tree-inline.cc | 15 +++++++++++++
> > > 2 files changed, 42 insertions(+)
> > > create mode 100644 gcc/testsuite/gcc.dg/torture/pr119599-1.c
> > >
> > > diff --git a/gcc/testsuite/gcc.dg/torture/pr119599-1.c
> > > b/gcc/testsuite/gcc.dg/torture/pr119599-1.c
> > > new file mode 100644
> > > index 00000000000..4fbd228a359
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/torture/pr119599-1.c
> > > @@ -0,0 +1,27 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-fdump-tree-einline" } */
> > > +
> > > +/* PR ipa/119599 */
> > > +/* inlining a noreturn function which returns
> > > + can cause an ICE when dealing finding an unreachable block.
> > > + We should get a __builtin_unreachable after the inliing. */
> > > +
> > > +
> > > +void baz (void);
> > > +
> > > +static inline __attribute__((always_inline, noreturn)) void
> > > +bar (void)
> > > +{
> > > + static volatile int t = 0;
> > > + if (t == 0)
> > > + baz ();
> > > +} /* { dg-warning "function does return" } */
> > > +
> > > +void
> > > +foo (void)
> > > +{
> > > + bar ();
> > > +}
> > > +
> > > +/* After inlining, we should have call to __builtin_unreachable now. */
> > > +/* { dg-final { scan-tree-dump "__builtin_unreachable " "einline" } } */
> > > diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc
> > > index 05843b8ccf0..da403cd1456 100644
> > > --- a/gcc/tree-inline.cc
> > > +++ b/gcc/tree-inline.cc
> > > @@ -4832,6 +4832,7 @@ expand_call_inline (basic_block bb, gimple *stmt,
> > > copy_body_data *id,
> > > gimple *simtenter_stmt = NULL;
> > > vec<tree> *simtvars_save;
> > > tree save_stack = NULL_TREE;
> > > + bool was_noreturn;
> > >
> > > /* The gimplifier uses input_location in too many places, such as
> > > internal_get_tmp_var (). */
> > > @@ -4843,6 +4844,8 @@ expand_call_inline (basic_block bb, gimple *stmt,
> > > copy_body_data *id,
> > > if (!call_stmt)
> > > goto egress;
> > >
> > > + was_noreturn = gimple_call_noreturn_p (call_stmt);
> > > +
> > > cg_edge = id->dst_node->get_edge (stmt);
> > > gcc_checking_assert (cg_edge);
> > > /* First, see if we can figure out what function is being called.
> > > @@ -5376,6 +5379,18 @@ expand_call_inline (basic_block bb, gimple *stmt,
> > > copy_body_data *id,
> > > if (purge_dead_abnormal_edges)
> > > bitmap_set_bit (to_purge, return_block->index);
> > >
> > > + /* If this was a noreturn function and if the return block is still
> > > + reachable, then add a call to __builtin_unreachable(). */
> > > + if (was_noreturn && EDGE_COUNT (return_block->preds) != 0)
> > > + {
> > > + gcall *call_stmt = gimple_build_builtin_unreachable
> > > (input_location);
> > > + gimple_stmt_iterator gsi = gsi_last_bb (return_block);
> > > + gsi_insert_after (&gsi, call_stmt, GSI_NEW_STMT);
> > > + tree fndecl = gimple_call_fndecl (call_stmt);
> > > + cg_edge->caller->create_edge (cgraph_node::get_create (fndecl),
> > > + call_stmt, return_block->count);
> > > + }
> > > +
> > > /* If the value of the new expression is ignored, that's OK. We
> > > don't warn about this for CALL_EXPRs, so we shouldn't warn about
> > > the equivalent inlined version either. */
> > > --
> > > 2.43.0
> > >
