llvm 14 warns with unaligned access for the packed struct.

[UmeshKalappa0]

llvm 14 warns like

:24:11: warning: field srcSyncOrNvDdr within 'struct info' is less aligned than 'union (unnamed union at :9:5)' and is usually due to 'struct info' being packed, which can lead to unaligned accesses [-Wunaligned-access] } srcSyncOrNvDdr; ^

https://godbolt.org/z/89o76xsxb

llvm 13 doesn't warns for the same chunk of code .

[efriedma-quic]

This warning was added in https://reviews.llvm.org/D116221. As far as I can tell, it's correctly triggering here according to the criteria set out in that patch.

That said, the warning could use some work. Having a unpacked struct nested in a packed struct is vaguely suspicious, but it's not undefined behavior. And the warning doesn't really explain how you might go about fixing the issue.

Note that the warning is only on by default for embedded ARM targets, so most users haven't seen the warning. Not sure why; if the point is to avoid undefined behavior, misaligned access is equally undefined on all targets. And if that isn't the point of the warning, I'm not sure what the point is.

CC @lenary @pbarrio

[lenary]

In this case, the fix is to put __attribute__((packed)) on the srcSync struct (which is what caused the union to have alignment of 2).

• We decided to only enable this for Arm targets where the compiler already thinks you're in strict alignment mode, because it is on the noisier side, but we think it does provide value. We wanted it downstream, but it made sense to us to contribute this upstream.
• The warning was specifically put in its own group so that it could be disabled - with -Wno-unaligned-access.
• I agree the wording could do with some improvement. Suggestions welcome.
• In the example given, the code is 'safe' (though it is obviously a trivial, reduced example), but it is also trivial to construct a case where the code becomes unsafe - pass the address of a.srcSyncOrNvDdr.srcSync to a function expecting a pointer to that struct. I'm not sure this is the only place where the information about alignment is lost, but we have found it to be a common one.
• I'm not sure this struct doesn't contain undefined behaviour. The uSrcSyncTimingMode field ends up at an alignment that is less than its natural alignment. (That said, I don't think it's fair for compiler developers to reply "this is undefined behaviour" at every opportunity, as the packed attribute is not actually in the C standard anyway).

[efriedma-quic]

Generally, for code we consider "suspicious", but not immediately wrong, we try to attach notes describing how to fix the warning.

The following is an example of a warning that works well in practice:

void f(int y) {
    int x;
    if (x = y) {
    }
}
<source>:3:11: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
    if (x = y) {
        ~~^~~
<source>:3:11: note: place parentheses around the assignment to silence this warning
    if (x = y) {
          ^
        (    )
<source>:3:11: note: use '==' to turn this assignment into an equality comparison
    if (x = y) {
          ^
          ==

Or consider -Waddress-of-packed-member; instead of diagnosing the struct itself, it diagnoses suspicious usage of that struct. Maybe won't catch certain cases, but it's much less likely to trigger false positives.

In terms of a pure wording fix, maybe something like the following:

warning: the alignment of the type of field srcSyncOrNvDdr is greater than the alignment of 'struct info'.
note: add '__attribute__((packed))' to 'union (unnamed union at :9:5)' to reduce its alignment.

We decided to only enable this for Arm targets where the compiler already thinks you're in strict alignment mode, because it is on the noisier side,

Some of the more annoying alignment bugs I've had to deal with involve issues when the compiler is not in strict alignment mode. For example, we generate ldrd, or a NEON load, and that causes an alignment fault.

I'm not sure this struct doesn't contain undefined behaviour. The uSrcSyncTimingMode field ends up at an alignment that is less than its natural alignment. (That said, I don't think it's fair for compiler developers to reply "this is undefined behaviour" at every opportunity, as the packed attribute is not actually in the C standard anyway).

As long as you don't take the address ("&" etc.) of the misaligned member, the compiler should reliably generate correct code. We have a bunch of infrastructure in the clang code generator to propagate the correct alignment in situations like this.

We should probably publish rules describing what is/isn't guaranteed, but I think a limited guarantee makes sense.

[clausecker]

This new error broke a bunch of ports on FreeBSD and some times it's not clear how to fix it. E.g. security/openvas now fails with the warning

FAILED: nasl/CMakeFiles/openvas_nasl_shared.dir/nasl_packet_forgery_v6.c.o 
/usr/bin/cc -DHAVE_LIBKSBA -DHAVE_NETSNMP -DOPENVASLIB_VERSION=\"22.7.5\" -DOPENVAS_CONF=\"/usr/local/etc/openvas/openvas.conf\" -DOPENVAS_FEED_LOCK_PATH=\"/var/lib/openvas/feed-update.lock\" -DOPENVAS_GPG_BASE_DIR=\"/usr/local/etc/openvas\" -DOPENVAS_NASL_VERSION=\"22.7.5\" -DOPENVAS_STATE_DIR=\"/var/lib/openvas\" -DOPENVAS_SYSCONF_DIR=\"/usr/local/etc/openvas\" -Dopenvas_nasl_shared_EXPORTS -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include -I/usr/local/include -I/usr/local/include/json-glib-1.0 -I/usr/local/include/gvm -I/usr/local/include/uuid -I/usr/local/include/p11-kit-1 -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing -D_FILE_OFFSET_BITS=64 -DLARGEFILE_SOURCE=1                                 -std=c11                                 -Wall                                 -Wextra                                 -Werror                                 -Wpedantic                                 -Wmissing-prototypes                                 -Wshadow                                 -Wsequence-point                                 -D_BSD_SOURCE                                 -D_ISOC11_SOURCE                                 -D_SVID_SOURCE                                 -D_DEFAULT_SOURCE -Wall -Wextra -fno-strict-aliasing -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing  -DNDEBUG -Wformat -Wformat-security -fPIC -MD -MT nasl/CMakeFiles/openvas_nasl_shared.dir/nasl_packet_forgery_v6.c.o -MF nasl/CMakeFiles/openvas_nasl_shared.dir/nasl_packet_forgery_v6.c.o.d -o nasl/CMakeFiles/openvas_nasl_shared.dir/nasl_packet_forgery_v6.c.o -c /usr/home/ports/main.ports/security/openvas/work/openvas-scanner-22.7.5/nasl/nasl_packet_forgery_v6.c
/usr/home/ports/main.ports/security/openvas/work/openvas-scanner-22.7.5/nasl/nasl_packet_forgery_v6.c:478:17: error: field tcpheader within 'struct v6pseudohdr' is less aligned than 'struct tcphdr' and is usually due to 'struct v6pseudohdr' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
  struct tcphdr tcpheader;
                ^
1 error generated.

for the code

struct v6pseudohdr
{
  struct in6_addr s6addr;
  struct in6_addr d6addr;
  u_short length;
  u_char zero1;
  u_char zero2;
  u_char zero3;
  u_char protocol;
  struct tcphdr tcpheader;
} __attribute__ ((packed));

As struct tcphdr is defined in a system header, it cannot be marked as packed. It's not clear to me how the code needs to be changed to avoid the error. Gcc seems to document __attribute__ ((packed)) as imbuing each field within structures marked as packed with the property of being possibly misaligned. Wouldn't it make more sense to warn/error out when a pointer to such a possibly misaligned field is taken and used in such a way that the “misaligned” property is lost?

[efriedma-quic]

it cannot be marked as packed. It's not clear to me how the code needs to be changed to avoid the error

It's not clear from your description why the struct is marked packed in the first place.

Gcc seems to document attribute ((packed)) as imbuing each field within structures marked as packed with the property of being possibly misaligned

Yes, clang works the same way, as long as you don't try to take the address of a field.

Wouldn't it make more sense to warn/error out when a pointer to such a possibly misaligned field is taken and used in such a way that the “misaligned” property is lost?

That is something we could detect, yes.

[clausecker]

Yes, clang works the same way, as long as you don't try to take the address of a field.

So there's nothing wrong with this code? Then I don't get why this error is indicated for the structure.