Bug 51628 - __attribute__((packed)) is unsafe in some cases
__attribute__((packed)) is unsafe in some cases
Status: NEW
Product: gcc
Classification: Unclassified
Component: c
4.5.1
: P3 normal
: ---
Assigned To: Not yet assigned to anyone
: wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-19 22:58 UTC by Keith Thompson
Modified: 2013-04-19 20:12 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-12-20 00:00:00


Attachments
packed.c test case with output in a comment (1.13 KB, text/plain)
2011-12-19 22:58 UTC, Keith Thompson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Thompson 2011-12-19 22:58:07 UTC
Created attachment 26147 [details]
packed.c test case with output in a comment

I've seen this problem with gcc 4.5.1 on SPARC Solaris 9.  I presume it will affect other versions of gcc on other platforms with strict alignment requirements (unlike x86, which supports misaligned accesses in hardware as I understand it).  I think it also applies to "#pragma pack".

The gcc extension __attribute__(packed), which applied to a struct, has the following semantics (quoting the gcc documentation for 4.5.2):

     The `packed' attribute specifies that a variable or structure field
     should have the smallest possible alignment--one byte for a
     variable, and one bit for a field, unless you specify a larger
     value with the `aligned' attribute.

When a program accesses a misaligned member of a packed struct, the compiler generates whatever code is necessary to read or write the correct value.

If the address of a misaligned member is stored in a pointer object, dereferencing that pointer doesn't give the compiler an opportunity to generate that extra code.

The attached program demonstrates the problem, and includes (as a comment) the output I get on Ubuntu x86 (ok) and Solaris 9 SPARC (bus error).

See also http://stackoverflow.com/questions/8568432/is-gccs-attribute-packed-pragma-pack-unsafe/

I don't believe it would be practical to fix this (though there might be some clever solution I haven't thought of).  But at least I suggest mentioning this issue in the documentation.
Comment 1 Keith Thompson 2011-12-19 23:05:50 UTC
A commenter on stackoverflow points out that a possible fix would be to permit the address of a member of a packed structure to be assigned only to a pointer object that is itself declared "packed".
Comment 2 Andrew Pinski 2011-12-19 23:27:24 UTC
I think this is just undefined at runtime which means we can only warn about it.  Which we do with -Walign
Comment 3 Keith Thompson 2011-12-20 00:36:52 UTC
I see no "-Walign" option, either in the versions of gcc I'm using or in the online documentation.  Were you thinking of a different option?

What I'm suggesting, primarily, is that the issue should be mentioned in the documentation of the "packed" attribute (and probably of "pragma #pack" as well).
Comment 4 Richard Biener 2011-12-20 10:20:19 UTC
The point is that even if you use sth like

typedef int myint __attribute__((aligned(1)));

to capture the misaligned pointer to the packed structure element:

myint *p = &s->i;

then accesses like '*p' will still assume an _aligned_ int at 'p' for
STRICT_ALIGNMENT targets.

That's a long-long-long-standing bug and a cause of major headache for
more modern GCCs even ...

The testcase with using a 'int *' pointer is indeed invalid though.
Comment 5 Eric Botcazou 2011-12-20 10:52:19 UTC
> The point is that even if you use sth like
> 
> typedef int myint __attribute__((aligned(1)));
> 
> to capture the misaligned pointer to the packed structure element:
> 
> myint *p = &s->i;
> 
> then accesses like '*p' will still assume an _aligned_ int at 'p' for
> STRICT_ALIGNMENT targets.
> 
> That's a long-long-long-standing bug and a cause of major headache for
> more modern GCCs even ...

That's a limitation rather than a bug.  Clearly, on strict-alignment targets, you must know what you're doing when you start to misalign things.  As for

 typedef int myint __attribute__((aligned(1)));

that's an abomination I don't even want to know of ;-)
Comment 6 rguenther@suse.de 2011-12-20 11:04:37 UTC
On Tue, 20 Dec 2011, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> 
> --- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-12-20 10:52:19 UTC ---
> > The point is that even if you use sth like
> > 
> > typedef int myint __attribute__((aligned(1)));
> > 
> > to capture the misaligned pointer to the packed structure element:
> > 
> > myint *p = &s->i;
> > 
> > then accesses like '*p' will still assume an _aligned_ int at 'p' for
> > STRICT_ALIGNMENT targets.
> > 
> > That's a long-long-long-standing bug and a cause of major headache for
> > more modern GCCs even ...
> 
> That's a limitation rather than a bug.  Clearly, on strict-alignment targets,
> you must know what you're doing when you start to misalign things.  As for
> 
>  typedef int myint __attribute__((aligned(1)));
> 
> that's an abomination I don't even want to know of ;-)

Huh, it's not.  It's the same as a packed struct or enum type.  Why
can't you strict-align people simply fix this case?

Richard.
Comment 7 Eric Botcazou 2011-12-20 11:18:24 UTC
> Huh, it's not.  It's the same as a packed struct or enum type.

No, it isn't, the mode is integral instead of BLKmode.  In Ada we do support misaligned integers, but we simply wrap them in a BLKmode record.

> Why can't you strict-align people simply fix this case?

Because integral modes are naturally aligned.  The only reasonable way to support the aforementioned abomination is to use the Ada approach.
Comment 8 rguenther@suse.de 2011-12-20 11:23:48 UTC
On Tue, 20 Dec 2011, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> 
> --- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-12-20 11:18:24 UTC ---
> > Huh, it's not.  It's the same as a packed struct or enum type.
> 
> No, it isn't, the mode is integral instead of BLKmode.  In Ada we do support
> misaligned integers, but we simply wrap them in a BLKmode record.
> 
> > Why can't you strict-align people simply fix this case?
> 
> Because integral modes are naturally aligned.  The only reasonable way to
> support the aforementioned abomination is to use the Ada approach.

You mean that handling the TYPE_ALIGN != MODE_ALIGN case when
expanding a MEM_REF (thus, INDIRECT_REF on old branches) won't work?
Why not?  You'd simply have to emit the same RTL as when expanding
that wrapped struct case.

Richard.
Comment 9 Eric Botcazou 2011-12-20 11:34:00 UTC
> You mean that handling the TYPE_ALIGN != MODE_ALIGN case when
> expanding a MEM_REF (thus, INDIRECT_REF on old branches) won't work?

But you cannot have TYPE_ALIGN < MODE_ALIGN (TYPE_MODE).
Comment 10 rguenther@suse.de 2011-12-20 11:56:22 UTC
On Tue, 20 Dec 2011, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> 
> --- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-12-20 11:34:00 UTC ---
> > You mean that handling the TYPE_ALIGN != MODE_ALIGN case when
> > expanding a MEM_REF (thus, INDIRECT_REF on old branches) won't work?
> 
> But you cannot have TYPE_ALIGN < MODE_ALIGN (TYPE_MODE).

You can.  Just check what you get with that aligned(1) int typedef.

For vector types you can anyways (by design) to allow unaligned
vector moves.

Is it documented anywhere that you can't take the address of
an unaligned structure member (given the struct is packed) on
STRICT_ALIGNMENT targets (or, when it's a vector component even
on non-STRICT_ALIGNMENT targets)?  Why does the C frontend not
warn for these cases (unconditionally?)?

I don't see at all that we can't support expanding such moves
properly.  After all we can extract an INT mode value from
unaligned memory (we are never generating a mem:BLK with
INT mode size in that case anyway, or a MEM:SI with MEM_ALIGN
less than SImode align anyway)
Comment 11 Eric Botcazou 2011-12-20 12:25:13 UTC
> You can.  Just check what you get with that aligned(1) int typedef.

Well, we're going in circles as this example precisely doesn't work.

> Is it documented anywhere that you can't take the address of
> an unaligned structure member (given the struct is packed) on
> STRICT_ALIGNMENT targets (or, when it's a vector component even
> on non-STRICT_ALIGNMENT targets)?  Why does the C frontend not
> warn for these cases (unconditionally?)?

Good question, but for a C maintainer.  The C front-end would have implemented something for a long time if it had cared about the issue, but apparently not.
In Ada we do care since Ada 2005, so we have implemented the necessary support.
Comment 12 Jakub Jelinek 2011-12-20 12:35:18 UTC
What Ada does looks just like a workaround for what should be done properly in the expander.  So no, IMHO we shouldn't be changing all other FEs and the middle-end (when it wants to generate them e.g. for memcpy) to do what Ada does, but we should change the expander.  It has all the information to perform the unaligned reads/writes, it just doesn't use them.
Comment 13 rguenther@suse.de 2011-12-20 13:21:02 UTC
On Tue, 20 Dec 2011, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> 
> --- Comment #11 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-12-20 12:25:13 UTC ---
> > You can.  Just check what you get with that aligned(1) int typedef.
> 
> Well, we're going in circles as this example precisely doesn't work.
> 
> > Is it documented anywhere that you can't take the address of
> > an unaligned structure member (given the struct is packed) on
> > STRICT_ALIGNMENT targets (or, when it's a vector component even
> > on non-STRICT_ALIGNMENT targets)?  Why does the C frontend not
> > warn for these cases (unconditionally?)?
> 
> Good question, but for a C maintainer.  The C front-end would have implemented
> something for a long time if it had cared about the issue, but apparently not.
> In Ada we do care since Ada 2005, so we have implemented the necessary support.

Fact is, the middle-end needs a way to support this (well, or "wants").
Otherwise stripping off component-refs does not work even for the
"long time working cases".  "Fixes" to avoid stripping them away
are not really fixes but workarounds around fixing this long-time issue.

But I see you are not going to work on fixing the expansion side
(which I hoped, since you have plenty of experience in this area
and strict-alignment targets)
Comment 14 Eric Botcazou 2011-12-20 17:43:07 UTC
> What Ada does looks just like a workaround for what should be done properly in
> the expander.  So no, IMHO we shouldn't be changing all other FEs and the
> middle-end (when it wants to generate them e.g. for memcpy) to do what Ada
> does, but we should change the expander.

Of course I have the exactly opposite viewpoint, since I think that we should keep the invariants we have: integral modes are naturally aligned and TYPE_ALIGN >= MODE_ALIGN (TYPE_MODE).  Breaking them for a few pathological cases that the C compiler has shun for years doesn't seem the best course of action.
Comment 15 incrediball 2013-04-02 00:58:40 UTC
I believe the discussion here is missing the point. Currently (at least with version 4.5 and ARM, which I am currently using) the situation is that the compiler generates broken code WITHOUT COMMENT when the address of something like an integer in a packed structure is assigned to a int* . Since the pointer type does not provide any information about the actual alignment of the integer, it is obviously impractical to fix when accessing the integer and we certainly do not want to have extra code inserted for handling the possibility that the int may not be aligned correctly. Therefore the compiler MUST see this as a type conflict and at least warn that the address of the member in a packed struct is incompatible with the pointer type.

I have several projects which require the use of packed structures and I simply cannot rule out that that I have not at some point accidentally assigned the address of a member variable to a pointer. It would therefore be very handy if the compiler could point this out. -Wcast-align certainly doesn't do this.
Comment 16 Richard Biener 2013-04-02 08:30:16 UTC
(In reply to comment #14)
> > What Ada does looks just like a workaround for what should be done properly in
> > the expander.  So no, IMHO we shouldn't be changing all other FEs and the
> > middle-end (when it wants to generate them e.g. for memcpy) to do what Ada
> > does, but we should change the expander.
> 
> Of course I have the exactly opposite viewpoint, since I think that we should
> keep the invariants we have: integral modes are naturally aligned and
> TYPE_ALIGN >= MODE_ALIGN (TYPE_MODE).  Breaking them for a few pathological
> cases that the C compiler has shun for years doesn't seem the best course of
> action.

Well.  I'm fine with forcing BLKmode for unaligned accesses, but not sure
if that is less invasive.  You'd basically have to change modes whenever
more precise knowledge about alignment appears ...
Comment 17 Richard Biener 2013-04-02 08:30:54 UTC
(In reply to comment #15)
> I believe the discussion here is missing the point. Currently (at least with
> version 4.5 and ARM, which I am currently using) the situation is that the
> compiler generates broken code WITHOUT COMMENT when the address of something
> like an integer in a packed structure is assigned to a int* . Since the pointer
> type does not provide any information about the actual alignment of the
> integer, it is obviously impractical to fix when accessing the integer and we
> certainly do not want to have extra code inserted for handling the possibility
> that the int may not be aligned correctly. Therefore the compiler MUST see this
> as a type conflict and at least warn that the address of the member in a packed
> struct is incompatible with the pointer type.
> 
> I have several projects which require the use of packed structures and I simply
> cannot rule out that that I have not at some point accidentally assigned the
> address of a member variable to a pointer. It would therefore be very handy if
> the compiler could point this out. -Wcast-align certainly doesn't do this.

Because there is no cast involved.  And that's because the types of the fields
are "wrong" as well.
Comment 18 incrediball 2013-04-02 20:21:06 UTC
Not sure if I can agree with (or understand) this comment. If we use my example of the address of an int in a packed structure being assigned to an int* then one could argue that there is a kind of implicit cast happening. The pointer is to an integer WITH the default alignment which would be 4 on the ARM system I work with. The int in the struct has an alignment of 1 and so the two types need to be regarded as being incompatible and there should be an unconditional warning or at least -Wcast-align should enable such a warning.

I don't know what BLKmode is but if you're speaking of making an int* type generally able to access the int regardless of its alignment, I think this is the wrong way to go because code size will explode and efficiency will go out the window. However if the int* would be declared with some special attribute then, sure, generate whatever code that is necessary to access the int and the above mentioned warning can be suppressed in this case (wouldn't the code generated be much the same code that is used to directly access the int in the packed struct anyway?) However this would be the second (optional) step, the first thing is that the compiler needs to emit a warning explaining that the code that it is generating is broken.
Comment 19 Eric Botcazou 2013-04-03 07:29:51 UTC
> Not sure if I can agree with (or understand) this comment. If we use my example
> of the address of an int in a packed structure being assigned to an int* then
> one could argue that there is a kind of implicit cast happening. The pointer is
> to an integer WITH the default alignment which would be 4 on the ARM system I
> work with. The int in the struct has an alignment of 1 and so the two types
> need to be regarded as being incompatible and there should be an unconditional
> warning or at least -Wcast-align should enable such a warning.

Yes, a warning should probably be implemented in the C family of compilers, but this is a very old issue and nobody is really interested.  Note that taking the address of the field is OK, the problem is dereferencing it.
Comment 20 rguenther@suse.de 2013-04-03 07:55:34 UTC
On Wed, 3 Apr 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> 
> --- Comment #19 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-04-03 07:29:51 UTC ---
> > Not sure if I can agree with (or understand) this comment. If we use my example
> > of the address of an int in a packed structure being assigned to an int* then
> > one could argue that there is a kind of implicit cast happening. The pointer is
> > to an integer WITH the default alignment which would be 4 on the ARM system I
> > work with. The int in the struct has an alignment of 1 and so the two types
> > need to be regarded as being incompatible and there should be an unconditional
> > warning or at least -Wcast-align should enable such a warning.
> 
> Yes, a warning should probably be implemented in the C family of compilers, but
> this is a very old issue and nobody is really interested.  Note that taking the
> address of the field is OK, the problem is dereferencing it.

One of the C frontend issues is that the type of the address of
the field of the packed struct is int *, not int attribute((aligned(1))) 
*.  And this is so because nothing adjusts the type of the FIELD_DECL
to be a less aligned type.  That is, we have

 <field_decl 0x7ffff6d245f0 i
    type <integer_type 0x7ffff6d175e8 int public SI
        size <integer_cst 0x7ffff6d1a0c0 constant 32>
        unit size <integer_cst 0x7ffff6d1a0e0 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0x7ffff6d175e8 
precision 32 min <integer_cst 0x7ffff6d1a060 -2147483648> max <integer_cst 
0x7ffff6d1a080 2147483647>
        pointer_to_this <pointer_type 0x7ffff6d1f2a0>>
    packed SI file t.c line 2 col 9 size <integer_cst 0x7ffff6d1a0c0 32> 
unit size <integer_cst 0x7ffff6d1a0e0 4>
    align 8 offset_align 128
    offset <integer_cst 0x7ffff6d02d80 type <integer_type 0x7ffff6d17000 
sizetype> constant 0>
    bit offset <integer_cst 0x7ffff6d02e00 type <integer_type 
0x7ffff6d170a8 bitsizetype> constant 0> context <record_type 
0x7ffff6e1c3f0 Foo>>

and alignof (x.i) and alignof (*&x.i) would not agree (if the
latter were not folded to x.i) for

struct Foo {
    int i;
} __attribute__((packed)) ;
struct Foo x;

for example (surprisingly) __alignof__ (*(char *)&x.i) is 4
while __alignof__ (*&x.i) is 1 (due to folding) and
__alignof__ (x.i) is 1 as well.

I'm quite sure that "fixing" this will have loads of fallout
throughout the frontend(s if 'fixed' in stor-layout.c).  Which
is why this issue is so old and unfixed.
Comment 21 Eric Botcazou 2013-04-03 08:51:44 UTC
> One of the C frontend issues is that the type of the address of
> the field of the packed struct is int *, not int attribute((aligned(1))) 
> *.  And this is so because nothing adjusts the type of the FIELD_DECL
> to be a less aligned type.  That is, we have
> 
>  <field_decl 0x7ffff6d245f0 i
>     type <integer_type 0x7ffff6d175e8 int public SI
>         size <integer_cst 0x7ffff6d1a0c0 constant 32>
>         unit size <integer_cst 0x7ffff6d1a0e0 constant 4>
>         align 32 symtab 0 alias set -1 canonical type 0x7ffff6d175e8 
> precision 32 min <integer_cst 0x7ffff6d1a060 -2147483648> max <integer_cst 
> 0x7ffff6d1a080 2147483647>
>         pointer_to_this <pointer_type 0x7ffff6d1f2a0>>
>     packed SI file t.c line 2 col 9 size <integer_cst 0x7ffff6d1a0c0 32> 
> unit size <integer_cst 0x7ffff6d1a0e0 4>
>     align 8 offset_align 128
>     offset <integer_cst 0x7ffff6d02d80 type <integer_type 0x7ffff6d17000 
> sizetype> constant 0>
>     bit offset <integer_cst 0x7ffff6d02e00 type <integer_type 
> 0x7ffff6d170a8 bitsizetype> constant 0> context <record_type 
> 0x7ffff6e1c3f0 Foo>>

This is on x86, right?  If the alignment of the field cannot be guaranteed to be that of its type, then it should be made a bit-field.  Maybe it's already made a bit-field on strict-alignment targets.
Comment 22 rguenther@suse.de 2013-04-03 09:20:21 UTC
On Wed, 3 Apr 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> 
> --- Comment #21 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-04-03 08:51:44 UTC ---
> > One of the C frontend issues is that the type of the address of
> > the field of the packed struct is int *, not int attribute((aligned(1))) 
> > *.  And this is so because nothing adjusts the type of the FIELD_DECL
> > to be a less aligned type.  That is, we have
> > 
> >  <field_decl 0x7ffff6d245f0 i
> >     type <integer_type 0x7ffff6d175e8 int public SI
> >         size <integer_cst 0x7ffff6d1a0c0 constant 32>
> >         unit size <integer_cst 0x7ffff6d1a0e0 constant 4>
> >         align 32 symtab 0 alias set -1 canonical type 0x7ffff6d175e8 
> > precision 32 min <integer_cst 0x7ffff6d1a060 -2147483648> max <integer_cst 
> > 0x7ffff6d1a080 2147483647>
> >         pointer_to_this <pointer_type 0x7ffff6d1f2a0>>
> >     packed SI file t.c line 2 col 9 size <integer_cst 0x7ffff6d1a0c0 32> 
> > unit size <integer_cst 0x7ffff6d1a0e0 4>
> >     align 8 offset_align 128
> >     offset <integer_cst 0x7ffff6d02d80 type <integer_type 0x7ffff6d17000 
> > sizetype> constant 0>
> >     bit offset <integer_cst 0x7ffff6d02e00 type <integer_type 
> > 0x7ffff6d170a8 bitsizetype> constant 0> context <record_type 
> > 0x7ffff6e1c3f0 Foo>>
> 
> This is on x86, right?  If the alignment of the field cannot be guaranteed to
> be that of its type, then it should be made a bit-field.  Maybe it's already
> made a bit-field on strict-alignment targets.

Note the FIELD_DECL is perfectly ok (align 8), it is its TREE_TYPE
that is "bogus", and this type is used when building the pointer type
used for taking the address of it (so you could argue _that_ is the
bug - it shouldn't literally take TREE_TYPE of a FIELD_DECL when
building the address of a COMPONENT_REF - the COMPONENT_REF
surely only caring about the type of the value not the storage).

Richard.