This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On Fri, 5 Oct 2018 at 12:56, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote: > > On 05/10/18 10:18, Christophe Lyon wrote: > > On Tue, 2 Oct 2018 at 11:08, Richard Earnshaw (lists) > > <Richard.Earnshaw@arm.com> wrote: > >> > >> On 02/10/18 09:59, Christophe Lyon wrote: > >>> On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists) > >>> <Richard.Earnshaw@arm.com> wrote: > >>>> > >>>> On 02/10/18 07:52, Christophe Lyon wrote: > >>>>> On Tue, 2 Oct 2018 at 00:25, Craig Howland <howland@lgsinnovations.com> 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 > >> > >> > >> commit 3721a6c503155fc92ea6ed414b92df37da0b6232 > >> Author: Christophe Lyon <christophe.lyon@linaro.org> > >> 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 <christophe.lyon@linaro.org> > >> > >> * 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); > > I think you need to put an unreachable note here as well, otherwise > you'll get a case falls through warning. You could put a break > statement, but that seems a bit non-intuitive.. > I didn't see any such warning, but here is a new patch according to your recommendation. Thanks Christophe > R. > > >> 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 >
Attachment:
newlib-noreturn.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |