Date: Fri, 9 Sep 2016 15:27:02 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Justin Hibbits <jhibbits@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r305636 - head/sys/ddb Message-ID: <20160909142012.W864@besplex.bde.org> In-Reply-To: <201609090416.u894GriK087316@repo.freebsd.org> References: <201609090416.u894GriK087316@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160909142012.W864>