Re: nvptx abort implementation

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi Tom!

On Wed, 2 May 2018 19:36:53 +0200, Tom de Vries <Tom_deVries@mentor.com> wrote:
> I.
> 
> atm, abort for nvptx is implemented here ( 
> newlib/libc/machine/nvptx/abort.c ) as:
> ...
> void __attribute__((noreturn))
> abort (void)
> {
>    for (;;)
>      __builtin_trap ();
> }
> ...
> 
> The __builtin_trap function is considered noreturn by gcc, so early in 
> the compiler the loop is removed, even at -O0. So, this is equivalent to 
> the shorter:
> ...
> void __attribute__((noreturn))
> abort (void)
> {
>    __builtin_trap ();
> }
> ...

ACK.


> II.
> 
> Then, the nvptx port of gcc implements __builtin_trap using the 'trap' 
> ptx insn.  [...]
> 
> So, in fact nvptx __builtin_trap can return, which is a gcc bug, that I 
> still need to file and fix.

ACK.


> III.
> 
> As for newlib, the ptx currently generated is:
> ...
> .visible .func abort
> {
>    trap;
> }
> ...
> 
> Based on the possibly-returning property of trap, we could expect 
> execution of random code after the trap. But, as it happens, the 
> translation from ptx to SASS adds an implicit 'ret' after the trap:
> ...
> .text.abort:
>          /*0008*/                   BPT.TRAP 0x1;
>          /*0010*/                   RET;
> ...

As I understand, it's not that "the translation from ptx to SASS adds an
implicit 'ret' after the trap", but more precisely ;-) that it adds an
implicit 'ret' at function end.  (With the same net result, of course.)

> So once we return from the abort we run into one more trap and an exit:
> ...
> call abort;
> trap; // (noreturn)
> exit; // (noreturn)
> ...
> which would then effectively abort.

ACK.


> IV.
> 
> I'd prefer a robust abort implementation that:
> - does not depend on the __builtin_trap gcc bug being fixed, and

Why not just do that?  (Even just on trunk, given that the current
implementation as used for release branches will effectively also abort?)

> - does not depend on the ptx to SASS translation inserting a ret
>    at the end of the function (given that there's nothing in the ptx
>    documentation to suggest that this is guaranteed behaviour)
> - does not depend on the noreturn calls being trailed by trap and exit

ACK.  (These would automatically fall out of "the __builtin_trap gcc bug
being fixed"?)

> So, I'm thinking of:
> ...
> void __attribute__((noreturn))
> abort (void)
> {
>    asm ("trap; exit;" ::: "memory");
>    while (1)
>      ;
> }
> ...
> 
> The while-true is there to prevent the warning "‘noreturn’ function does 
> return".
> 
> I'll prepare a patch for this and test it.

Hmm, I won't be objecting, but (with "the __builtin_trap gcc bug being
fixed") a simple '__builtin_trap' call in 'abort' seems sufficient to me?


Grüße
 Thomas