[PATCH] posix_types.h: make __NFDBITS match glibc definition

[PATCH] posix_types.h: make __NFDBITS match glibc definition

[Josh Boyer]

Recent glibc made a change to suppress sign-conversion warnings from FD_SET
(glibc commit ceb9e56b3d1).  That patch solved the particular error it was
aiming to, however applications that #include <linux/types.h> after
including <sys/select.h> can now hit a build failure if -Werror=sign-compare
and -D_FORTIFY_SOURCE=2 is passed to gcc.  This can be seen when building
this trivial application against a recent enough glibc:

| #include <sys/select.h>
| #include <linux/types.h>
|
| int main(int argc, char **argv)
| {
|   fd_set fds;
|   FD_ZERO(&fds);
|   FD_SET(0, &fds);
|   return FD_ISSET(0, &fds);
| }

It was suggested the kernel should either match the glibc definition of
__NFDBITS in linux/posix_types.h or remove it entirely.  Given that we
don't know what applications may be relying on the header having a
definition, we opt for the former.

This resolves https://bugzilla.redhat.com/show_bug.cgi?id=837641

Reported-by: Jeff Law <law@redhat.com>
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 include/linux/posix_types.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/posix_types.h b/include/linux/posix_types.h
index f04c98c..cc8530e 100644
--- a/include/linux/posix_types.h
+++ b/include/linux/posix_types.h
@@ -19,7 +19,7 @@
  * use the ones here. 
  */
 #undef __NFDBITS
-#define __NFDBITS	(8 * sizeof(unsigned long))
+#define __NFDBITS	(8 * (int) sizeof(long int))
 
 #undef __FD_SETSIZE
 #define __FD_SETSIZE	1024
-- 
1.7.10.4

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 11:12 AM, Josh Boyer <jwboyer@redhat.com> wrote:
>
> +#define __NFDBITS      (8 * (int) sizeof(long int))

I don't know if the type change is needed, but who the hell uses "long int"?

Somebody is confused. Grepping the kernel sources, I am saddened to
see any of these at all. I certainly would never want to add one.

                Linus

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Josh Boyer]

On Tue, Jul 24, 2012 at 11:20:11AM -0700, Linus Torvalds wrote:
> On Tue, Jul 24, 2012 at 11:12 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> >
> > +#define __NFDBITS      (8 * (int) sizeof(long int))
> 
> I don't know if the type change is needed, but who the hell uses "long int"?

Not strictly, no.  I believe in my brief tests, just the int cast was
sufficient.  I can respin if you'd like.  I probably need to anyway to
CC stable on this.

> Somebody is confused. Grepping the kernel sources, I am saddened to
> see any of these at all. I certainly would never want to add one.

Thoughtless copying on my part.  Glibc actually uses sizeof(__fd_mask),
but that is a typedef long int.  I'm pretty sure that varianet doesn't
make things better.

josh

[PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Josh Boyer]

Recent glibc made a change to suppress sign-conversion warnings from FD_SET
(glibc commit ceb9e56b3d1).  That patch solved the particular error it was
aiming to, however applications that #include <linux/types.h> after
including <sys/select.h> can now hit a build failure if -Werror=sign-compare
and -D_FORTIFY_SOURCE=2 is passed to gcc.  This can be seen when building
this trivial application against a recent enough glibc:

| #include <sys/select.h>
| #include <linux/types.h>
|
| int main(int argc, char **argv)
| {
|   fd_set fds;
|   FD_ZERO(&fds);
|   FD_SET(0, &fds);
|   return FD_ISSET(0, &fds);
| }

It was suggested the kernel should either match the glibc definition of
__NFDBITS in linux/posix_types.h or remove it entirely.  Given that we
don't know what applications may be relying on the header having a
definition, we make our definition compatible with glibc.

This resolves https://bugzilla.redhat.com/show_bug.cgi?id=837641

Reported-by: Jeff Law <law@redhat.com>
CC: <stable@vger.kernel.org>
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---

v2: Avoid the type change to long int

 include/linux/posix_types.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/posix_types.h b/include/linux/posix_types.h
index f04c98c..0bfc9cc 100644
--- a/include/linux/posix_types.h
+++ b/include/linux/posix_types.h
@@ -19,7 +19,7 @@
  * use the ones here. 
  */
 #undef __NFDBITS
-#define __NFDBITS	(8 * sizeof(unsigned long))
+#define __NFDBITS	(8 * (int) sizeof(unsigned long))
 
 #undef __FD_SETSIZE
 #define __FD_SETSIZE	1024
-- 
1.7.10.4

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 11:32 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> Recent glibc made a change to suppress sign-conversion warnings from FD_SET
> (glibc commit ceb9e56b3d1).  That patch solved the particular error it was
> aiming to, however applications that #include <linux/types.h> after
> including <sys/select.h> can now hit a build failure if -Werror=sign-compare
> and -D_FORTIFY_SOURCE=2 is passed to gcc.  This can be seen when building
> this trivial application against a recent enough glibc:

Looking more at this, I now hate it for *another* reason.

Making __NFDBITS be a signed value turns __FDELT() and __FDMASK() into
potentially pure and utter crap. Doing signed divisions (or modulus)
is a disaster - suddenly it's not just a bit shift any more.

Guys, the glibc people really seem to not have thought their change
through. Or maybe they fixed their __FDELT/__FDMASK at the same time?

                    Linus

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Josh Boyer]

On Tue, Jul 24, 2012 at 11:46:25AM -0700, Linus Torvalds wrote:
> On Tue, Jul 24, 2012 at 11:32 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> > Recent glibc made a change to suppress sign-conversion warnings from FD_SET
> > (glibc commit ceb9e56b3d1).  That patch solved the particular error it was
> > aiming to, however applications that #include <linux/types.h> after
> > including <sys/select.h> can now hit a build failure if -Werror=sign-compare
> > and -D_FORTIFY_SOURCE=2 is passed to gcc.  This can be seen when building
> > this trivial application against a recent enough glibc:
> 
> Looking more at this, I now hate it for *another* reason.
> 
> Making __NFDBITS be a signed value turns __FDELT() and __FDMASK() into
> potentially pure and utter crap. Doing signed divisions (or modulus)
> is a disaster - suddenly it's not just a bit shift any more.
> 
> Guys, the glibc people really seem to not have thought their change
> through. Or maybe they fixed their __FDELT/__FDMASK at the same time?

Jeff can probably answer this better than I can, so likely best to wait
for him.

FWIW, the definitions of __FD_ELT/__FD_MASK in glibc are:

#define __FD_ELT(d)     ((d) / __NFDBITS)
#define __FD_MASK(d)    ((__fd_mask) 1 << ((d) % __NFDBITS))

where __fd_mask is 'typdef long int'.

__NFDBITS was changed to int type with glibc commit eb0b6cb6e in 2009,
and __FDELT/__FDMASK were basically renamed with glibc commit
e529793b508 in 2011.

josh

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:03 PM, Josh Boyer <jwboyer@redhat.com> wrote:
>
> FWIW, the definitions of __FD_ELT/__FD_MASK in glibc are:
>
> #define __FD_ELT(d)     ((d) / __NFDBITS)
> #define __FD_MASK(d)    ((__fd_mask) 1 << ((d) % __NFDBITS))
>
> where __fd_mask is 'typdef long int'.

Yeah, that's not good.

If __NFDBITS is signed (and it is), and "d" is a signed type, that
division and modulus now create stupid extra code with conditionals
(assuming 'd' isn't constant, of course).

So changing the sign of __NFDBITS has these kinds of subtle side
effects that clearly the glibc people didn't actually think about.

What was the *advantage* of that stupidity?

Quite frankly, if you want to make NFDBITS be an "int", then it should
have been done at that

   #define NFDBITS ((int)__NFDBITS)

level, not at "__NFDBITS". Exactly because the unsigned type there matters.

Does anybody in the glibc camp care about efficient and small code AT ALL?

                  Linus

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Jeff Law]

On 07/24/12 13:09, Linus Torvalds wrote:
> On Tue, Jul 24, 2012 at 12:03 PM, Josh Boyer <jwboyer@redhat.com> wrote:
>>
>> FWIW, the definitions of __FD_ELT/__FD_MASK in glibc are:
>>
>> #define __FD_ELT(d)     ((d) / __NFDBITS)
>> #define __FD_MASK(d)    ((__fd_mask) 1 << ((d) % __NFDBITS))
>>
>> where __fd_mask is 'typdef long int'.
>
> Yeah, that's not good.
>
> If __NFDBITS is signed (and it is), and "d" is a signed type, that
> division and modulus now create stupid extra code with conditionals
> (assuming 'd' isn't constant, of course).
>
> So changing the sign of __NFDBITS has these kinds of subtle side
> effects that clearly the glibc people didn't actually think about.
>
> What was the *advantage* of that stupidity?
>
> Quite frankly, if you want to make NFDBITS be an "int", then it should
> have been done at that
>
>     #define NFDBITS ((int)__NFDBITS)
>
> level, not at "__NFDBITS". Exactly because the unsigned type there matters.
>
> Does anybody in the glibc camp care about efficient and small code AT ALL?
Please refer to the original discussion where they did evaluate the cost 
of this change and tested that the final change made no difference to 
the generated code.

http://sourceware.org/bugzilla/show_bug.cgi?id=14210

Needlessly slamming these folks doesn't help anything.

Jeff

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Jeff Law]

On 07/24/12 12:24, Josh Boyer wrote:
> On Tue, Jul 24, 2012 at 11:20:11AM -0700, Linus Torvalds wrote:
>> On Tue, Jul 24, 2012 at 11:12 AM, Josh Boyer <jwboyer@redhat.com> wrote:
>>>
>>> +#define __NFDBITS      (8 * (int) sizeof(long int))
>>
>> I don't know if the type change is needed, but who the hell uses "long int"?
>
> Not strictly, no.  I believe in my brief tests, just the int cast was
> sufficient.  I can respin if you'd like.  I probably need to anyway to
> CC stable on this.
All that's strictly necessary is that cast to (int).  That avoids the 
problem.

Jeff

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:15 PM, Jeff Law <law@redhat.com> wrote:
>
> Please refer to the original discussion where they did evaluate the cost of
> this change and tested that the final change made no difference to the
> generated code.

Umm. That bugzilla entry seems to be talking about a *sane* change, namely

-  ({ unsigned long int __d = (d);					    \
+  ({ unsigned long int __d = (unsigned long int) (d);			    \

in __FD_ELT(), which is totally different from the one Josh talks about.

In fact, that glibc change looks fine. The patch Josh has been pushing
has changed __NFDBITS to signed, and generates bad code. But
apparently that patch then didn't come from glibc at all?

                   Linus

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Jeff Law]

On 07/24/12 13:24, Linus Torvalds wrote:
> On Tue, Jul 24, 2012 at 12:15 PM, Jeff Law <law@redhat.com> wrote:
>>
>> Please refer to the original discussion where they did evaluate the cost of
>> this change and tested that the final change made no difference to the
>> generated code.
>
> Umm. That bugzilla entry seems to be talking about a *sane* change, namely
>
> -  ({ unsigned long int __d = (d);					    \
> +  ({ unsigned long int __d = (unsigned long int) (d);			    \
>
> in __FD_ELT(), which is totally different from the one Josh talks about.
Right.  Josh's change is necessary to prevent warnings from folks 
(incorrectly) using posix_types.h instead of select.h after the change 
in that BZ was made.  That's why I originally stated that, arguably, 
posix_types.h really should go away or just use the definitions provided 
by glibc.


Jeff

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:19 PM, Jeff Law <law@redhat.com> wrote:
>
> All that's strictly necessary is that cast to (int).  That avoids the
> problem.

.. and it causes other problems instead, namely the crap code generation for __.

Apparently glibc fixed it totally differently, and the kernel actually
doesn't care at all. We'd probably be best off just removing those
#defines entirely. Especially since the kernel doesn't even *use*
those things.

The kernel _does_ have these odd #define's in <linux/time.h>:

  #define NFDBITS                 __NFDBITS

  #define FD_SETSIZE              __FD_SETSIZE
  #define FD_SET(fd,fdsetp)       __FD_SET(fd,fdsetp)
  #define FD_CLR(fd,fdsetp)       __FD_CLR(fd,fdsetp)
  #define FD_ISSET(fd,fdsetp)     __FD_ISSET(fd,fdsetp)
  #define FD_ZERO(fdsetp)         __FD_ZERO(fdsetp)

but apart from __NFDBITS and __FD_SETSIZE, the kernel doesn't even
seem to define those __FD_xyx macros at all (although possibly they
are hiding in some odd auto-generated headers, I didn't check). I
think this is all silly left-overs that nobody really wants any more.
glibc clearly doesn't.

             Linus

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> .. and it causes other problems instead, namely the crap code generation for __.

oops. Edit error. That __ should be __FDELT/__FDMASK.

But note that those are only the internal kernel macros that aren't
even *used* by the kernel. They are some left-over from olden days,
and seem to only cause odd interactions with glibc.

                 Linus

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Josh Boyer]

On Tue, Jul 24, 2012 at 12:37:36PM -0700, Linus Torvalds wrote:
> On Tue, Jul 24, 2012 at 12:19 PM, Jeff Law <law@redhat.com> wrote:
> >
> > All that's strictly necessary is that cast to (int).  That avoids the
> > problem.
> 
> .. and it causes other problems instead, namely the crap code generation for __.
> 
> Apparently glibc fixed it totally differently, and the kernel actually
> doesn't care at all. We'd probably be best off just removing those
> #defines entirely. Especially since the kernel doesn't even *use*
> those things.
> 
> The kernel _does_ have these odd #define's in <linux/time.h>:
> 
>   #define NFDBITS                 __NFDBITS
> 
>   #define FD_SETSIZE              __FD_SETSIZE
>   #define FD_SET(fd,fdsetp)       __FD_SET(fd,fdsetp)
>   #define FD_CLR(fd,fdsetp)       __FD_CLR(fd,fdsetp)
>   #define FD_ISSET(fd,fdsetp)     __FD_ISSET(fd,fdsetp)
>   #define FD_ZERO(fdsetp)         __FD_ZERO(fdsetp)
> 
> but apart from __NFDBITS and __FD_SETSIZE, the kernel doesn't even
> seem to define those __FD_xyx macros at all (although possibly they
> are hiding in some odd auto-generated headers, I didn't check). I
> think this is all silly left-overs that nobody really wants any more.
> glibc clearly doesn't.

I'd be happy to come up with a patch that drops them, but since they're
in a user visible header file I was concerned somebody might be using
them explicitly from posix_types.h.  People do weird crap like not use
glibc all the time.

josh

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Josh Boyer]

On Tue, Jul 24, 2012 at 01:26:39PM -0600, Jeff Law wrote:
> On 07/24/12 13:24, Linus Torvalds wrote:
> >On Tue, Jul 24, 2012 at 12:15 PM, Jeff Law <law@redhat.com> wrote:
> >>
> >>Please refer to the original discussion where they did evaluate the cost of
> >>this change and tested that the final change made no difference to the
> >>generated code.
> >
> >Umm. That bugzilla entry seems to be talking about a *sane* change, namely
> >
> >-  ({ unsigned long int __d = (d);					    \
> >+  ({ unsigned long int __d = (unsigned long int) (d);			    \
> >
> >in __FD_ELT(), which is totally different from the one Josh talks about.

So glibc has multiple definitions of __FD_ELT.  I originally quoted the
one from misc/sys/select.h, but the one from the first patch in the
glibc bugzilla entry is patching misc/bits/select2.h.  I'm going to
guess that through some kind of implies or header chain, the second is used.

However, the actually commit for that glibc bug is ceb9e56b3d1f8 and
that actually doesn't keep (unsigned long) here.  It does this:

diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
index 5e06f8f..ded3f2f 100644
--- a/debug/fdelt_chk.c
+++ b/debug/fdelt_chk.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2011 Free Software Foundation, Inc.
+/* Copyright (C) 2011, 2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -18,10 +18,10 @@
 #include <sys/select.h>
 
 
-unsigned long int
-__fdelt_chk (unsigned long int d)
+long int
+__fdelt_chk (long int d)
 {
-  if (d >= FD_SETSIZE)
+  if (d < 0 || d >= FD_SETSIZE)
     __chk_fail ();
 
   return d / __NFDBITS;
diff --git a/misc/bits/select2.h b/misc/bits/select2.h
index 9679925..76ae368 100644
--- a/misc/bits/select2.h
+++ b/misc/bits/select2.h
@@ -1,5 +1,5 @@
 /* Checking macros for select functions.
-   Copyright (C) 2011 Free Software Foundation, Inc.
+   Copyright (C) 2011, 2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -21,14 +21,15 @@
 #endif
 
 /* Helper functions to issue warnings and errors when needed.  */
-extern unsigned long int __fdelt_chk (unsigned long int __d);
-extern unsigned long int __fdelt_warn (unsigned long int __d)
+extern long int __fdelt_chk (long int __d);
+extern long int __fdelt_warn (long int __d)
   __warnattr ("bit outside of fd_set selected");
 #undef __FD_ELT
 #define	__FD_ELT(d) \
   __extension__								    \
-  ({ unsigned long int __d = (d);					    \
+  ({ long int __d = (d);						    \
      (__builtin_constant_p (__d)					    \
-      ? (__d >= __FD_SETSIZE						    \
-	 ? __fdelt_warn (__d) : (__d / __NFDBITS))			    \
+      ? (0 <= __d && __d < __FD_SETSIZE					    \
+	 ? (__d / __NFDBITS)						    \
+	 : __fdelt_warn (__d))						    \
       : __fdelt_chk (__d)); })


> Right.  Josh's change is necessary to prevent warnings from folks
> (incorrectly) using posix_types.h instead of select.h after the
> change in that BZ was made.  That's why I originally stated that,
> arguably, posix_types.h really should go away or just use the
> definitions provided by glibc.

The warnings being specifically:

[root@localhost ~]# cat foo.c 
#include <sys/select.h>
#include <linux/types.h>

int foo(void)
{
  fd_set fds;
  FD_ZERO(&fds);
  FD_SET(0, &fds);
  return FD_ISSET(0, &fds);
}

[root@localhost ~]# gcc -Wextra -Werror -O2 -D_FORTIFY_SOURCE=2 -c foo.c -save-temps
foo.c: In function ‘foo’:
foo.c:8:162: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
foo.c:8:184: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
foo.c:9:162: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
foo.c:9:184: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
cc1: all warnings being treated as errors

As I said in the commit log, -Wsign-compare and -D_FORTIFY_SOURCE=2 are
required.  Now you have as much info as I do on this particular awesome
issue.

josh

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:26 PM, Jeff Law <law@redhat.com> wrote:
>
> Right.  Josh's change is necessary to prevent warnings from folks
> (incorrectly) using posix_types.h instead of select.h after the change in
> that BZ was made.  That's why I originally stated that, arguably,
> posix_types.h really should go away or just use the definitions provided by
> glibc.

I think we should likely keep __FD_SETSIZE, since that really is a
valid kernel value (that the kernel actually uses). The rest looks
*entirely* bogus.

And the reason I emphasize the "entirely" is literally that the kernel
headers don't even define the full __FD_SET/CLR/ISSET() functionality.
We never use those inside the kernel itself, and we don't even
*define* them. So it's not even useful to some non-glibc user, afaik,
because the kernel only exports those really *odd* internal names. It
really looks entirely like some remnant from long long long ago when
the kernel headers actually used to have the whole functionality, but
then the functionality was removed, and now only some building blocks
are left - but not enough to really do anything useful with.

Just enough to be annoying.

                  Linus

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:43 PM, Josh Boyer <jwboyer@redhat.com> wrote:
>+  ({ long int __d = (d);                                                   \
>      (__builtin_constant_p (__d)                                           \
>-      ? (__d >= __FD_SETSIZE                                               \
>-        ? __fdelt_warn (__d) : (__d / __NFDBITS))                          \
>+      ? (0 <= __d && __d < __FD_SETSIZE                                            \
>+        ? (__d / __NFDBITS)                                                \
>+        : __fdelt_warn (__d))                                              \
>       : __fdelt_chk (__d)); })

Ugh. This depends intimately on gcc doing the whole value range
propagation thing, and probably generates horrible code when not
optimizing, but whatever. It's irrelevant.

I vote we get rid of the crap. We'll force-redefine __FD_SETSIZE,
because that's the one we really care about. And then just use our
internal names for anything else.

IOW, just something like this (whitespace-damaged on purpose, because
it won't even compile as-is: we'll also need to replace the few uses
of __NFDBITS in the kernel with BITS_PER_LONG).

   diff --git a/include/linux/posix_types.h b/include/linux/posix_types.h
  index f04c98cf44f3..8a79a5021dc1 100644
  --- a/include/linux/posix_types.h
  +++ b/include/linux/posix_types.h
  @@ -15,26 +15,14 @@
    */

   /*
  - * Those macros may have been defined in <gnu/types.h>. But we always
  - * use the ones here.
  + * This macro may have been defined in <gnu/types.h>. But we always
  + * use the one here.
    */
  -#undef __NFDBITS
  -#define __NFDBITS	(8 * sizeof(unsigned long))
  -
   #undef __FD_SETSIZE
   #define __FD_SETSIZE	1024

  -#undef __FDSET_LONGS
  -#define __FDSET_LONGS	(__FD_SETSIZE/__NFDBITS)
  -
  -#undef __FDELT
  -#define	__FDELT(d)	((d) / __NFDBITS)
  -
  -#undef __FDMASK
  -#define	__FDMASK(d)	(1UL << ((d) % __NFDBITS))
  -
   typedef struct {
  -	unsigned long fds_bits [__FDSET_LONGS];
  +	unsigned long fds_bits [__FD_SETSIZE / (8*sizeof(long))];
   } __kernel_fd_set;

   /* Type of a signal handler.  */

Hmm?

              Linus

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:41 PM, Josh Boyer <jwboyer@redhat.com> wrote:
>
> I'd be happy to come up with a patch that drops them, but since they're
> in a user visible header file I was concerned somebody might be using
> them explicitly from posix_types.h.  People do weird crap like not use
> glibc all the time.

Yeah, I agree that somebody could possibly use them.

But the odd thing is, we don't actually export anything *useful*. It's
not like we export the define for __FD_ISSET() etc, which would be
something somebody really wants. No, that kernel header only exports
the (unused by the kernel) building blocks for creating __FD_ISSET().

And I suspect (but it must be before even the old bitkeeper tree) that
we *used* to implement __FD_ISSET() long ago. We removed it, and
nobody used it, so nobody even noticed that we removed it - but left
some now unused stuff behind.

                      Linus

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Jeff Law]

On 07/24/12 13:43, Linus Torvalds wrote:
>
> I think we should likely keep __FD_SETSIZE, since that really is a
> valid kernel value (that the kernel actually uses). The rest looks
> *entirely* bogus.
I can certainly live with that.

Josh, looks like you've got marching orders :-)

Of course there'll be some kind of fallout, there always is.



> And the reason I emphasize the "entirely" is literally that the kernel
> headers don't even define the full __FD_SET/CLR/ISSET() functionality.
I know.  When this first came to my attention I feared I'd find another 
implementation of FD_{SET,CLR}/ISSET.

Jeff

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Andreas Schwab]

Josh Boyer <jwboyer@redhat.com> writes:

> So glibc has multiple definitions of __FD_ELT.  I originally quoted the
> one from misc/sys/select.h, but the one from the first patch in the
> glibc bugzilla entry is patching misc/bits/select2.h.

See also eb0b6cb.

> I'm going to
> guess that through some kind of implies or header chain, the second is used.

Look at the end of misc/sys/select.h.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Josh Boyer]

On Tue, Jul 24, 2012 at 12:55:38PM -0700, Linus Torvalds wrote:
> On Tue, Jul 24, 2012 at 12:43 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> >+  ({ long int __d = (d);                                                   \
> >      (__builtin_constant_p (__d)                                           \
> >-      ? (__d >= __FD_SETSIZE                                               \
> >-        ? __fdelt_warn (__d) : (__d / __NFDBITS))                          \
> >+      ? (0 <= __d && __d < __FD_SETSIZE                                            \
> >+        ? (__d / __NFDBITS)                                                \
> >+        : __fdelt_warn (__d))                                              \
> >       : __fdelt_chk (__d)); })
> 
> Ugh. This depends intimately on gcc doing the whole value range
> propagation thing, and probably generates horrible code when not
> optimizing, but whatever. It's irrelevant.
> 
> I vote we get rid of the crap. We'll force-redefine __FD_SETSIZE,
> because that's the one we really care about. And then just use our
> internal names for anything else.

Your vote counts more than mine ;).  I just want the problem solved in a
reasonable fasion since it's causing build failures now and the bug was
stalled.
 
> IOW, just something like this (whitespace-damaged on purpose, because
> it won't even compile as-is: we'll also need to replace the few uses
> of __NFDBITS in the kernel with BITS_PER_LONG).
> 
>    diff --git a/include/linux/posix_types.h b/include/linux/posix_types.h
>   index f04c98cf44f3..8a79a5021dc1 100644
>   --- a/include/linux/posix_types.h
>   +++ b/include/linux/posix_types.h
>   @@ -15,26 +15,14 @@
>     */
> 
>    /*
>   - * Those macros may have been defined in <gnu/types.h>. But we always
>   - * use the ones here.
>   + * This macro may have been defined in <gnu/types.h>. But we always
>   + * use the one here.
>     */
>   -#undef __NFDBITS
>   -#define __NFDBITS	(8 * sizeof(unsigned long))
>   -
>    #undef __FD_SETSIZE
>    #define __FD_SETSIZE	1024
> 
>   -#undef __FDSET_LONGS
>   -#define __FDSET_LONGS	(__FD_SETSIZE/__NFDBITS)
>   -
>   -#undef __FDELT
>   -#define	__FDELT(d)	((d) / __NFDBITS)
>   -
>   -#undef __FDMASK
>   -#define	__FDMASK(d)	(1UL << ((d) % __NFDBITS))
>   -
>    typedef struct {
>   -	unsigned long fds_bits [__FDSET_LONGS];
>   +	unsigned long fds_bits [__FD_SETSIZE / (8*sizeof(long))];
>    } __kernel_fd_set;
> 
>    /* Type of a signal handler.  */
> 
> Hmm?

Seems fine to me.  In addition to the s/__NFDBITS/BITS_PER_LONG change,
I'm guessing you'll want the odd FD_ISSET/CLR/ZERO/etc macros in
linux/time.h killed as well?

If you don't get to it before me, I'll try whipping something up either
later this evening or first thing in the morning.

josh

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 12:59 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And I suspect (but it must be before even the old bitkeeper tree) that
> we *used* to implement __FD_ISSET() long ago. We removed it, and
> nobody used it, so nobody even noticed that we removed it - but left
> some now unused stuff behind.

Nope, I found it. It was in the arch-specific header files, and then
moved to asm-generic, and then removed entirely by commit 8b3d1cda4f5f
("posix_types: Remove fd_set macros"). Fairly recently, in fact
(February).

But even before they were removed, they were inside #ifdef __KERNEL__
protection, so it wasn't available to user code.

And to be *really* crazy, it turns out that by the time we removed
those __FD_ISSET() etc macros, they didn't even use the helper macros
any more. So instead of using

 #define __FD_ISSET(d, set) ((set)->fds_bits[__FDELT(d)] & __FDMASK(d))

(like the original arch-specific ones had done), we had already
implicitly stopped using those macros inside the kernel when we moved
to the generic version (that did all this by hand in inline
functions).

So the only users of __FDELT/__FDMASK seem to have gone away many
moons ago. I really don't think that any sane user space could be
using those left-over helpers.

Of course, the magic word there is "sane". I'm sure there are insane
users somewhere. But I don't think we should care.

           Linus

Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

[Linus Torvalds]

On Tue, Jul 24, 2012 at 1:10 PM, Josh Boyer <jwboyer@redhat.com> wrote:
>
> Seems fine to me.  In addition to the s/__NFDBITS/BITS_PER_LONG change,
> I'm guessing you'll want the odd FD_ISSET/CLR/ZERO/etc macros in
> linux/time.h killed as well?

Yes. They don't make sense, since the kernel doesn't even *define*
those underscore versions.

> If you don't get to it before me, I'll try whipping something up either
> later this evening or first thing in the morning.

Being the merge window, I already have spent more time than I should
looking at kernel history of just *why* we do that odd thing. I'll go
back to merging Xen stuff and let you play with this..

               Linus

Re: [PATCH] posix_types.h: make __NFDBITS match glibc definition

[Andreas Schwab]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> But even before they were removed, they were inside #ifdef __KERNEL__
> protection, so it wasn't available to user code.

They used to be exported for __GLIBC__ < 2, but that was removed in
2008.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."