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

On 05/10/18 10:18, Christophe Lyon wrote:
> On Tue, 2 Oct 2018 at 11:08, Richard Earnshaw (lists)
> <[email protected]> wrote:
>>
>> On 02/10/18 09:59, Christophe Lyon wrote:
>>> On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists)
>>> <[email protected]> wrote:
>>>>
>>>> On 02/10/18 07:52, Christophe Lyon wrote:
>>>>> On Tue, 2 Oct 2018 at 00:25, Craig Howland <[email protected]> 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 <[email protected]>
>> 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  <[email protected]>
>>     
>>     	* 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