> On 30 Apr 2025, at 09:26, Richard Biener <richard.guent...@gmail.com> wrote:
>
> 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]).
We have nearly reached the same overall design as my original patch.
In that case, I provided a target hook so that targets could opt in or out
of the treatment (and, presumably, could even make that conditional on
some -munreachble-is-trap).
(FAOD, I don’t have any specific attachment to any solution, so long as
we eventually get one)
Iain
>
> 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
