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

[Christophe Lyon <christophe dot lyon at linaro dot org>]

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