svn commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Ngie Cooper (yaneurabeya)]

> On Aug 3, 2017, at 00:53, Bruce Evans <brde at optusnet.com.au> wrote:
> 
> On Thu, 3 Aug 2017, Ngie Cooper wrote:
> 
>> Log:
>> Fix the return types for printf and putchar to match their libc and
>> POSIX equivalents
>> 
>> Both printf and putchar return int, not void.
>> 
>> This will allow code that leverages the libcalls and checks/rely on the
>> return type to interchangeably between loader code and non-loader
>> code.
>> 
>> MFC after:	1 month
>> 
>> Modified:
>> head/sys/boot/arm/at91/libat91/lib.h
>> head/sys/boot/arm/at91/libat91/printf.c
>> head/sys/boot/arm/at91/libat91/putchar.c
>> head/sys/boot/arm/ixp425/boot2/ixp425_board.c
>> head/sys/boot/arm/ixp425/boot2/lib.h
>> head/sys/boot/i386/boot2/boot2.c
> 
> This is wrong for at least i386/boot2.  It isn't part of the loader, and
> saves space by not returning unused values.
> 
>> Modified: head/sys/boot/i386/boot2/boot2.c
>> ==============================================================================
>> --- head/sys/boot/i386/boot2/boot2.c	Thu Aug  3 03:45:48 2017	(r321968)
>> +++ head/sys/boot/i386/boot2/boot2.c	Thu Aug  3 05:27:05 2017	(r321969)
>> @@ -114,8 +114,8 @@ void exit(int);
>> static void load(void);
>> static int parse(void);
>> static int dskread(void *, unsigned, unsigned);
>> -static void printf(const char *,...);
>> -static void putchar(int);
>> +static int printf(const char *,...);
>> +static int putchar(int);
> 
> These are freestanding static functions, so they have nothing to do
> with library functions except their name is a hint that they are
> similar.
> 
> Since they are static, -funit-at-a-time might allow the unused return values
> to be optimized away.  Then returning unused values would be just an
> obfuscation.
> 
> This file still has a static memcpy() which is quite different from the
> libc version.  It doesn't return an unused value, and its arg types are
> all different (no newfangled size_t or newerfangled restrict).
> 
> Freestanding versions (static and otherwise) cause problems with builtins.
> -ffreestanding turns off all builtins.  The static memcpy used to be
> ifdefed so as to use __builtin_memcpy instead of the static one if the
> compiler is gcc.  That apparently broke with gcc-4.2, since the builtin
> will call libc memcpy() in some cases, although memcpy() is unavailable
> in the freestanding case.

I get the point about them being freestanding functions, but if the functions aren’t meant to be compatible they should be named differently. Part of the issue some code that bridged loader and non-loader users (bootdevtest and zfsboottest for example) relied on feature parity (in part because the ZFS code required it and because of how things are compiled/linked together). If compilers get pedantic enough, then they’ll flag these issues as errors and we’ll have to address these compatibilities at that point.

My intent was ok (I think), but the implementation I did is wrong, so I’ll revert the change completely and leave it for someone else to deal with the incompatibilities (I’ll just integrate bootdevtest and zfsboottest into buildworld under sys/boot so they won’t remain broken for weeks on end, like they were recently).

Thanks,
-Ngie
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 842 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20170803/ced009d5/attachment.sig>