[ARM] Make _kill() a noreturn function (was: [ARM] Add endless loop to avoid a compiler warning on noreturn functions.)
Richard Earnshaw (lists)
Richard.Earnshaw@arm.com
Fri Oct 5 11:02:00 GMT 2018
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
More information about the Newlib
mailing list