Re: [PATCH] 2.5 fast poll on ppc64

Re: [PATCH] 2.5 fast poll on ppc64

[Mikael Pettersson]

On Thu, 26 Dec 2002 14:53:40 +0100, Manfred Spraul wrote:
>Mikael Pettersson wrote:
>
>>Anton Blanchard writes:
>> > I was unable to boot 2.5-BK on ppc64 and narrowed it down to the fast
>> > poll patch.  I found:
>> > 
>> > offsetof(struct poll_list, entries) == 12 but
>> > sizeof(struct poll_list) == 16
...
>>To me (I'm a compiler writer) it looks like your compiler did NOT
>>mess up. Assuming struct pollfd has 32-bit alignment, the compiler
>>is doing the right thing by starting entries[] at offset 12.
>>The 16-byte size for struct poll_list is because the 'next' field
>>has 64-bit alignment, which forces the compiler to pad struct
>>poll_lists's size to a multiple of 8 bytes, i.e. 16 bytes in this
>>case. So the compiler is not broken.
>>  
>>
>I would agree, but there is a special restriction on offsetof() and 
>sizeof() of structures with flexible array members: section 6.7.2.1, 
>clause 16:
>
>First, the size of the structure shall be equal to the offset of the last element of an otherwise identical structure that replaces the flexible array member with an array of unspecified length.
>
>The simplest test case I've found is
>
>struct a1 { int a; char b; short c[];};
>struct a2 { int a; char b; short c[1];};
>
>    sizeof(struct a{1,2}) is 8.
>    offsetof(struct a{1,2}, c) is 6.
>
>--> sizeof(struct a1) != offsetof(struct a2, c)

Oh dear. I checked my C9x draft copy and you seem to be right.
The standard states that sizeof(struct a1) == offsetof(struct a1, c),
but both gcc (2.95.3 and 3.2) and Intel's icc (7.0) get it wrong on x86:
they make sizeof(struct a1) == 8 but offsetof(struct a1, c) == 6.

Technically speaking, the kernel code which uses 'entries[0]' is
non-compliant since the proper syntax is 'entries[]', but the empty
array size syntax isn't implemented in gcc 2.95.3.

>>But the old code which assumed pp+1 == pp->entries is so horribly
>>broken I can't find words for it. s/pp+1/pp->entries/ is the correct fix.

My mistake. The old code is Ok for C99, but broken for ANSI-C.

>I agree. Should we fix the kmalloc allocations, too?
>
>-	pp = kmalloc(sizeof(struct poll_list)+
>+	pp = kmalloc(offsetof(struct poll_list,entries)+
>			sizeof(struct pollfd)*
>			(i>POLLFD_PER_PAGE?POLLFD_PER_PAGE:i),
>				GFP_KERNEL);

Yes, this should be changed as you suggest. The old code only
works in C99-compliant implementations, but we now know that both
gcc and icc get this wrong, so it seems prudent to revert to
the classical formulation, using the 'entries[0]' declaration
syntax and offsetof() instead of sizeof().

/Mikael

Re: [PATCH] 2.5 fast poll on ppc64

[Linus Torvalds]

On Thu, 26 Dec 2002, Mikael Pettersson wrote:
> 
> Technically speaking, the kernel code which uses 'entries[0]' is
> non-compliant since the proper syntax is 'entries[]', but the empty
> array size syntax isn't implemented in gcc 2.95.3.

The two things have two totally different semantics:

 - array[0] is a zero-sized array, and is a long-time gcc extension that 
   has nothing to do with the modern "flexible array". The kernel uses 
   zero-sized arrays, because flexible arrays simply aren't historically
   supported by gcc at all.

 - array[] is the standard "flexible array" thing, and has different rules 
   than array[0]. For example, sizeof() is undefined on flexible arrays, 
   but is well-defined on zero-sized ones (at zero). Also, the alignment 
   constraints are potentially quite different.

The gcc people want to make the two behave fairly similarly to ease 
implementation issues, but they are definitely not the same.

> My mistake. The old code is Ok for C99, but broken for ANSI-C.

The old code is ugly and arguably always broken, I agree 100% with making 
the usage code use "pp->entries". Whether the allocators use "sizeof" or 
"offsetof" is secondary, at worst it ends up being conservative.

		Linus

Re: [PATCH] 2.5 fast poll on ppc64

[Manfred Spraul]

Mikael Pettersson wrote:

>>struct a1 { int a; char b; short c[];};
>>struct a2 { int a; char b; short c[1];};
>>
>>   sizeof(struct a{1,2}) is 8.
>>   offsetof(struct a{1,2}, c) is 6.
>>
>>--> sizeof(struct a1) != offsetof(struct a2, c)
>>    
>>
>
>Oh dear. I checked my C9x draft copy and you seem to be right.
>The standard states that sizeof(struct a1) == offsetof(struct a1, c),
>but both gcc (2.95.3 and 3.2) and Intel's icc (7.0) get it wrong on x86:
>they make sizeof(struct a1) == 8 but offsetof(struct a1, c) == 6.
>  
>
I've filed a gcc bug, no 9058: As the reply, I got a mail from Joseph 
Myers with links to TC to the C99 standard:
http://std.dkuug.dk/JTC1/SC22/WG14/www/docs/n983.htm
http://std.dkuug.dk/JTC1/SC22/WG14/www/docs/n987.htm

>  
>
>>I agree. Should we fix the kmalloc allocations, too?
>>
>>-	pp = kmalloc(sizeof(struct poll_list)+
>>+	pp = kmalloc(offsetof(struct poll_list,entries)+
>>			sizeof(struct pollfd)*
>>			(i>POLLFD_PER_PAGE?POLLFD_PER_PAGE:i),
>>				GFP_KERNEL);
>>    
>>
>
>Yes, this should be changed as you suggest. The old code only
>works in C99-compliant implementations, but we now know that both
>gcc and icc get this wrong, so it seems prudent to revert to
>the classical formulation, using the 'entries[0]' declaration
>syntax and offsetof() instead of sizeof().
>  
>
I found an even simpler formula:

    offsetof(struct poll_list,entries[i>POLLFD_PER_PAGE?POLLFD_PER_PAGE:i];

I'll ask the gcc guys if that formula is a portable solution.

--
    Manfred

Re: [PATCH] 2.5 fast poll on ppc64

[Richard Henderson]

On Thu, Dec 26, 2002 at 09:55:15PM +0100, Mikael Pettersson wrote:
> >I would agree, but there is a special restriction on offsetof() and 
> >sizeof() of structures with flexible array members: section 6.7.2.1, 
> >clause 16:
> >
> >First, the size of the structure shall be equal to the offset of the
> >last element of an otherwise identical structure that replaces the
> >flexible array member with an array of unspecified length.
[...]
> Oh dear. I checked my C9x draft copy and you seem to be right.
> The standard states that sizeof(struct a1) == offsetof(struct a1, c),
> but both gcc (2.95.3 and 3.2) and Intel's icc (7.0) get it wrong on x86:
> they make sizeof(struct a1) == 8 but offsetof(struct a1, c) == 6.

Indeed, *every* compiler that supports flexible array members
does it this way.  On the gcc lists we've conjectured that this
will wind up being a Defect Report, and so have elected not to
change anything for the nonce.

That said, you should also note that "struct S foo[0]" is *not*
a flexible array member; "struct S foo[]" is.  The former is
gcc's zero-length array extension.  There are subtle differences
between the two features.  See the gcc docs for details.


r~

Re: [PATCH] 2.5 fast poll on ppc64

[Mikael Pettersson]

Anton Blanchard writes:
 > I was unable to boot 2.5-BK on ppc64 and narrowed it down to the fast
 > poll patch.  I found:
 > 
 > offsetof(struct poll_list, entries) == 12 but
 > sizeof(struct poll_list) == 16
 > 
 > This means pp+1 did not match up with pp->entries. Im not sure what the
 > alignment requirements are for a zero length struct (ie is this a
 > compiler bug) but the following patch fixes the problem and also changes
 > ->len to a long to ensure 8 byte alignment of ->entries on 64bit archs.
 > 
 > Anton
 > 
 > ===== fs/select.c 1.15 vs edited =====
 > --- 1.15/fs/select.c	Sat Dec 21 20:42:41 2002
 > +++ edited/fs/select.c	Thu Dec 26 17:31:16 2002
 > @@ -362,7 +362,7 @@
 >  
 >  struct poll_list {
 >  	struct poll_list *next;
 > -	int len;
 > +	long len;
 >  	struct pollfd entries[0];
 >  };

To me (I'm a compiler writer) it looks like your compiler did NOT
mess up. Assuming struct pollfd has 32-bit alignment, the compiler
is doing the right thing by starting entries[] at offset 12.
The 16-byte size for struct poll_list is because the 'next' field
has 64-bit alignment, which forces the compiler to pad struct
poll_lists's size to a multiple of 8 bytes, i.e. 16 bytes in this
case. So the compiler is not broken.

 >  
 > @@ -471,7 +471,7 @@
 >  			walk->next = pp;
 >  
 >  		walk = pp;
 > -		if (copy_from_user(pp+1, ufds + nfds-i, 
 > +		if (copy_from_user(pp->entries, ufds + nfds-i, 
 >  				sizeof(struct pollfd)*pp->len)) {

But the old code which assumed pp+1 == pp->entries is so horribly
broken I can't find words for it. s/pp+1/pp->entries/ is the correct fix.

/Mikael

Re: [PATCH] 2.5 fast poll on ppc64

[Manfred Spraul]

Mikael Pettersson wrote:

>Anton Blanchard writes:
> > I was unable to boot 2.5-BK on ppc64 and narrowed it down to the fast
> > poll patch.  I found:
> > 
> > offsetof(struct poll_list, entries) == 12 but
> > sizeof(struct poll_list) == 16
> > 
> > This means pp+1 did not match up with pp->entries. Im not sure what the
> > alignment requirements are for a zero length struct (ie is this a
> > compiler bug) but the following patch fixes the problem and also changes
> > ->len to a long to ensure 8 byte alignment of ->entries on 64bit archs.
> > 
> > Anton
> > 
> > ===== fs/select.c 1.15 vs edited =====
> > --- 1.15/fs/select.c	Sat Dec 21 20:42:41 2002
> > +++ edited/fs/select.c	Thu Dec 26 17:31:16 2002
> > @@ -362,7 +362,7 @@
> >  
> >  struct poll_list {
> >  	struct poll_list *next;
> > -	int len;
> > +	long len;
> >  	struct pollfd entries[0];
> >  };
>
>To me (I'm a compiler writer) it looks like your compiler did NOT
>mess up. Assuming struct pollfd has 32-bit alignment, the compiler
>is doing the right thing by starting entries[] at offset 12.
>The 16-byte size for struct poll_list is because the 'next' field
>has 64-bit alignment, which forces the compiler to pad struct
>poll_lists's size to a multiple of 8 bytes, i.e. 16 bytes in this
>case. So the compiler is not broken.
>  
>
I would agree, but there is a special restriction on offsetof() and 
sizeof() of structures with flexible array members: section 6.7.2.1, 
clause 16:

First, the size of the structure shall be equal to the offset of the last element of an otherwise identical structure that replaces the flexible array member with an array of unspecified length.

The simplest test case I've found is

struct a1 { int a; char b; short c[];};
struct a2 { int a; char b; short c[1];};

    sizeof(struct a{1,2}) is 8.
    offsetof(struct a{1,2}, c) is 6.

--> sizeof(struct a1) != offsetof(struct a2, c)

> >  
> > @@ -471,7 +471,7 @@
> >  			walk->next = pp;
> >  
> >  		walk = pp;
> > -		if (copy_from_user(pp+1, ufds + nfds-i, 
> > +		if (copy_from_user(pp->entries, ufds + nfds-i, 
> >  				sizeof(struct pollfd)*pp->len)) {
>
>But the old code which assumed pp+1 == pp->entries is so horribly
>broken I can't find words for it. s/pp+1/pp->entries/ is the correct fix.
>  
>
I agree. Should we fix the kmalloc allocations, too?

-	pp = kmalloc(sizeof(struct poll_list)+
+	pp = kmalloc(offsetof(struct poll_list,entries)+
			sizeof(struct pollfd)*
			(i>POLLFD_PER_PAGE?POLLFD_PER_PAGE:i),
				GFP_KERNEL);


--
    Manfred

[PATCH] 2.5 fast poll on ppc64

[Anton Blanchard]

Hi,

I was unable to boot 2.5-BK on ppc64 and narrowed it down to the fast
poll patch.  I found:

offsetof(struct poll_list, entries) == 12 but
sizeof(struct poll_list) == 16

This means pp+1 did not match up with pp->entries. Im not sure what the
alignment requirements are for a zero length struct (ie is this a
compiler bug) but the following patch fixes the problem and also changes
->len to a long to ensure 8 byte alignment of ->entries on 64bit archs.

Anton

===== fs/select.c 1.15 vs edited =====
--- 1.15/fs/select.c	Sat Dec 21 20:42:41 2002
+++ edited/fs/select.c	Thu Dec 26 17:31:16 2002
@@ -362,7 +362,7 @@
 
 struct poll_list {
 	struct poll_list *next;
-	int len;
+	long len;
 	struct pollfd entries[0];
 };
 
@@ -471,7 +471,7 @@
 			walk->next = pp;
 
 		walk = pp;
-		if (copy_from_user(pp+1, ufds + nfds-i, 
+		if (copy_from_user(pp->entries, ufds + nfds-i, 
 				sizeof(struct pollfd)*pp->len)) {
 			err = -EFAULT;
 			goto out_fds;

Re: [PATCH] 2.5 fast poll on ppc64

[Manfred Spraul]

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

Anton Blanchard wrote:

>Hi,
>
>I was unable to boot 2.5-BK on ppc64 and narrowed it down to the fast
>poll patch.  I found:
>
>offsetof(struct poll_list, entries) == 12 but
>sizeof(struct poll_list) == 16
>
>This means pp+1 did not match up with pp->entries. Im not sure what the
>alignment requirements are for a zero length struct (ie is this a
>compiler bug) but the following patch fixes the problem and also changes
>->len to a long to ensure 8 byte alignment of ->entries on 64bit archs.
>  
>
That seems to be the root of the problem: ->entries requires 4 byte 
alignment, while struct poll_list requires 8 byte alignment. Thus the 
compiler aligns entries to a 4-byte boundary, and rounds up 
sizeof(struct poll_list) to 16.

Attached is a small test case that shows the problem on both 64-bit and 
32-bit platforms. I'm not sure if this is a gcc bug: the Compaq C 
compiler on Tru64 and MSVC on Windows generate the same structure layout.

--
    Manfred

[-- Attachment #2: test.c --]
[-- Type: text/plain, Size: 1524 bytes --]

/*
 * Testcase for a possible compiler bug:
 * structure with flexible array member.
 * offsetof(struct,flexible_member) != sizeof(struct).
 *
 * That seems to be a violation of 6.7.2.1, constraint 16 of the C99
 * standard:
 * "First, the size of the structure shall be equal to the offset of the
 * last element of an otherwise identical structure that replaces the
 * flexible array member with an array of unspecified length.
 */
#include <stdio.h>

/*
 * structure: sizeof() = 4.
 * alignment: 2, due to the short.
 */
struct pollfd {
	short fd;
	char events;
	char revents;
};

/*
 * sizeof: 12
 * alignment: 4, due to the int.
 */
struct n2 {
	/* offset 0 */
	int m;
	/* offset 4 */
	char n;
	/* one byte padding, 'struct pollfd' requires 2 byte alignment */
	/* offset 6 */
	struct pollfd a[1];
	/* offset 10: two bytes padding for 4 byte alignment,
	 * required for the int in this structure */
};

/*
 * sizeof: 8.
 * XXX
 * Is this correct?
 * offsetof(struct n1, a) is 6
 * --> offsetof(struct n1, a) != sizeof(struct n1)
 * XXX
 */
struct n1 {
	/* offset 0 */
	int m;
	/* offset 4 */
	char n;
	/* one byte padding, 'struct pollfd' required 2 byte alignment */
	/* offset 6 */
	struct pollfd a[];
};
	
#define offsetof(a,b) \
		(int)&((a*)NULL)->b

int main(void)
{
	printf("pollfd: sizeof: %d.\n", sizeof(struct pollfd));
	printf("fix: sizeof: %d, offsetof: %d.\n", sizeof(struct n2), offsetof(struct n2, a));
	printf("var: sizeof: %d, offsetof: %d.\n", sizeof(struct n1), offsetof(struct n1, a));
	return 0;
}