From owner-svn-src-all@FreeBSD.ORG Sat Nov 29 13:49:40 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E3451130; Sat, 29 Nov 2014 13:49:39 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id A7BBAA08; Sat, 29 Nov 2014 13:49:39 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 21002780C45; Sun, 30 Nov 2014 00:49:31 +1100 (AEDT) Date: Sun, 30 Nov 2014 00:49:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Takahashi Yoshihiro Subject: Re: svn commit: r275241 - head/sys/boot/pc98/boot2 In-Reply-To: <201411291222.sATCMWJv060917@svn.freebsd.org> Message-ID: <20141129235225.C1402@besplex.bde.org> References: <201411291222.sATCMWJv060917@svn.freebsd.org> 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=BdjhjNd2 c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=18GXijtb-ZROKZIxSG0A:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org 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: Sat, 29 Nov 2014 13:49:40 -0000 On Sat, 29 Nov 2014, Takahashi Yoshihiro wrote: > Log: > MFi386: r275059, r275061, r275062 and r275191 (by rdivacky) > > Shrink boot2 by a couple more bytes. These changes don't look too good to me. One is just a bug. > Modified: head/sys/boot/pc98/boot2/boot2.c > ============================================================================== > --- head/sys/boot/pc98/boot2/boot2.c Sat Nov 29 11:50:19 2014 (r275240) > +++ head/sys/boot/pc98/boot2/boot2.c Sat Nov 29 12:22:31 2014 (r275241) > ... > @@ -533,6 +534,7 @@ parse() > const char *cp; > unsigned int drv; > int c, i, j; > + size_t k; This still has a style bug (unsorting). > > while ((c = *arg++)) { > if (c == ' ' || c == '\t' || c == '\n') > @@ -555,7 +557,7 @@ parse() > #if SERIAL > } else if (c == 'S') { > j = 0; > - while ((unsigned int)(i = *arg++ - '0') <= 9) > + while ((i = *arg++ - '0') <= 9) > j = j * 10 + i; > if (j > 0 && i == -'0') { > comspeed = j; This used to work. The cast was an obfuscated way of checking for characters less than '0'. Removing the cast breaks the code. Garbage non-digits below '0' (including all characters below '\0') are now treated as digits. This still asks for the pessimization of promoting *arg++ to int before checking it. The compiler might be able to unpessimize this, but large code shouldn't be asked for. The following should be better: uint8_t uc; ... /* Check that the compiler generates 8-bit ops. */ while ((uc = *arg++ - '0') <= 9) j = j * 10 + uc; if (j > 0 && uc == (uint8_t)-'0') { comspeed = j; I don't see a correct way to avoid the second check of uc. Subtracting '0' and using the unsigned obfuscation to check for digits is used in several other places: drv = *arg - '0'; if (drv > 9) ...; This works since 'drv' is unsigned. Drive numbers above 9 are not supported, so uint8_t could be used here, saving 1 byte. At least 1 more byte can be saved in each of 'drv == -1' and 'if (drv == -1)'. 'drv' would probably need to be promoted before use; it is only used once to initialize dsk.drive. It can be promoted there using no extra bytes by statically initializing dsk.drive to 0 and storing only 8 bits from 'drv'. The total savings should be 4-5 bytes. dsk.unit = *arg - '0'; if (arg[1] != ',' || dsk.unit > 9) ...; dsk.unit is also unsigned. Unit numbers above 9 are not supported, so uint8_t should work here too, but since dsk.unit probably needs to be a 32-bit type and there is no local variable like 'drv', the savings are not as easy. dsk.slice = WHOLE_DISK_SLICE; ... dsk.slice = *arg - '0' + 1; if (dsk.slice > NDOSPART + 1) ...; Now disk.slice is already uint8_t, so this code should be optimal. However, when dsk.slice is used it has to be promoted and about 3 bytes are wasted there. To avoid this wastage, make disk.slice plain int (no need for unsigned hacks), initialize it all to 0, and store only the low 8 bits in the above. There are so many of these that it might be smaller to use a function that handles the general case. I suspect that this is not in fact smaller, since it would need complications like passing back the endptr (like strtol(), not atoi()). Bruce