Re: [ARM] Make _kill() a noreturn function (was: [ARM] Add endless loop to avoid a compiler warning on noreturn functions.)
Christophe Lyon <[email protected]>
| Newsgroups | gmane.comp.lib.newlib |
|---|---|
| Message-ID | <CAKdteObeQ0CgqYXmt-9E1mLL=+C=p1B0v1vVmV-U0YJAftNBuA@mail.gmail.com> |
On Tue, 2 Oct 2018 at 11:08, Richard Earnshaw (lists) <[email protected]> wrote: > > On 02/10/18 09:59, Christophe Lyon wrote: > > On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists) > > <[email protected]> wrote: > >> > >> On 02/10/18 07:52, Christophe Lyon wrote: > >>> On Tue, 2 Oct 2018 at 00:25, Craig Howland <[email protected]> wrote: > >>>> > >>>> On 10/01/2018 05:27 PM, Christophe Lyon wrote: > >>>>> Hi, > >>>>> > >>>>> While building newlib for ARM, I noticed GCC warnings for _exit() that > >>>>> the compiler thinks they return a value despite being noreturn. > >>>>> > >>>>> Like other targets, this small adds an endless loop to avoid the warning. > >>>>> > >>>>> OK? > >>>>> > >>>>> Christophe > >>>> The proper fix (for both places) is to add noreturn to the _kill() prototype in > >>>> the file. (Which presumably is true, otherwise _exit() will return. I did test > >>>> that it fixes the warning.) (It would not be surprising if it also needed to be > >>>> added to the _kill() source, itself.) > >>> > >>> Well, when compiled with ARM_RDI_MONITOR, _kill does seem to return: > >>> #if SEMIHOST_V2 > >>> if (_has_ext_exit_extended ()) > >>> return do_AngelSWI (insn, block); > >>> else > >>> #endif > >>> return do_AngelSWI (insn, (void*)block[0]); > >>> > >> > >> do_AngelSWI is a multi-purpose call that will normally return, so that > >> can't be marked no-return. > > Indeed. > > > >> > >> I think the right fix here is to remove the "return" from the statements > >> and add __builtin_unreachable () at the end of the function. > >> > > By "function", do you mean _kill or _exit ? > > IIUC the patch should: > > - remove "return" from _kill > > - add _builtin_unreachable to both _kill and _exit > > - add "noreturn" to _kill prototype > > > > Unless there are cases where this version of _kill can kill > > another thread/process and thus actually return to its caller? > > > > Well if _kill can be properly marked as no-return then _exit can also > and you don't need to have a second _bi_unreachable(): that's only > needed when GCC cannot deduce the control flow from semantic analysis of > the code. > > don't forget that this code is duplicated across both newlib/sys/arm and > libgloss, so please update both instances. > OK, here is an updated version. > R. > > > > >> R. > >> > >>> I guess the noreturn should not be added to > >>> newlib/libc/include/sys/signal.h > >>> because it depends on the actual target implementation of _kill? > >>> > >>>> Craig > >> >
newlib-1.txt
(text/plain, 2.7 KB)
commit 3721a6c503155fc92ea6ed414b92df37da0b6232 Author: Christophe Lyon <[email protected]> Date: Mon Oct 1 15:52:42 2018 +0000 [ARM] Make _kill() a noreturn function. AngelSWI_Reason_ReportException does not return accoring to the ARM documentation, so it is valid to mark _kill() as noreturn. This way, the compiler does not warn about _exit() returning a value despite being noreturn. 2018-10-01 Christophe Lyon <[email protected]> * libgloss/arm/_exit.c (_exit): Declare _kill() as noreturn. * libgloss/arm/_exit.c (_kill): Likewise. Remove the return statements. * newlib/libc/sys/arm/syscalls.c (_kill): Likewise.. diff --git a/libgloss/arm/_exit.c b/libgloss/arm/_exit.c index ca2d21c..4a071df 100644 --- a/libgloss/arm/_exit.c +++ b/libgloss/arm/_exit.c @@ -1,6 +1,6 @@ #include <_ansi.h> -int _kill (int, int); +int _kill (int, int) __attribute__((__noreturn__)); void _exit (int); void diff --git a/libgloss/arm/_kill.c b/libgloss/arm/_kill.c index 278ded7..34a6ffd 100644 --- a/libgloss/arm/_kill.c +++ b/libgloss/arm/_kill.c @@ -2,7 +2,7 @@ #include <signal.h> #include "swi.h" -int _kill (int, int); +int _kill (int, int) __attribute__((__noreturn__)); int _kill (int pid, int sig) @@ -41,12 +41,14 @@ _kill (int pid, int sig) #if SEMIHOST_V2 if (_has_ext_exit_extended ()) - return do_AngelSWI (insn, block); + do_AngelSWI (insn, block); else #endif - return do_AngelSWI (insn, (void*)block[0]); + do_AngelSWI (insn, (void*)block[0]); #else asm ("swi %a0" :: "i" (SWI_Exit)); #endif + + __builtin_unreachable(); } diff --git a/newlib/libc/sys/arm/syscalls.c b/newlib/libc/sys/arm/syscalls.c index 6e70467..d9e6742 100644 --- a/newlib/libc/sys/arm/syscalls.c +++ b/newlib/libc/sys/arm/syscalls.c @@ -30,7 +30,7 @@ int _stat (const char *, struct stat *); int _fstat (int, struct stat *); void * _sbrk (ptrdiff_t); pid_t _getpid (void); -int _kill (int, int); +int _kill (int, int) __attribute__((__noreturn__)); void _exit (int); int _close (int); int _swiclose (int); @@ -432,15 +432,17 @@ _kill (int pid, int sig) /* Note: The pid argument is thrown away. */ switch (sig) { case SIGABRT: - return do_AngelSWI (AngelSWI_Reason_ReportException, - (void *) ADP_Stopped_RunTimeError); + do_AngelSWI (AngelSWI_Reason_ReportException, + (void *) ADP_Stopped_RunTimeError); default: - return do_AngelSWI (AngelSWI_Reason_ReportException, - (void *) ADP_Stopped_ApplicationExit); + do_AngelSWI (AngelSWI_Reason_ReportException, + (void *) ADP_Stopped_ApplicationExit); } #else asm ("swi %a0" :: "i" (SWI_Exit)); #endif + + __builtin_unreachable(); } void