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.