From owner-svn-src-head@freebsd.org Mon Mar 11 09:18:13 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 60C651538F10; Mon, 11 Mar 2019 09:18:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id A7EC08C425; Mon, 11 Mar 2019 09:18:12 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id C851A439225; Mon, 11 Mar 2019 20:18:02 +1100 (AEDT) Date: Mon, 11 Mar 2019 20:18:01 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Justin Hibbits cc: Alexey Dokuchaev , Justin Hibbits , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r344960 - head/sys/powerpc/powerpc In-Reply-To: <20190310171640.31bb9c54@titan.knownspace> Message-ID: <20190311191740.J2090@besplex.bde.org> References: <201903090318.x293IcLc023548@repo.freebsd.org> <20190309085058.GA60945@FreeBSD.org> <20190310171640.31bb9c54@titan.knownspace> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=UJetJGXy c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=JGUapKmP4T1tokrbwaUA:9 a=JNDmUP7PZXdV0YNV:21 a=JfYhJGOWAlVr0w_c:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-Rspamd-Queue-Id: A7EC08C425 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.86 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; NEURAL_HAM_SHORT(-0.86)[-0.859,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 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: Mon, 11 Mar 2019 09:18:13 -0000 On Sun, 10 Mar 2019, Justin Hibbits wrote: > On Sat, 9 Mar 2019 08:50:58 +0000 > Alexey Dokuchaev wrote: > >> On Sat, Mar 09, 2019 at 03:18:38AM +0000, Justin Hibbits wrote: >>> ... >>> Log: >>> powerpc: Print trap frame address for fatal traps >>> ... >>> Modified: head/sys/powerpc/powerpc/trap.c >>> ============================================================================== >>> --- head/sys/powerpc/powerpc/trap.c Sat Mar 9 03:15:09 >>> 2019 (r344959) +++ head/sys/powerpc/powerpc/trap.c >>> Sat Mar 9 03:18:37 2019 (r344960) @@ -546,6 +546,7 @@ >>> printtrap(u_int vector, struct trapframe *frame, int i printf(" >>> current msr = 0x%" PRIxPTR "\n", mfmsr()); printf(" >>> lr = 0x%" PRIxPTR " (0x%" PRIxPTR ")\n", frame->lr, >>> frame->lr - (register_t)(__startkernel - KERNBASE)); No one wrote that as stated above. Some mail program mangled it. >>> + printf(" frame = %p\n", frame); >>> printf(" curthread = %p\n", curthread); >> >> Some of those are printed with %PRIxPTR, some with %p. Perhaps this >> inconsistency can be avoided? >> ... > > PRIxPTR is for printing a pointer-like value that's not a pointer. The > other values printed are of type register_t, not void *. PRI*any is for writing style bugs in the source and sometimes in the output. Sometimes also type errors. It should never be used. The above (not the 1 line in this commit, but nearby) demonstrates common errors in using it: - the format given by PRIany is not fully specified. It may contain a lebgth modifier. Thus it is unusable in carefully formatted output where you would want to add your own length modifier. It also requires knowing too much about this feature to know that adding an 0x prefix or perhaps a '#', '0' or ' ' modifier doesn't mess up the format. This is not much of a problem here. The lengths are only accidentally the same. Maybe they are even the same as for %p format. - first type error: PRIxPTR only has defined behaviour for printing variables of type uintptr_t, but mfmsr() has type register_t. The type mismatch is not only logical, but physical (register_t is signed). Correctness depends on register_t having the same size as uintptr_t and the treatment of signed values as unsigned not being too magic (I think it is implementation-defined but not undefined; however, printing uintptr_t using PRTdPTR gives undefined behaviour on values not representable as intptr_t. It requires knowing too much to know that the above use of PRIxPTR is correct enough. As usual, it is easier to not use PRIany. Just cast to uintmax_t and use %jx. This is much shorter and clearer after correcting the above by adding a cast to uintptr_t. Using uintmax_t has the minor problem that it is wasteful for 32-bit args and will become wasteful for 64-bit args and more wasteful for 32-bit args when uintmax_t becomes 128, 256, ... bits. - second type error: %p only has defined behaviour for printing variables of type void *, but curthread has type struct thread *. This can be fixed by casting to void *, or fixed better by casting to uintmax_t and using %jx. More details below. - poor formatting from %p. %p is guaranteed to bad for formatted output. It is specified to give an (any) implementation-defined sequence of printing characters. To use it except for low quality debugging output, not quite as above (the above attempts medium quality, with some alignment of fields but no attention to field widths for number values), you first have to know what the implementation defines, then don't use it when it is unsuitable. It is easiest to never use it. In FreeBSD, printf(3) documents its format as being as if it is %#x or %#lx. This gives no control over the field width. Its format is different and undocumented for printf(9), but not as bad -- field widths and maybe other things now work for %p. The 0x prefix usually given by '#' is still unavoidable. Conversion of pointers to uintmax_t or intmax_t gives full control over the format, just like for converted integer types. This is not quite easier and clearer for pointers. 3 casts are needed to go from an arbitrary pointer to a uintmax_t. First to const volatile void * (not to plain void *, since that gives cast-qual warnings if the pointer is const or volatile). Then to uintptr_t. Then to uintmax_t. In theory, conversion of a pointer to an integer using these casts may change its representation, and we should use a different method to see the original bits. %p is no good for this of course -- it might convert to an integer using casts too, and its its format is poorly documented. To see the original bits, memcpy() the pointer to an array of bytes and print the bytes. In practice, conversion using casts preserves all the bits, and the only difference between printing the integer and printing the bytes is that printf() has better support for the former and the output may be more difficult to understand even if it is in hex, due to endianness differences. ps (in both the kernel and userland IIRC) has some support for printing kernel addresses in a compressed format with a fixed number of leading 0 and/or f nybbles suppressed. This is most needed and also most broken for 64-bit addresses. On some arches, all kernel addresses are in the top half of the address space, so plain %[lx] format gives the same field width for all kernel addresses, but on 64-bit arches this is 16 bytes wide and tends to break formatting in other ways. Bruce