From owner-svn-src-head@freebsd.org Fri Sep 9 05:27:11 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E5AE1BD2696; Fri, 9 Sep 2016 05:27:11 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 76B413D1; Fri, 9 Sep 2016 05:27:11 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id AD057104824F; Fri, 9 Sep 2016 15:27:03 +1000 (AEST) Date: Fri, 9 Sep 2016 15:27:02 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Justin Hibbits cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r305636 - head/sys/ddb In-Reply-To: <201609090416.u894GriK087316@repo.freebsd.org> Message-ID: <20160909142012.W864@besplex.bde.org> References: <201609090416.u894GriK087316@repo.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=VIkg5I7X c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=TitAgUKP1XWEueYwuxwA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Sep 2016 05:27:12 -0000 On Fri, 9 Sep 2016, Justin Hibbits wrote: > Log: > Correct the type of db_cmd_loop_done. > > On big endian hardware that uses 1 byte bool a type mismatch of bool vs int will > cause the least signifcant byte of db_cmd_loop_done to be set, but the MSB to be > read, and read as 0. This causes ddb to stay in an infinite loop. Hrmph. I objected to converting ddb to bool. > Modified: head/sys/ddb/db_command.c > ============================================================================== > --- head/sys/ddb/db_command.c Fri Sep 9 02:02:13 2016 (r305635) > +++ head/sys/ddb/db_command.c Fri Sep 9 04:16:53 2016 (r305636) > @@ -59,7 +59,7 @@ __FBSDID("$FreeBSD$"); > /* > * Exported global variables > */ > -bool db_cmd_loop_done; > +int db_cmd_loop_done; > db_addr_t db_dot; > db_addr_t db_last_addr; > db_addr_t db_prev; It used to use boolean_t, but was misinitialized with 0 and 1 instead of FALSE and TRUE. Then it used bool, but was misinitialized with 0 and 1 instead of false and true. Now it is initialized with matching types for the literals, but its type regressed 2 steps. But what was the problem? (int)1 (or any nonzero value) is converted to (bool)true on assigment. The final value is always 1 if converted back to an int, and probably also when viewed as bits in memory. When bool has 8 bits big endian, this means that it has bits 0b00000001 where you can't really know the bit order within a byte. The bugs were actually that: - this variable was not declared in a header file, first as boolean_t, then as bool - it was misdeclared as 'extern int' in db_run.c. This accidentally matched boolean_t, but not bool - conversion to bool in 1 place gave a type mismatch. This might have caused memory overruns on all systems, but the bug was more obvious on big-endian systems. On i386, I get the variable addresses: ... c0bc8288 B db_dot c0bc828c B db_last_addr c0bc8290 B db_prev c0bc8294 B db_next c0bc8298 B db_cmd_loop_done c0bc829c b db_last_command c0bc82a0 b escstate.4091 c0bc82a4 b db_lbuf_start c0bc82a8 b db_lbuf_end ... when compiled by gcc-4.2.1 (gcc-4.2.1 generates the same '.comm xxx,1,1' directive as clang, as required by the ABI. The variable is pessimized for both space and time (one reason I don't like to use compiler bool). The ABI specifies packing to 8 bits but not to 1 bit and not expansion to 32 bits. The linker expands it to 32 bits anyway, but the compiler doesn't know this so it has to use slightly larger and slower instructions to use it. Compilers do OK for bools in static functions, but for public functions they don't even trust the ABI, and on x86 do 2 extra movzbl (register) instructions and a few extra loads and stores to do null conversions of bools in each of the caller and callee in code like 'extern bool b(bool); bool q(bool r) { return (b(r)); }'. These extra instructions fix some bool-int mismatches on little-endian systems only. Loading 32 bits from db_cmd_loop_done might give garbage in the top bits if the top bits are actually for another variable, but zero-extending the low bits fixes this (a 32-bit store would clobber the other variable). But in the big-endian case, it would be the garbage top bits that are zero-extended. It is impossible to determine where the low bits are so as to adjust, starting from the incorrect type. This is perhaps the main disadvantage of big-endian format. Bruce