Date: Mon, 6 Jul 2015 14:14:38 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Justin Hibbits <jrh29@alumni.cwru.edu> Cc: "Bjoern A. Zeeb" <bz@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org Subject: Re: svn commit: r285168 - head/sys/powerpc/powerpc Message-ID: <20150706123931.T983@besplex.bde.org> In-Reply-To: <CAHSQbTBXUmzHZmRUPnfEBj4XhoRKci7T3%2B%2Bjt4sfajCsqucP0A@mail.gmail.com> References: <201507051530.t65FUGQn085794@repo.freebsd.org> <CAHSQbTBXUmzHZmRUPnfEBj4XhoRKci7T3%2B%2Bjt4sfajCsqucP0A@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 5 Jul 2015, Justin Hibbits wrote: > On Jul 5, 2015 08:30, "Bjoern A. Zeeb" <bz@freebsd.org> wrote: >> >> Author: bz >> Date: Sun Jul 5 15:30:16 2015 >> New Revision: 285168 >> URL: https://svnweb.freebsd.org/changeset/base/285168 >> >> Log: >> Fix GENERIC64 and LINT64 powerpc builds after r285144. >> >> Modified: >> head/sys/powerpc/powerpc/trap.c >> >> Modified: head/sys/powerpc/powerpc/trap.c >> > ============================================================================== >> --- head/sys/powerpc/powerpc/trap.c Sun Jul 5 14:24:35 2015 > (r285167) >> +++ head/sys/powerpc/powerpc/trap.c Sun Jul 5 15:30:16 2015 > (r285168) >> @@ -426,10 +426,10 @@ printtrap(u_int vector, struct trapframe >> ver = mfpvr() >> 16; >> #if defined(AIM) >> if (MPC745X_P(ver)) >> - printf(" msssr0 = 0x%x\n", >> + printf(" msssr0 = 0x%\n", >> mfspr(SPR_MSSSR0)); >> #elif defined(BOOKE) >> - printf(" mcsr = 0x%x\n", >> + printf(" mcsr = 0x%" PRIxPTR "\n", >> mfspr(SPR_MCSR)); >> #endif >> break; >> > > Sigh, thanks. Since those are 32 bit registers I only tested my 32 bit > compiles. This change introduces a style bug and just moves the type error from a harmful one to a harmless one. It uses the PRI* mistake. powerpc only used this mistake 8 times before (1 PRIxPTR in mmu_oea64.c and 7 PRIxPTR's in trap.c). The type in the mistake doesn't even match the type of the arg. PRIxPTR is for printing uintptr_t, but the arg type is register_t. These are logically very differemt, but happen to be the same physically (both are 32 bits or both 64 bits, according to LP32 or LP64). All of the old uses are wrong too. The printf format is always uintptr_t, but: - in mmu_oea64.c, the arg type is vm_paddr_t. This follows the LP32/LP64 switch too - in trap.c, the 7 uses are all to print things in the stack frame. The arg type is always register_t. amd64 and i386 never use the PRI* mistake. They are simpler because the LP32/LP64 switch is at the arch level. i386 would have complications printing things like vm_paddr_t that depend on PAE, but the PRI* mistake is useless for handling such complications, since it has no support for the vm_paddr_t. Similarly for almost all types. register_t too. arm uses the PRI* mistake just 2 times. Both uses are just silly. PRIu32 is used to print a type that isn't even a uint32_t, but the binary promotion of a uint32_t (in the expression raw / 1000, where `raw' has type uint32_t). This is just an obfuscation, except on systems with ints smaller than 32 bits where it is a micro-optimization for speed. But POSIX now requires ints to be at least 32 bits. So the binary promotion of uint32_t is almost known. It it is uint32_t if ints are 32 bits, else int. Both cases can be handled by either %u or %d format. Strictly, %u should be used if ints are 32 bits, else %d should be used. But since `raw' <= UINT32_MAX, raw / 1000 < INT_MAX, so %d always works too. %u always works too because raw / 1000 is always nonnegative. Perhaps PRIu32 works right in this case. Args are subject to unary promotions, and this will change the type from uint32_t to int iff ints are larger than 32 bits, just like for the binary promotion. PRIu32 must be %d and not %u then, unless it can be shown that both %d and %u work and that the compiler doesn't warn about the harmless sign mismatch. If ints are less than 32 bits, then the binary and unary promotions of uint32_t are uint32_t again, and PRIu32 works right. To print a uint32_t without using PRI*, there is nothing better than casting it to u_long. This works well, and should be done in MI code that supports arches with ints less than 32 bits. But arm is MD, and only supports 32-bit ints. It shouldn't handle these complications. Since arm only supports 32-bit ints, it should above these complications by declaring 'raw' as plain u_int instead of uint32_t. (raw is only used in a local table that uses int for everything else. Making it unsigned is probably wrong too.) arm64, mips, sparc64 and kern never use the PRI* mistake. Printing register_t in MD code is very easy. Just cast to u_long and use %lx format. This handles the LP32/LP64 switch correctly in all cases. In the LP64 case, everything is really long or u_long and the cast only fixes the sign error than register_t is a signed type on most arches. In the LP32 case, all types are really 32 bits. All should be u_int, but register_t is usually signed int, and perhaps some broken types are long or u_long. The cast now fixes the sign error and tells the compiler to not compain about the logical type mismatch between %lx format and u_int args, so that the format can be %lx for both LP32 and LP64 case; the cast acts only on logical types so it doesn't cost anything at runtime. Printing register_t in MI code is not so easy. MI code shouldn't assume either LP32 or LP64, so it should cast to uintmax_t, but that has runtime costs if register_t is smaller than uintmax_t. The only excuse for PRI*'s existence is to avoid these runtime costs. But it is useless except for the standard types that it supports. The bug in the original version was that it didn't cast to u_long. amd64 and i386 can just omit such casts and hard-code the format as %lx for amd64 and %x for i386 (except, compilers should warn about the mismatch between the signed type register_t and the unsigned format). I can only find powerpc using this method in 1 printf (in cpu.c). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150706123931.T983>