From owner-svn-src-head@freebsd.org Thu Jul 14 20:41:10 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D95C1B99E8E; Thu, 14 Jul 2016 20:41:10 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 6EB461158; Thu, 14 Jul 2016 20:41:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 894701049B83; Fri, 15 Jul 2016 06:41:01 +1000 (AEST) Date: Fri, 15 Jul 2016 06:41:00 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mark Johnston cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r302854 - head/sys/kern In-Reply-To: <201607141849.u6EIn53i038324@repo.freebsd.org> Message-ID: <20160715053515.S2290@besplex.bde.org> References: <201607141849.u6EIn53i038324@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=M8SwUHEs c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=elF9NkvWo_qpgCsjwWgA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Jul 2016 20:41:11 -0000 > 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