From owner-freebsd-arch@FreeBSD.ORG Tue Sep 9 21:03:35 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 170A0551 for ; Tue, 9 Sep 2014 21:03:35 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id ADD698DF for ; Tue, 9 Sep 2014 21:03:34 +0000 (UTC) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id s89L3TdM056813 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 10 Sep 2014 00:03:29 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua s89L3TdM056813 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s89L3T4C056812; Wed, 10 Sep 2014 00:03:29 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 10 Sep 2014 00:03:29 +0300 From: Konstantin Belousov To: yaneurabeya@gmail.com Subject: Re: [RFC] Add __arraycount from NetBSD to sys/cdefs.h Message-ID: <20140909210329.GE2737@kib.kiev.ua> References: <8D279BDC-7D40-4750-8DA7-A4535DD2E458@bsdimp.com> <146C3E96-7461-4D30-8B7F-5E20F72CF061@gmail.com> <20140909083333.GA2737@kib.kiev.ua> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="WEBrq8xPpGVQldHe" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: "freebsd-arch@FreeBSD.org Arch" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Sep 2014 21:03:35 -0000 --WEBrq8xPpGVQldHe Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 09, 2014 at 03:13:45AM -0700, yaneurabeya@gmail.com wrote: > Hi kib@! >=20 > On Sep 9, 2014, at 1:33, Konstantin Belousov wrote: >=20 > > On Mon, Sep 08, 2014 at 09:29:54AM -0700, yaneurabeya@gmail.com wrote: > >> On Sep 3, 2014, at 21:17, Warner Losh wrote: > >>=20 > >>> On Sep 3, 2014, at 9:45 PM, Garrett Cooper wr= ote: > >>>=20 > >>>> Hi all, > >>>> In order to ease porting code and reduce divergence with NetBSD > >>>> when importing code (a large chunk of which for me are tests), I wou= ld > >>>> like to move nitems to sys/cdefs.h and alias __arraycount to nitems. > >>>> Here's the __arraycount #define in lib/libnetbsd/sys/cdefs.h: > >>>>=20 > >>>> 44 /* > >>>> 45 * Return the number of elements in a statically-allocated array, > >>>> 46 * __x. > >>>> 47 */ > >>>> 48 #define __arraycount(__x) (sizeof(__x) / sizeof(__x[0])) > >>>>=20 > >>>> Here's the nitems #define in sys/sys/param.h: > >>>>=20 > >>>> 277 #define nitems(x) (sizeof((x)) / sizeof((x)[0])) > >>>>=20 > >>>> sys/cdefs.h gets pulled in automatically with sys/param.h, so > >>>> anything using nitems will continue to function like before (see bel= ow > >>>> for more details). I've attached a patch which addresses all hardcod= ed > >>>> definitions in the tree added by FreeBSD developers. > >>>> If there aren't any major concerns with my proposed change, I'll > >>>> put it up for review on Phabricator. > >>>> Thank you! > >>>> -Garrett > >>>>=20 > >>>> $ cat cdefs_pound_define.c > >>>> #include > >>>>=20 > >>>> #ifdef _SYS_CDEFS_H_ > >>>> #warning "sys/cdefs.h has been included" > >>>> #endif > >>>> $ cc -c cdefs_pound_define.c > >>>> cdefs_pound_define.c:4:2: warning: "sys/cdefs.h has been included" [= -W#warnings] > >>>> #warning "sys/cdefs.h has been included" > >>>> ^ > >>>> 1 warning generated. > >>>> $ cc -D_KERNEL -c cdefs_pound_define.c > >>>> cdefs_pound_define.c:4:2: warning: "sys/cdefs.h has been included" [= -W#warnings] > >>>> #warning "sys/cdefs.h has been included" > >>>> ^ > >>>> 1 warning generated. > >>>> $ gcc -c cdefs_pound_define.c > >>>> cdefs_pound_define.c:4:2: warning: #warning "sys/cdefs.h has been in= cluded" > >>>> $ gcc -D_KERNEL -c cdefs_pound_define.c > >>>> cdefs_pound_define.c:4:2: warning: #warning "sys/cdefs.h has been in= cluded? > >>>=20 > >>> I wouldn?t bother changing the nitems #define. There?s no need, reall= y, to do that. > >>=20 > >> Rethinking my proposal, I agree. I had lofty hopes for unifying the ma= cros, but the functional duplication (1 line) is harmless. > >>=20 > >>> I?d also be more inclined to believe the test if you tested what the = thing does rather than test for an artificial, implementation defined side = effect. > >>=20 > >> Sure. I provided a lazy proof instead of a full proof :). > >>=20 > >>> But honestly the amount of duplication saved here is rather tiny? > >>=20 > >> Indeed! Thank you for the input :) ? I?ve attached a new patch which d= oesn?t disturb nitems or sys/param.h. > >=20 > > Since this is seriously discussed on arch@, I think it is worth look > > at the following: > >=20 > > git grep -e 'sizeof.*/sizeof' -- | grep -v '^contrib' | grep '#.*define= ' | wc -l > > 118 > > These are only defines, not uses, not counting direct use of > > sizeof(x)/sizeof(x[0]) in the code. >=20 > It?s a bit less painful if you omit cddl, crypto, and gnu: >=20 > $ git grep -e 'sizeof.*/sizeof' -- | egrep -v '^(cddl|contrib|crypto|gnu)= ' | grep '#.*define' | wc -l > 105 >=20 > > Not all of them are for equiv macros, but significant part repeats ntim= es() > > in variations. At least Linuxish ARRAY_SIZE/DRM_ARRAY_SIZE is much more > > popular than __arraysize() which appears three (!) times. >=20 > Hmm.. that?s a bit surprising. >=20 > > I do not see any reason to pollute central header cdefs.h with sparcely > > used macro for some compat code. >=20 > This is one compelling reason ? it?s used 225 times in the test code in N= etBSD, spanning 92 files, and 15 times spanning 10 files in the rump kernel. >=20 > $ grep -rl __arraycount netbsd/src/tests/ | wc -l; grep -r __arraycount n= etbsd/src/tests/ | wc -l > 92 > 225 > $ grep -rl __arraycount netbsd/src/sys/rump | wc -l; grep -r __arraycount= netbsd/src/sys/rump | wc -l > 10 > 15 >=20 > My goal is to pull in as much test code from NetBSD that I?ve done on my = github fork and apply it to FreeBSD with as little deviations as possible, = wherever possible. The less we deviate and can push back and forth between = FreeBSD and NetBSD, the easier it will be in the long run for both parties = to develop and maintain test code and other code from NetBSD. >=20 > That being said, there might be a compromise that could be struck where t= he compat code can stay confined to libnetbsd. If I change the code bsd.tes= t.mk or contributed code to explicitly use -include or -I, then point to th= e lib/libnetbsd/sys/cdefs.h header, then this could become a non-issue: >=20 > -include filename > Adds an implicit #include into the predefines buffer which is = read > before the source file is preprocessed. >=20 > -Idirectory > Add the specified directory to the search path for include fil= es. >=20 > Does that seem like an appropriate solution? If it is isolated to tests, I have no objections nor comments. >=20 > Thanks! > -Garrett >=20 > PS For what it?s worth, nitems use has caught on like wildfire (it beats = out __arraycount and the ad hoc sizeof): >=20 > $ git grep -e nitems -- | egrep -v '^(cddl|contrib|crypto|gnu)' | wc -l > 551 It might be worth to clean up the __arraycount or other equivalent macros in the non-contributed code (only because the issue is already discussed much more than it worth). E.g., sys/kern/stack_protector.c can happily use nitems(), I am sure that there are much more such places. --WEBrq8xPpGVQldHe Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUD2sgAAoJEJDCuSvBvK1BZaEP/3bYiHQmUOOCUA3O3w0EllCm dJPm7PSfnbd0oAeFm6Y47dtH/napTcbctYGKe+/Dznd2wUz8sgFVhjXynlxBEazl LShyizXEel3c0et5KCZ9bkP58oio9Cd20jhb82KcX8hvvlN97UcuvHr59M9Zpxu6 Vs/p5WCNCv70uTcPn06R0GFO8ldgy2pTPSj9AvavfcNk++rb/rpvkl/Gt2h+74S5 UjiNI7gNn+MACYcgtaDvAbsug2M6X4d0aaS5b0YWuwj7YCx8QG/+1N8R2KCn6SrF r5BmgQ/1ngw7pbazvZ5cdc5AQ+qfMX/EmBeBYwkCthR9+RAh0FNJyRTbXB36D820 1X5nQw9buFZZNrFrAriiSkLQN4lYg4csHgQkErA88pcG8U7yNTnAScqQmFK7iFRK xfm5HeAsUw6qr2naCRoMnz75n0WODP5+TmcT1Np57/4nw5RpYFCexQNP1D0Wd7N4 XK8+OwrtBGgM52aVgJF9Dle9oxJntRtq6bdZyYCq613tS7OmBMd9lavCnWGht6cR BHwMTMs/Y7ijASa5whxHTCTtGQ5FIIVKqNlQT5d1YgzrXiNLARDDu9Z6UYK/UjiZ pbRmyIhdi3o/J5qgLW9pOcI4O6FcDwe0NxRaN+GqjxjSgRUQlGX5D5xAIv0kezuH DkUa3IeS11cnR/wHmKD4 =fmQy -----END PGP SIGNATURE----- --WEBrq8xPpGVQldHe--