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

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 $

(...)

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

index 4f20ef0..fe7c7e2 100644
@@ -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 <[email protected]>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Hi Marcel && Denis,

Hi Brian,

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

Hi Brian,

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

Hi Denis, Marcel,

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:

index 845ccb3..b4a1d42 100644
@@ -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;

Hi Brian,

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

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

Hi Denis,

<...>

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.

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:

index 4f20ef0..d1d848b 100644
@@ -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)

<...>