| Message ID | CAKdteOakzKmCEnCA5EZbd8Mc3HCy39nRR6=DDx2poWz6G1tvxQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ARM] Add endless loop to avoid a compiler warning on noreturn functions. | expand |
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.) Craig
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]); 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
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. I think the right fix here is to remove the "return" from the statements and add __builtin_unreachable () at the end of the function. 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
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? > 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 >
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. 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 >>
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 > >> > 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); 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
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.. 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
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 > commit 90f4cbb62838bc39e2587888c330859e3d4bd7e7 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 d2a65cb..87ddca7 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,18 @@ _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); + __builtin_unreachable(); 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
On 08/10/18 09:25, Christophe Lyon wrote: > 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. I think it was added to GCC last year. Not sure if it's on by default, though. Anyway, pushed. Thanks, R. > > 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 >> >> >> newlib-noreturn.txt >> >> >> commit 90f4cbb62838bc39e2587888c330859e3d4bd7e7 >> 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 d2a65cb..87ddca7 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,18 @@ _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); >> + __builtin_unreachable(); >> 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
diff --git a/libgloss/arm/_exit.c b/libgloss/arm/_exit.c index ca2d21c..90220d2 100644 --- a/libgloss/arm/_exit.c +++ b/libgloss/arm/_exit.c @@ -12,4 +12,8 @@ _exit (int status) Note: The RDI implementation of _kill throws away both its arguments. */ _kill (status, -1); + + /* Convince GCC that this function never returns. */ + for (;;) + ; } diff --git a/newlib/libc/sys/arm/syscalls.c b/newlib/libc/sys/arm/syscalls.c index 6e70467..311b28d 100644 --- a/newlib/libc/sys/arm/syscalls.c +++ b/newlib/libc/sys/arm/syscalls.c @@ -452,6 +452,10 @@ _exit (int status) Note: The RDI implementation of _kill throws away both its arguments. */ _kill(status, -1); + + /* Convince GCC that this function never returns. */ + for (;;) + ; } pid_t
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 commit 72ab15cd60be82262be4b0bdb79aa78376724c8c Author: Christophe Lyon <christophe.lyon@linaro.org> Date: Mon Oct 1 15:52:42 2018 +0000 [ARM] Add endless loop to avoid a compiler warning on noreturn functions. 2018-10-01 Christophe Lyon <christophe.lyon@linaro.org> * libgloss/arm/_exit.c (_exit): Add endless loop. * newlib/libc/sys/arm/syscalls.c (_exit): Likewise.