From owner-freebsd-current@FreeBSD.ORG Fri Nov 25 18:25:27 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B4939106564A; Fri, 25 Nov 2011 18:25:27 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 243398FC15; Fri, 25 Nov 2011 18:25:27 +0000 (UTC) Received: from 63.imp.bsdimp.com (63.imp.bsdimp.com [10.0.0.63]) (authenticated bits=0) by harmony.bsdimp.com (8.14.4/8.14.3) with ESMTP id pAPIGFxU012506 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES128-SHA bits=128 verify=NO); Fri, 25 Nov 2011 11:16:16 -0700 (MST) (envelope-from imp@bsdimp.com) Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Warner Losh In-Reply-To: <20111125150324.G1018@besplex.bde.org> Date: Fri, 25 Nov 2011 11:16:15 -0700 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20111123070036.GA29952@thorin> <20111124141821.O932@besplex.bde.org> <20111125150324.G1018@besplex.bde.org> To: Bruce Evans X-Mailer: Apple Mail (2.1084) X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (harmony.bsdimp.com [10.0.0.6]); Fri, 25 Nov 2011 11:16:16 -0700 (MST) Cc: Kostik Belousov , freebsd-arch@freebsd.org, Adrian Chadd , freebsd-current@freebsd.org, Robert Millan Subject: Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Nov 2011 18:25:27 -0000 Hey Bruce, These sound like good suggestions, but I'd hoped to actually go through = all these files with a fine-toothed comb to see which ones were still = relevant. You've found a bunch of good areas to clean up, but I'd like = to humbly suggest they be done in a follow-on commit. Warner On Nov 24, 2011, at 10:54 PM, Bruce Evans wrote: > On Thu, 24 Nov 2011, Robert Millan wrote: >=20 >> 2011/11/24 Bruce Evans : >>> Now it adds lots of namespace pollution (all of , = including >>> all of its namespace pollution), just to get 1 new symbol defined. >>=20 >> Well, my initial patch (see mail with same subject modulo "v2") = didn't >> have this problem. Now that __FreeBSD_kernel__ is defined, many >> #ifdefs can be simplified, but maybe it's not desireable for all of >> them. At least not until we can rely on the compiler to define this >> macro. >>=20 >> So in this particular case maybe it's better to use the other = approach? >>=20 >> See attachment. >=20 > That is clean enough, except for some style bugs. (I thought of worse > ways like duplicating the logic of , or directing > to only declare version macros, or putting version = macros > in a little separate param header and including that. The latter = would > be cleanest, but gives even more includes, and not worth it for this, > but it would have been better for __FreeBSD_version. I don't like > having to recompile half the universe according to dependencies on > because only __FreeBSD_version__ in it changed. Basic > headers rarely change apart from that. BTW, a recent discussion in > the POSIX mailing list says that standardized generation of = depenedencies > should not generate dependencies on system headers. This would break > the effect of putting mistakes like __FreeBSD_version__ in any system > header :-).) >=20 > % diff -ur sys.old/cam/scsi/scsi_low.h sys/cam/scsi/scsi_low.h > % --- sys.old/cam/scsi/scsi_low.h 2007-12-25 18:52:02.000000000 = +0100 > % +++ sys/cam/scsi/scsi_low.h 2011-11-13 14:12:41.121908380 +0100 > % @@ -53,7 +53,7 @@ > % #define SCSI_LOW_INTERFACE_XS > % #endif /* __NetBSD__ */ > % % -#ifdef __FreeBSD__ > % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) > % #define SCSI_LOW_INTERFACE_CAM > % #define CAM > % #endif /* __FreeBSD__ */ >=20 > This also fixes some style bugs (tab instead of space after `#ifdef'). > But it doesn't fix others (tab instead of space after `#ifdef', and > comment on a short ifdef). And it introduces a new one (the comment > on the ifdef now doesn't even match the code). >=20 > cam has a highly non-KNF style, so it may require all of these style > bugs except the comment not matching the code. This makes it hard > for non-cam programmers to maintain. According to grep, it prefers > a tab to a space after `#ifdef' by a ratio of 89:38 in a version > checked out a year or two ago. But in 9.0-BETA1, the counts have > blown out and the ratio has reduced to 254:221. The counts are > more than doubled because the first version is a cvs checkout and > the second version is a svn checkout, and it is too hard to filter > out the svn duplicates. I guess the ratio changed because the new > ata subsystem is not bug for bug compatible with cam style. Anywyay, > there never was a consistent cam style to match. >=20 > % @@ -64,7 +64,7 @@ > % #include > % #endif /* __NetBSD__ */ > % % -#ifdef __FreeBSD__ > % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) > % #include > % #include > % #include >=20 > Same problems, but now the ifdef is larger (but not large enough to > need a comment on its endif), so the inconsistent comment is not > visible in the patch. >=20 > % [... similarly throught cam] >=20 > % diff -ur sys.old/contrib/altq/altq/if_altq.h = sys/contrib/altq/altq/if_altq.h > % --- sys.old/contrib/altq/altq/if_altq.h 2011-03-10 = 19:49:15.000000000 +0100 > % +++ sys/contrib/altq/altq/if_altq.h 2011-11-13 14:12:41.119907128 = +0100 > % @@ -29,7 +29,7 @@ > % #ifndef _ALTQ_IF_ALTQ_H_ > % #define _ALTQ_IF_ALTQ_H_ > % % -#ifdef __FreeBSD__ > % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) > % #include /* XXX */ > % #include /* XXX */ > % #include /* XXX */ > % @@ -51,7 +51,7 @@ > % int ifq_len; > % int ifq_maxlen; > % int ifq_drops; > % -#ifdef __FreeBSD__ > % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) > % struct mtx ifq_mtx; > % #endif > % >=20 > No new problems, but I wonder how this even compiles when the ifdefs > are not satisfed. Here we are exporting mounds of kernel data = structures > to userland. There is a similar mess in . There it has > no ifdefs at all for the lock, mutex and event headers there, and you > didn't touch it. is unfortunately actually needed in > userland. The mutexes in its data structures cannot simply be left > out, since then the data structures become incompatible with the = actual > ones. I don't see how the above can work with the mutex left out. >=20 > By "not even compiles", I meant the header itself, but there should be > no problems there because the second ifdef should kill the only use of > all the headers. And userland should compile since it shouldn't use > the ifdefed out (kernel) parts of the data struct. But leaving out > the data substructures changes the ABI, so how could any application = that > actually uses the full structure work? And if nothing uses it, it > shouldn't be exported. >=20 > Exporting the full pollution of sys/lock.h, sys/mutex.h and = sys/event.h > to userland is probably an implementation bug. This is partially = fixed > in my version if by including these headers only for > the _KERNEL case. sys/_lock.h and sys/_mutex.h are enough for = declaring > the mutex, and sys/event*.h isn't even used in the userland parts of > . But this probably wouldn't help you much, since the > underscored mutex headers are just as unavailable as the = non-underscored > ones on non-FreeBSD systems. >=20 > I checked that this file doesn't have any comments on the changed = ifdefs > to get out of sync (it just has a misformatted one for the endif for > _KERNEL and a backwards one for the one for the idempotency endif). > I didn't check this for other files. >=20 > % diff -ur sys.old/contrib/pf/net/if_pflog.h = sys/contrib/pf/net/if_pflog.h > % --- sys.old/contrib/pf/net/if_pflog.h 2011-06-28 = 13:57:25.000000000 +0200 > % +++ sys/contrib/pf/net/if_pflog.h 2011-11-13 14:12:41.130906469 = +0100 > % @@ -30,7 +30,7 @@ > % #define PFLOGIFS_MAX 16 > % % struct pflog_softc { > % -#ifdef __FreeBSD__ > % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) > % struct ifnet *sc_ifp; /* the interface pointer = */ > % #else > % struct ifnet sc_if; /* the interface */ >=20 > Another very fundamental ABI difference in a clearer form. It this = only > used in the kernel, and if so, why is it exported to userland? = Wouldn't > a _KERNEL ifdef work better for avoiding the latter than a > __FreeBSD_kernel__ ifdef? (It would be with && instead of ||.) >=20 > [... similarly for all the pf headers] >=20 > % diff -ur sys.old/dev/firewire/firewirereg.h = sys/dev/firewire/firewirereg.h > % --- sys.old/dev/firewire/firewirereg.h 2007-07-20 = 05:42:57.000000000 +0200 > % +++ sys/dev/firewire/firewirereg.h 2011-11-13 14:12:41.122907609 = +0100 > % @@ -75,7 +75,7 @@ > % }; > % % struct firewire_softc { > % -#if defined(__FreeBSD__) && __FreeBSD_version >=3D 500000 > % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = __FreeBSD_version >=3D 500000 > % struct cdev *dev; > % #endif > % struct firewire_comm *fc; >=20 > The line is now too long. >=20 > This change seems to be wrong. I think the __FreeBSD__ ifdef is only > there because of broken compilers. The correct code is simply `#if > __FreeBSD_version >=3D 500000', where with non-broken compilers > __FreeBSD_version is replaced by 0 if it is not defined. But broken > compilers warn when undefined identifiers are used in arithmetic cpp > expressions. Some users like to break their compilers using -Wundef > to give the warnings; -Werror then gives full breakage (C compilers > are permitted to give spurious diagnostics but not to fail to compile > C code.) This is normally worked around by checking if the identifier > is defined before it is used in an arithmetic expression. But here a > different identifier is checked for being defined. FreeBSD gcc = defines > both __FreeBSD__ and __FreeBSD_version__ together, so this works under > FreeBSD. I think it only works accidentally under FreeBSD, and it > still doesn't work with your changed ifdef under non-FreeBSD. >=20 > grepping for `FreeBSD.*FreeBSD_version' in $(find /sys -name *.h) = shows > very few lines, and even fewer lines with this bug. Most matching > lines do the unsurprising check that __FreeBSD_version__ is defined > before using it. The exceptions are just the above, 2 lines in > if_lmc.h and about 5 lines in ipfilter (some on comments on endifs > for ipfilter. Even ipfilter does the unsurprising comparison in > 4 lines). >=20 > grepping for `FreeBSD_version' in $(find /sys -name *.h) shows many = more > lines than the above. It follows that many headers cannot be compiled > by broken compilers, and little would be lost, at least in the kernel, > by removing all the checks that __FreeBSD_version__ is defined before > using it. But since this inconsistency intersects with your changes, > perhaps the checks are there mainly for cases that escape to userland, > where the broken compilers are more common. >=20 > % diff -ur sys.old/dev/lmc/if_lmc.h sys/dev/lmc/if_lmc.h > % --- sys.old/dev/lmc/if_lmc.h 2009-11-19 19:21:51.000000000 = +0100 > % +++ sys/dev/lmc/if_lmc.h 2011-11-13 14:12:41.124908302 +0100 > % @@ -984,7 +984,7 @@ > % #endif > % u_int32_t address1; /* buffer1 bus address */ > % u_int32_t address2; /* buffer2 bus address */ > % -#if (defined(__FreeBSD__) || defined(__NetBSD__) || = defined(__OpenBSD__)) > % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || = defined(__NetBSD__) || defined(__OpenBSD__)) > % bus_dmamap_t map; /* bus dmamap for this = descriptor */ > % # define TLP_BUS_DSL_VAL (sizeof(bus_dmamap_t) & TLP_BUS_DSL) > % #else >=20 > The line is now too long. ABI differences when __FreeBSD__ etc. is = not > defined indicated that the declaration probably doesn't actually work > without the definitions, so the whole struct probably shouldn't be > exported. >=20 > % @@ -1035,7 +1035,7 @@ > % #elif BSD > % struct mbuf *head; /* tail-queue of mbufs */ > % struct mbuf *tail; > % -# if (defined(__FreeBSD__) || defined(__NetBSD__) || = defined(__OpenBSD__)) > % +# if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || = defined(__NetBSD__) || defined(__OpenBSD__)) > % bus_dma_tag_t tag; /* bus_dma tag for desc array */ > % bus_dmamap_t map; /* bus_dma map for desc array */ > % bus_dma_segment_t segs[2]; /* bus_dmamap_load() or = bus_dmamem_alloc() */ >=20 > As above. >=20 > % @@ -1068,7 +1068,7 @@ > % */ > % #define IOREF_CSR 1 /* access Tulip CSRs with IO cycles if 1 */ > % % -#if (defined(__FreeBSD__) && defined(DEVICE_POLLING)) > % +#if ((defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = defined(DEVICE_POLLING)) > % # define DEV_POLL 1 > % #else > % # define DEV_POLL 0 >=20 > As above, plus DEVICE_POLLING is only defined in a kernel options = header, > so there shouldn't be ifdefs on it in header files, even in the = kernel. >=20 > % @@ -1151,7 +1151,7 @@ > % # endif > % #endif > % % -#ifdef __FreeBSD__ > % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) > % struct callout callout; /* watchdog needs this = */ > % struct device *dev; /* base device pointer = */ > % bus_space_tag_t csr_tag; /* bus_space needs this = */ >=20 > But I may misunderstand how __FreeBSD_kernel__ works. Data structures > containing kernel things like the kernel watchdog callout shouldn't be > exported. Given that they are, they should be exported with the full > kernel mess so that their layout for the non-kernel parts doesn't > depend on ifdefs. The __FreeBSD__ ifdef was bogus, but had no effect > under FreeBSD since FreeBSD gcc always defines it. A _KERNEL ifdef > would have given a wrong ABI in userland. Your __FreeBSD_kernel__ > ifdef seems to be equivalent to a _KERNEL ifdef for non-FreeBSD. Thus > it gives a changed ABI for non-FreeBSD. I don't see how the changed > ABI can work unless it is not used. But if it is not used, then the > whold ABI should be ifdefed out. This may be beyond the scope of > your changes. >=20 > % @@ -1428,7 +1428,7 @@ > % #endif > % % #if (defined(__bsdi__) || /* unconditionally */ \ > % - (defined(__FreeBSD__) && (__FreeBSD_version < 503000)) || \ > % + ((defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = (__FreeBSD_version < 503000)) || \ > % (defined(__NetBSD__) && (__NetBSD_Version__ < 106000000)) || \ > % (defined(__OpenBSD__) && ( OpenBSD < 200111))) > % # define IFQ_ENQUEUE(ifq, m, pa, err) \ >=20 > Another long line. The /* unconditionally */ comment is now even more > grossly mismatched with the code. >=20 > % @@ -1531,7 +1531,7 @@ > % static int t1_ioctl(softc_t *, struct ioctl *); > % % #if IFNET > % -# if ((defined(__FreeBSD__) && (__FreeBSD_version < 500000)) ||\ > % +# if (((defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = __FreeBSD_version < 500000) ||\ > % defined(__NetBSD__) || defined(__OpenBSD__) || = defined(__bsdi__)) > % static void netisr_dispatch(int, struct mbuf *); > % # endif >=20 > Another long line. There are many old style bugs in these ifdefs, = starting > with __bsdi__ being first in the previous ifdef and last in this one. >=20 > % @@ -1570,7 +1570,7 @@ > % static void core_interrupt(void *, int); > % static void user_interrupt(softc_t *, int); > % #if BSD > % -# if (defined(__FreeBSD__) && defined(DEVICE_POLLING)) > % +# if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = defined(DEVICE_POLLING) > % static int fbsd_poll(struct ifnet *, enum poll_cmd, int); > % # endif > % static intr_return_t bsd_interrupt(void *); >=20 > Another long line. Now the declaration is of a kernel function, so = the > condition for if is clearly nonsense. Why not just `#ifdef = DEVICE_POLLING' > or no ifdef at all. The function is static in a .c file, so declaring = it > as static in a public header is worse than its ifdefs. Better yet, = the > ifdef for it here didn't match the ifdef for it in the .c file, and is > now further from matching (the C file has an ifdef on BSD, __FreeBSD__ > and DEVICE_POLLING, while the header file had an ifdef on _KERNEL, > KERNEL, __KERNEL__, __FreeBSD__ and DEVICE_POLLING). Fixing mistakes = in > DEVICE_POLLING is clearly beyond the scope of your changes. >=20 > % @@ -1638,7 +1638,7 @@ > % static int attach_card(softc_t *, const char *); > % static void detach_card(softc_t *); > % % -#ifdef __FreeBSD__ > % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) > % static int fbsd_probe(device_t); > % static int fbsd_detach(device_t); > % static int fbsd_shutdown(device_t); >=20 > This file is especially bad. >=20 > % diff -ur sys.old/netinet/sctp_constants.h = sys/netinet/sctp_constants.h > % --- sys.old/netinet/sctp_constants.h 2011-09-17 = 10:50:29.000000000 +0200 > % +++ sys/netinet/sctp_constants.h 2011-11-13 14:12:41.145908872 = +0100 > % @@ -1020,7 +1020,7 @@ > % #define SCTP_GETTIME_TIMEVAL(x) (getmicrouptime(x)) > % #define SCTP_GETPTIME_TIMEVAL(x) (microuptime(x)) > % #endif > % -/*#if defined(__FreeBSD__) || defined(__APPLE__)*/ > % +/*#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || = defined(__APPLE__)*/ > % /*#define SCTP_GETTIME_TIMEVAL(x) { \*/ > % /* (x)->tv_sec =3D ticks / 1000; \*/ > % /* (x)->tv_usec =3D (ticks % 1000) * 1000; \*/ >=20 > Line too long. >=20 > % diff -ur sys.old/netinet/sctp_pcb.h sys/netinet/sctp_pcb.h > % --- sys.old/netinet/sctp_pcb.h 2011-09-14 10:15:21.000000000 = +0200 > % +++ sys/netinet/sctp_pcb.h 2011-11-13 14:12:41.148909632 +0100 > % @@ -240,7 +240,7 @@ > % * All static structures that anchor the system must be here. > % */ > % struct sctp_epinfo sctppcbinfo; > % -#if defined(__FreeBSD__) && defined(SMP) && = defined(SCTP_USE_PERCPU_STAT) > % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = defined(SMP) && defined(SCTP_USE_PERCPU_STAT) > % struct sctpstat *sctpstat; > % #else > % struct sctpstat sctpstat; > % @@ -632,7 +632,7 @@ > % struct sctp_inpcb *, > % uint8_t co_off); > % % -#if defined(__FreeBSD__) && defined(SCTP_MCORE_INPUT) && = defined(SMP) > % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = defined(SCTP_MCORE_INPUT) && defined(SMP) > % void > % sctp_queue_to_mcore(struct mbuf *m, int off, int cpu_to_use); > % >=20 > Lines too long. How would the kernel options be defined for = non-FreeBSD? > I.e., why have any __FreeBSD* ifdefs at all here? >=20 >=20 > % diff -ur sys.old/netinet/sctp_structs.h sys/netinet/sctp_structs.h > % --- sys.old/netinet/sctp_structs.h 2011-10-10 18:31:18.000000000 = +0200 > % +++ sys/netinet/sctp_structs.h 2011-11-13 14:12:41.150907531 = +0100 > % @@ -108,7 +108,7 @@ > % typedef int (*inp_func) (struct sctp_inpcb *, void *ptr, uint32_t = val); > % typedef void (*end_func) (void *ptr, uint32_t val); > % % -#if defined(__FreeBSD__) && defined(SCTP_MCORE_INPUT) && = defined(SMP) > % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = defined(SCTP_MCORE_INPUT) && defined(SMP) > % /* whats on the mcore control struct */ > % struct sctp_mcore_queue { > % TAILQ_ENTRY(sctp_mcore_queue) next; >=20 > Line too long. >=20 > % diff -ur sys.old/netinet/sctp_uio.h sys/netinet/sctp_uio.h > % --- sys.old/netinet/sctp_uio.h 2011-08-14 22:55:32.000000000 = +0200 > % +++ sys/netinet/sctp_uio.h 2011-11-13 14:12:41.152905989 +0100 > % @@ -1056,7 +1056,7 @@ > % % #define SCTP_STAT_INCR(_x) SCTP_STAT_INCR_BY(_x,1) > % #define SCTP_STAT_DECR(_x) SCTP_STAT_DECR_BY(_x,1) > % -#if defined(__FreeBSD__) && defined(SMP) && = defined(SCTP_USE_PERCPU_STAT) > % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && = defined(SMP) && defined(SCTP_USE_PERCPU_STAT) > % #define SCTP_STAT_INCR_BY(_x,_d) = (SCTP_BASE_STATS[PCPU_GET(cpuid)]._x +=3D _d) > % #define SCTP_STAT_DECR_BY(_x,_d) = (SCTP_BASE_STATS[PCPU_GET(cpuid)]._x -=3D _d) > % #else >=20 > As usual. >=20 > Bruce > _______________________________________________ > freebsd-arch@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to = "freebsd-arch-unsubscribe@freebsd.org" >=20 >=20