All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH][RESEND] do not redefine userspace's NULL #define
@ 2012-04-13 19:24 Lubos Lunak
  2012-04-13 19:39 ` Linus Torvalds
  0 siblings, 1 reply; 16+ messages in thread
From: Lubos Lunak @ 2012-04-13 19:24 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Andrew Morton, linux-kernel, Arnd Bergmann

GCC's NULL is actually __null, which allows detecting some questionable
NULL usage and warn about it. Moreover each platform/compiler should have
its own stddef.h anyway (which is different from linux/stddef.h).
So there's no good reason to override what the compiler provides.
Keep the #define conditionally, in order to keep the headers self-contained.

Signed-off-by: Luboš Luňák <l.lunak@suse.cz>
---

 There have been no further comments after this last version, and I do not see 
how this change should affect anything in either kernel builds or userspace 
builds, except for fixing the issue of overriding the proper NULL definition, 
so I consider this the proper and safe fix.

 include/linux/stddef.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 6a40c76..ce225a9 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -3,12 +3,13 @@
 
 #include <linux/compiler.h>
 
-#undef NULL
+#ifndef NULL
 #if defined(__cplusplus)
 #define NULL 0
 #else
 #define NULL ((void *)0)
 #endif
+#endif
 
 #ifdef __KERNEL__
 
-- 
1.7.7
-- 
 Lubos Lunak
 l.lunak@suse.cz

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 19:24 [PATCH][RESEND] do not redefine userspace's NULL #define Lubos Lunak
@ 2012-04-13 19:39 ` Linus Torvalds
  2012-04-13 21:02   ` Arnd Bergmann
                     ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Linus Torvalds @ 2012-04-13 19:39 UTC (permalink / raw)
  To: Lubos Lunak; +Cc: Andrew Morton, linux-kernel, Arnd Bergmann

On Fri, Apr 13, 2012 at 12:24 PM, Lubos Lunak <l.lunak@suse.cz> wrote:
> GCC's NULL is actually __null, which allows detecting some questionable
> NULL usage and warn about it. Moreover each platform/compiler should have
> its own stddef.h anyway (which is different from linux/stddef.h).
> So there's no good reason to override what the compiler provides.
> Keep the #define conditionally, in order to keep the headers self-contained.

There's no way user should ever include the linux internal stddef.h.

And quite frankly, kernel-external definitions of NULL have
traditionally been pure sh*t (ie plain "0" without the cast to a
pointer), so I'm not entirely convinced about this patch.

So what is the *actual* thing this helps with?

                    Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 19:39 ` Linus Torvalds
@ 2012-04-13 21:02   ` Arnd Bergmann
  2012-04-16  7:43     ` Martin Schwidefsky
  2012-04-13 21:46   ` Lubos Lunak
  2012-04-13 22:01   ` Peter Seebach
  2 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2012-04-13 21:02 UTC (permalink / raw)
  To: Linus Torvalds, Martin Schwidefsky
  Cc: Lubos Lunak, Andrew Morton, linux-kernel

On Friday 13 April 2012, Linus Torvalds wrote:
> There's no way user should ever include the linux internal stddef.h.
> 
> And quite frankly, kernel-external definitions of NULL have
> traditionally been pure sh*t (ie plain "0" without the cast to a
> pointer), so I'm not entirely convinced about this patch.
> 
> So what is the actual thing this helps with?

I think it used to get included implictly though other exported
kernel headers, but I found only one instance where this is still
true, in the s390 version of asm/ptrace.h.

Martin, does this work for you?

8<-----
headers: do not export linux/stddef.h

There is no reason to export linux/stddef.h and users should never
include it because the header conflicts with the standard stddef.h.

The only other exported header file that actually includes linux/stddef.h
is the s390 asm/ptrace.h, but there is no reason for it to do so, so
we can remove both the export and the inclusion.
When the file is no longer exported, we can also remove the extra lines
that attempt to make it safe for inclusion

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/s390/include/asm/ptrace.h |    1 -
 include/linux/Kbuild           |    1 -
 include/linux/stddef.h         |    8 --------
 3 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h
index aeb77f0..e2acdd5 100644
--- a/arch/s390/include/asm/ptrace.h
+++ b/arch/s390/include/asm/ptrace.h
@@ -183,7 +183,6 @@
 #define PTRACE_OLDSETOPTIONS         21
 
 #ifndef __ASSEMBLY__
-#include <linux/stddef.h>
 #include <linux/types.h>
 
 typedef union
diff --git a/include/linux/Kbuild b/include/linux/Kbuild
index 3c9b616..96391c7 100644
--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -351,7 +351,6 @@ header-y += sonypi.h
 header-y += sound.h
 header-y += soundcard.h
 header-y += stat.h
-header-y += stddef.h
 header-y += string.h
 header-y += suspend_ioctls.h
 header-y += swab.h
diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 6a40c76..daf42b8 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -3,14 +3,7 @@
 
 #include <linux/compiler.h>
 
-#undef NULL
-#if defined(__cplusplus)
-#define NULL 0
-#else
 #define NULL ((void *)0)
-#endif
-
-#ifdef __KERNEL__
 
 enum {
 	false	= 0,
@@ -23,6 +16,5 @@ enum {
 #else
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
-#endif /* __KERNEL__ */
 
 #endif

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 19:39 ` Linus Torvalds
  2012-04-13 21:02   ` Arnd Bergmann
@ 2012-04-13 21:46   ` Lubos Lunak
  2012-04-14  8:28     ` Arnd Bergmann
  2012-04-13 22:01   ` Peter Seebach
  2 siblings, 1 reply; 16+ messages in thread
From: Lubos Lunak @ 2012-04-13 21:46 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Andrew Morton, linux-kernel, Arnd Bergmann

On Friday 13 of April 2012, Linus Torvalds wrote:
> On Fri, Apr 13, 2012 at 12:24 PM, Lubos Lunak <l.lunak@suse.cz> wrote:
> > GCC's NULL is actually __null, which allows detecting some questionable
> > NULL usage and warn about it. Moreover each platform/compiler should have
> > its own stddef.h anyway (which is different from linux/stddef.h).
> > So there's no good reason to override what the compiler provides.
> > Keep the #define conditionally, in order to keep the headers
> > self-contained.
>
> There's no way user should ever include the linux internal stddef.h.

 If that were the case, one of the earlier versions of the patch would have 
been correct then, but as Arnd has pointed out, user applications do include 
the header. It's even as simple as:

#include <signal.h>

( -> bits/sigcontext.h -> asm/sigcontext.h -> linux/types.h -> 
linux/posix_types.h -> linux/stddef.h ).

> And quite frankly, kernel-external definitions of NULL have
> traditionally been pure sh*t (ie plain "0" without the cast to a
> pointer), so I'm not entirely convinced about this patch.

 Quite frankly, I do not care what happens with it in kernel builds. I just 
don't want the suboptimal definition to leak to userspace. If you instead 
make sure linux/stddef.h is never used by userspace, that's fine with me.

> So what is the *actual* thing this helps with?

 Random example:

/home/tinderbox/clang-master-build/vcl/unx/gtk/fpicker/SalGtkFilePicker.cxx:216:66: 
warning: missing sentinel in function call [-Wsentinel]
            GTK_CELL_LAYOUT (m_pLists[i]), pCell, "text", 0, NULL);
                                                                 ^
                                                                 , NULL

 I used the Clang compiler for the more detailed error output (and the in this 
context somewhat amusing fix-it hint), but it's of course the same with GCC.

 And from 'man gcc' (4.6):

       -Wstrict-null-sentinel (C++ and Objective-C++ only)
           Warn also about the use of an uncasted "NULL" as sentinel.  When 
compiling only with GCC this is a valid sentinel, as "NULL" is defined 
to "__null".  Although it is a null pointer constant not a null pointer, it 
is guaranteed to be of the same size as a pointer.  But this use is not 
portable across different compilers.

-- 
 Lubos Lunak
 l.lunak@suse.cz

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 19:39 ` Linus Torvalds
  2012-04-13 21:02   ` Arnd Bergmann
  2012-04-13 21:46   ` Lubos Lunak
@ 2012-04-13 22:01   ` Peter Seebach
  2012-04-13 22:24     ` Linus Torvalds
  2 siblings, 1 reply; 16+ messages in thread
From: Peter Seebach @ 2012-04-13 22:01 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Lubos Lunak, Andrew Morton, linux-kernel, Arnd Bergmann

On Fri, 13 Apr 2012 12:39:20 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> And quite frankly, kernel-external definitions of NULL have
> traditionally been pure sh*t (ie plain "0" without the cast to a
> pointer), so I'm not entirely convinced about this patch.

I was going to dispute this, and point out that I'm pretty sure
the C++ standard specifically requires the plain-integer 0/0L
definition.  Then I realized this did not actually contradict
your description.

Maybe the thing to do would be to ensure that NULL goes to __null,
then define that to be ((void *) 0) if the compiler doesn't provide
it?  The magic behavior of __null seems like it'd be preferable
where it is available.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 22:01   ` Peter Seebach
@ 2012-04-13 22:24     ` Linus Torvalds
  2012-04-13 23:18       ` Lubos Lunak
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2012-04-13 22:24 UTC (permalink / raw)
  To: Peter Seebach; +Cc: Lubos Lunak, Andrew Morton, linux-kernel, Arnd Bergmann

On Fri, Apr 13, 2012 at 3:01 PM, Peter Seebach
<peter.seebach@windriver.com> wrote:
>
> I was going to dispute this, and point out that I'm pretty sure
> the C++ standard specifically requires the plain-integer 0/0L
> definition.  Then I realized this did not actually contradict
> your description.

Yeah, the C++ definition i spure crap. Although I think even the worst
C++ people realized that, and realized that they were wrong. So most
of them seem to have figured out that defining NULL to 0 is insane and
totally wrong.

(IOW, if you don't get a warning for

   int i = NULL;

or get a warning for passing NULL to a routine that takes "int", your
language is pure and utter sh*t. Yes, K&R C made that mistake too, but
it got fixed. The fact that the C++ people used to *codify* that
insane braindamage is just sad).

> Maybe the thing to do would be to ensure that NULL goes to __null,
> then define that to be ((void *) 0) if the compiler doesn't provide
> it?  The magic behavior of __null seems like it'd be preferable
> where it is available.

So if gcc guarantees that __null has the correct semantics, I could
imagine replacing the kernel ((void *)0) with __null.

But unless we *know* that the incoming NULL is good, there's no way I
will let the kernel ever make the mistake of taking '0'.

                   Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 22:24     ` Linus Torvalds
@ 2012-04-13 23:18       ` Lubos Lunak
  2012-04-14  0:44         ` Linus Torvalds
  0 siblings, 1 reply; 16+ messages in thread
From: Lubos Lunak @ 2012-04-13 23:18 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Peter Seebach, Andrew Morton, linux-kernel, Arnd Bergmann

On Saturday 14 of April 2012, Linus Torvalds wrote:
> On Fri, Apr 13, 2012 at 3:01 PM, Peter Seebach
> > Maybe the thing to do would be to ensure that NULL goes to __null,
> > then define that to be ((void *) 0) if the compiler doesn't provide
> > it?  The magic behavior of __null seems like it'd be preferable
> > where it is available.
>
> So if gcc guarantees that __null has the correct semantics, I could
> imagine replacing the kernel ((void *)0) with __null.

 __null apparently exists only with g++, C does not have the stronger type 
safety that prevents ((void*)0) from being usable in C++, so __null is not 
needed there. So this is really only about userspace C++ code.

 So how about this one? It does not change anything in the C mode and keeps it 
the way you want, and it fixes the C++ mode.

8<-----
headers: do not redefine userspace's NULL #define when compiling C++ code

GCC's NULL is actually __null when compiling C++, which allows detecting some
questionable NULL usage and warn about it, so do not redefine NULL to 0.
Use the 0 definition only if NULL does not exists, to keep kernel headers
self-contained (and GCC's stddef.h will possibly redefine it later to __null).
In C mode ((void*)0) works fine.

Signed-off-by: Luboš Luňák <l.lunak@suse.cz>
---
 include/linux/stddef.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 6a40c76..13dfc92 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -3,10 +3,12 @@
 
 #include <linux/compiler.h>
 
-#undef NULL
 #if defined(__cplusplus)
+#ifndef NULL
 #define NULL 0
+#endif
 #else
+#undef NULL
 #define NULL ((void *)0)
 #endif
 
-- 
1.7.7


-- 
 Lubos Lunak
 l.lunak@suse.cz

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 23:18       ` Lubos Lunak
@ 2012-04-14  0:44         ` Linus Torvalds
  2012-04-14  6:43           ` Lubos Lunak
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2012-04-14  0:44 UTC (permalink / raw)
  To: Lubos Lunak; +Cc: Peter Seebach, Andrew Morton, linux-kernel, Arnd Bergmann

On Fri, Apr 13, 2012 at 4:18 PM, Lubos Lunak <l.lunak@suse.cz> wrote:
>> imagine replacing the kernel ((void *)0) with __null.
>
>  __null apparently exists only with g++, C does not have the stronger type
> safety that prevents ((void*)0) from being usable in C++

Please don't continue to spread this total bogosity.

The reason C++ cannot use "(void *)0" has nothing to do with "stronger
type safety". That's a total idiotic lie by C++ apologists, and I hate
hearing it repeated over and over again.

And it really *is* a lie. The C++ type system isn't even "stronger",
it's just different, and it's actively *broken* wrt NULL. Always has
been.

The sane thing to do for C++ would always have been to recognize that
"(void *)0" is not a "void pointer" - it's just NULL. INSTEAD, the
morons involved said "no, it's a void pointer, and instead we'll make
'0' be NULL".

Which is clearly insane, but is also technically simply *wrong*.

0 is an integer, it's not NULL _either_, and it's just a more stupid
version of NULL.

So the C++ people then completely made up the argument that "C++ has a
stronger type system, we can't use 'void *', so the C style ((void
*)0) is wrong for C++".

Which is utter and complete bullshit, and any amount of brains would
have realized that (since C++ at the same time happily continued to
special case the *integer* zero).

I don't hate __null, but I absolutely despise the crazy C++ apologists
who try to claim that C++ was somehow "right" in not accepting (void
*)0, and that it was somehow about "type safety". No, it was not. It
has always been just nothing but a moronic hang-up, and it has always
been *wrong*.

So don't spread that lie. It was wrong. The fact is, the *constant* 0
is the special one, and C++ could have (and should have) just accepted
that that constant should have been cast to (void *) and that is what
NULL is defined to. Instead, C++ used the *weaker* type system of
saying that the *integer* constant 0 is NULL, which is pure and utter
garbage.

And then they lie, and claim that their *weaker* type system NULL is
"stronger". Pure idiocy.

                 Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-14  0:44         ` Linus Torvalds
@ 2012-04-14  6:43           ` Lubos Lunak
  2012-04-14  7:51             ` Linus Torvalds
  0 siblings, 1 reply; 16+ messages in thread
From: Lubos Lunak @ 2012-04-14  6:43 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Peter Seebach, Andrew Morton, linux-kernel, Arnd Bergmann

On Saturday 14 of April 2012, Linus Torvalds wrote:
> On Fri, Apr 13, 2012 at 4:18 PM, Lubos Lunak <l.lunak@suse.cz> wrote:
> >> imagine replacing the kernel ((void *)0) with __null.
> >
> >  __null apparently exists only with g++, C does not have the stronger
> > type safety that prevents ((void*)0) from being usable in C++
>
> Please don't continue to spread this total bogosity.
>
> The reason C++ cannot use "(void *)0" has nothing to do with "stronger
> type safety". That's a total idiotic lie by C++ apologists, and I hate
> hearing it repeated over and over again.
>
> And it really *is* a lie. The C++ type system isn't even "stronger",

$ cat b.c
void foo()
    {
    int* a;
    void* b;
    char c;
    a = b = &c;
    }
$ gcc -Wall b.c -c
$ g++ -Wall b.c -c
b.c: In function ‘void foo()’:
b.c:6:14: error: invalid conversion from ‘void*’ to ‘int*’

> it's just different, and it's actively *broken* wrt NULL. Always has
> been.
>
> The sane thing to do for C++ would always have been to recognize that
> "(void *)0" is not a "void pointer" - it's just NULL.

 As much as I agree that it was stupid to copy the special-casing of 0 from C 
and they should have instead special-cased (void*)0, the reality is that it 
took them until C++11 to add that to the standard, and even there they did it 
differently, so C++ now has to live with the legacy of NULL being possibly 
implemented in a stupid way.

 The reality is also that g++ people recognized this quite some time back, 
implemented NULL in a sane way, and kernel headers still replace that 
implementation with one that, as you yourself say, is technically wrong. So 
how about my patch that fixes that? The rest is a discussion about things 
that should have been but are not and is completely pointless by now.

8<-----
headers: do not redefine userspace's NULL #define when compiling C++ code

GCC's NULL is actually __null when compiling C++, which allows detecting some
questionable NULL usage and warn about it.

Signed-off-by: Luboš Luňák <l.lunak@suse.cz>
---
 include/linux/stddef.h |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 6a40c76..a75f4b9 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -3,12 +3,18 @@
 
 #include <linux/compiler.h>
 
-#undef NULL
 #if defined(__cplusplus)
-#define NULL 0
+#ifndef NULL
+#ifdef __GNUG__
+#define NULL __null
 #else
-#define NULL ((void *)0)
+#define NULL 0
 #endif
+#endif
+#else /* __cplusplus */
+#undef NULL
+#define NULL ((void *)0)
+#endif /* __cplusplus */
 
 #ifdef __KERNEL__
 
-- 
1.7.7


-- 
 Lubos Lunak
 l.lunak@suse.cz

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-14  6:43           ` Lubos Lunak
@ 2012-04-14  7:51             ` Linus Torvalds
  2012-04-14  8:21               ` Lubos Lunak
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2012-04-14  7:51 UTC (permalink / raw)
  To: Lubos Lunak; +Cc: Peter Seebach, Andrew Morton, linux-kernel, Arnd Bergmann

On Fri, Apr 13, 2012 at 11:43 PM, Lubos Lunak <l.lunak@suse.cz> wrote:
>>
>> And it really *is* a lie. The C++ type system isn't even "stronger",
>
> $ cat b.c
> void foo()
>    {
>    int* a;
>    void* b;
>    char c;
>    a = b = &c;
>    }
> $ gcc -Wall b.c -c
> $ g++ -Wall b.c -c
> b.c: In function ‘void foo()’:
> b.c:6:14: error: invalid conversion from ‘void*’ to ‘int*’

You're missing the point. Yes, the C++ type system is different, and
"void *" doesn't convert without a warning.

But "(void *)0" isn't just any "void *". It's NULL. It's special.
Except the C++ people didn't get the memo, and in fact, they argued
themselves blue in the face about not getting the memo.

And a C++ person who says that "(vodi *)0" is just any "void *" is a
*fucking*moron*, because by exactly the same argument, "0" is just any
"int". Yet in C++, if you do

   int * a = 0;

you don't get the warning.

See? There is absolutely *zero* excuse for the idiotic traditional C++
brain damage of saying "NULL cannot be '(void *)0', because 'void *'
will warn".

Anybody who says that is lying. Because it is the *exact* same logic
as "NULL cannot be '0', because 'int' will warn".

See my point? It's not actually the type that is the important thing.
It's the *constant". But using a plain "0" constant was always a
mistake because of the confusion with the normal and common integer
zero constant that is used all over.

The C people actually understood and realized that it was a mistake,
and they fixed it. Sure, C compilers still (for legacy reasons) allow
plain 0 for NULL, but that is pure backwards compatibility, and I
think it's sad and wrong. But at least C standards people realized
that the legacy model was stupid, and everybody accepts that NULL is
much better off as being "(void *)0".

The C++ people? They are morons, and they never got it, and in fact
they spent much of their limited mental effort arguing against it.

They should just have made "(void *)0" be NULL. As it is, everybody
finally realized that NULL really can't be "0" after all, at which
point the C++ people were too embarrassed to admit that they had been
wrong all along, so the compiler people started making up special
"__null" things. For no really good reason, except that the C++ people
had spent so much time trying convince themselves that their stupid
model was right.

The whole "it's a stronger type system, so NULL has to be 0" is pure
and utter garbage. It's wrong. It's stupid.

In reality, the C++ type system is actually often *weaker* with 'void
*' even outside of the special case of NULL (and NULL really *is* a
special case - it is the *constant* 0. It's not the "integer 0" or
anything like that, and even K&R understood that). The C++ problems
wrt "void *" just result in more explicit casting. And then the
explicit casting actually makes for a *weaker* typesystem than the C
model which just says "ok, 'void *' doesn't need casts". Because now
you have a type override expliticly written in that software that
actually throws all type safety away entirely.

So "Yes, C++ will warn about things that C does not warn about". But
that doesn't make the type system stronger. It just makes C++
apologists claim it is stronger, even though in practice it forces
people to add casts that actually make it weaker.

Yeah, I'm not a fan of C++. It's a cruddy language.

                       Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-14  7:51             ` Linus Torvalds
@ 2012-04-14  8:21               ` Lubos Lunak
  2012-04-14  8:27                 ` Linus Torvalds
  0 siblings, 1 reply; 16+ messages in thread
From: Lubos Lunak @ 2012-04-14  8:21 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Peter Seebach, Andrew Morton, linux-kernel, Arnd Bergmann

On Saturday 14 of April 2012, Linus Torvalds wrote:
> Yeah, I'm not a fan of C++. It's a cruddy language.

 Whatever. There's no need for you to waste time on lecturing me on C++, I 
know it well enough, including the pitfalls. And I especially know well about 
C++'s NULL, I got enough theoretical lectures from gcc people while making 
them not bury warnings about incorrect NULL usage in the rather 
useless -Wconversion. The reality is what it is regardless of how much you or 
I like it.

 How about the patch? Are there any *actual* problems with it or can it please 
go in?

-- 
 Lubos Lunak
 l.lunak@suse.cz

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-14  8:21               ` Lubos Lunak
@ 2012-04-14  8:27                 ` Linus Torvalds
  2012-04-14  8:54                   ` Lubos Lunak
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2012-04-14  8:27 UTC (permalink / raw)
  To: Lubos Lunak; +Cc: Peter Seebach, Andrew Morton, linux-kernel, Arnd Bergmann

On Sat, Apr 14, 2012 at 1:21 AM, Lubos Lunak <l.lunak@suse.cz> wrote:
>
>  How about the patch? Are there any *actual* problems with it or can it please
> go in?

If we're going to change that thing, I'd actually prefer to just move
it inside the #ifdef __KERNEL__ entirely, and get rid of the cplusplus
case.

IOW, something like this (obviously white-space-mangled) patch:

    diff --git a/include/linux/stddef.h b/include/linux/stddef.h
    index 6a40c76bdcf1..1747b6787b9e 100644
    --- a/include/linux/stddef.h
    +++ b/include/linux/stddef.h
    @@ -3,14 +3,10 @@

     #include <linux/compiler.h>

    +#ifdef __KERNEL__
    +
     #undef NULL
    -#if defined(__cplusplus)
    -#define NULL 0
    -#else
     #define NULL ((void *)0)
    -#endif
    -
    -#ifdef __KERNEL__

     enum {
     	false	= 0,

which protects the kernel NULL definition along with the whole
offsetof and true/false ones too. And just gets rid of the insane C++
case entirely.

                Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 21:46   ` Lubos Lunak
@ 2012-04-14  8:28     ` Arnd Bergmann
  0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2012-04-14  8:28 UTC (permalink / raw)
  To: Lubos Lunak; +Cc: Linus Torvalds, Andrew Morton, linux-kernel

On Friday 13 April 2012, Lubos Lunak wrote:
>  If that were the case, one of the earlier versions of the patch would have 
> been correct then, but as Arnd has pointed out, user applications do include 
> the header. It's even as simple as:
> 
> #include <signal.h>
> 
> ( -> bits/sigcontext.h -> asm/sigcontext.h -> linux/types.h -> 
> linux/posix_types.h -> linux/stddef.h ).
> 

I knew there must have been something wrong with my thinking and I
clearly missed the posix_types.h reference. I accidentally did
'git grep linux/stddef.h obj/usr/include', instead of 'grep -r'.

I've tried to be more thorough this time, and I think that nothing
stops us from removing the linux/stddef.h include from the exported
version of posix_types.h, but we should probably make sure that we
still include linux/compiler.h, because a lot of stuff that includes
linux/posix_types.h expects that. Alternatively, we can just move the
NULL definition inside of #ifdef __KERNEL__.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-14  8:27                 ` Linus Torvalds
@ 2012-04-14  8:54                   ` Lubos Lunak
  2012-04-14  9:30                     ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: Lubos Lunak @ 2012-04-14  8:54 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Peter Seebach, Andrew Morton, linux-kernel, Arnd Bergmann

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

On Saturday 14 of April 2012, Linus Torvalds wrote:
> On Sat, Apr 14, 2012 at 1:21 AM, Lubos Lunak <l.lunak@suse.cz> wrote:
> >  How about the patch? Are there any *actual* problems with it or can it
> > please go in?
>
> If we're going to change that thing, I'd actually prefer to just move
> it inside the #ifdef __KERNEL__ entirely, and get rid of the cplusplus
> case.
>
> IOW, something like this (obviously white-space-mangled) patch:
...
> which protects the kernel NULL definition along with the whole
> offsetof and true/false ones too. And just gets rid of the insane C++
> case entirely.

 That's what the original version of my patch did, but Arnd pointed out that 
other exported headers might use NULL and thus not be self-contained in 
userspace after this change, breaking backwards source compatibility. But I'd 
expect such breakages to be rather unlikely (stddef.h is probably pulled in 
by pretty much everything), so if you're fine with it, I'm ok with this 
solution too.

-- 
 Lubos Lunak
 l.lunak@suse.cz

[-- Attachment #2: Lubos Lunak <l.lunak@suse.cz>: [PATCH] do not export kernel's NULL #define to userspace --]
[-- Type: message/rfc822, Size: 1389 bytes --]

From: Lubos Lunak <l.lunak@suse.cz>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org
Subject: [PATCH] do not export kernel's NULL #define to userspace
Date: Wed, 21 Mar 2012 14:08:24 +0100
Message-ID: <201203211408.25040.l.lunak@suse.cz>

GCC's NULL is actually __null, which allows detecting some questionable
NULL usage and warn about it. Moreover each platform/compiler should have
its own stddef.h anyway (which is different from linux/stddef.h).
So there's no good reason to leak kernel's NULL to userspace and
override what the compiler provides.

Signed-off-by: Luboš Luňák <l.lunak@suse.cz>
---
 include/linux/stddef.h |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 6a40c76..1747b67 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -3,14 +3,10 @@
 
 #include <linux/compiler.h>
 
+#ifdef __KERNEL__
+
 #undef NULL
-#if defined(__cplusplus)
-#define NULL 0
-#else
 #define NULL ((void *)0)
-#endif
-
-#ifdef __KERNEL__
 
 enum {
 	false	= 0,
-- 
1.7.3.4
-- 
 Lubos Lunak
 l.lunak@suse.cz

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-14  8:54                   ` Lubos Lunak
@ 2012-04-14  9:30                     ` Arnd Bergmann
  0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2012-04-14  9:30 UTC (permalink / raw)
  To: Lubos Lunak; +Cc: Linus Torvalds, Peter Seebach, Andrew Morton, linux-kernel

On Saturday 14 April 2012, Lubos Lunak wrote:
>  That's what the original version of my patch did, but Arnd pointed out that 
> other exported headers might use NULL and thus not be self-contained in 
> userspace after this change, breaking backwards source compatibility. But I'd 
> expect such breakages to be rather unlikely (stddef.h is probably pulled in 
> by pretty much everything), so if you're fine with it, I'm ok with this 
> solution too.

Yes, I stand corrected on that one. I have checked that actually no exported
headers rely on the definition of NULL, so your original patch is good.

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][RESEND] do not redefine userspace's NULL #define
  2012-04-13 21:02   ` Arnd Bergmann
@ 2012-04-16  7:43     ` Martin Schwidefsky
  0 siblings, 0 replies; 16+ messages in thread
From: Martin Schwidefsky @ 2012-04-16  7:43 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Linus Torvalds, Lubos Lunak, Andrew Morton, linux-kernel

On Fri, 13 Apr 2012 21:02:14 +0000
Arnd Bergmann <arnd@arndb.de> wrote:

> On Friday 13 April 2012, Linus Torvalds wrote:
> > There's no way user should ever include the linux internal stddef.h.
> > 
> > And quite frankly, kernel-external definitions of NULL have
> > traditionally been pure sh*t (ie plain "0" without the cast to a
> > pointer), so I'm not entirely convinced about this patch.
> > 
> > So what is the actual thing this helps with?
> 
> I think it used to get included implictly though other exported
> kernel headers, but I found only one instance where this is still
> true, in the s390 version of asm/ptrace.h.
> 
> Martin, does this work for you?
> 
> 8<-----
> headers: do not export linux/stddef.h
> 
> There is no reason to export linux/stddef.h and users should never
> include it because the header conflicts with the standard stddef.h.
> 
> The only other exported header file that actually includes linux/stddef.h
> is the s390 asm/ptrace.h, but there is no reason for it to do so, so
> we can remove both the export and the inclusion.
> When the file is no longer exported, we can also remove the extra lines
> that attempt to make it safe for inclusion
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Kernel compiles and boots. After protecting the stddef.h #include in
include/linux/posix_types.h with #ifdef __KERNEL__ gdb and strace
could be build as well.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-04-16  7:44 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-13 19:24 [PATCH][RESEND] do not redefine userspace's NULL #define Lubos Lunak
2012-04-13 19:39 ` Linus Torvalds
2012-04-13 21:02   ` Arnd Bergmann
2012-04-16  7:43     ` Martin Schwidefsky
2012-04-13 21:46   ` Lubos Lunak
2012-04-14  8:28     ` Arnd Bergmann
2012-04-13 22:01   ` Peter Seebach
2012-04-13 22:24     ` Linus Torvalds
2012-04-13 23:18       ` Lubos Lunak
2012-04-14  0:44         ` Linus Torvalds
2012-04-14  6:43           ` Lubos Lunak
2012-04-14  7:51             ` Linus Torvalds
2012-04-14  8:21               ` Lubos Lunak
2012-04-14  8:27                 ` Linus Torvalds
2012-04-14  8:54                   ` Lubos Lunak
2012-04-14  9:30                     ` Arnd Bergmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.