Re: [PATCH] Define NULL as __null in C++ mode when using GCC or Clang.

[James Y Knight <jyknight@google.com>]

[-- Attachment #1: Type: text/plain, Size: 5696 bytes --]

On Tue, Jul 9, 2019 at 10:04 PM Szabolcs Nagy <nsz@port70.net> wrote:

> * James Y Knight <jyknight@google.com> [2019-07-09 19:04:13 -0400]:
> > On Tue, Jul 9, 2019 at 3:38 PM Rich Felker <dalias@libc.org> wrote:
> > > On Tue, Jul 09, 2019 at 03:19:00PM -0400, James Y Knight wrote:
> > > > Both GCC and Clang ship their own stddef.h which does this (musl's
> > > > stddef.h is simply ignored). But, musl also defines the macro in a
> > > > number of other headers.
> > >
> > > This was intentional at the time it was done. At least in the present
> > > version of C++ at the time, __null was not a conforming definition.
> > > For example, conforming applications could parse the stringification
> > > of NULL and expect to get something that's a valid null pointer
> > > constant. This could possibly be revisited, if it's changed, and if it
> > > can be done in a way that doesn't involve compiler-specific stuff.
> > >
> >
> > The C++11 standard simply says "The macro NULL is an
> implementation-defined
> > C++ null pointer constant in this International Standard." (and links to
> > the definition of null pointer constant -- "A null pointer constant is an
> > integral constant expression (5.19) prvalue of integer type that
> evaluates
> > to zero or a prvalue of type std::nullptr_t").
> >
> > If the implementation defines __null to be a null pointer constant, which
> > is an integral constant expression of integer type that evaluates to zero
> > (which it does), that seems entirely valid. I see nothing that restricts
> > what it may stringify to.
>
> it is clear that 0L is a conforming definition for all
> conforming c++ compilers.
>
> it is less clear if __null is so in all compilers that
> define __GNUC__.


Sure, it's theoretically possible that some unknown compiler pretends to be
GCC, but then implements __null incorrectly. But if it does, why does musl
care? A conforming C++ implementation certainly requires a great many
components to be implemented correctly, not least of which is the compiler.

so normally one would expect a compelling reason to do

such change.


The warning when you're using NULL as a "long" instead of a pointer is a
very important warning -- so important it's on by default without any
warning flags. It's _extremely_ surprising behavior for most developers
that foo(NULL) can call a foo(long) overload in preference to a foo(int*)
overload. This is why the compiler went to the trouble of implementing
__null in the first place. It's only somewhat less important, now that
'nullptr' is becoming more prevalent.


> > > Mostly, getting musl's definition simply degrades warning diagnostics

>
> > > in C++ slightly -- e.g. GCC can no longer emit this warning:
> > > >   warning: passing NULL to non-pointer argument 1 of 'int foo(long
> int)'
> > > > [-Wconversion-null]
> > >
> > > The current definition however catches bugs where NULL is wrongly used
> > > to terminate a variadic argument list where (void*)0 or (char*)0 is
> > > actually needed. See commits 41d7c77d6a2e74294807d35062e4cd1d48ab72d3
> > > and c8a9c22173f485c8c053709e1dfa0a617cb6be1a. So both choices are a
> > > tradeoff in diagnostic capability.
> >
> >
> > Ehh. Actually musl's definition actually breaks the intended semantics of
> > that warning as well. The following program:
> > #include <unistd.h>
> > int main() {
> >   execl("foo", "bar", NULL);
> > }
> > does indeed not warn with "g++ -Wall foo.cc" when NULL is __null, and it
> > does warn if NULL is 0L.
> >
> > However, it was an intentional choice, not an accident, to suppress the
> > warning in the former case. This kind of usage is widespread,, and in
> > practice it does not trigger buggy behavior (because __null is guaranteed
> > to be the same size as a pointer). GCC does have a separate
> > "-Wstrict-null-sentinel" warning flag, if you want to be more pedantic
> > about this not-standards-compliant code.
>
> if a variadic function implementation calls va_arg(ap,char*)
> on a NULL argument then the behaviour is undefined in c++,
> but it is well-defined in posix! so the pattern is legitimately
> widespread but users often mistakenly compile c code as c++.
>

While it's not defined by C++, I do not believe there are any platforms GCC
or Clang supports where passing __null to a varargs function instead of
(void*)0 causes a problem. The intent is certainly that it's in-practice
safe to use as a varargs-terminator, by being defined to be the correct
size.


> how can we ensure that an optimizing c++ compiler won't
> silently break such code? gcc rutinely eliminates code if
> it can spot that a null pointer is passed to a library
> function where that is undefined.


not diagnosing such correctness issues when you can is bad
> practice: it leaves around ticking time bombs.


There's always a trade-off between which warnings should be enabled by
default or in -Wall. If this were an _actual_ codegen problem, for sure
-Wstrict-null-sentinel should be enabled by default. However, as a
very-common idiom which does not cause an actual problem, having this not
emit a warning in the default flag sets seems justifiable.


> (the warnings __null can provide are rarely time bombs)
>

Without the warning, developers may not realize that the wrong overload is
being called. And maybe their software is working anyways, for whatever
reason -- until someone converts NULL to nullptr, causing an unexpected
behavior change.


> but i think NULL is dangerously broken in c++ either way,
> so both the current and the proposed definition is fine by me,
> i just slightly prefer the current definition.
>

I think everyone agrees NULL is broken in C++, which is why we now have
nullptr.

[-- Attachment #2: Type: text/html, Size: 8029 bytes --]