need for packed attribute

need for packed attribute

[Oliver Neukum]

Hi,

is there any architecture for which packed is required in structures like this:

/* All standard descriptors have these 2 fields at the beginning */
struct usb_descriptor_header {
	__u8  bLength;
	__u8  bDescriptorType;
};

	Regards
		Oliver

Re: need for packed attribute

[Russell King]

On Fri, Jan 06, 2006 at 07:15:43PM +0100, Oliver Neukum wrote:
> Hi,
> 
> is there any architecture for which packed is required in structures like this:
> 
> /* All standard descriptors have these 2 fields at the beginning */
> struct usb_descriptor_header {
> 	__u8  bLength;
> 	__u8  bDescriptorType;
> };

sizeof(struct usb_descriptor_header) will be 4 on ARM.  If this
concerns you, you need to pack the structure thusly:

struct usb_descriptor_header {
	__u8  bLength;
	__u8  bDescriptorType;
} __attribute__((packed));
	

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: need for packed attribute

[Benjamin LaHaise]

On Fri, Jan 06, 2006 at 06:38:46PM +0000, Russell King wrote:
> > /* All standard descriptors have these 2 fields at the beginning */
> > struct usb_descriptor_header {
> > 	__u8  bLength;
> > 	__u8  bDescriptorType;
> > };
> 
> sizeof(struct usb_descriptor_header) will be 4 on ARM.  If this
> concerns you, you need to pack the structure thusly:

Interesting.  Perhaps we should add -Wpadded to CFLAGS in order to remind 
people, although that might take a fair bit of work to clean up existing 
structure definitions.

		-ben
-- 
"You know, I've seen some crystals do some pretty trippy shit, man."
Don't Email: <dont@kvack.org>.

Re: need for packed attribute

[Pete Zaitcev]

On Thu, 12 Jan 2006 17:26:17 +0000, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> On Thu, Jan 12, 2006 at 09:20:06AM -0800, Pete Zaitcev wrote:

> > P.S. I am repeating myself as Katon, but I am yet to see why any of
> > this matters. Neither Russell nor Oliver ever presented a case where
> > an unpacked struct caused breakage in USB.
> 
> If you would like to refresh your memory (which is obviously faulty)
> you'll see that my involvement in this thread was merely to answer
> a simple question about structure sizes.

Isn't it exactly what I just wrote, and you quoted?

> It was not a bug report about USB breaking.  Therefore, I have no
> case to present.

The thread started with Oliver posting a patch. And later, he appealed
to your authority.

It seems that we both are saying that there's no problem, no case,
no nothing.

-- Pete

Re: need for packed attribute

[Russell King]

On Thu, Jan 12, 2006 at 09:20:06AM -0800, Pete Zaitcev wrote:
> P.S. I am repeating myself as Katon, but I am yet to see why any of
> this matters. Neither Russell nor Oliver ever presented a case where
> an unpacked struct caused breakage in USB.

If you would like to refresh your memory (which is obviously faulty)
you'll see that my involvement in this thread was merely to answer
a simple question about structure sizes.

It was not a bug report about USB breaking.  Therefore, I have no
case to present.

(And WTF do soo many people assume that I'm somehow always reporting
a bloody bug and have to present such cases when I get copied on
emails to answer questions?  I don't understand the stupid mentality
there - this is _not_ the first time this has happened in the 12 days
of 2006 so far.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: need for packed attribute

[Pete Zaitcev]

On Thu, 12 Jan 2006 13:27:12 +0100 (MET), Mikael Pettersson <mikpe@csd.uu.se> wrote:

> [...] Do you have any
> information about why gcc is doing this on ARM/Linux?

Russell forgot to explain it, but the reason for this weirdness is real.
It is so you can do things like this:

struct foo {
	char x, y;
};

struct bar {
	long g;
};

	char *p;
	struct bar *bp;
	p = kmalloc(sizeof(struct foo)+sizeof(struct bar));
	bp = p + sizeof(struct foo);

Notice that sizes are aligned even in sensible ABIs whenever you
have anything bigger than a char inside a struct, in order to let
arrays of structures work properly. As a side effect, the construct
above would be aligned whenever struct foo contained a long. So most
of the time we see the same result, ARM or not.

So this is not really a question of whatever some silly document
specifies, but what is workable in real life C programming.

Funnily enough, you are not safe depending on the ABI to make this
sort of padding (so our favourite alloc_netdev() and alloc_ieee80211
only work by accident with the trailing u8 priv[]). For example, on
sparc(32) the ABI alignes to 32 bits only, but the ldd instruction
traps if a 64-bit value is not aligned to 64 bits, so if the struct
bar in the above example had a long long, it would still trap.

Another funny thing about the above is that once you mark struct foo
packed, the example breaks. So, nobody should do use packed structs
in such constructs ... unless everything is packed. The pack attribute
has significant properties of cancer.

-- Pete

P.S. I am repeating myself as Katon, but I am yet to see why any of
this matters. Neither Russell nor Oliver ever presented a case where
an unpacked struct caused breakage in USB.

P.P.S. The USB stack was careful to use correct sizes historically.
One grep of the source will tell you that all this stench emanates from
the newer code, in particular the gadget and its attendant components,
such as usbtest. Guess who wrote it: same gentleman who advocated adding
((packed)) to _all_ structures "used to talk to hardware". He just has
no respect for coding practices, that's all. And some other gentleman,
otherwise highly respected for his sharp eye for races and locking
problems, is only too happy to copy-paste and to forward patches which
offer no justification.

Re: need for packed attribute

[Russell King]

On Thu, Jan 12, 2006 at 05:30:11PM +0100, Mikael Pettersson wrote:
> OK, thanks for this info. It means that GCC is the definitive authority
> on calling conventions and data layouts, not the AAPCS; I wasn't aware of
> that before.

BTW, it's worth noting that the new EABI stuff has it's own set of
problems.  We have r0 to r6 to pass 32-bit or 64-bit arguments.
With EABI, 64-bit arguments will be aligned to an _even_ numbered
register.  Hence:

long sys_foo(int a, long long b, int c, long long d);

will result in the following register layouts:

	EABI				Current
r0	a				a
r1	unused				\_ b
r2	\_ b				/
r3	/				c
r4	c				\_ d
r5					/
r6	... out of space for 'd'	... room for one other int.
r7	syscall number

and as such will be uncallable with this argument ordering for EABI.

We've already had to sanitise sys_fadvise64_64() because of this.

So, I forsee more problems appearing with EABI, not less. ;(

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: need for packed attribute

[Mikael Pettersson]

Russell King writes:
 > > As fas as I can tell, the AAPCS document (v2.03 7th Oct 2005) requires
 > > that a simple "struct foo { unsigned char c; };" should have both size
 > > and alignment equal to 1, but gcc makes them both 4. Do you have any
 > > information about why gcc is doing this on ARM/Linux? Is there an accurate
 > > ABI document for ARM/Linux somewhere?
 > 
 > That's the new EABI, which is a major change to the existing ABI which
 > the kernel and all of userspace is currently built using.
 > 
 > The old ABI has it's roots in 1993 when the kernel and userland was
 > initially built using an ANSI C compiler, and the work being done to
 > port GCC was to make it compliant with that version of the ABI.  This
 > ABI is documented only in dead-tree form.
 > 
 > Due to lack of manpower on the Linux side (iow, more or less just me)
 > this became the ABI of the early ARM Linux a.out toolchain.  At that
 > time, I did not consider this to be a problem - it wasn't a problem
 > as far as the kernel was concerned.
 > 
 > When ELF came along, other folk worked on the toolchain, but they stuck
 > with that ABI - you could not transition between the a.out ABI to the
 > ELF ABI without breaking the kernel - structure layouts would change.
 > 
 > Hence, this is the existing ABI we have.  Changing the padding or
 > alignment of structures changes the kernel ABI, making it incompatible
 > with current userland.

OK, thanks for this info. It means that GCC is the definitive authority
on calling conventions and data layouts, not the AAPCS; I wasn't aware of
that before.

(My interest in this issue comes from working on a port of a functional
programming language's JIT compiler and runtime system to XScale.)

/Mikael

Re: need for packed attribute

[Russell King]

On Thu, Jan 12, 2006 at 01:47:29PM +0000, Russell King wrote:
> Due to lack of manpower on the Linux side (iow, more or less just me)
> this became the ABI of the early ARM Linux a.out toolchain.  At that
> time, I did not consider this to be a problem - it wasn't a problem
> as far as the kernel was concerned.

Before someone takes that the wrong way - Richard Earnshaw worked on
porting binutils + gcc to the ARM architecture.  I worked on converting
that toolchain to work for ARM Linux - supporting the a.out shared
libraries and other Linux specific features.

Changing the ABI was, and still is completely outside my level of
knowledge of gcc.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: need for packed attribute

[Russell King]

On Thu, Jan 12, 2006 at 01:27:12PM +0100, Mikael Pettersson wrote:
> On Fri, 6 Jan 2006 18:38:46 +0000, Russell King wrote:
> >> is there any architecture for which packed is required in structures like this:
> >> 
> >> /* All standard descriptors have these 2 fields at the beginning */
> >> struct usb_descriptor_header {
> >> 	__u8  bLength;
> >> 	__u8  bDescriptorType;
> >> };
> >
> >sizeof(struct usb_descriptor_header) will be 4 on ARM.
> 
> I found this surprising, but gcc-3.4.5 for ARM seems to agree with you.
> 
> As fas as I can tell, the AAPCS document (v2.03 7th Oct 2005) requires
> that a simple "struct foo { unsigned char c; };" should have both size
> and alignment equal to 1, but gcc makes them both 4. Do you have any
> information about why gcc is doing this on ARM/Linux? Is there an accurate
> ABI document for ARM/Linux somewhere?

That's the new EABI, which is a major change to the existing ABI which
the kernel and all of userspace is currently built using.

The old ABI has it's roots in 1993 when the kernel and userland was
initially built using an ANSI C compiler, and the work being done to
port GCC was to make it compliant with that version of the ABI.  This
ABI is documented only in dead-tree form.

Due to lack of manpower on the Linux side (iow, more or less just me)
this became the ABI of the early ARM Linux a.out toolchain.  At that
time, I did not consider this to be a problem - it wasn't a problem
as far as the kernel was concerned.

When ELF came along, other folk worked on the toolchain, but they stuck
with that ABI - you could not transition between the a.out ABI to the
ELF ABI without breaking the kernel - structure layouts would change.

Hence, this is the existing ABI we have.  Changing the padding or
alignment of structures changes the kernel ABI, making it incompatible
with current userland.

We're working on a set of patches to bring ARM Linux to be compatible
with the new EABI (which you've found above).  This involves adding a
compatibility layer to the kernel to convert structures between the
old layouts and the new layouts.

I'm sure you can appreciate the size of this problem given the issues
with running 32-bit apps on 64-bit machines - it's the same kind of
issue.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: need for packed attribute

[Mikael Pettersson]

On Fri, 6 Jan 2006 18:38:46 +0000, Russell King wrote:
>> is there any architecture for which packed is required in structures like this:
>> 
>> /* All standard descriptors have these 2 fields at the beginning */
>> struct usb_descriptor_header {
>> 	__u8  bLength;
>> 	__u8  bDescriptorType;
>> };
>
>sizeof(struct usb_descriptor_header) will be 4 on ARM.

I found this surprising, but gcc-3.4.5 for ARM seems to agree with you.

As fas as I can tell, the AAPCS document (v2.03 7th Oct 2005) requires
that a simple "struct foo { unsigned char c; };" should have both size
and alignment equal to 1, but gcc makes them both 4. Do you have any
information about why gcc is doing this on ARM/Linux? Is there an accurate
ABI document for ARM/Linux somewhere?

/Mikael