Fix ARM BUILD_BUG_ON() errors with batman-adv

  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(-)
  

-----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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlKaRsQACgkQpGgxIkP9cwdDnACeJGU2FXi48yjDwZ2YcPCJBruq
vKgAnRc4p1dukczVgFATo2wb+Bflpdom
=RrAs
-----END PGP SIGNATURE-----
  

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.
  

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 SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlKagnYACgkQpGgxIkP9cwe1KgCeMUkV6xpNOsM4Bilc1e1uHpCG
8BAAnjsHJ/AHn8COhwiZ5wrKiIpQ0CFa
=D4xa
-----END PGP SIGNATURE-----
  

-----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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlKbR6oACgkQpGgxIkP9cwdpYACgjcWLZ7EGFDatHTsByPz7idsY
YOgAnRq6JTmOv8zI3OrRiBRtOIprgOew
=Jxbq
-----END PGP SIGNATURE-----
  

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...
  

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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlKbnrgACgkQpGgxIkP9cwc0ygCdHPgWg1SxUmyQOyC1xR+uNzkr
zfYAn3cEXLSMfxTCkP3CYibehI7D66tA
=AYNJ
-----END PGP SIGNATURE-----
  

> 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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlKcgiYACgkQpGgxIkP9cwfeeQCfef0xDfD2TucJBhzoTmDfwA29
Oc4An3Etz3USDC0cf/+R+Gyx8LEtpq16
=XZn5
-----END PGP SIGNATURE-----
  

> 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.
  

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.