From owner-svn-src-head@freebsd.org Tue Feb 21 13:46:42 2017 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 881CACE6E6E; Tue, 21 Feb 2017 13:46:42 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 2930038A; Tue, 21 Feb 2017 13:46:41 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 18A933C4D65; Wed, 22 Feb 2017 00:46:31 +1100 (AEDT) Date: Wed, 22 Feb 2017 00:46:31 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Conrad Meyer cc: "Pedro F. Giffuni" , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313982 - in head/sys/dev: agp al_eth an arcmsr bce beri/virtio bhnd/cores/usb buslogic ce cm cp ctau cx de ed fatm fe firewire hptiop hptmv iicbus isp le md ncr netmap ofw patm pccard ... In-Reply-To: Message-ID: <20170221234517.L3157@besplex.bde.org> References: <201702200343.v1K3hCk3060716@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.2 cv=VbSHBBh9 c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=Vr6Wp3CjCwVzRyYK444A:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 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: Tue, 21 Feb 2017 13:46:42 -0000 On Mon, 20 Feb 2017, Conrad Meyer wrote: > On Sun, Feb 19, 2017 at 7:43 PM, Pedro F. Giffuni wrote: >> ... >> Log: >> sys/dev: Replace zero with NULL for pointers. >> >> Makes things easier to read, plus architectures may set NULL to something >> different than zero. > ... > I like the change for style reasons. > > The comment about architectures with non-zero NULL is a little > misleading, though. This change has no impact on non-zero bit pattern > NULL architectures. The zero pointer value in C is NULL and NULL is > the zero pointer value in C. It may have a bit pattern other than > zero (i.e., printf("%p", NULL) may be non-zero and memset(&p, 0, > sizeof(p)) is bogus in portable code) but assigning the logical zero > value is always legitimate. This is well known. Some details in the above are mis-stated: - that this change has no effect might depend on using prototypes. The changed areas sometimes used NULL for function args near plain 0 in assignments. This is just a larger style bug than using either 0 or NULL consistently, since everything in the kernel uses prototypes. This was a bug in K&R code. Correct K&R has cast 0 or NULL in function args, but this is painful, so nearby assignments shouldn't use the same style. NULL can be: (a) an integer constant expression with value 0. E.g., (2 + 2L - 4ULL) (b) ((void *)(a)) So for unprototypes functions taking a pointer arg, uncast NULL is only correct if NULL happens to be ((void *)(a)) and the type that the function takes is 'void *' or possibly 'qualified void *'. (I forget if there is magic to allow qualifiers.) - there is no such thing as the zero pointer value. There are only null pointer constants. Null pointer constants are (a) and (b) as above. These arq quite different from null pointers. (a) only has value 0 as an integer. Casting this to (void *) gives a pointer, but I think it is still magic, so can have a different representation than when it is converted to a null pointer: np = (void *)0; may either retain all-bits-0 or change them to a nonzero magic value in the cast of 0, then convert to different magic bits (even back to all-bits 0) in np. - printf("%p", NULL) is not just unportable, but undefined in general, since printf() is variadic so this is broken like the unprototyped case. It is undefined even if pointers have all bits 0, since NULL - the changed code retains the style bug of explicit initialization of static null pointers. It is implicitly to null pointer constants which get converted in the same way as integer 0 or NULL. > After all, NULL is just a casted zero value: > > #define NULL ((void *)0) Hmm, that is correct for C, but this is an implementation detail. NULL is actually a convoluted mess: from sys/_null.h X #ifndef NULL X X #if !defined(__cplusplus) X #define NULL ((void *)0) The non-C++ case is now simple, but this defeats the excuse for existence of this file. This file was originally just to support C and allow defining NULL in a central place as 0L for __LP64__ only, while keeping the old definition as 0 for other cases, with extra complications for __KERNEL. Only broken programs noticed the difference. Mainly ones not using prototypes which has mostly gone away before this change was made. The ifdef allowed exposing type errors by changing the definition to weirder ones. 0L for __LP64__ and 0 for !__LP64__ did the reverse. ((void *)0) is even more forgiving, except it exposes the type error of using NULL for integer 0. Eventually the definition was changed to ((void *)0) as above. I don't like this since it only helps broken C programs and is wrong for C++. The file was retained to support C++, and grew messes. X #else X #if __cplusplus >= 201103L X #define NULL nullptr X #elif defined(__GNUG__) && defined(__GNUC__) && __GNUC__ >= 4 X #define NULL __null X #else X #if defined(__LP64__) X #define NULL (0L) The commit that changed the C case to use ((void *)0) was missing parentheses. The commit that fixed this also added parentheses that were not missing to 0L. This remains as a style bug. X #else X #define NULL 0 X #endif /* __LP64__ */ X #endif /* __GNUG__ */ X #endif /* !__cplusplus */ Style bugs: - tab instead of space after #endif - no comment on any #else, and perhaps too comments on too many #endif's. Comments on the inner ifdefs unimprove readabilty, but the set of ifdefs is so convoluted that it is hard to see which #else or #elseif is inner. > Maybe this is moot. I don't believe any architecture FreeBSD actually > supports has non-zero bitpattern NULL, but something weird like CHERI > might. Compilers should do portability checks on it. In fact, the convolutions in this file do little except define nullptr and __null to let compilers do more checking. Only the C++ case even has them. Compilers can do most checks using just the C90 definition of null pointer constants. Bruce