Date: Tue, 22 May 2012 16:49:37 -0400 From: gnn@freebsd.org To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, "David E. O'Brien" <obrien@freebsd.org> Subject: Re: svn commit: r235797 - head/contrib/gcc Message-ID: <861umcx7oe.wl%gnn@neville-neil.com> In-Reply-To: <20120523050739.H3621@besplex.bde.org> References: <201205221818.q4MII7lk019626@svn.freebsd.org> <20120523050739.H3621@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?861umcx7oe.wl%gnn>