This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [ARM] Make _kill() a noreturn function (was: [ARM] Add endless loop to avoid a compiler warning on noreturn functions.)
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