2006-12-06 13:20:47

by Phil Endecott

[permalink] [raw]
Subject: Subtleties of __attribute__((packed))

Dear All,

I used to think that this:

struct foo {
int a __attribute__((packed));
char b __attribute__((packed));
... more fields, all packed ...
};

was exactly the same as this:

struct foo {
int a;
char b;
... more fields ...
} __attribute__((packed));

but it is not, in a subtle way.

Maybe you experts all know this already, but it was new to me so I
thought I ought to share it, since there have been a few patches
recently changing the first form to the second form to avoid gcc
warnings. (See for example
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blobdiff;h=1a7a3f50e40b0a956f44511e42b124a6be98b30b;hp=74f6889f834f1679f09ccd8bbc772fdafd6aade2;hb=e2bf2e26c0915d54208315fc8c9864f1d987217a;f=arch/powerpc/platforms/iseries/main_store.h
or http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6a878184c202395ea17212f111ab9ec4b5f6d6ee)

The difference comes when you declare a variable of this struct type
like this:

char c;
struct foo f;

If you use the first form in the declaration of struct foo, a gap will
be left between c and f so that the start of the struct is aligned.
But if you use the second form, f will be packed immediately after c, unaligned.

On x86 of course none of this matters for correct behaviour since the
hardware supports unaligned accesses. Assuming that your hardware
doesn't do unaligned accesses then some code will still work. In
particular, if you access f like this:

f.a++;

or probably

func1(f.a); // func1 takes an int

then gcc will generate the necessary byte-shuffling code. However, if
you write this:

func2(&f.a); // func2 takes an int*

then an unaligned pointer is passed to func2. When func2 dereferences
the pointer the hardware fails in some way.

GCC does not seem to generate an error or warning when you take the
address of an unaligned field like this.

I believe that the solution is to write something like this:

struct foo {
int a;
char b;
... more fields ...
} __attribute__((packed)) __attribute__((aligned(4)));

Now the fields within the struct will be packed, but variables of the
struct type will be aligned to a 4-byte boundary.

It could be that the kernel code is all safe. I discovered this issue
in user code that used structs from the kernel headers. But I
encourage anyone who changed their use of __attribute__((packed)) to
avoid a gcc warning to review what they have done, especially if they
have tested it only on x86.

Thanks for your attention, and please forgive me if you all know all
this already!

Regards,

Phil.


(You are welcome to cc: me with any replies.)






2006-12-06 14:02:49

by Frederik Deweerdt

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

On Wed, Dec 06, 2006 at 01:20:41PM +0000, Phil Endecott wrote:
> Dear All,
>
> I used to think that this:
>
> struct foo {
> int a __attribute__((packed));
> char b __attribute__((packed));
> ... more fields, all packed ...
> };
>
> was exactly the same as this:
>
> struct foo {
> int a;
> char b;
> ... more fields ...
> } __attribute__((packed));
>
> but it is not, in a subtle way.
>
This is likely a gcc bug isn't it? The gcc info page states:
Specifying this attribute for `struct' and `union' types is
equivalent to specifying the `packed' attribute on each of the
structure or union members.

Regards,
Frederik

2006-12-06 14:24:46

by Phil Endecott

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

Frederik Deweerdt wrote:
> On Wed, Dec 06, 2006 at 01:20:41PM +0000, Phil Endecott wrote:
>> I used to think that this:
>>
>> struct foo {
>> int a __attribute__((packed));
>> char b __attribute__((packed));
>> ... more fields, all packed ...
>> };
>>
>> was exactly the same as this:
>>
>> struct foo {
>> int a;
>> char b;
>> ... more fields ...
>> } __attribute__((packed));
>>
>> but it is not, in a subtle way.
>>
> This is likely a gcc bug isn't it? The gcc info page states:
> Specifying this attribute for `struct' and `union' types is
> equivalent to specifying the `packed' attribute on each of the
> structure or union members.

A gcc *documentation* bug?

I asked on the gcc list about this before posting here, and although
replies are still coming in the first opinion was "it's doing exactly
what you asked it to do".

Phil.




2006-12-06 15:04:25

by Jan Blunck

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

On 12/6/06, Phil Endecott <[email protected]> wrote:
> Dear All,
>
> I used to think that this:
>
> struct foo {
> int a __attribute__((packed));
> char b __attribute__((packed));
> ... more fields, all packed ...
> };
>
> was exactly the same as this:
>
> struct foo {
> int a;
> char b;
> ... more fields ...
> } __attribute__((packed));
>
> but it is not, in a subtle way.
>

The same code is generated. The difference is that usually packing the
whole struct isn't as error-prone as packing every element. Besides
that the gcc warns about packing objects that have an alignment of 1.
This is the reason why we should use the second approach.

2006-12-06 15:22:44

by Phil Endecott

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

Jan Blunck wrote:
> On 12/6/06, Phil Endecott <[email protected]> wrote:
>> I used to think that this:
>>
>> struct foo {
>> int a __attribute__((packed));
>> char b __attribute__((packed));
>> ... more fields, all packed ...
>> };
>>
>> was exactly the same as this:
>>
>> struct foo {
>> int a;
>> char b;
>> ... more fields ...
>> } __attribute__((packed));
>>
>> but it is not, in a subtle way.
>>
>
> The same code is generated. [...]

I don't think so. Example:

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

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

$ arm-linux-gnu-gcc -O2 -S -W -Wall test1.c

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


Compare with:

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

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

$ arm-linux-gnu-gcc -O2 -S -W -Wall test2.c

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



Phil.





2006-12-06 15:54:47

by Jan Blunck

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

On Wed, Dec 06, Phil Endecott wrote:

> I don't think so. Example:
>
> struct test {
> int a __attribute__((packed));
> int b __attribute__((packed));
> };
>
> char c = 1;
> struct test t = { .a=2, .b=3 };
>
> $ arm-linux-gnu-gcc -O2 -S -W -Wall test1.c
>
> .file "test2.c"
> .global c
> .data
> .type c, %object
> .size c, 1
> c:
> .byte 1
> .global t
> .align 2 <<<<<<<<===== t is aligned
> .type t, %object
> .size t, 8
> t:
> .word 2
> .word 3
> .ident "GCC: (GNU) 4.1.2 20061028 (prerelease) (Debian 4.1.1-19)"
>
>
> Compare with:
>
> struct test {
> int a;
> int b;
> } __attribute__((packed));
>
> char c = 1;
> struct test t = { .a=2, .b=3 };
>
> $ arm-linux-gnu-gcc -O2 -S -W -Wall test2.c
>
> .file "test1.c"
> .global c
> .data
> .type c, %object
> .size c, 1
> c:
> .byte 1
> .global t <<<<<< "align" has gone, t is unaligned
> .type t, %object
> .size t, 8
> t:
> .4byte 2
> .4byte 3
> .ident "GCC: (GNU) 4.1.2 20061028 (prerelease) (Debian 4.1.1-19)"
>

Maybe the arm backend is somehow broken. AFAIK (and I verfied it on S390 and
i386) the alignment shouldn't change.

struct foo {
int a;
char b;
int c;
};

struct bar1 {
char a __attribute__((__packed__));
struct foo b __attribute__((__packed__));
};

struct bar2 {
char a;
struct foo b;
} __attribute__((__packed__));

struct bar3 {
char a;
struct foo b;
};

struct bar1 packed1 = { 10, { 20, 30, 40 } };
struct bar2 packed2 = { 50, { 60, 70, 80 } };
struct bar3 unpacked = { 90, { 100, 110, 120 } };

s390x-linux-gcc -S packed2.c:
packed2.c:9: warning: '__packed__' attribute ignored for field of type 'char'

.file "packed2.c"
.globl packed1
.data
.align 2
.type packed1, @object
.size packed1, 13
packed1:
.byte 10
.4byte 20
.byte 30
.zero 3
.4byte 40
.globl packed2
.align 2
.type packed2, @object
.size packed2, 13
packed2:
.byte 50
.4byte 60
.byte 70
.zero 3
.4byte 80
.globl unpacked
.align 4
.type unpacked, @object
.size unpacked, 16
unpacked:
.byte 90
.zero 3
.long 100
.byte 110
.zero 3
.long 120
.ident "GCC: (GNU) 4.1.2 20060531 (prerelease) (SUSE Linux)"
.section .note.GNU-stack,"",@progbits

2006-12-06 16:02:43

by Andreas Schwab

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

Jan Blunck <[email protected]> writes:

> Maybe the arm backend is somehow broken.

A packed structure is something quite different than a structure of packed
members.

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstra?e 5, 90409 N?rnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2006-12-06 16:13:59

by Phil Endecott

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

Jan Blunk wrote:
> Maybe the arm backend is somehow broken. AFAIK (and I verfied it on S390 and
> i386) the alignment shouldn't change.

To see a difference with your example structs you need to compare these two:

struct wibble1 {
char c;
struct bar1 b1;
};

struct wibble2 {
char c;
struct bar2 b2;
};

struct wibble1 w1 = { 1, { 2, {3,4,5} } };
struct wibble2 w2 = { 1, { 2, {3,4,5} } };

Can you try that with your compilers? I get:

w1:
.byte 1
.space 3 <<<----
.byte 2
.4byte 3
.byte 4
.space 3
.4byte 5
.space 3
.global w2
.align 2
.type w2, %object
.size w2, 16
w2:
.byte 1
.byte 2
.4byte 3
.byte 4
.space 3
.4byte 5
.space 2


Phil.




2006-12-06 16:26:22

by Jan Blunck

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

On Wed, Dec 06, Phil Endecott wrote:

>
> To see a difference with your example structs you need to compare these two:
>
> struct wibble1 {
> char c;
> struct bar1 b1;
> };
>
> struct wibble2 {
> char c;
> struct bar2 b2;
> };
>
> struct wibble1 w1 = { 1, { 2, {3,4,5} } };
> struct wibble2 w2 = { 1, { 2, {3,4,5} } };
>
> Can you try that with your compilers? I get:
>

As I expected, I get:

.file "packed2a.c"
.globl w1
.data
.align 2
.type w1, @object
.size w1, 14
w1:
.byte 1
.byte 2
.4byte 3
.byte 4
.zero 3
.4byte 5
.globl w2
.align 2
.type w2, @object
.size w2, 14
w2:
.byte 1
.byte 2
.4byte 3
.byte 4
.zero 3
.4byte 5
.ident "GCC: (GNU) 4.1.2 20060531 (prerelease) (SUSE Linux)"
.section .note.GNU-stack,"",@progbits

2006-12-06 16:41:23

by Jan Blunck

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

On Wed, Dec 06, Andreas Schwab wrote:

> Jan Blunck <[email protected]> writes:
>
> > Maybe the arm backend is somehow broken.
>
> A packed structure is something quite different than a structure of packed
> members.
>

Well, right ... and where do you see a structure of packed members?

http://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Type-Attributes.html#Type-Attributes

2006-12-06 17:29:00

by Andreas Schwab

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

Jan Blunck <[email protected]> writes:

> Well, right ... and where do you see a structure of packed members?

Read <http://sourceware.org/ml/binutils/2006-12/msg00039.html>.

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstra?e 5, 90409 N?rnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2006-12-06 17:54:35

by Russell King

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

On Wed, Dec 06, 2006 at 04:54:39PM +0100, Jan Blunck wrote:
> Maybe the arm backend is somehow broken. AFAIK (and I verfied it on S390 and
> i386) the alignment shouldn't change.

Please read the info pages:

`packed'
This attribute, attached to an `enum', `struct', or `union' type
definition, specifies that the minimum required memory be used to
represent the type.

Specifying this attribute for `struct' and `union' types is
equivalent to specifying the `packed' attribute on each of the
structure or union members. Specifying the `-fshort-enums' flag
on the line is equivalent to specifying the `packed' attribute on
all `enum' definitions.

Note that it says *nothing* about alignment. It says "minimum required
memory be used to represent the type." which implies that the internals
of the structure are packed together as tightly as possible.

It does not say "and as such the struct may be aligned to any alignment".

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

2006-12-06 18:05:20

by David Miller

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

From: Russell King <[email protected]>
Date: Wed, 6 Dec 2006 17:54:23 +0000

> It does not say "and as such the struct may be aligned to any alignment".

Consider the implication for arrays and pointer arithmetic, it's just
a logical consequence, that's all. It's why the alignment cannot be
assumed for packed structures.

If you have, for example:

struct example {
char b;
short c;
} __attribute__((packed));

And I give you:

extern void foo(struct example *p);

and go:

foo(p + 1);

It is clear that the compiler must assume that all instances
of a packed structure are not necessarily aligned properly.

Even if "p" is aligned, "p + 1" definitely won't be. And this
goes for any array indexing of the given packed structure.

That's why every pointer to such a struct must be assumed to be
unaligned in these cases.

So even though the documentation may not say this explicitly, it's an
implicit logical side effect of packed structures.

2006-12-07 09:48:36

by Jan Blunck

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

On Wed, Dec 06, Russell King wrote:

> On Wed, Dec 06, 2006 at 04:54:39PM +0100, Jan Blunck wrote:
> > Maybe the arm backend is somehow broken. AFAIK (and I verfied it on S390 and
> > i386) the alignment shouldn't change.
>

Once again: I refered to "packed attribute on the struct vs. packed attribute
on each member of the struct". The alignment shouldn't be different.

> Please read the info pages:
>
> `packed'
> This attribute, attached to an `enum', `struct', or `union' type
> definition, specifies that the minimum required memory be used to
> represent the type.
>
> Specifying this attribute for `struct' and `union' types is
> equivalent to specifying the `packed' attribute on each of the
> structure or union members. Specifying the `-fshort-enums' flag
> on the line is equivalent to specifying the `packed' attribute on
> all `enum' definitions.
>
> Note that it says *nothing* about alignment. It says "minimum required
> memory be used to represent the type." which implies that the internals
> of the structure are packed together as tightly as possible.
>
> It does not say "and as such the struct may be aligned to any alignment".
>

And this is why it makes sense to think about align attribute when you use
packed.

2006-12-07 10:30:25

by Andreas Schwab

[permalink] [raw]
Subject: Re: Subtleties of __attribute__((packed))

Jan Blunck <[email protected]> writes:

> Once again: I refered to "packed attribute on the struct vs. packed attribute
> on each member of the struct". The alignment shouldn't be different.

You are mistaken. The alignment of a non-packed structure can be greater
than the maximum alignment of the containing members.

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstra?e 5, 90409 N?rnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."