[ARM] Make _kill() a noreturn function (was: [ARM] Add endless loop to avoid a compiler warning on noreturn functions.)
Christophe Lyon
christophe.lyon@linaro.org
Mon Oct 8 08:55:00 GMT 2018
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
>
-------------- next part --------------
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
More information about the Newlib
mailing list