From 51e51a72265acc675e834f2a3f0e86f9ae294391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Wed, 15 Jan 2020 18:16:42 +0100 Subject: [PATCH] Low: fix compiler's complaints about possibly unaligned pointer access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > In function ‘format_peers’: main.c:230:14: > warning: taking address of packed member of ‘struct booth_site’ may > result in an unaligned pointer value [-Waddress-of-packed-member] > 230 | localtime(&s->last_recv)); > | ^~~~~~~~~~~~~ > In function ‘booth_tcp_open’: transport.c:656:42: > warning: taking address of packed member of ‘struct booth_site’ > may result in an unaligned pointer value [-Waddress-of-packed-member] > 656 | rv = connect_nonb(s, (struct sockaddr *)&to->sa6, ..., ...); > | ^~~~~~~~ > In function ‘message_recv’: transport.c:1108:7: > warning: taking address of packed member of ‘struct booth_site’ > may result in an unaligned pointer value [-Waddress-of-packed-member] > 1108 | time(&source->last_recv); > | ^~~~~~~~~~~~~~~~~~ and > In function ‘store_geo_attr’: attr.c:282:13: > warning: taking address of packed member of ‘struct geo_attr’ may > result in an unaligned pointer value [-Waddress-of-packed-member] > 282 | get_time(&a->update_ts); > | ^~~~~~~~~~~~~ > timer.h:43:48: note: in definition of macro ‘get_time’ > 43 | #define get_time(p) clock_gettime(BOOTH_CLOCK, p) > | ^ > In function ‘append_attr’: attr.c:337:18: > warning: taking address of packed member of ‘struct geo_attr’ may > result in an unaligned pointer value [-Waddress-of-packed-member] > 337 | if (is_time_set(&a->update_ts)) { > | ^~~~~~~~~~~~~ > attr.c:338:16: warning: taking address of packed member of > ‘struct geo_attr’ may result in an unaligned pointer value > [-Waddress-of-packed-member] > 338 | ts = wall_ts(&a->update_ts); > | ^~~~~~~~~~~~~ The solution for the affected, historically packed structs (which is the trigger of the possible unaligned access in the first place), assuming we want to carry on with this binary terseness, is that we assure the struct itself is always aligned per its first member. This first member is hence automatically guaranteed to be aligned just as well, and shall there be any other address-of referenced members, they immediately follow (rule being that these are ordered from the biggest natural alignment to the lowest, for which C11 _Alignof operator is used). We build on already established assumptions that data declarations are readily controllable with non-standardized attribute annotations. Shall this (or reliance on C11 construct, but hey, it's 2020) be in violation with universality (it apparently hadn't been so far), both "packed" and "aligned" are to be to be conditionally dropped together (which will also restore no-complaints state as a side effect). Signed-off-by: Jan Pokorný --- src/booth.h | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/src/booth.h b/src/booth.h index ea89e400..94c0f3cc 100644 --- a/src/booth.h +++ b/src/booth.h @@ -160,6 +160,8 @@ struct attr_msg { /* GEO attributes * attributes should be regularly updated. + * Regarding alignment and respective layout, see the commentary + * at struct booth_site. */ struct geo_attr { /** Update timestamp. */ @@ -171,7 +173,7 @@ struct geo_attr { /** Who set it (currently unused) struct booth_site *origin; */ -} __attribute__((packed)); +} __attribute__((packed, aligned(_Alignof(timetype)))); struct hmac { /** hash id, currently set to constant BOOTH_HASH */ @@ -282,6 +284,25 @@ typedef enum { /** @{ */ struct booth_site { + /* for a packed struct, we'd rather put subjects to address-of + operator first, in biggest-to-lowest _Alignof(typeof()) + order, since then it's enough to "just" align whole-struct + boundaries to _Alignof() and the misaligned + pointer access is no longer at risk; + alternatively, we may explicitly align respective members + at their original in-struct offsets, but that would just + introduce additional paddings */ + time_t last_recv; + union { + struct sockaddr_in sa4; + struct sockaddr_in6 sa6; + }; + + /* data locality related to the above use-by-reference members */ + unsigned short family; + int saddrlen; + int addrlen; + /** Calculated ID. See add_site(). */ int site_id; int type; @@ -300,16 +321,8 @@ struct booth_site { int index; uint64_t bitmask; - unsigned short family; - union { - struct sockaddr_in sa4; - struct sockaddr_in6 sa6; - }; - int saddrlen; - int addrlen; - /** statistics */ - time_t last_recv; + /* time_t last_recv; // previous location */ unsigned int sent_cnt; unsigned int sent_err_cnt; unsigned int resend_cnt; @@ -321,7 +334,7 @@ struct booth_site { /** last timestamp seen from this site */ uint32_t last_secs; uint32_t last_usecs; -} __attribute__((packed)); +} __attribute__((packed, aligned(_Alignof(time_t))));