From owner-svn-src-all@FreeBSD.ORG Tue May 22 20:48:42 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id C999C1065672; Tue, 22 May 2012 20:48:42 +0000 (UTC) (envelope-from gnn@neville-neil.com) Received: from vps.hungerhost.com (vps.hungerhost.com [216.38.53.176]) by mx1.freebsd.org (Postfix) with ESMTP id 7C81F8FC0A; Tue, 22 May 2012 20:48:42 +0000 (UTC) Received: from [209.249.190.124] (port=41221 helo=punk.neville-neil.com.neville-neil.com) by vps.hungerhost.com with esmtpa (Exim 4.77) (envelope-from ) id 1SWw0X-00010i-Pj; Tue, 22 May 2012 16:48:41 -0400 Date: Tue, 22 May 2012 16:49:37 -0400 Message-ID: <861umcx7oe.wl%gnn@neville-neil.com> From: gnn@freebsd.org To: Bruce Evans In-Reply-To: <20120523050739.H3621@besplex.bde.org> References: <201205221818.q4MII7lk019626@svn.freebsd.org> <20120523050739.H3621@besplex.bde.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/23.4 (amd64-portbld-freebsd10.0) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - vps.hungerhost.com X-AntiAbuse: Original Domain - freebsd.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - neville-neil.com Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, "David E. O'Brien" Subject: Re: svn commit: r235797 - head/contrib/gcc X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 May 2012 20:48:42 -0000 At Wed, 23 May 2012 06:05:06 +1000 (EST), Bruce Evans wrote: > > On Tue, 22 May 2012, David E. O'Brien wrote: > > > Log: > > Do not incorrectly warn when printing a quad_t using "%qd" on 64-bit platforms. > > I think I like this, since it is technically correct, and will find a > different set of type mismatches. > > > Modified: head/contrib/gcc/c-format.c > > ============================================================================== > > --- head/contrib/gcc/c-format.c Tue May 22 17:44:01 2012 (r235796) > > +++ head/contrib/gcc/c-format.c Tue May 22 18:18:06 2012 (r235797) > > @@ -287,7 +287,11 @@ static const format_length_info printf_l > > { > > { "h", FMT_LEN_h, STD_C89, "hh", FMT_LEN_hh, STD_C99 }, > > { "l", FMT_LEN_l, STD_C89, "ll", FMT_LEN_ll, STD_C9L }, > > +#ifdef __LP64__ > > + { "q", FMT_LEN_l, STD_EXT, NULL, 0, 0 }, > > +#else > > { "q", FMT_LEN_ll, STD_EXT, NULL, 0, 0 }, > > +#endif > > { "L", FMT_LEN_L, STD_C89, NULL, 0, 0 }, > > { "z", FMT_LEN_z, STD_C99, NULL, 0, 0 }, > > { "Z", FMT_LEN_z, STD_EXT, NULL, 0, 0 }, > > > > Of course, %qd should never be used. On LP32, quad_t is long long, while > on LP64, quad_t is long, so any use of %qd required messy ifdefs or > casting a quad_t arg to long long to work on LP64. Now, %qd actually > matches quad_t on LP64, so casting to long long is no longer needed, but > anything that does it is broken and would require changing to cast to > quad_t, or perhaps to omit the cast. You might find too much bugware that > (1) uses %qd > (2) uses it on args that don't always have type quad_t > (3) casts to long long. Casting to quad_t didn't work on LP64 before, so > probably nothing in FreeBSD does it. > > Grepping for %q in /sys finds only a few uses of %q with a few type > mismatches (different mismatches before and after this commit). (I > didn't grep for more compicated formats with stuff between the % and the > q.): > > % ./compat/ndis/subr_ntoskrnl.c: printf("timer sets: %qu\n", ntoskrnl_timer_sets); > % ./compat/ndis/subr_ntoskrnl.c: printf("timer reloads: %qu\n", ntoskrnl_timer_reloads); > % ./compat/ndis/subr_ntoskrnl.c: printf("timer cancels: %qu\n", ntoskrnl_timer_cancels); > % ./compat/ndis/subr_ntoskrnl.c: printf("timer fires: %qu\n", ntoskrnl_timer_fires); > > This was broken before on LP64. It now works accidentally. All these %qu > formats are bogus, since the variables don't have type quad_t; they have > type uint64_t. Now that %q works, quad_t's are actually easier to print > that int64_t's since there is a format letter just for them. But this > only helps if the variables actually have type quad_t. > > % ./dev/esp/ncr53c9x.c: panic("%s: lun %qx for ecb %p does not exist", __func__, > % ./dev/esp/ncr53c9x.c: panic("%s: slot %d for lun %qx has %p instead of ecb " > > This is under DIAGNOSTIC and is now broken. Again the variable doesn't have > type [u_]quad_t. It has type int64_t. This is printed using the doubly- > incompatible format %qx (signed type but unsigned format; int64 type but > quad format letter). Then to be bug for bug compatible with old gcc, this > incompatible format was cast to a different doubly-incompatible type. > > % ./fs/msdosfs/msdosfs_vnops.c: printf(" va_blocksize %lx, va_rdev %x, va_bytes %qx, va_gen %lx\n", > > This is under the non-option MSDOSFS_DEBUG. It was broken before, but now > works, since va_bytes actually has type u_quad_t and was not bogusly cast > to unsigned long long). > > % ./fs/nfsclient/nfs_clstate.c: printf("lck typ=%d fst=%qd end=%qd\n", > % ./fs/nfsclient/nfs_clstate.c: printf("lck typ=%d fst=%qd end=%qd\n", > > These are under !__FreeBSD__ ifdefs. The ifdefs are related to the type > errors. The types are u_int64_t. Under FreeBSD, they are printed > correctly with only 1 type error for each: they are bogusly cast to > intmax_t (sign error), then printed with %ju (this matches the sign of > the variables but is a sign error relative to the cast. The errors > normally cancel). Under !__FreeBSD__, they are printed using %qd, > without any casts. There is now a sign error in the format, and type > mismatches. This would now compiler under FreeBSD despite all the > type mismatches -- the sign error doesn't matter in practice; u_int64_t > matches u_quad_t on all supported arches, and after your changes the > logical mismatch is no longer detected by gcc. > > % ./geom/geom_map.c: ret = sscanf(line, "search:%qi:%qi:%63c", > > This has more than the usual density of bugs: > - scanf() is unusable but is used > - %q format is used > - % the variables are further from being quad_t's than usual. They are > off_t's. off_t happens to have type int64_t, so this works accidentally > on all supported arches. > - %qi is an unusual spelling of %qd. > > You didn't change gcc for scanf. "q" for it still maps to FMT_LEN_ll. > The above should never have compiled on LP64. > > % ./gnu/fs/xfs/FreeBSD/xfs_buf.c: printf("bread failed specvp %p blkno %qd BBTOB(len) %ld\n", > > The variable has type xfs_daddr_t, which is __s64, which is signed long long > int in xfs/FreeBSD. This used to be compatible with %qd, but no longer is. > xfs shouldn't compile any more on LP64. > > Now I prefer the old way, after fixing the bugs found by switching. > It finds more bugs under FreeBSD, and is bug for bug compatible with > distribution gcc and probably with other system's headers under > !FreeBSD. Except under FreeBSD, I prefer %q to be an error. The above > shows it only being used 12 times in /sys, with most uses of it being > bugs. Fixing these bugs would leave about 1 correct use of it -- for > printing the quad_t in unreachable code in msdosfs. This use would be > easy to avoid too (just cast to uintmax_t). Uses in userland are > hopefully east to fix and avoid too. In an old userland, there were > only about 50, with most in netstat/inet6.c, tcopy.c, ftpd, fsirand, > and contrib'ed code. Of course you can't make it an error for the > contrib'ed code. > Instead of replying to the original commit I'll just add to the chain. Note that this change broke buildworld: src/usr.sbin/ppp/throughput.c: In function 'throughput_disp': src/usr.sbin/ppp/throughput.c:119: warning: format '%6qu' expects type 'long unsigned int', but argu And several more. Best, George