Date: Mon, 19 Mar 2012 21:53:19 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Robert Millan <rmh@FreeBSD.org> Cc: Adrian Chadd <adrian@FreeBSD.org>, freebsd-arch@FreeBSD.org Subject: Re: [PATCH] Add compatibility <sys/io.h> Message-ID: <20120319204531.B1232@besplex.bde.org> In-Reply-To: <CAOfDtXMmR-On5k7rLKNqXhGTPLkc4XOao672HcS-9vAbVJxiSQ@mail.gmail.com> References: <CAOfDtXPGPP0reN9NTBw_5%2BNwXZ56Yy0oyx_fH%2BDOvmpc1O%2BQdQ@mail.gmail.com> <CAJ-Vmonu_ApSd192cjvsW6k3eNNK4Kz=MmAMe_e=zmwbrS8Ayw@mail.gmail.com> <CAOfDtXOCyjga5QHz98Re3jXkefNzB-MbULcAVUQH89ToVLkw9g@mail.gmail.com> <20120312121852.P1122@besplex.bde.org> <CAOfDtXMmR-On5k7rLKNqXhGTPLkc4XOao672HcS-9vAbVJxiSQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-147523586-1332154399=:1232 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Mon, 19 Mar 2012, Robert Millan wrote: > El 12 de mar=C3=A7 de 2012 3:18, Bruce Evans <brde@optusnet.com.au> ha es= crit: >> I would prefer to make it fail to build if it gets the arg order wrong, >> but I don't see how to do that, since both args are integers. > > I don't think you can. E.g. consider: > > outb (0x60, 0x61); > > then only semantical analysis could tell. > >>> Looks good. =C2=A0So you suggest we tell userspace users to switch to >>> bus_space_write_*? >> >> >> The problem with that is that if you don't do the switch yourself then >> most users won't even know that it is necessary. =C2=A0You have to remov= e >> the functionaility in cpufunc.h and/or add messy userland ifdefs as >> well as messy kernel ifdefs to unremove it, so that users who don't >> know what they are doing and which you haven't adjusted get warned >> by build failures. =C2=A0All users that knew what they were doing have t= o >> do it differently. > > Okay, I see your point. Maybe we can try a conservative approach and > just issue warnings. How does this look for a start? % Index: sys/i386/include/cpufunc.h % =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D % --- sys/i386/include/cpufunc.h=09(revision 233095) % +++ sys/i386/include/cpufunc.h=09(working copy) % @@ -42,6 +42,10 @@ % #error this file needs sys/cdefs.h as a prerequisite % #endif %=20 % +#ifndef _KERNEL % +#warning "No user-serviceable parts inside. For user-space I/O, use the = bus_space(9) family of functions." % +#endif % + Too many style bugs for me :-) : - line too long - multiple sentences. These are unusual in error messages, and help give the previous bug - terminated message. Error messages are not terminated with a ".", except possibly if they have multiple sentences, each terminated with a ".". I don't know the exact rule for more normal use. This sentence is terminated, unlike the last one for the previous point. - Capitalized sentences. Again it is unclear what should be capitalized for multiple sentences. The one that started this point is capitalized, unlike all the others here except for the one at the end of the list. - sentence break of 1 space instead of 2. All of these style bugs are avoided in all user-anti-serviceable messages in sys/*.h. All 8 there say '#error "no user-serviceable parts inside"', except 1 says "no assembler-serviceable parts inside", and 5 misspell "serviceable" as "servicable". This phrasing is of course a joke. I know prefer the more correct "no user-serving parts inside". #warning must be used instead of #error here since we don't want to break this use completely, but there is a technical problem with this. #warning is only a gcc extension. This is handled in gratuitously different horrible ways in all places that #warning is used in sys/*.h: % dir.h: % #ifdef __CC_SUPPORTS_WARNING % #warning "The information in this file should be obtained from <dirent.h>= " % #warning "and is provided solely (and temporarily) for backward compatibi= lity." % #endif The condition for #warning being available is given by the rather over-engineered feature test macro __CC_SUPPORTS_WARNING. To make the above correct, this file grew an otherwise-unnecessary include of <sys/cdefs.h> just before the above. The warning message is rather formal and verbose. It is formatted nicely enough using multiple #warning statements to avoid the long lines. gcc prints the multiple #warning statements even with -Werror. It even prints multiple #error statements and doesn't abort on the first one. % syslimits.h: % #if !defined(_KERNEL) && !defined(_LIMITS_H_) && !defined(_SYS_PARAM_H_) % #ifndef _SYS_CDEFS_H_ % #error this file needs sys/cdefs.h as a prerequisite % #endif % #ifdef __CC_SUPPORTS_WARNING % #warning "No user-serviceable parts inside." % #endif % #endif Now the ifdefs are even more horrible. The #error for not including sys/cdefs.h is bogus. We are trying to trap the error of including this file directly. It must only be included by <limits.h> or <sys/param.h>. This has nothing to do with sys/cdefs.h, but we print a #error about the latter. The primary warning about the former is the usual cryptic "!user-serviceable" one, except it now has style bugs. This ifdef tangle is apparently the result of: - using #warning instead of #error for no reason - adding the __CC_SUPPORTS_WARNING ifdef to hide this bug. This ifdef was originally spelled __GNUC__ - when converting from the __GNUC__ spelling to __CC_SUPPORTS_WARNING, sys/cdefs.h became a prerequisite, but instead of just including it, an ifdef for its presence was added and the #error message for that - the _KERNEL ifdef is also bogus. Untangling this and fixing the message gives: % #if !defined(_LIMITS_H_) && !defined(_SYS_PARAM_H_) % #error "no user-serviceable parts inside" % #endif or the message could be decrytped to #error "this file must only be included directly by <limits.h> or <sys/para= m.h>" but I don't want to spell out the implementation details like this in all #error messages. % timeb.h: % #ifdef __GNUC__ % #warning "this file includes <sys/timeb.h> which is deprecated" % #endif The old spelling of __CC_SUPPORTS_WARNING here is an anachronism. The other ifdefs on #warning date from 1997 and 2002, but were changed to the new spelling in 2005. This one dates from 2010 but hasn't caught up with the 2005 change. I fixed the spelling errors long ago in my version, so I didn't notice them at first when I grepped for them while writing this. After adding a few more of these messages, I now have the following in sys/*.h: %%% _task.h:#error "no user-servicable parts inside" eventhandler.h:#error "no user-serviceable parts inside" libkern.h:#error "no user-serviceable parts inside" mutex.h:#error "no assembler-serviceable parts inside" pcpu.h:#error "no user-serviceable parts inside" pcpu.h:#error "no assembler-serviceable parts inside" smp.h:#error "no user-serviceable parts inside" smp.h:#error "no assembler-serviceable parts inside" syslimits.h:#error "no user-serviceable parts inside" taskqueue.h:#error "no user-serviceable parts inside" timetc.h:#error "no user-serviceable parts inside" %%% Kernel headers really shouldn't need these, but adding just one takes a lot of work since it tends to expose kernel headers leaking to userland with full kernel pollution like kernel mutexes and locks. The "no assembler-serviceable parts inside" are even less needed. I added them mainly in places where there were bogus LOCORE ifdefs, to detect any cases where asm files actually still include C headers (C except for a tiny part ifdefed by LOCORE). genassym mostly handles this better although it takes a few more seconds to hack on. Bruce --0-147523586-1332154399=:1232--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120319204531.B1232>