Warn-to-error on -Werror=cast-align

Warn-to-error on -Werror=cast-align

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 7049 bytes --]

In the BlueZ build, we apparently use stricter Warn to Error checking than what is currently set up for ELL.

I got my son a new Raspberry PI, and the first thing he tried to build was ELL and BlueZ...  On THAT PATFORM
ONLY, Raspbian (Raspbian 10 (buster)) with kernel 4.19, we get failures building ell/dbus.c.  Everything builds
correctly in the ELL tree, using standard bootstrap-configure, however when using -Werror=cast-align, I get the
following (see below) build issues in dbus.c, genl.c, dhcp-transport.c, plus a few unit tests...  In BlueZ
these errors show up with the *default* build procedure:

So my question for Marcel is:  Do we need the alignment check in BlueZ?

And for Denis and Marcel, Do we care if there are alignment warnings in ELL?

Here is a dump of the make output after:

$ export CFLAGS=-Werror=cast-align
$ ./bootstrap-configure

pi(a)raspberrypi:~/ell $ make -k
make --no-print-directory all-am
  CC       ell/genl.lo
ell/genl.c: In function ‘l_genl_attr_next’:
ell/genl.c:1677:5: error: cast increases required alignment of target type [-Werror=cast-align]
     (struct nlattr*)(((char*)(nla)) + \
     ^
ell/genl.c:1738:20: note: in expansion of macro ‘NLA_NEXT’
  attr->next_data = NLA_NEXT(nla, attr->next_len);
                    ^~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:1864: ell/genl.lo] Error 1
  CC       ell/dbus.lo
In file included from ell/dbus.c:38:
ell/dbus.c: In function ‘classic_free’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dbus.c:582:3: note: in expansion of macro ‘l_container_of’
   l_container_of(dbus, struct l_dbus_classic, super);
   ^~~~~~~~~~~~~~
ell/dbus.c: In function ‘classic_recv_message’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dbus.c:661:3: note: in expansion of macro ‘l_container_of’
   l_container_of(dbus, struct l_dbus_classic, super);
   ^~~~~~~~~~~~~~
ell/dbus.c: In function ‘classic_add_match’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dbus.c:812:3: note: in expansion of macro ‘l_container_of’
   l_container_of(dbus, struct l_dbus_classic, super);
   ^~~~~~~~~~~~~~
ell/dbus.c: In function ‘classic_remove_match’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dbus.c:836:3: note: in expansion of macro ‘l_container_of’
   l_container_of(dbus, struct l_dbus_classic, super);
   ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:1864: ell/dbus.lo] Error 1
  CC       ell/dhcp-transport.lo
In file included from ell/dhcp-transport.c:44:
ell/dhcp-transport.c: In function ‘_dhcp_default_transport_broadcast’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dhcp-transport.c:203:3: note: in expansion of macro ‘l_container_of’
   l_container_of(s, struct dhcp_default_transport, super);
   ^~~~~~~~~~~~~~
ell/dhcp-transport.c: In function ‘_dhcp_default_transport_send’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dhcp-transport.c:246:3: note: in expansion of macro ‘l_container_of’
   l_container_of(s, struct dhcp_default_transport, super);
   ^~~~~~~~~~~~~~
ell/dhcp-transport.c: In function ‘_dhcp_default_transport_bind’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dhcp-transport.c:316:3: note: in expansion of macro ‘l_container_of’
   l_container_of(s, struct dhcp_default_transport, super);
   ^~~~~~~~~~~~~~
ell/dhcp-transport.c: In function ‘_dhcp_default_transport_open’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dhcp-transport.c:454:3: note: in expansion of macro ‘l_container_of’
   l_container_of(s, struct dhcp_default_transport, super);
   ^~~~~~~~~~~~~~
ell/dhcp-transport.c: In function ‘_dhcp_default_transport_close’:
ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
ell/dhcp-transport.c:476:3: note: in expansion of macro ‘l_container_of’
   l_container_of(s, struct dhcp_default_transport, super);
   ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:1864: ell/dhcp-transport.lo] Error 1
  CC       unit/test-genl-msg.o
unit/test-genl-msg.c: In function ‘parse_set_station’:
unit/test-genl-msg.c:63:10: error: cast increases required alignment of target type [-Werror=cast-align]
  nlmsg = (struct nlmsghdr *) set_station_request;
          ^
unit/test-genl-msg.c: In function ‘parse_set_rekey_offload’:
unit/test-genl-msg.c:159:10: error: cast increases required alignment of target type [-Werror=cast-align]
  nlmsg = (struct nlmsghdr *) set_rekey_offload_request;
          ^
unit/test-genl-msg.c: In function ‘parse_libnl_nested’:
unit/test-genl-msg.c:287:10: error: cast increases required alignment of target type [-Werror=cast-align]
  nlmsg = (struct nlmsghdr *) libnl_nested;
          ^
cc1: all warnings being treated as errors
make[1]: *** [Makefile:1848: unit/test-genl-msg.o] Error 1
  CC       unit/test-dbus-watch.o
In file included from ./ell/ell.h:23,
                 from unit/test-dbus-watch.c:30:
unit/test-dbus-watch.c: In function ‘test_add_match’:
./ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
unit/test-dbus-watch.c:111:3: note: in expansion of macro ‘l_container_of’
   l_container_of(dbus, struct filter_test_state, dbus);
   ^~~~~~~~~~~~~~
unit/test-dbus-watch.c: In function ‘test_remove_match’:
./ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
   (type *)((char *) __mptr - offsetof(type, member)); \
   ^
unit/test-dbus-watch.c:127:3: note: in expansion of macro ‘l_container_of’
   l_container_of(dbus, struct filter_test_state, dbus);
   ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:1848: unit/test-dbus-watch.o] Error 1
make[1]: Target 'all-am' not remade because of errors.
make: *** [Makefile:1199: all] Error 2
pi(a)raspberrypi:~/ell $ 

Re: Warn-to-error on -Werror=cast-align

[=?unknown-8bit?q?Micha=C5=82?= Lowas-Rzechonek]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

On 12/29, Gix, Brian wrote:
> In the BlueZ build, we apparently use stricter Warn to Error checking than what is currently set up for ELL.
> 
> I got my son a new Raspberry PI, and the first thing he tried to build was ELL and BlueZ...  On THAT PATFORM
> ONLY, Raspbian (Raspbian 10 (buster)) with kernel 4.19, we get failures building ell/dbus.c.  Everything builds
> correctly in the ELL tree, using standard bootstrap-configure, however when using -Werror=cast-align, I get the
> following (see below) build issues in dbus.c, genl.c, dhcp-transport.c, plus a few unit tests...  In BlueZ
> these errors show up with the *default* build procedure:
> 
(...)
> ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
>    (type *)((char *) __mptr - offsetof(type, member)); \
>    ^
> ell/dbus.c:582:3: note: in expansion of macro ‘l_container_of’
>    l_container_of(dbus, struct l_dbus_classic, super);
>    ^~~~~~~~~~~~~~

FWIW, I've encountered that on iMX.6 as well. I've fixed that with:

diff --git a/ell/util.h b/ell/util.h
index 4f20ef0..fe7c7e2 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -37,7 +37,7 @@ extern "C" {
 
 #define l_container_of(ptr, type, member) ({                           \
                const __typeof__(((type *) 0)->member) *__mptr = (ptr); \
-               (type *)((char *) __mptr - offsetof(type, member));     \
+               (type *)(void *)((char *) __mptr - offsetof(type, member));     \
        })
 
 #define likely(x)   __builtin_expect(!!(x), 1)

But I'm not sure this is correct...

regards
-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: Warn-to-error on -Werror=cast-align

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

Hi Marcel && Denis,

On Mon, 2019-12-30 at 08:44 +0100, Michał Lowas-Rzechonek wrote:
> On 12/29, Gix, Brian wrote:
> > (...)
> > ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
> >    (type *)((char *) __mptr - offsetof(type, member)); \
> >    ^
> > ell/dbus.c:582:3: note: in expansion of macro ‘l_container_of’
> >    l_container_of(dbus, struct l_dbus_classic, super);
> >    ^~~~~~~~~~~~~~
> 
> FWIW, I've encountered that on iMX.6 as well. I've fixed that with:
> 
> diff --git a/ell/util.h b/ell/util.h
> index 4f20ef0..fe7c7e2 100644
> --- a/ell/util.h
> +++ b/ell/util.h
> @@ -37,7 +37,7 @@ extern "C" {
>  
>  #define l_container_of(ptr, type, member) ({                           \
>                 const __typeof__(((type *) 0)->member) *__mptr = (ptr); \
> -               (type *)((char *) __mptr - offsetof(type, member));     \
> +               (type *)(void *)((char *) __mptr - offsetof(type, member));     \
>         })


Any opinion on this change?  It is causing us build fails on some platforms (including Raspberry PI 4).

I have not yet had a chance to run the battery of unit tests with this change in ELL, and am not even really
sure of the if this warning means that there could be some unforseen impact.

Otherwise, perhaps we should just create an ELL patch that makes clears up these warnings (Michał?).

If this is done, we should make sure it gets all of them, and not just the dbus.c instances.


>  
>  #define likely(x)   __builtin_expect(!!(x), 1)
> 
> But I'm not sure this is correct...
> 
> regards

Re: Warn-to-error on -Werror=cast-align

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

Hi Brian,

>>> (...)
>>> ell/util.h:40:3: error: cast increases required alignment of target type [-Werror=cast-align]
>>>   (type *)((char *) __mptr - offsetof(type, member)); \
>>>   ^
>>> ell/dbus.c:582:3: note: in expansion of macro ‘l_container_of’
>>>   l_container_of(dbus, struct l_dbus_classic, super);
>>>   ^~~~~~~~~~~~~~
>> 
>> FWIW, I've encountered that on iMX.6 as well. I've fixed that with:
>> 
>> diff --git a/ell/util.h b/ell/util.h
>> index 4f20ef0..fe7c7e2 100644
>> --- a/ell/util.h
>> +++ b/ell/util.h
>> @@ -37,7 +37,7 @@ extern "C" {
>> 
>> #define l_container_of(ptr, type, member) ({                           \
>>                const __typeof__(((type *) 0)->member) *__mptr = (ptr); \
>> -               (type *)((char *) __mptr - offsetof(type, member));     \
>> +               (type *)(void *)((char *) __mptr - offsetof(type, member));     \
>>        })
> 
> 
> Any opinion on this change?  It is causing us build fails on some platforms (including Raspberry PI 4).
> 
> I have not yet had a chance to run the battery of unit tests with this change in ELL, and am not even really
> sure of the if this warning means that there could be some unforseen impact.
> 
> Otherwise, perhaps we should just create an ELL patch that makes clears up these warnings (Michał?).
> 
> If this is done, we should make sure it gets all of them, and not just the dbus.c instances.

I am fine with this patch, but I am also not the expert in memory layout of structs for different architectures. So if we do not encounter issues on x86 and ARM (I will test PowerPC later) then I am fine with changing this.

Regards

Marcel

Re: Warn-to-error on -Werror=cast-align

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

Hi Brian,

> Any opinion on this change?  It is causing us build fails on some platforms (including Raspberry PI 4).
>

I would think this is a compiler false positive.  If there was indeed an 
issue, then just casting to void as an intermediate step just hides it 
without fixing anything.

I'd attempt to figure out if the compiler is at fault, and if it is, 
then #pragma around this.

Regards,
-Denis

Re: Warn-to-error on -Werror=cast-align

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

Hi Denis, Marcel, 

On Tue, 2020-01-07 at 09:51 -0600, Denis Kenzior wrote:
> Hi Brian,
> 
> > Any opinion on this change?  It is causing us build fails on some platforms (including Raspberry PI 4).
> > 
> 
> I would think this is a compiler false positive.  If there was indeed an 
> issue, then just casting to void as an intermediate step just hides it 
> without fixing anything.

For what it is worth, the *usage* of l_container_of in all dbus.c instances could be attacked from a different
direction...

In dbus.c, various functions are passed a (struct l_dbus *) argument, and function of l_container_of recovers
the pointer to a (struct l_dbus_classic *) member called "super".

And this is l_dbus_classic:

struct l_dbus_classic {
	struct l_dbus super;
	void *auth_command;
	enum auth_state auth_state;
	struct l_hashmap *match_strings;
	int *fd_buf;
	unsigned int num_fds;
};


So because super is the first member, the "recovered container pointer" should *always* be equal to the
original pointer, and because the structure is *private to ell/dbus.c*, we can work around this warning by not
using l_container_of(), and instead simply casting the point directly in the 4 places the macro is used. The
only time this would not work is if a compiler decides to re-arrange the members of the container structure.

RFC patch:
diff --git a/ell/dbus.c b/ell/dbus.c
index 845ccb3..b4a1d42 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -102,7 +102,7 @@ struct l_dbus {
 };
 
 struct l_dbus_classic {
-       struct l_dbus super;
+       struct l_dbus super; /* this member *must* be first */
        void *auth_command;
        enum auth_state auth_state;
        struct l_hashmap *match_strings;
@@ -578,8 +578,7 @@ static void dbus_init(struct l_dbus *dbus, int fd)
 
 static void classic_free(struct l_dbus *dbus)
 {
-       struct l_dbus_classic *classic =
-               l_container_of(dbus, struct l_dbus_classic, super);
+       struct l_dbus_classic *classic = (struct l_dbus_classic *) dbus;
        unsigned int i;
 
        for (i = 0; i < classic->num_fds; i++)
@@ -657,8 +656,7 @@ static bool classic_send_message(struct l_dbus *dbus,
 
 static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
 {
-       struct l_dbus_classic *classic =
-               l_container_of(dbus, struct l_dbus_classic, super);
+       struct l_dbus_classic *classic = (struct l_dbus_classic *) dbus;
        int fd = l_io_get_fd(dbus->io);
        struct dbus_header hdr;
        struct msghdr msg;
@@ -808,8 +806,7 @@ static bool classic_add_match(struct l_dbus *dbus, unsigned int id,
                                const struct _dbus_filter_condition *rule,
                                int rule_len)
 {
-       struct l_dbus_classic *classic =
-               l_container_of(dbus, struct l_dbus_classic, super);
+       struct l_dbus_classic *classic = (struct l_dbus_classic *) dbus;
        char *match_str;
        struct l_dbus_message *message;
 
@@ -832,8 +829,7 @@ static bool classic_add_match(struct l_dbus *dbus, unsigned int id,
 
 static bool classic_remove_match(struct l_dbus *dbus, unsigned int id)
 {
-       struct l_dbus_classic *classic =
-               l_container_of(dbus, struct l_dbus_classic, super);
+       struct l_dbus_classic *classic = (struct l_dbus_classic *) dbus;
        char *match_str = l_hashmap_remove(classic->match_strings,
                                                L_UINT_TO_PTR(id));
        struct l_dbus_message *message;

Re: Warn-to-error on -Werror=cast-align

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

Hi Brian,

> So because super is the first member, the "recovered container pointer" should *always* be equal to the
> original pointer, and because the structure is *private to ell/dbus.c*, we can work around this warning by not
> using l_container_of(), and instead simply casting the point directly in the 4 places the macro is used. The
> only time this would not work is if a compiler decides to re-arrange the members of the container structure.

Unless of course the compiler decides to reorder the structure elements 
somehow.  So you'd also need to make the structure packed or play some 
other tricks and hope no-one rearranges the structure in the future.

Also, l_container_of is used in iwd.

And really, why are we trying to fix something that isn't broken? :)

Look carefully at what that macro is doing, it is casting to (char *) 
only for purposes of pointer arithmetic.  There's no possibility of any 
misalignment.  The compiler is just being silly.

Regards,
-Denis

Re: Warn-to-error on -Werror=cast-align

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On Tue, 2020-01-07 at 09:37 -0800, Brian Gix wrote:
> Hi Denis, Marcel, 
> 
> On Tue, 2020-01-07 at 09:51 -0600, Denis Kenzior wrote:
> > Hi Brian,
> > 
> > > Any opinion on this change?  It is causing us build fails on some platforms (including Raspberry PI 4).
> > > 
> > 
> > I would think this is a compiler false positive.  If there was indeed an 
> > issue, then just casting to void as an intermediate step just hides it 
> > without fixing anything.

A quicker fix, that only applies when the member container being recovered is the first member, is to change
the macro to consider 0 octet offsets, which I believe most compilers will efficiently optimize:


diff --git a/ell/util.h b/ell/util.h
index 4f20ef0..d1d848b 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -35,10 +35,10 @@
 extern "C" {
 #endif
 
-#define l_container_of(ptr, type, member) ({                           \
+#define l_container_of(ptr, type, member) (offsetof(type, member) ? ({ \
                const __typeof__(((type *) 0)->member) *__mptr = (ptr); \
                (type *)((char *) __mptr - offsetof(type, member));     \
-       })
+       }) : ((type *) ptr))
 
 #define likely(x)   __builtin_expect(!!(x), 1)
 #define unlikely(x) __builtin_expect(!!(x), 0)
 
<...>

Re: Warn-to-error on -Werror=cast-align

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

On Tue, 2020-01-07 at 11:56 -0600, Denis Kenzior wrote:
> Hi Brian,
> 
> > So because super is the first member, the "recovered container pointer" should *always* be equal to the
> > original pointer, and because the structure is *private to ell/dbus.c*, we can work around this warning by
> > not
> > using l_container_of(), and instead simply casting the point directly in the 4 places the macro is used.
> > The
> > only time this would not work is if a compiler decides to re-arrange the members of the container
> > structure.
> 
> Unless of course the compiler decides to reorder the structure elements 
> somehow.  So you'd also need to make the structure packed or play some 
> other tricks and hope no-one rearranges the structure in the future.
> 
> Also, l_container_of is used in iwd.
> 
> And really, why are we trying to fix something that isn't broken? :)

I guess I wasn't clear...   This is broken in BlueZ, which compiles everything with -Werror=cast-align,
when compiling for ARM-8 architectures.

> 
> Look carefully at what that macro is doing, it is casting to (char *) 
> only for purposes of pointer arithmetic.  There's no possibility of any 
> misalignment.  The compiler is just being silly.
> 
> Regards,
> -Denis

Re: Warn-to-error on -Werror=cast-align

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

Hi Denis,

On Tue, 2020-01-07 at 10:07 -0800, Brian Gix wrote:
> On Tue, 2020-01-07 at 11:56 -0600, Denis Kenzior wrote:
> > 

<...>

> > And really, why are we trying to fix something that isn't broken? :)
> 
> I guess I wasn't clear...   This is broken in BlueZ, which compiles everything with -Werror=cast-align,
> when compiling for ARM-8 architectures.
> 
> > Look carefully at what that macro is doing, it is casting to (char *) 
> > only for purposes of pointer arithmetic.  There's no possibility of any 
> > misalignment.  The compiler is just being silly.
> > 

If this is indeed the "compiler being silly" then this would seem to be an argument in *favor* of void-casting
the final macro output, before casting to the desired type...  This is Michał's original solution, and should
work pretty universally.  It is also what Marcel has signalled that he believes is acceptable.