Date: Thu, 15 Dec 2016 16:32:10 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r310045 - head/sys/ddb Message-ID: <20161215160138.C1437@besplex.bde.org> In-Reply-To: <1943330.5ZCxhTZVMq@ralph.baldwin.cx> References: <201612140018.uBE0ICrE004686@repo.freebsd.org> <2285301.DAKmd1GIbI@ralph.baldwin.cx> <20161214140645.W3397@besplex.bde.org> <1943330.5ZCxhTZVMq@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 14 Dec 2016, John Baldwin wrote:
> On Wednesday, December 14, 2016 07:24:46 PM Bruce Evans wrote:
>> ...
>> 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
That is a good fix if you commit it and keep the casts to an unsigned type.
> from HEAD (though perhaps re-commit the 'val' part of initializing diff if
> making that API consistent is a good thing to do).
I guess you fixed the intptr_t vs db_expr_t mismatches.
Did you look at fixing the printf formats? I think ddb casts args to longs
too perfectly to match buggy long formats, so no errors should have been
detected at compile time. I don't like casting to intptr_t on all arches
to support 1 strange one where this is needed, but as usual casting to
the widest type is less ugly than using PRI*. sys/ddb has only 26 lines
of casts to long, 9 to unsigned long and 2 to long long, so fixing them
all won't be too painful.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20161215160138.C1437>