[Bug c/99299] New: Need a recoverable version of __builtin_trap()

[Bug c/99299] New: Need a recoverable version of __builtin_trap()

[christophe.leroy at csgroup dot eu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

            Bug ID: 99299
           Summary: Need a recoverable version of __builtin_trap()
           Product: gcc
           Version: 10.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: christophe.leroy at csgroup dot eu
  Target Milestone: ---

Linux kernel implements WARN() and WARN_ON() asserts using trap instructions.

Because gcc __builtin_trap() is not recoverable, Linux Kernel has hand code the
trap, at the moment using 'twnei'. This leads to sub-optimal code generation.

As the powerpc trap instruction is recoverable as it generated a recoverable
exception, it would be extremely usefull to also have a recoverable version of
__builtin_trap() in gcc. Maybe call it __buitin_recoverable_trap()

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-02-27
     Ever confirmed|0                           |1

--- Comment #1 from Segher Boessenkool <segher at gcc dot gnu.org> ---
[ Do we not have a keyword for feature requests, btw?  I don't see one. ]

The only thing needed for GCC is to have a __builtin_trap_no_noreturn (or
something with a less horrible name ;-) ), that does exactly that: it's the
same as __builtin_trap, just not noreturn.  This is useful on most
architectures, not just PowerPC.

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #1)
> [ Do we not have a keyword for feature requests, btw?  I don't see one. ]

Usually enhancement is enough to mark it as a feature request.

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

--- Comment #3 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Ah, thank you.  Well except there is no keyword called that?

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
'enhancement' Importance is the magic we use, in the end it's a missed
optimization since you refer to sub-optimal code gen.

I'm not sure what your proposed not noreturn trap() would do in terms of
IL semantics compared to a not specially annotated general call?

"recoverable" likely means resuming after the trap, not on an exception
path (so it'll not be a throw())?

The only thing that might be useful to the middle-end would be marking
the function as not altering the memory state.  But I suppose it should
still serve as a barrier for code motion of both loads and stores, even
of those loads/stores are known to not trap.  The only magic we'd have
for this would be __attribute__((const,returns_twice)).  Which likely
will be more detrimental to general optimization.

So - what's the "sub-optimal code generation" you refer to from the
(presumably) volatile asm() you use for the trap?

[yeah, asm() on GIMPLE is less optimized than a call]

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[sirl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

Franz Sirl <sirl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sirl at gcc dot gnu.org

--- Comment #5 from Franz Sirl <sirl at gcc dot gnu.org> ---
For the naming I suggest __builtin_debugtrap() to align with clang. Maybe with
an aliased __debugbreak() on Windows platforms.

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

--- Comment #6 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #4)
> I'm not sure what your proposed not noreturn trap() would do in terms of
> IL semantics compared to a not specially annotated general call?

Nothing I think?  But __builtin_trap *is* very different: it ends BBs.

> "recoverable" likely means resuming after the trap, not on an exception
> path (so it'll not be a throw())?

"recoverable" is super unclear.  For example, on Power the hardware has a
concept "recoverable interrupt", which set MSR[RI]=1, and traps never do.
This is a very different concept as what is wanted here, which has nothing
to do with recoverability, and is simply about not being an abort() (which
__builtin_trap *is*!)

> The only thing that might be useful to the middle-end would be marking
> the function as not altering the memory state.  But I suppose it should
> still serve as a barrier for code motion of both loads and stores, even
> of those loads/stores are known to not trap.  The only magic we'd have
> for this would be __attribute__((const,returns_twice)).  Which likely
> will be more detrimental to general optimization.
> 
> So - what's the "sub-optimal code generation" you refer to from the
> (presumably) volatile asm() you use for the trap?
> 
> [yeah, asm() on GIMPLE is less optimized than a call]

The rs6000 backend can optimise the used instructions: we have trap_if
instructions, both with registers and with immediates.  A single
instruction can do a comparison and a conditional trap.  This works great
with __builtin_trap, *if* the kernel's trap handler has abort() semantics.

__builtin_trap_no_abort() maybe?

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Franz Sirl from comment #5)
> For the naming I suggest __builtin_debugtrap() to align with clang. Maybe
> with an aliased __debugbreak() on Windows platforms.

Those are terrible names.  This would *not* be used more often than
__builtin_trap, for debugging.

In general, builtins should say what they *do*, nott what you imagine they
will be used for.

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[sirl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

--- Comment #8 from Franz Sirl <sirl at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #7)
> (In reply to Franz Sirl from comment #5)
> > For the naming I suggest __builtin_debugtrap() to align with clang. Maybe
> > with an aliased __debugbreak() on Windows platforms.
> 
> Those are terrible names.  This would *not* be used more often than
> __builtin_trap, for debugging.
> 
> In general, builtins should say what they *do*, nott what you imagine they
> will be used for.

Let me re-phrase it. If the result of this discussion will result in a builtin
that will be eqivalent to an "int 3" (or was it ud2? on x86) without "noreturn"
as it is with clang, I would really prefer it to be called
__builtin_debugtrap() even if the name is not great. Having different naming
between GCC and clang seems worse to me.

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

--- Comment #9 from Segher Boessenkool <segher at gcc dot gnu.org> ---
The i386 port has

===
(define_insn "trap"
  [(trap_if (const_int 1) (const_int 6))]
  ""
{
#ifdef HAVE_AS_IX86_UD2
  return "ud2";
#else
  return ASM_SHORT "0x0b0f";
#endif
}
  [(set_attr "length" "2")])
===

which implements __builtin_trap, and can implement __builtin_trap_no_abort
just fine as well, if your OS kernel (or similar) can return after a ud2.

If clang uses terribly confusing names (or semantics, or syntax, etc.) we
should not copy that from them.  *Especially* when that already conflicts
with names they copied from us.

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |DUPLICATE

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Dup of bug 84595.

*** This bug has been marked as a duplicate of bug 84595 ***

[Bug middle-end/99299] Need a recoverable version of __builtin_trap()

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99299

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |NEW
         Resolution|DUPLICATE                   |---
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=84595

--- Comment #11 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Not a duplicate.