From owner-svn-src-all@FreeBSD.ORG Mon Apr 13 13:07:46 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id C2E3DBC5; Mon, 13 Apr 2015 13:07:46 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 83214782; Mon, 13 Apr 2015 13:07:46 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 33C531A4C74; Mon, 13 Apr 2015 23:07:36 +1000 (AEST) Date: Mon, 13 Apr 2015 23:07:35 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Andrew Turner Subject: Re: svn commit: r281307 - head/sys/boot/efi/boot1 In-Reply-To: <20150413114937.469db8ce@bender> Message-ID: <20150413221737.O2186@besplex.bde.org> References: <201504091015.t39AFlkh016216@svn.freebsd.org> <20150409212938.T3716@besplex.bde.org> <20150413114937.469db8ce@bender> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=L/MkHYj8 c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=DRGEgjlppHb5nJda_S0A:9 a=CjuIK1q_8ugA:10 Cc: Andrew Turner , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Apr 2015 13:07:47 -0000 On Mon, 13 Apr 2015, Andrew Turner wrote: > On Thu, 9 Apr 2015 21:38:02 +1000 (EST) > Bruce Evans 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 is doesn't declare printf() as like itself. This works provided is not (ab)used for the -ffreestanding case. 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. 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. 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