| Message ID | 52DA65F4.5070501@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 18/01/14 12:31, Chen Gang wrote: > Unfortunately, not all compilers assumes the structures within a pack > region also need be packed (e.g. metag), so need add a pack explicitly > to satisfy all compilers. > > The related error (under metag with allmodconfig): > > MODPOST 2952 modules > ERROR: "__compiletime_assert_431" [net/batman-adv/batman-adv.ko] undefined! > ERROR: "__compiletime_assert_432" [net/batman-adv/batman-adv.ko] undefined! > ERROR: "__compiletime_assert_429" [net/batman-adv/batman-adv.ko] undefined! > ERROR: "__compiletime_assert_428" [net/batman-adv/batman-adv.ko] undefined! > ERROR: "__compiletime_assert_423" [net/batman-adv/batman-adv.ko] undefined! > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> David, what do you think about this change? Can "__packed __aligned(2)" generate a different structure padding than "#pragma pack(2)" ? I am not really sure about the difference between the two. But if we have the possibility that the padding may change then this patch should go into net, otherwise we will have a protocol compatibility problem between 3.13 and 3.14. Cheers, > --- > net/batman-adv/packet.h | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/net/batman-adv/packet.h b/net/batman-adv/packet.h > index 0a381d1..9206b48 100644 > --- a/net/batman-adv/packet.h > +++ b/net/batman-adv/packet.h > @@ -154,7 +154,6 @@ enum batadv_tvlv_type { > BATADV_TVLV_ROAM = 0x05, > }; > > -#pragma pack(2) > /* the destination hardware field in the ARP frame is used to > * transport the claim type and the group id > */ > @@ -162,8 +161,7 @@ struct batadv_bla_claim_dst { > uint8_t magic[3]; /* FF:43:05 */ > uint8_t type; /* bla_claimframe */ > __be16 group; /* group id */ > -}; > -#pragma pack() > +} __packed __aligned(2); > > /** > * struct batadv_ogm_packet - ogm (routing protocol) packet > @@ -281,7 +279,6 @@ struct batadv_icmp_packet_rr { > * misalignment of the payload after the ethernet header. It may also lead to > * leakage of information when the padding it not initialized before sending. > */ > -#pragma pack(2) > > /** > * struct batadv_unicast_packet - unicast packet for network payload > @@ -300,7 +297,7 @@ struct batadv_unicast_packet { > /* "4 bytes boundary + 2 bytes" long to make the payload after the > * following ethernet header again 4 bytes boundary aligned > */ > -}; > +} __packed __aligned(2); > > /** > * struct batadv_unicast_4addr_packet - extended unicast packet > @@ -316,7 +313,7 @@ struct batadv_unicast_4addr_packet { > /* "4 bytes boundary + 2 bytes" long to make the payload after the > * following ethernet header again 4 bytes boundary aligned > */ > -}; > +} __packed __aligned(2); > > /** > * struct batadv_frag_packet - fragmented packet > @@ -347,7 +344,7 @@ struct batadv_frag_packet { > uint8_t orig[ETH_ALEN]; > __be16 seqno; > __be16 total_size; > -}; > +} __packed __aligned(2); > > /** > * struct batadv_bcast_packet - broadcast packet for network payload > @@ -368,7 +365,7 @@ struct batadv_bcast_packet { > /* "4 bytes boundary + 2 bytes" long to make the payload after the > * following ethernet header again 4 bytes boundary aligned > */ > -}; > +} __packed __aligned(2); > > /** > * struct batadv_coded_packet - network coded packet > @@ -404,9 +401,8 @@ struct batadv_coded_packet { > uint8_t second_orig_dest[ETH_ALEN]; > __be32 second_crc; > __be16 coded_len; > -}; > +} __packed __aligned(2); > > -#pragma pack() > > /** > * struct batadv_unicast_tvlv - generic unicast packet with tvlv payload >
On Saturday 18 January 2014 14:03:26 Antonio Quartulli wrote: > On 18/01/14 12:31, Chen Gang wrote: > > Unfortunately, not all compilers assumes the structures within a pack > > region also need be packed (e.g. metag), so need add a pack explicitly > > to satisfy all compilers. > > > > The related error (under metag with allmodconfig): > > MODPOST 2952 modules > > > > ERROR: "__compiletime_assert_431" [net/batman-adv/batman-adv.ko] > > undefined! > > ERROR: "__compiletime_assert_432" [net/batman-adv/batman-adv.ko] > > undefined! > > ERROR: "__compiletime_assert_429" [net/batman-adv/batman-adv.ko] > > undefined! > > ERROR: "__compiletime_assert_428" [net/batman-adv/batman-adv.ko] > > undefined! > > ERROR: "__compiletime_assert_423" [net/batman-adv/batman-adv.ko] > > undefined! > > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > > David, what do you think about this change? > > > Can "__packed __aligned(2)" generate a different structure padding than > "#pragma pack(2)" ? > > I am not really sure about the difference between the two. But if we > have the possibility that the padding may change then this patch should > go into net, otherwise we will have a protocol compatibility problem > between 3.13 and 3.14. (Chen: sorry I didn't twig what you were referring to before about #pragma pack not working) It appears that the following gcc patch adds support for #pragma pack: http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html I gave it a quick spin on metag gcc (which is unfortunately stuck on an old version) and it seems to fix my simple test case so that #pragma pack(2) becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__). However, the __packed and __aligned are linux specific macros to abstract compiler details, whereas #pragma pack appears to be a compiler-specific WIN32 style equivalent to GCC's __attribute__((packed)) and __attribute__((aligned(2))) (these are what __packed and __aligned use in compiler-gcc.h). Therefore I believe using the Linux abstractions is still more correct here. Cheers James
On 19/01/14 02:10, James Hogan wrote: > > It appears that the following gcc patch adds support for #pragma pack: > http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html > > I gave it a quick spin on metag gcc (which is unfortunately stuck on an old > version) and it seems to fix my simple test case so that #pragma pack(2) > becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__). > Then I personally think that it is better to fix metag gcc instead of changing the kernel. Actually there are many different spots where "#pragma pack" is used. batman-adv is just the only one having compile time checks for structure sizes. > > However, the __packed and __aligned are linux specific macros to abstract > compiler details, whereas #pragma pack appears to be a compiler-specific WIN32 > style equivalent to GCC's __attribute__((packed)) and > __attribute__((aligned(2))) (these are what __packed and __aligned use in > compiler-gcc.h). > > Therefore I believe using the Linux abstractions is still more correct here. If you really think so, I'd suggest to grep in the kernel and catch all the other occurrences of "#pragma pack" and change them all (assuming that using __attribute__((aligned(2))) is the way to go). Cheers,
On 01/19/2014 05:30 PM, Antonio Quartulli wrote: > On 19/01/14 02:10, James Hogan wrote: >> >> It appears that the following gcc patch adds support for #pragma pack: >> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html >> >> I gave it a quick spin on metag gcc (which is unfortunately stuck on an old >> version) and it seems to fix my simple test case so that #pragma pack(2) >> becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__). >> > > Then I personally think that it is better to fix metag gcc instead of > changing the kernel. > > Actually there are many different spots where "#pragma pack" is used. > batman-adv is just the only one having compile time checks for structure > sizes. > What Antonio said sounds reasonable to me. >> >> However, the __packed and __aligned are linux specific macros to abstract >> compiler details, whereas #pragma pack appears to be a compiler-specific WIN32 >> style equivalent to GCC's __attribute__((packed)) and >> __attribute__((aligned(2))) (these are what __packed and __aligned use in >> compiler-gcc.h). >> >> Therefore I believe using the Linux abstractions is still more correct here. > > If you really think so, I'd suggest to grep in the kernel and catch all > the other occurrences of "#pragma pack" and change them all (assuming > that using __attribute__((aligned(2))) is the way to go). > Thanks.
Hi Antonio, On 19/01/14 09:30, Antonio Quartulli wrote: > On 19/01/14 02:10, James Hogan wrote: >> It appears that the following gcc patch adds support for #pragma pack: >> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html >> >> I gave it a quick spin on metag gcc (which is unfortunately stuck on an old >> version) and it seems to fix my simple test case so that #pragma pack(2) >> becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__). >> > > Then I personally think that it is better to fix metag gcc instead of > changing the kernel. Indeed it makes sense to patch metag gcc to be safe in the presence of unportable code like this. > Actually there are many different spots where "#pragma pack" is used. > batman-adv is just the only one having compile time checks for structure > sizes. Well the only vaguely interesting one I can find outside of drivers is in fs/udf, and even that seems specific to CD-ROMs and DVDs. If you care about portability then Chen's patch looks reasonable to me. Cheers James
diff --git a/net/batman-adv/packet.h b/net/batman-adv/packet.h index 0a381d1..9206b48 100644 --- a/net/batman-adv/packet.h +++ b/net/batman-adv/packet.h @@ -154,7 +154,6 @@ enum batadv_tvlv_type { BATADV_TVLV_ROAM = 0x05, }; -#pragma pack(2) /* the destination hardware field in the ARP frame is used to * transport the claim type and the group id */ @@ -162,8 +161,7 @@ struct batadv_bla_claim_dst { uint8_t magic[3]; /* FF:43:05 */ uint8_t type; /* bla_claimframe */ __be16 group; /* group id */ -}; -#pragma pack() +} __packed __aligned(2); /** * struct batadv_ogm_packet - ogm (routing protocol) packet @@ -281,7 +279,6 @@ struct batadv_icmp_packet_rr { * misalignment of the payload after the ethernet header. It may also lead to * leakage of information when the padding it not initialized before sending. */ -#pragma pack(2) /** * struct batadv_unicast_packet - unicast packet for network payload @@ -300,7 +297,7 @@ struct batadv_unicast_packet { /* "4 bytes boundary + 2 bytes" long to make the payload after the * following ethernet header again 4 bytes boundary aligned */ -}; +} __packed __aligned(2); /** * struct batadv_unicast_4addr_packet - extended unicast packet @@ -316,7 +313,7 @@ struct batadv_unicast_4addr_packet { /* "4 bytes boundary + 2 bytes" long to make the payload after the * following ethernet header again 4 bytes boundary aligned */ -}; +} __packed __aligned(2); /** * struct batadv_frag_packet - fragmented packet @@ -347,7 +344,7 @@ struct batadv_frag_packet { uint8_t orig[ETH_ALEN]; __be16 seqno; __be16 total_size; -}; +} __packed __aligned(2); /** * struct batadv_bcast_packet - broadcast packet for network payload @@ -368,7 +365,7 @@ struct batadv_bcast_packet { /* "4 bytes boundary + 2 bytes" long to make the payload after the * following ethernet header again 4 bytes boundary aligned */ -}; +} __packed __aligned(2); /** * struct batadv_coded_packet - network coded packet @@ -404,9 +401,8 @@ struct batadv_coded_packet { uint8_t second_orig_dest[ETH_ALEN]; __be32 second_crc; __be16 coded_len; -}; +} __packed __aligned(2); -#pragma pack() /** * struct batadv_unicast_tvlv - generic unicast packet with tvlv payload
Unfortunately, not all compilers assumes the structures within a pack region also need be packed (e.g. metag), so need add a pack explicitly to satisfy all compilers. The related error (under metag with allmodconfig): MODPOST 2952 modules ERROR: "__compiletime_assert_431" [net/batman-adv/batman-adv.ko] undefined! ERROR: "__compiletime_assert_432" [net/batman-adv/batman-adv.ko] undefined! ERROR: "__compiletime_assert_429" [net/batman-adv/batman-adv.ko] undefined! ERROR: "__compiletime_assert_428" [net/batman-adv/batman-adv.ko] undefined! ERROR: "__compiletime_assert_423" [net/batman-adv/batman-adv.ko] undefined! Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- net/batman-adv/packet.h | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)