Re: Packing of structure fields and whole structs

Re: Packing of structure fields and whole structs

[Andrew Haley]

Phil Endecott writes:

 > I am trying to understand the subtleties of __attribute__((packed)).  I 
 > have some code that works on x86, where unaligned accesses work, but 
 > fails on ARM where they do not.
 > 
 > As far as I can see, if I declare a struct with the packed attribute 
 > applied to the whole struct, like this:
 > 
 > struct test {
 >    int a;
 >    int b;
 > } __attribute__((packed));
 > 
 > and then use it like this:
 > 
 > {
 >    char c;
 >    struct test t;
 > }
 > 
 > Then t will be packed next to c, and t.a and t.b will be unaligned.  
 > This is not what I want!
 > 
 > There are lots of examples in the Linux kernel headers (if you have a 
 > source tree handy, grep for them).  Here's one example picked at random:
 > 
 > #define FDDI_K_OUI_LEN	3
 > struct fddi_snap_hdr
 > 	{
 > 	__u8	dsap;					/* always 0xAA */
 > 	__u8	ssap;					/* always 0xAA */
 > 	__u8	ctrl;					/* always 0x03 */
 > 	__u8	oui[FDDI_K_OUI_LEN];	/* organizational universal id */
 > 	__be16	ethertype;				/* packet type ID field */
 > 	} __attribute__ ((packed));
 > 
 > As far as I can see, making this packed can only cause trouble.  If 
 > packed wasn't there there would still be no gaps between the fields - 
 > would there? - and variables of this type would be word-aligned, so the 
 > 16-bit field would be 16-bit aligned, and it would work on an ARM.  But 
 > by declaring it as packed then a variable of this type will be packed 
 > at any alignment (e.g. char c; struct fddi_snap_hdr x;) and the 16-bit 
 > field may be unaligned.
 > 
 > Am I completely misunderstanding all of this?

I've redirected this to gcc-help.  It's not appropriate for gcc, which
is about the development *of* gcc, not about using gcc.

gcc should always generate correct code for accessing unaligned data.
If in your case it isn't doing so, that's a bug.  Have you found that
is happening?

Andrew.

Re: Packing of structure fields and whole structs

[Phil Endecott]

Andrew Haley wrote:
> gcc should always generate correct code for accessing unaligned data.

but Paul Brook wrote:
> You can't reliably take the address of a member of a packed structure (gcc doesn't 
> have unaligned pointers).

Are you both right?  At first your comments look contradictory, but my 
slow brain is starting to understand now.

I think you're saying that I can declare something that is unaligned:

   char c;
   struct foo f;  // unaligned

and then operate on it locally:

   f.a++;

because gcc knows that f.a is unaligned and on a machine that doesn't 
do unaligned accesses it can generate appropriate byte-shuffling code.  
But if I take its address:

   func(&f.a);

it won't work, because func() assumes that the pointer it is passed is aligned.

Is this correct?  A quick test seems to indicate that this is indeed 
what happens.

But I don't get any error or warning when I do this.  Would you agree 
that an error (or at least a "big fat warning") would be appropriate at 
the point where I wrote &f.a?

Thanks for your help.

Regards,

Phil.

Re: Packing of structure fields and whole structs

[Andrew Haley]

Phil Endecott writes:
 > Andrew Haley wrote:
 > > gcc should always generate correct code for accessing unaligned data.
 > 
 > but Paul Brook wrote:
 > > You can't reliably take the address of a member of a packed
 > > structure (gcc doesn't have unaligned pointers).
 > 
 > Are you both right?  At first your comments look contradictory, but
 > my slow brain is starting to understand now.
 > 
 > I think you're saying that I can declare something that is
 > unaligned:
 > 
 >    char c;
 >    struct foo f;  // unaligned
 > 
 > and then operate on it locally:
 > 
 >    f.a++;
 > 
 > because gcc knows that f.a is unaligned and on a machine that
 > doesn't do unaligned accesses it can generate appropriate
 > byte-shuffling code.  But if I take its address:
 > 
 >    func(&f.a);
 > 
 > it won't work, because func() assumes that the pointer it is passed
 > is aligned.
 > 
 > Is this correct?  A quick test seems to indicate that this is indeed 
 > what happens.

Sounds right.  A member of a packed struct must be accessed in there,
where it is unaligned.  

 > But I don't get any error or warning when I do this.  Would you
 > agree that an error (or at least a "big fat warning") would be
 > appropriate at the point where I wrote &f.a?

Not necessarily.  It's perfectly appropriate to do something like:

  memcpy (&f.a, foo, sizeof f.a);

And we don't want to generate bazillions of warnings for that.

Andrew.

Re: Packing of structure fields and whole structs

[Phil Endecott]

Andrew Haley wrote:
> Phil Endecott writes:
>  >    char c;
>  >    struct foo f;  // unaligned
>  > 
>  >    func(&f.a);
>  > 
>  > it won't work, because func() assumes that the pointer it is passed
>  > is aligned.
>  > But I don't get any error or warning when I do this.  Would you
>  > agree that an error (or at least a "big fat warning") would be
>  > appropriate at the point where I wrote &f.a?
>
> Not necessarily.  It's perfectly appropriate to do something like:
>
>   memcpy (&f.a, foo, sizeof f.a);

Yes, but you can observe that memcpy takes a void* and not generate a 
warning in that case.

Re: Packing of structure fields and whole structs

[Paul Brook]

> I think you're saying that I can declare something that is unaligned:
>
>    char c;
>    struct foo f;  // unaligned
>
> and then operate on it locally:
>
>    f.a++;
>
> because gcc knows that f.a is unaligned and on a machine that doesn't
> do unaligned accesses it can generate appropriate byte-shuffling code.
> But if I take its address:
>
>    func(&f.a);
>
> it won't work, because func() assumes that the pointer it is passed is
> aligned. Is this correct?

Almost. Your conclusion is correct, the logic you use to get there is not 
quite right.

The important distinction is whether the access is done through the packed 
struct type. In the former case the compiler sees that we're accessing a 
member of a struct foo (it would still work if f were (struct foo *)). In the 
latter case func() doesn't know anything about struct foo, it only sees that 
type of the pointer, which it assumes is aligned.

This is similar to the aliasing exception for unions: If fields are accessed 
via the union type gcc knows they can alias. If they are accessed directly 
all bets are off.

Paul

Re: Packing of structure fields and whole structs

[Phil Endecott]

>> I think you're saying that I can declare something that is unaligned:
>>
>>    char c;
>>    struct foo f;  // unaligned
>>
>> and then operate on it locally:
>>
>>    f.a++;
>>
>> because gcc knows that f.a is unaligned and on a machine that doesn't
>> do unaligned accesses it can generate appropriate byte-shuffling code.
>> But if I take its address:
>>
>>    func(&f.a);
>>
>> it won't work, because func() assumes that the pointer it is passed is
>> aligned. Is this correct?

The Linux people have read this in the gcc docs:

   Specifying this attribute [packed] for `struct' and `union' types is equivalent
   to specifying the `packed' attribute on each of the structure or 
union members.

I think this is almost, but not quite, true - specifying the attribute 
on the struct itself also affects packing when the struct is used, not 
just the packing of the fields themselves.

Do people agree that this bit of the docs needs tweaking?

Phil.

Re: Packing of structure fields and whole structs

[Paul Brook]

> The Linux people have read this in the gcc docs:
>
>    Specifying this attribute [packed] for `struct' and `union' types is
> equivalent to specifying the `packed' attribute on each of the structure or
> union members.
>
> I think this is almost, but not quite, true - specifying the attribute
> on the struct itself also affects packing when the struct is used, not
> just the packing of the fields themselves.

No it doesn't. The normal rules apply.
The alignment of a structure is generally the maximum alignment of its 
members. In a packed structure all fields have byte alignment, therefore the 
structure as a whole also has byte alignment.

IMHO this is the only definition that makes sense.

Paul

Re: Packing of structure fields and whole structs

[Phil Endecott]

Let me make this concrete again.  With this test program:

struct test {
   int a __attribute__((packed));
   int b __attribute__((packed));
};

char c = 1;
struct test t = { .a=2, .b=3 };

what can I assume about the alignment of t.a?  As I now understand it, 
t.a and t.b both have alignment of 1, so t has alignment of 1, so t can 
be packed immediately after c with no gap.  It is then unsafe (on some 
hardware) to take &t.a and pass it to a function that wants an int*.

What does gcc actually do?  Well on x86, it does indeed pack t 
immediately after c:

	.file	"test2.c"
globl c
	.data
	.type	c, @object
	.size	c, 1
c:
	.byte	1
globl t
	.type	t, @object
	.size	t, 8
t:
	.long	2
	.long	3
	.ident	"GCC: (GNU) 4.1.2 20060920 (prerelease) (Debian 4.1.1-14)"
	.section	.note.GNU-stack,"",@progbits

On the other hand, on ARM it leaves a gap:

	.file	"test2.c"
	.global	c
	.data
	.type	c, %object
	.size	c, 1
c:
	.byte	1
	.global	t
	.align	2          <<<------
	.type	t, %object
	.size	t, 8
t:
	.word	2
	.word	3
	.ident	"GCC: (GNU) 4.1.2 20061028 (prerelease) (Debian 4.1.1-19)"


But, if I declare struct test like this:

struct test {
   int a;
   int b;
} __attribute__((packed));

then the ARM compiler generates the same layout as the x86 compiler, 
without the gap.

The practical consequence of this is that making the "harmless" change 
of moving the attribute from the members to the struct as a whole 
causes code that previously worked to not work.  Would you say that 
such code was "always wrong", or is there an issue here?

Thanks for all the advice.


Phil.

Re: Packing of structure fields and whole structs

[Paul Brook]

On Wednesday 06 December 2006 16:48, Phil Endecott wrote:
> Let me make this concrete again.  With this test program:
>
> struct test {
>    int a __attribute__((packed));
>    int b __attribute__((packed));
> };
>
> char c = 1;
> struct test t = { .a=2, .b=3 };
>
> what can I assume about the alignment of t.a?  As I now understand it,
> t.a and t.b both have alignment of 1, so t has alignment of 1, so t can
> be packed immediately after c with no gap.  It is then unsafe (on some
> hardware) to take &t.a and pass it to a function that wants an int*.

Correct. It may be unsafe on all hardware because the compiler is allowed to 
assume that the low 2 bits of an int* are zero.

> On the other hand, on ARM it leaves a gap:

> struct test {
>    int a;
>    int b;
> } __attribute__((packed));
>
> then the ARM compiler generates the same layout as the x86 compiler,
> without the gap.

This is because the ARM ABI you're using specifies that all structures have a 
minimum alignment of 4 bytes. ie.
struct foo {
 char c;
};
has size and alignmet 4.

Making the structure packed overrides that.

Paul

Re: Packing of structure fields and whole structs

[Phil Endecott]

Paul Brook wrote:
> [..] the ARM ABI you're using specifies that all structures have a 
> minimum alignment of 4 bytes.
>
> Making the structure packed overrides that.

.. whereas making all of the members of the struct packed doesn't.  OK.

Is there still a case for a documentation fix?  That quote again:

   Specifying this attribute [packed] for `struct' and `union' types is
   equivalent to specifying the `packed' attribute on each of the 
structure or
   union members.