svn commit: r310045 - head/sys/ddb

[John Baldwin]

On Wednesday, December 14, 2016 07:24:46 PM Bruce Evans wrote:
> On Tue, 13 Dec 2016, John Baldwin wrote:
> 
> > On Wednesday, December 14, 2016 12:18:12 AM John Baldwin wrote:
> >>
> >> Log:
> >>   Use casts to force an unsigned comparison in db_search_symbol().
> >>
> >>   On all of our platforms, db_expr_t is a signed integer while
> >>   db_addr_t is an unsigned integer value.  db_search_symbol used variables
> >>   of type db_expr_t to hold the current offset of the requested address from
> >>   the "best" symbol found so far.  This value was initialized to '~0'.
> >>   When a new symbol is found from a symbol table, the associated diff for the
> >>   new symbol is compared against the existing value as 'if (newdiff < diff)'
> >>   to determine if the new symbol had a smaller diff and was thus a closer
> >>   match.
> >>
> >>   On 64-bit MIPS, the '~0' was treated as a negative value (-1).  A lookup
> >>   that found a perfect match of an address against a symbol returned a diff
> >>   of 0.  However, in signed comparisons, 0 is not less than -1.  As a result,
> >>   DDB on 64-bit MIPS never resolved any addresses to symbols.  Workaround
> >>   this by using casts to force an unsigned comparison.
> >
> > I am somewhat unsure of why this worked on other architectures.  amd64
> > treated ~0 as 0xffffffff which when assigned to a 64-bit register was
> 
> It is treated the same on all non-broken arches, and I doubt that MIPS64
> is broken here.
> 
> ~0 is -1 on all arches (since no supported arches are 1's complement),
> but it was assigned to 'unsigned diff' which gives 0xffffffff on all
> supported arches.  Then diff is assigned to 'size_t newdiff', giving
> no change in type or value on 32-bit arches, and zero-extension to the
> same value but larger type on 64-bit arches.
> 
> Note that the type of diff is very broken.  It is unsigned.  This corrupts
> not only ~0, but also the final difference returned in *offp.  The return
> value also gets corrupted from 32 bits unsigned to 32 bits signed on
> 32-bit arches, so offsets >= 2**31 are fragile on all arches.  I think
> most cases work because the offset is 0.
> 
> I don't see how initializing to 'val' can help.  It still gets corrupted
> to unsigned, and I would expect it to make the problem much worse because
> the truncation gives an even smaller value.  E.g., if val is any multiple
> of 2**32, truncating it gives 0.  Without the truncation, 'val' is a good
> upper bound for the offset.

I should have mentioned this change in the commit.  Other symbol resolution
APIs in the kernel follow the convention that the raw address is returned
as the offset ('diff') if no matching symbol is found.  This change made
db_search_symbol() consistent with that.

> Perhaps the problem is more with the initialization of newdiff.  It is
> initialized to the corrupted value in diff.  I don't know the details of
> the API, but if X_db_search_symbol() uses this as an input parameter to
> limit the search, it might be happier with the smaller corrupted value
> than the larger one.  X_db_search_symbol() seems to have working types
> and values, though still too much hard-coding of types.
> 
> Your casts to uintmax_t have no effect.  The types are already unsigned,
> and the corrupted values cannot be fixed by later casts.

So I got myself into a bit of a mess and the hints are in my commit message
(but obscurely).  I am working within a branch and one of the things I had
done in this branch was to fix numerous warnings compiling DDB with a MIPS
n32 kernel.  n32 is quite special in that pointers are 32-bits while registers
are 64-bits, so db_addr_t is a uint32_t, and db_expr_t was a int64_t.  I had
"fixed" newdiff and diff to both be db_expr_t to match the type that
X_db_search_symbol() returns and that is how I got into trouble as that
changed 'diff' to be signed instead of unsigned.  I will revert the commit
from HEAD (though perhaps re-commit the 'val' part of initializing diff if
making that API consistent is a good thing to do).

-- 
John Baldwin