svn commit: r333010 - head/sys/mips/mips

[Bruce Evans]

On Fri, 27 Apr 2018, John Baldwin wrote:

> On Thursday, April 26, 2018 09:29:30 AM Ian Lepore wrote:
>> On Thu, 2018-04-26 at 19:01 +0800, Li-Wen Hsu wrote:
>>> On Thu, Apr 26, 2018 at 7:59 AM, Ian Lepore <ian at freebsd.org> wrote:
>>>>
>>>> On Wed, 2018-04-25 at 19:46 +0000, Li-Wen Hsu wrote:
>>>>>
>>>>> Author: lwhsu (ports committer)
>>>>> Date: Wed Apr 25 19:46:39 2018
>>>>> New Revision: 333010
>>>>> URL: https://svnweb.freebsd.org/changeset/base/333010
>>>>>
>>>>> Log:
>>>>>   Fix mips32 build after r332951.
>>>>>
>>>>>   Approved by:        jhb
>>>>>
>>>>> Modified:
>>>>>   head/sys/mips/mips/pm_machdep.c
>>>>>
>>>>> Modified: head/sys/mips/mips/pm_machdep.c
>>>>> ==============================================================================
>>>>> --- head/sys/mips/mips/pm_machdep.c   Wed Apr 25 18:59:29 2018        (r333009)
>>>>> +++ head/sys/mips/mips/pm_machdep.c   Wed Apr 25 19:46:39 2018        (r333010)
>>>>> @@ -264,7 +264,7 @@ ptrace_single_step(struct thread *td)
>>>>>               va = locr0->pc + 4;
>>>>>       }
>>>>>       if (td->td_md.md_ss_addr) {
>>>>> -             printf("SS %s (%d): breakpoint already set at %lx (va %lx)\n",
>>>>> +             printf("SS %s (%d): breakpoint already set at %zx (va %zx)\n",
>>>>>                   p->p_comm, p->p_pid, td->td_md.md_ss_addr, va); /* XXX */
>>>>>               error = EFAULT;
>>>>>               goto out;
>>>>> @@ -500,7 +500,7 @@ ptrace_clear_single_step(struct thread *td)
>>>>>
>>>>>       if (error != 0) {
>>>>>               log(LOG_ERR,
>>>>> -                 "SS %s %d: can't restore instruction at %lx: %x\n",
>>>>> +                 "SS %s %d: can't restore instruction at %zx: %x\n",
>>>>>                   p->p_comm, p->p_pid, td->td_md.md_ss_addr,
>>>>>                   td->td_md.md_ss_instr);
>>>>>       }
>>>>>
>>>> This isn't right either.  %z is for size_t values, both md_ss_addr and
>>>> va are integers and a plain %x should be the right format.
>>> But it will break mips64:
>>>
>>> cc1: warnings being treated as errors
>>> /home/lwhsu/src/sys/mips/mips/pm_machdep.c: In function 'ptrace_single_step':
>>> /home/lwhsu/src/sys/mips/mips/pm_machdep.c:268: warning: format '%x'
>>> expects type 'unsigned int', but argument 4 has type 'uintptr_t'
>>> [-Wformat]
>>> /home/lwhsu/src/sys/mips/mips/pm_machdep.c:268: warning: format '%x'
>>> expects type 'unsigned int', but argument 5 has type 'uintptr_t'
>>> [-Wformat]
>>> /home/lwhsu/src/sys/mips/mips/pm_machdep.c: In function
>>> 'ptrace_clear_single_step':
>>> /home/lwhsu/src/sys/mips/mips/pm_machdep.c:505: warning: format '%x'
>>> expects type 'unsigned int', but argument 5 has type 'uintptr_t'
>>> [-Wformat]
>>> *** [pm_machdep.o] Error code 1
>>>
>>> Another way is cast arguments to uintmax_t and use %jx.  Will that be better?
>> Oh, my bad, my source was out of date. Now I see that the types are not
>> plain integers anymore. In that case, I think the only options are to
>> cast to uintmax_t and use %jx, or cast to void* and use %p.
>
> The %z does happen to work in this case since size_t == uintptr_t on MIPS,
> but I think %p is the best route since these really are pointers.  I've
> build-tested a patch to use %p and will commit it in a bit.

Ugh, these really aren't (kernel data) pointers.  They are user(?)
instruction pointers.  The correct integer type for representing
(both kernel and user) instruction pointers is uintfptr_t.  vm_offset_t
is next best.  vm_offset_t must be large enough to represent any virtual
address.  Both it and uintfptr_t were recently broken for at least
kernel profiling purposes on i386.  i386 still has a flat address
space, so a bit is not needed to distinguish function addresses from
data addresses, but kernel instruction addresses are now
indistinguishable from user instruction addresses.  ddb backtracing and
pmc have problems with this too, but are used more and have some fixes.

%p is only good for simple printing of pointers even for data pointers,
since it doesn't allow controlling the format.  E.g., it forces an 0x
prefix and doesn't allow controlling the field width or padding (except
possibly in the kernel where it has undocumented extensions).  This
gives bugs like wasting 2 columns for printing '0x' for wchan in db_ps
(wchan is about the least useful kernel pointer to print by default,
but that is another bug).  ps(1) has better printing for kernel
pointers, but still has lots of bogus like casting ki_wchan to long
instead of to uintptr_t for printing it, but before that it is an ABI
design bug for ki_wchan to be a kernel pointer instead of a good
representation of a kernel pointer as an integer.  ki_wchan ends up as
being raw bits via a type pun.

Bruce