From: Russell King rmk+kernel@arm.linux.org.uk
The following errors were observed on ARM during a randconfig build. This patch addresses them by ensuring that the batadv_header structure is appropriately packed; this structure contains three 8-bit elements so there should be no undesired side effect from this packing.
net/batman-adv/main.c: In function 'batadv_init': net/batman-adv/main.c:425:279: error: call to '__compiletime_assert_425' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_unicast_4addr_packet, src) != 10 net/batman-adv/main.c:426:267: error: call to '__compiletime_assert_426' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_unicast_packet, dest) != 4 net/batman-adv/main.c:427:275: error: call to '__compiletime_assert_427' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_unicast_tvlv_packet, dst) != 4 net/batman-adv/main.c:428:261: error: call to '__compiletime_assert_428' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_frag_packet, dest) != 4 net/batman-adv/main.c:429:271: error: call to '__compiletime_assert_429' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_icmp_packet, icmph.dst) != 4 net/batman-adv/main.c:430:277: error: call to '__compiletime_assert_430' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_icmp_packet_rr, icmph.dst) != 4
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- net/batman-adv/packet.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/batman-adv/packet.h b/net/batman-adv/packet.h index 207459b62966..4039f25794e0 100644 --- a/net/batman-adv/packet.h +++ b/net/batman-adv/packet.h @@ -171,7 +171,7 @@ struct batadv_header { /* the parent struct has to add a byte after the header to make * everything 4 bytes aligned again */ -}; +} __attribute__((packed));
/** * struct batadv_ogm_packet - ogm (routing protocol) packet
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 30/11/13 20:15, Russell King - ARM Linux wrote:
From: Russell King rmk+kernel@arm.linux.org.uk
The following errors were observed on ARM during a randconfig build. This patch addresses them by ensuring that the batadv_header structure is appropriately packed; this structure contains three 8-bit elements so there should be no undesired side effect from this packing.
net/batman-adv/main.c: In function 'batadv_init': net/batman-adv/main.c:425:279: error: call to '__compiletime_assert_425' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_unicast_4addr_packet, src) != 10 net/batman-adv/main.c:426:267: error: call to '__compiletime_assert_426' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_unicast_packet, dest) != 4 net/batman-adv/main.c:427:275: error: call to '__compiletime_assert_427' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_unicast_tvlv_packet, dst) != 4 net/batman-adv/main.c:428:261: error: call to '__compiletime_assert_428' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_frag_packet, dest) != 4 net/batman-adv/main.c:429:271: error: call to '__compiletime_assert_429' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_icmp_packet, icmph.dst) != 4 net/batman-adv/main.c:430:277: error: call to '__compiletime_assert_430' declared with attribute error: BUILD_BUG_ON failed: offsetof(struct batadv_icmp_packet_rr, icmph.dst) != 4
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- net/batman-adv/packet.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/batman-adv/packet.h b/net/batman-adv/packet.h index 207459b62966..4039f25794e0 100644 --- a/net/batman-adv/packet.h +++ b/net/batman-adv/packet.h @@ -171,7 +171,7 @@ struct batadv_header { /* the parent struct has to add a byte after the header to make * everything 4 bytes aligned again */ -}; +} __attribute__((packed));
Russell,
this attribute was there in the past and then was removed on purpose because on some architectures it makes the compiler generating very inefficient code (you can check http://article.gmane.org/gmane.org.freifunk.batman/8317 for more details).
So we removed the __packed attribute and we took care of properly aligning all the structures by adding "alignment/reserved members" when needed.
I don't know the ARM architecture at all (I don't know if the other batman-adv developers do), so could you please provide here some more details about why that static check is failing? We would like to address this issue differently rather than re-adding the __packed attribute.
Thanks a lot.
Best Regards,
- -- Antonio Quartulli
On Sat, Nov 30, 2013 at 09:12:52PM +0100, Antonio Quartulli wrote:
I don't know the ARM architecture at all (I don't know if the other batman-adv developers do), so could you please provide here some more details about why that static check is failing? We would like to address this issue differently rather than re-adding the __packed attribute.
The reason is this struct becomes a size of 4 bytes, even though it only contains three uint8_t members. This offsets the members of all structs that this struct is embedded in.
If you don't wish to fix this, then please make your subsystem depend on !ARM because it's otherwise impossible to fix and can never work on ARM.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/12/13 00:05, Russell King - ARM Linux wrote:
On Sat, Nov 30, 2013 at 09:12:52PM +0100, Antonio Quartulli wrote:
I don't know the ARM architecture at all (I don't know if the other batman-adv developers do), so could you please provide here some more details about why that static check is failing? We would like to address this issue differently rather than re-adding the __packed attribute.
The reason is this struct becomes a size of 4 bytes, even though it only contains three uint8_t members. This offsets the members of all structs that this struct is embedded in.
If you don't wish to fix this, then please make your subsystem depend on !ARM because it's otherwise impossible to fix and can never work on ARM.
I'd like to fix this.
Actually I can't really understand your explanation: struct batadv_header is always used inside another parent structure and the latter always has a uint8_t following the batadv_header, thus filling that gap and aligning it to 4bytes. I think this is also why we don't get this compiler error on any other architecture. Or am I missing something?
I'll install a toolchain for ARM and I'll try to inspect it better. If we have to make a change we should do it before 3.13 is release since this change could possibly alter the packet layout.
- -- Antonio Quartulli
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/12/13 01:27, Antonio Quartulli wrote:
On 01/12/13 00:05, Russell King - ARM Linux wrote:
On Sat, Nov 30, 2013 at 09:12:52PM +0100, Antonio Quartulli wrote:
I don't know the ARM architecture at all (I don't know if the other batman-adv developers do), so could you please provide here some more details about why that static check is failing? We would like to address this issue differently rather than re-adding the __packed attribute.
The reason is this struct becomes a size of 4 bytes, even though it only contains three uint8_t members. This offsets the members of all structs that this struct is embedded in.
If you don't wish to fix this, then please make your subsystem depend on !ARM because it's otherwise impossible to fix and can never work on ARM.
I'd like to fix this.
Actually I can't really understand your explanation: struct batadv_header is always used inside another parent structure and the latter always has a uint8_t following the batadv_header, thus filling that gap and aligning it to 4bytes. I think this is also why we don't get this compiler error on any other architecture. Or am I missing something?
I'll install a toolchain for ARM and I'll try to inspect it better. If we have to make a change we should do it before 3.13 is release since this change could possibly alter the packet layout.
It looks like that the ARM compiler cannot pack the structures properly.
So, given these two structures:
struct batadv_header { uint8_t packet_type; uint8_t version; uint8_t ttl; };
struct batadv_unicast_packet { struct batadv_header header; uint8_t ttvn; uint8_t dest[ETH_ALEN]; };
we have the compiler saying that offset_of dest in struct batadv_unicast_packet is 5.
This means that struct batadv_header is padded to 4 bytes even if it is enclosed in struct batadv_unicast_packet and a proper 1byte member is put right after it.
Am I wrong or this is a problem in the ARM compiler not doing the right assumption? On x86 and x86_64 offset_of dest is 4, as expected.
Regards,
- -- Antonio Quartulli
On Sun, Dec 01, 2013 at 03:28:58PM +0100, Antonio Quartulli wrote:
On 01/12/13 01:27, Antonio Quartulli wrote:
On 01/12/13 00:05, Russell King - ARM Linux wrote:
On Sat, Nov 30, 2013 at 09:12:52PM +0100, Antonio Quartulli wrote:
I don't know the ARM architecture at all (I don't know if the other batman-adv developers do), so could you please provide here some more details about why that static check is failing? We would like to address this issue differently rather than re-adding the __packed attribute.
The reason is this struct becomes a size of 4 bytes, even though it only contains three uint8_t members. This offsets the members of all structs that this struct is embedded in.
If you don't wish to fix this, then please make your subsystem depend on !ARM because it's otherwise impossible to fix and can never work on ARM.
I'd like to fix this.
Actually I can't really understand your explanation: struct batadv_header is always used inside another parent structure and the latter always has a uint8_t following the batadv_header, thus filling that gap and aligning it to 4bytes. I think this is also why we don't get this compiler error on any other architecture. Or am I missing something?
I'll install a toolchain for ARM and I'll try to inspect it better. If we have to make a change we should do it before 3.13 is release since this change could possibly alter the packet layout.
It looks like that the ARM compiler cannot pack the structures properly.
This is not a compiler bug. This is a bug in how you understand the C specifications.
we have the compiler saying that offset_of dest in struct batadv_unicast_packet is 5.
Correct.
This means that struct batadv_header is padded to 4 bytes even if it is enclosed in struct batadv_unicast_packet and a proper 1byte member is put right after it.
Am I wrong or this is a problem in the ARM compiler not doing the right assumption? On x86 and x86_64 offset_of dest is 4, as expected.
The C standards allow implementations to pad structures as they see fit for performance reasons. The only thing which C guarantees is that the order of the structure members are specified. Padding is allowed to be added between members and at the end of the structure.
The ARM compilers have for the last 20 years always aligned the size of structs to a multiple of 32 bits to ensure that members following a structure are appropriately aligned.
Changing this behaviour is completely out of the question: that would be similar to asking for the x86 compiler to change the way it lays out its structures.
The only solutions are: use the GCC packed attribute, redesign the structures, or just accept that it won't ever be usable on ARM.
Frankly, I don't care about having this protocol working on ARM. My report was just because it was found by one of my randconfig builds. I've learned my lesson now - don't report bugs...
The ARM compilers have for the last 20 years always aligned the size of structs to a multiple of 32 bits to ensure that members following a structure are appropriately aligned.
ARM has (or had) another one for the unwary, bitfields were also always 32bits.
...
The only solutions are: use the GCC packed attribute, redesign the structures...
It is probably enough to mark the inner structure containing the three byte fields 'packed'. Marking it aligned(1) might also have the desired effect. The outer structure should then be ok. But would need to use a specially named attribute so it doesn't get removed.
In the past I've had to replace a struct with an array in order to avoid a similar alignment rule for structs.
David
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/12/13 11:24, David Laight wrote:
...
The only solutions are: use the GCC packed attribute, redesign the structures...
It is probably enough to mark the inner structure containing the three byte fields 'packed'. Marking it aligned(1) might also have the desired effect. The outer structure should then be ok. But would need to use a specially named attribute so it doesn't get removed.
This may work with the structures I reported in a previous email, but it is not a good solution for us because we have other more complex substructs that cannot be packed that way.
I think we will simply duplicate the members and avoid substructs in our packets.
- -- Antonio Quartulli
From: Antonio Quartulli On 02/12/13 11:24, David Laight wrote:
...
The only solutions are: use the GCC packed attribute, redesign the structures...
It is probably enough to mark the inner structure containing the three byte fields 'packed'. Marking it aligned(1) might also have the desired effect. The outer structure should then be ok. But would need to use a specially named attribute so it doesn't get removed.
This may work with the structures I reported in a previous email, but it is not a good solution for us because we have other more complex substructs that cannot be packed that way.
You really don't want to have structures that need to be (say) 7 bytes long and contain a 32bit value that will always be correctly aligned.
IIRC adding __attribute__((aligned(1))) to a structure isn't the same as marking it 'packed' - it doesn't normally reduce the alignment for structures (but can be used to force a reduced alignment on a structure member - eg a 4byte aligned 64bit integer).
I don't have an arm compiler handy.
I think we will simply duplicate the members and avoid substructs in our packets.
Can be easiest...
David
From: Antonio Quartulli antonio@meshcoding.com Date: Mon, 02 Dec 2013 13:50:46 +0100
This may work with the structures I reported in a previous email, but it is not a good solution for us because we have other more complex substructs that cannot be packed that way.
I think we will simply duplicate the members and avoid substructs in our packets.
I would strongly suggest just making everything a multiple of 4 bytes in size.
From: Antonio Quartulli antonio@meshcoding.com Date: Sun, 01 Dec 2013 15:28:58 +0100
Am I wrong or this is a problem in the ARM compiler not doing the right assumption? On x86 and x86_64 offset_of dest is 4, as expected.
These alignment behaviors are defined by the processor ABI, there is no set of global rules that apply, so it behaves differently on different CPUs. What you observe is correct behavior for compilation on ARM processors.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/12/13 20:21, David Miller wrote:
From: Antonio Quartulli antonio@meshcoding.com Date: Sun, 01 Dec 2013 15:28:58 +0100
Am I wrong or this is a problem in the ARM compiler not doing the right assumption? On x86 and x86_64 offset_of dest is 4, as expected.
These alignment behaviors are defined by the processor ABI, there is no set of global rules that apply, so it behaves differently on different CPUs. What you observe is correct behavior for compilation on ARM processors.
Ok. So, as far as I understand from your and Russell's explanation, the only way to allow ARM to support batman-adv is to ensure that all the _structures_ (no matter if it is a parent structure or not) sent over the wire have a size multiple of 32bits.
If we want to fix this, I think we have to re-work a little more the packet layout before 3.13 is released.
People using batman-adv on ARM-equipped smartphones (and similar) will be happy about this :)
Thanks to both for the hints and for the explanations.
- -- Antonio Quartulli
From: Russell King - ARM Linux linux@arm.linux.org.uk Date: Sat, 30 Nov 2013 19:15:53 +0000
so there should be no undesired side effect from this packing.
There is a huge side effect from ever using the packed attribute, in that the compiler can assume absolutely nothing about the alignment of any object or sub-object of the type you apply this attribute to.
Even if it is "obvious" that some members will be aligned, the compiler cannot take advantage of this assumption because this attribute also means that an array of such elements might have arbitrary alignment. So you when you get a pointer to one of these objects, the compiler has to assume the worst possible case.
This means using 4 byte loads to load a 32-bit quantity, always, unconditionally, no matter what.
That's why we should do whatever is necessary to align things properly by hand, and use packed only as the last possible and least desirable resort.
I'm not applying this, please try work to implement this more acceptably first.
Thanks.
On Sat, Nov 30, 2013 at 04:05:47PM -0500, David Miller wrote:
From: Russell King - ARM Linux linux@arm.linux.org.uk Date: Sat, 30 Nov 2013 19:15:53 +0000
so there should be no undesired side effect from this packing.
There is a huge side effect from ever using the packed attribute, in that the compiler can assume absolutely nothing about the alignment of any object or sub-object of the type you apply this attribute to.
Even if it is "obvious" that some members will be aligned, the compiler cannot take advantage of this assumption because this attribute also means that an array of such elements might have arbitrary alignment. So you when you get a pointer to one of these objects, the compiler has to assume the worst possible case.
This means using 4 byte loads to load a 32-bit quantity, always, unconditionally, no matter what.
That's why we should do whatever is necessary to align things properly by hand, and use packed only as the last possible and least desirable resort.
I'm not applying this, please try work to implement this more acceptably first.
Okay, someone else fixes this problem then, I've no idea how this can be fixed.
David,
From: Russell King - ARM Linux linux@arm.linux.org.uk Date: Sat, 30 Nov 2013 19:15:53 +0000
so there should be no undesired side effect from this packing.
There is a huge side effect from ever using the packed attribute, in that the compiler can assume absolutely nothing about the alignment of any object or sub-object of the type you apply this attribute to.
Even if it is "obvious" that some members will be aligned, the compiler cannot take advantage of this assumption because this attribute also means that an array of such elements might have arbitrary alignment. So you when you get a pointer to one of these objects, the compiler has to assume the worst possible case.
This means using 4 byte loads to load a 32-bit quantity, always, unconditionally, no matter what.
That's why we should do whatever is necessary to align things properly by hand, and use packed only as the last possible and least desirable resort.
so far we have two feasible options how to handle batadv_header:
1) Russells patch 2) unrolling these 3 bytes of header into every packet structure
I understand your concerns for __packed in generally, but in this case it might not be that bad:
* batadv_header is only used by embedding it in other structs (like batadv_unicast_packet, batadv_broadcast_packet ...) which are well aligned - I guess there should be no performance problem when accessing through the parent structure (like unicast_packet->header.version) * there is one exception in batadv_interface_rx() where we use this batadv_header directly, but we can easily replace that by using e.g. batadv_bcast_packet instead. * It's easier to read and maintain than copying these 3 bytes in every other structure.
Therefore I'd suggest to pick Russels patch, I can send a fix for batadv_interface_rx() later, too.
Thanks, Simon
From: Simon Wunderlich sw@simonwunderlich.de Date: Mon, 2 Dec 2013 18:58:47 +0100
Therefore I'd suggest to pick Russels patch, I can send a fix for batadv_interface_rx() later, too.
You are ignoring the downside of Russel's patch which is that it negatively impacts many architectures, in that now the compiler can make no assumptions as to the alignment of a structure marked with "packed" and therefore, for example, a 32-bit load will be done with 4 byte loads, shifts, and masking.
Please just 4 byte align your structures. I can't believe that you cannot make this work cleanly.
Why can't you just include that fourth final byte in the batadv_header structure and have the sub-header types just interpret it differently or ignore it?
This is getting frustrating, I seemed clear to me that the non-packed suggestions given to you so far were both reasonable and easy to implement.
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
David Laight -
David Miller -
Russell King - ARM Linux -
Simon Wunderlich