Date: Mon, 13 Apr 2015 23:07:35 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andrew Turner <andrew@fubar.geek.nz> Cc: Andrew Turner <andrew@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r281307 - head/sys/boot/efi/boot1 Message-ID: <20150413221737.O2186@besplex.bde.org> In-Reply-To: <20150413114937.469db8ce@bender> References: <201504091015.t39AFlkh016216@svn.freebsd.org> <20150409212938.T3716@besplex.bde.org> <20150413114937.469db8ce@bender>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 13 Apr 2015, Andrew Turner wrote:
> On Thu, 9 Apr 2015 21:38:02 +1000 (EST)
> Bruce Evans <brde@optusnet.com.au> wrote:
>
>> On Thu, 9 Apr 2015, Andrew Turner wrote:
>>
>>> Log:
>>> Print error values with hex to make it easier to find the EFI
>>> error type.
>>>
>>> Modified:
>>> head/sys/boot/efi/boot1/boot1.c
>>>
>>> Modified: head/sys/boot/efi/boot1/boot1.c
>>> ==============================================================================
>>> --- head/sys/boot/efi/boot1/boot1.c Thu Apr 9 10:12:58
>>> 2015 (r281306) +++ head/sys/boot/efi/boot1/boot1.c
>>> Thu Apr 9 10:15:47 2015 (r281307) @@ -330,18 +330,18 @@
>>> load(const char *fname) status =
>>> systab->BootServices->LoadImage(TRUE, image, bootdevpath, buffer,
>>> bufsize, &loaderhandle); if (EFI_ERROR(status))
>>> - printf("LoadImage failed with error %d\n", status);
>>> + printf("LoadImage failed with error %lx\n",
>>> status);
>>
>> How would anyone guess that a number like "10" is in hex?
>>
>> Hex numbers should usually be printed using "%#..." format. If the
>> boot loader doesn't have that, then use an 0x prefix.
>>
>> This shouldn't compile. 'status' cannot have type int and type
>> unsigned long at the same time. clang warns even without -Wformat in
>> CFLAGS.
>
> It is either uint32_t on 32-bit architectures, or uint64_t on 64-bit
> architectures. I know it's wrong on 32-bit, however on both
> architectures we use this code a long is 32-bit.
That allows it to run, but not to compile.
It only compiles because format checking is broken.
Format checking is broken because printf() is not declared as
__printflike(). Normally, this doesn't matter, because the
compiler knows that printf() is like itself. However,
-ffreestanding makes printf() just another function so the
compiler must not know anything special about it unless you
tell it. The kernel is careful about this and declares printf()
as __printflike() in sys/systm.h. Boot programs are not careful
about this.
Even <stdio.h> is doesn't declare printf() as like itself. This
works provided <stdio.h> is not (ab)used for the -ffreestanding
case. <stdio.h> declares functions as __printflike() more or less
iff they were not in C90. For example, it declares snprintf() as
__printflike() since this was necessary before C99 standardized it
and is still necessary to support -std=N where N is older than c99.
Boot programs are also not careful about __restrict. <stdio.h> is
careful about this. It seems to be careful in the same way as
for __printflike(), but that means using it for old functions like
printf() too, since it was never automatic in C90.
boot1.c actually gets its buggy declaration of printf() by having
its own static function named printf() and not declaring this
as __printflike(). Similarly for all of its other printflike
functions.
Similarly in many other boot programs. Grepping for ^printf finds
7 more instances of the bug in 7 different files. Most seem to be
the result of using cut and paste to copy the bug from i386/boot2.
There is not a single instance of __printflike() in the 8 files.
There are 2 instances of in the whole boot hierarchy.
Boot headers aren't so broken. <stand.h> uses __printflike() but
hasn't caught up with 'restrict' yet. libstand has a reduced
printf() but this is still too bloated to use in the small boot1 and
boot2 programs.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150413221737.O2186>