Date: Wed, 23 May 2012 06:05:06 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "David E. O'Brien" <obrien@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r235797 - head/contrib/gcc Message-ID: <20120523050739.H3621@besplex.bde.org> In-Reply-To: <201205221818.q4MII7lk019626@svn.freebsd.org> References: <201205221818.q4MII7lk019626@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120523050739.H3621>