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

Post by Christophe Lyon
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

2018-10-02 06:52:15 UTC

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?

Richard Earnshaw (lists)

2018-10-02 08:55:33 UTC

#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.

Post by Christophe Lyon
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?

Christophe Lyon

2018-10-02 08:59:42 UTC

On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists)

#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.

Post by Richard Earnshaw (lists)
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?

Post by Christophe Lyon
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?

Richard Earnshaw (lists)

2018-10-02 09:08:53 UTC

This post might be inappropriate. Click to display it.

Christophe Lyon

2018-10-05 09:18:17 UTC

On Tue, 2 Oct 2018 at 11:08, Richard Earnshaw (lists)

Post by Christophe Lyon
On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists)

#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.

Post by Richard Earnshaw (lists)
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 ?
- 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.

Post by Christophe Lyon
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?

Richard Earnshaw (lists)

2018-10-05 10:56:32 UTC

Post by Christophe Lyon
On Tue, 2 Oct 2018 at 11:08, Richard Earnshaw (lists)

Post by Christophe Lyon
On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists)

#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.

Post by Richard Earnshaw (lists)
I think the right fix here is to remove the "return" from the statements
and add __builtin_unreachable () at the end of the function.

OK, here is an updated version.

Post by Christophe Lyon
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?

newlib-1.txt
commit 3721a6c503155fc92ea6ed414b92df37da0b6232
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.
* 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) {
- 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.

Post by Richard Earnshaw (lists)
- 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

2018-10-08 08:25:57 UTC

On Fri, 5 Oct 2018 at 12:56, Richard Earnshaw (lists)

Post by Christophe Lyon
On Tue, 2 Oct 2018 at 11:08, Richard Earnshaw (lists)

Post by Christophe Lyon
On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists)

#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.

Post by Richard Earnshaw (lists)
I think the right fix here is to remove the "return" from the statements
and add __builtin_unreachable () at the end of the function.

OK, here is an updated version.

Post by Christophe Lyon
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?

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

Richard Earnshaw (lists)

2018-10-08 13:37:40 UTC

Post by Christophe Lyon
On Fri, 5 Oct 2018 at 12:56, Richard Earnshaw (lists)

Post by Christophe Lyon
On Tue, 2 Oct 2018 at 11:08, Richard Earnshaw (lists)

Post by Christophe Lyon
On Tue, 2 Oct 2018 at 10:55, Richard Earnshaw (lists)

#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.

Post by Richard Earnshaw (lists)
I think the right fix here is to remove the "return" from the statements
and add __builtin_unreachable () at the end of the function.

OK, here is an updated version.

Post by Christophe Lyon
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?

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.

Post by Christophe Lyon
Thanks
Christophe