[ARM] Add endless loop to avoid a compiler warning on noreturn functions.

Message ID CAKdteOakzKmCEnCA5EZbd8Mc3HCy39nRR6=DDx2poWz6G1tvxQ@mail.gmail.com
State New
Headers show
Series
  • [ARM] Add endless loop to avoid a compiler warning on noreturn functions.
Related show

Commit Message

Christophe Lyon Oct. 1, 2018, 9:27 p.m.
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.

Comments

Craig Howland Oct. 1, 2018, 10:24 p.m. | #1
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
Christophe Lyon Oct. 2, 2018, 6:52 a.m. | #2
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
Richard Earnshaw (lists) Oct. 2, 2018, 8:55 a.m. | #3
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
Christophe Lyon Oct. 2, 2018, 8:59 a.m. | #4
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

>
Richard Earnshaw (lists) Oct. 2, 2018, 9:08 a.m. | #5
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

>>
Christophe Lyon Oct. 5, 2018, 9:18 a.m. | #6
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
Richard Earnshaw (lists) Oct. 5, 2018, 10:56 a.m. | #7
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
Christophe Lyon Oct. 8, 2018, 8:25 a.m. | #8
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
Richard Earnshaw (lists) Oct. 8, 2018, 1:37 p.m. | #9
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

Patch

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