| Message ID | 20121121204216.GJ2315@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
> 2012-11-21 Jakub Jelinek <jakub@redhat.com> > > * ree.c (struct ext_modified): Add ATTRIBUTE_PACKED. Are you sure that this will compile with non-GCC compilers?
On Thu, Nov 22, 2012 at 12:26:01AM +0100, Eric Botcazou wrote: > > 2012-11-21 Jakub Jelinek <jakub@redhat.com> > > > > * ree.c (struct ext_modified): Add ATTRIBUTE_PACKED. > > Are you sure that this will compile with non-GCC compilers? I'm not sure, but I hope it will. ansidecl.h has #ifndef GCC_VERSION #define GCC_VERSION (__GNUC__ * 1000 + __GNUC_MINOR__) #endif /* GCC_VERSION */ ... #if (GCC_VERSION < 2007) # define __attribute__(x) #endif ... #ifndef ATTRIBUTE_PACKED # define ATTRIBUTE_PACKED __attribute__ ((packed)) #endif So in theory it should expand to nothing for non-GCC compilers. I've tested (on a short testcase matching what the decl does) GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now) and clang 3.0. Jakub
> I'm not sure, but I hope it will. ansidecl.h has > #ifndef GCC_VERSION > #define GCC_VERSION (__GNUC__ * 1000 + __GNUC_MINOR__) > #endif /* GCC_VERSION */ > ... > #if (GCC_VERSION < 2007) > # define __attribute__(x) > #endif > ... > #ifndef ATTRIBUTE_PACKED > # define ATTRIBUTE_PACKED __attribute__ ((packed)) > #endif > > So in theory it should expand to nothing for non-GCC compilers. > I've tested (on a short testcase matching what the decl does) > GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now) > and clang 3.0. OK, some other attributes are defined in the same way, thanks.
On 22/11/12 07:52, Jakub Jelinek wrote: > On Thu, Nov 22, 2012 at 12:26:01AM +0100, Eric Botcazou wrote: >>> 2012-11-21 Jakub Jelinek <jakub@redhat.com> >>> >>> * ree.c (struct ext_modified): Add ATTRIBUTE_PACKED. >> >> Are you sure that this will compile with non-GCC compilers? > > I'm not sure, but I hope it will. ansidecl.h has > #ifndef GCC_VERSION > #define GCC_VERSION (__GNUC__ * 1000 + __GNUC_MINOR__) > #endif /* GCC_VERSION */ > ... > #if (GCC_VERSION < 2007) > # define __attribute__(x) > #endif > ... > #ifndef ATTRIBUTE_PACKED > # define ATTRIBUTE_PACKED __attribute__ ((packed)) > #endif > > So in theory it should expand to nothing for non-GCC compilers. > I've tested (on a short testcase matching what the decl does) > GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now) > and clang 3.0. > > Jakub > That's not going to help if those other compilers need packedness to eliminate padding. The /old/ ARM ABI used to require that all structs were padded out to 32 bits. It looks to me as though this code is just non-portable and as such needs to be rewritten :-( R.
On Wed, Nov 28, 2012 at 03:02:12PM +0000, Richard Earnshaw wrote: > >#if (GCC_VERSION < 2007) > ># define __attribute__(x) > >#endif > >... > >#ifndef ATTRIBUTE_PACKED > ># define ATTRIBUTE_PACKED __attribute__ ((packed)) > >#endif > > > >So in theory it should expand to nothing for non-GCC compilers. > >I've tested (on a short testcase matching what the decl does) > >GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now) > >and clang 3.0. > > > > Jakub > > > > That's not going to help if those other compilers need packedness to > eliminate padding. The /old/ ARM ABI used to require that all > structs were padded out to 32 bits. > > It looks to me as though this code is just non-portable and as such > needs to be rewritten :-( Why? There is no hard requirement that this must be 2 byte long instead of 4, it is a pure optimization, don't want to waste too much memory unnecessarily. Jakub
On 28/11/12 15:05, Jakub Jelinek wrote: > On Wed, Nov 28, 2012 at 03:02:12PM +0000, Richard Earnshaw wrote: >>> #if (GCC_VERSION < 2007) >>> # define __attribute__(x) >>> #endif >>> ... >>> #ifndef ATTRIBUTE_PACKED >>> # define ATTRIBUTE_PACKED __attribute__ ((packed)) >>> #endif >>> >>> So in theory it should expand to nothing for non-GCC compilers. >>> I've tested (on a short testcase matching what the decl does) >>> GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now) >>> and clang 3.0. >>> >>> Jakub >>> >> >> That's not going to help if those other compilers need packedness to >> eliminate padding. The /old/ ARM ABI used to require that all >> structs were padded out to 32 bits. >> >> It looks to me as though this code is just non-portable and as such >> needs to be rewritten :-( > > Why? There is no hard requirement that this must be 2 byte long instead of > 4, it is a pure optimization, don't want to waste too much memory > unnecessarily. > > Jakub > Ah, OK. I read your mail as implying that it didn't work when it wasn't 2 bytes long... R.
--- gcc/ree.c.jj 2012-11-21 16:00:13.000000000 +0100 +++ gcc/ree.c 2012-11-21 18:43:27.025706082 +0100 @@ -475,7 +475,7 @@ enum ext_modified_kind EXT_MODIFIED_SEXT }; -struct ext_modified +struct ATTRIBUTE_PACKED ext_modified { /* Mode from which ree has zero or sign extended the destination. */ ENUM_BITFIELD(machine_mode) mode : 8;