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