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 <[email protected]>
---
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
--
1.7.11.7
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 <[email protected]>
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
>
--
Antonio Quartulli
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 <[email protected]>
>
> 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,
--
Antonio Quartulli
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.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
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