Date: Fri, 15 Jul 2016 06:41:00 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mark Johnston <markj@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r302854 - head/sys/kern Message-ID: <20160715053515.S2290@besplex.bde.org> In-Reply-To: <201607141849.u6EIn53i038324@repo.freebsd.org> References: <201607141849.u6EIn53i038324@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> Log:
> Let DDB's buf printer handle NULL pointers in the buf page array.
I noticed some other bugs in this code:
> Modified: head/sys/kern/vfs_bio.c
> ==============================================================================
> --- head/sys/kern/vfs_bio.c Thu Jul 14 17:31:29 2016 (r302853)
> +++ head/sys/kern/vfs_bio.c Thu Jul 14 18:49:05 2016 (r302854)
> @@ -4725,8 +4725,12 @@ DB_SHOW_COMMAND(buffer, db_show_buffer)
> for (i = 0; i < bp->b_npages; i++) {
> vm_page_t m;
2 style bugs.
> m = bp->b_pages[i];
> - db_printf("(%p, 0x%lx, 0x%lx)", (void *)m->object,
> - (u_long)m->pindex, (u_long)VM_PAGE_TO_PHYS(m));
> + if (m != NULL)
> + db_printf("(%p, 0x%lx, 0x%lx)", m->object,
This loses the careful cast of m->object to void *. %p gives undefined
behaviour on pointers that are not void *. All other nearby %p's and
most elsewhere have this bug.
%p is a bad format anyway. On exotic arches, the cast might actually
change the representation, but %p is only useful in debugging code and
there you would usually prefer to see the raw bits. %p also gives little
control over the format (none in userland, and undocumented extensions
in the kernel). To get control of the format, the value can be printed
using %<...>j[dx...] after casting to (uintmax_t)(uintptr_t)(void *)
(sometimes to const/volatile void *). This cast is even uglier but more
needed. It may still corrupt the resolution. To get full control,
pointers should be printed by copying there bits and printing the array.
The explicit 0x prefix should only be used in tabular formats. Here the
value is often 0. This should be printed using %#.
> + (u_long)m->pindex,
This is broken on all 32-bit systems. pindex is 64 bits to handle large
file offsets (64-bit file offset needs 52-bit pindex with 4K pages). The
casts truncates to 32 bits.
> + (u_long)VM_PAGE_TO_PHYS(m));
This is broken on 32-bit systems with large physical adress spaces (only
i386 with PAE?). vm_paddr_t is 64 bits to handle large physical offsets.
i386 with PAE needs only 44. This reduces to 32.
printf format checking gives too many printf format bugs from bad fixes
using bogus casts to make the arg type match the format.
> + else
> + db_printf("( ??? )");
Perhaps just "()". "???" looks like an error. There shouldn't be any
spaces near the parentheses since that is not English style and the nonzero
case doesn't use them.
I think the parentheses should be braces. We're printing a (sub)struct and
C uses braces for structs. Also, the array could be in brackets. It
is now delimited by a ':' and a newline.
> if ((i + 1) < bp->b_npages)
Style bug (extra parentheses).
> db_printf(",");
> }
Other bugs in this function:
- no cast to void * for bp, b_bufobj, b_data, b_dep, b_kvabase
- b*flags is uint32_t is cast to u_int. The cast is only needed with 16-bit
ints, but BSD never supported them and POSIX has required >= 32 bit ints
since 2001.
- formatting in both the source and output for one line was broken by adding
b_dep. It and the previous line were limit to 4 fields to keep the output
line lengths below 80 on 32 bit arches. There are many %p formats and
possibly large integers, so this formatting probably never worked on 64-
bit arches. b_dep is a 5th field on a line. The source formatting is
obfuscated so that it is not obviously too long in the output.
- after printing 2 lines ending in b_dep using 1 db_printf(), we use another
db_printf() for the next line. I prefer 1 printf() in 1 source line per
output line.
- related fields are mostly grouped logically. The newer fields b_bufobj
and b_dep are not. b_dep on the 3rd line would be better. I might
express the arrays b_data and b_kvasize as %p[%d] with the size not named.
This gives more chance of 4 fields/line fitting.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160715053515.S2290>