From owner-svn-src-all@FreeBSD.ORG Sun Oct 27 16:29:14 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 0764DBA5; Sun, 27 Oct 2013 16:29:14 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id A5B752F8D; Sun, 27 Oct 2013 16:29:13 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id C694BD42279; Mon, 28 Oct 2013 03:07:47 +1100 (EST) Date: Mon, 28 Oct 2013 03:07:45 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Lepore Subject: Re: svn commit: r257199 - in head/sys/arm: allwinner arm broadcom/bcm2835 freescale/imx include lpc mv mv/orion rockchip sa11x0 samsung/exynos tegra ti ti/omap4/pandaboard versatile xilinx xscale/i8032... In-Reply-To: <201310270051.r9R0pl3j024025@svn.freebsd.org> Message-ID: <20131028015840.V929@besplex.bde.org> References: <201310270051.r9R0pl3j024025@svn.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=YYGEuWhf c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=0gCR9s_oylQA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=XT1p1CXzdcAA:10 a=_QVvMFUpw_FK3yl96rUA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 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: Sun, 27 Oct 2013 16:29:14 -0000 On Sun, 27 Oct 2013, Ian Lepore wrote: > Log: > Remove all #include from arm code. It's already > included by vm/pmap.h, which is a prerequisite for arm/machine/pmap.h > so there's no reason to ever include it directly. > > Thanks to alc@ for pointing this out. I noticed this style bug in the recent commit, but didn't complain before. Including machine/pmap.h directly is unwarranted chumminess with the implementation. When vm/pmap.h is also included, it less than that. The chumminess is merely unwarranted in just a few cases. vm/map.h is not includable in some asm files. machine/pmap.h is includable in asm files, but shouldn't be on some arches. genassym should be used. The hack of including machine/pmap.h is currently used in 5 asm files (essentially the same 2: 2 on i386 cloned to amd64 and 1 of these cloned to xen/i386). All file counts are for /sys. In 2004, there were: - 2 .c files that include only machine/pmap.h (chummy) - 20 .c files that include vm/pmap.h and machine/pmap.h (less than that) - 231 .c files that include vm/pmap.h only (correct) The style bugs have expanded exponentially since then of course :-(. In -current before this comment, there are: - 9 .c files that include only machine/pmap.h (chummy) - 80 .c files that include vm/pmap.h and machine/pmap.h (less than that) - 421 .c files that include vm/pmap.h only (correct) The files with mere unwarranted chumminess are 2 in sparc64 (in 2004 and now), 6 in mips (now) and 1 in powerpc (now). MD files can reasonably use only the level, but there is no reason to avoid the vm level for just pmap. The 9 files that avoid vm for pmap use vm includes for most other things. These file counts by grep don't show the complexities from nested pollution. machine/pmap.h is now included nested in 7 headers. This is pollution except in vm/vmap.h. vm/pmap.h is now included in 32 headers. This is gross pollution in vm/vm_page.h. It is perhaps needed in sys/user.h, where it is marked as a mistake by XXX. The others are mostly for drivers doing bad things like using vtophys(). Similarly for many other nested includes, except it is usually not so clear that the machine includes should only be included directly in 1 place. > Modified: head/sys/arm/allwinner/a10_machdep.c > ============================================================================== > --- head/sys/arm/allwinner/a10_machdep.c Sat Oct 26 23:41:11 2013 (r257198) > +++ head/sys/arm/allwinner/a10_machdep.c Sun Oct 27 00:51:46 2013 (r257199) > @@ -45,7 +45,6 @@ __FBSDID("$FreeBSD$"); > #include > #include /* For trapframe_t, used in */ > #include > -#include > > #include Some other style bugs are visible in the patch. Saying what an include is for is a bogus "come from" comment. Such comments are usually rotten, and the above is no exception. trapframe_t is a style bug (an "opaque" type that must be visible to use it) that still exists (only arm has it, at least now). But arm/machine/machdep.h doesn't use it now. I don't know if machine/machdep.h needed to use it. But the optimization of passing full trap frames without copying them has been lost because it depended on undefined behaviour in C. Everything now passes pointers to trap frames instead. arm/machine/machdep.h spells the pointers correctly as "struct trapframe *" after forward-declaring an incomplete "struct trapframe", so it doesn't actually use trapframe_t. Perhaps the include is now included for some unclaimed reason. arm spells "struct trapframe" correctly in 45 places. It uses trapframe_t in 29 places. 6 of these are for the same false "come from" comment. machine/frame.h is still included nested in too many headers. arm is not worse than i386 here. > Modified: head/sys/arm/arm/minidump_machdep.c > ============================================================================== > --- head/sys/arm/arm/minidump_machdep.c Sat Oct 26 23:41:11 2013 (r257198) > +++ head/sys/arm/arm/minidump_machdep.c Sun Oct 27 00:51:46 2013 (r257199) > @@ -43,7 +43,6 @@ __FBSDID("$FreeBSD$"); > #endif > #include > #include > -#include > #include > #include > #include Missing blank line before the includes. There order is improved. > Modified: head/sys/arm/arm/nexus.c > ============================================================================== > --- head/sys/arm/arm/nexus.c Sat Oct 26 23:41:11 2013 (r257198) > +++ head/sys/arm/arm/nexus.c Sun Oct 27 00:51:46 2013 (r257199) > @@ -56,7 +56,6 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > -#include > > #include > #include This also fixes the misplaced blank line. The order of the 2 includes visible is still backwards. > --- head/sys/arm/freescale/imx/imx_machdep.c Sat Oct 26 23:41:11 2013 (r257198) > +++ head/sys/arm/freescale/imx/imx_machdep.c Sun Oct 27 00:51:46 2013 (r257199) > @@ -41,7 +41,6 @@ __FBSDID("$FreeBSD$"); > #include > #include /* For trapframe_t, used in */ > #include > -#include > > #include > #include The above count of 6 of these false "come from" comments is for a ~Sep 10 version of -current. There are now 10 of them visible in this patch alone. Bruce