[ARM] Make _kill() a noreturn function (was: [ARM] Add endless loop to avoid a compiler warning on noreturn functions.)

[Christophe Lyon]

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
> >>
>
-------------- next part --------------
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