Warn when taking address of packed member

[rogfer01]

Summary

This patch implements PR#22821.

Taking the address of a packed member is dangerous since the reduced
alignment of the pointee is lost. This can lead to memory alignment
faults in some architectures if the pointer value is dereferenced.

This change adds a new warning to clang emitted when taking the address
of a packed member. A packed member is either a field/data member
declared as attribute((packed)) or belonging to a struct/class
declared as such. The associated flag is -Waddress-of-packed-member.
Conversions (either implicit or via a valid casting) to pointer types
with lower or equal alignment requirements (e.g. void* or char*)
silence the warning.

[aaron.ballman]

Thank you for working on this diagnostic! A few questions and comments below.

lib/Sema/SemaExpr.cpp

10518

Missing a full stop at the end of the sentence.

10519

Can use const auto * here instead of repeating the type name.

10527

I wonder if we should diagnose only when the resulting pointer is actually misaligned. For instance, the following code should be just fine:

struct __attribute__((packed)) S {
  int i;
};

void f(S s) {
  int *ip = &s.i; // Why should this warn?
}

Have you run this patch over any large code bases to see how high the false-positive rate is? How do you envision users silencing this diagnostic if they have a false-positive? Something like &(s.x) (since I you're not ignoring paren imp casts)?

test/Sema/address-packed.c

2

You should run clang-format over this file (or indeed, the entire patch).

3

I would split this test case into two, one in Sema for C and one in SemaCXX for C++.

[rogfer01]

lib/Sema/SemaExpr.cpp

10527

I think it should warn because in this case &s == &s.i and the alignment of the whole struct is 1 (due to the packed attribute) so s.i is also aligned to 1 which would not be the suitable alignment for an int.

I'm currently building Firefox, I'll add a comment once it ends.

Regarding silencing the diagnostic: -Wno-address-of-packed-member should do but I understand it might be too coarse in some cases.

[rogfer01]

Firefox build has ended and exposes 62 diagnostics, 44 unique ocurrences and 10 different diagnostics (shown below) in networking code.

taking address of packed member 'address' of class or structure 'sctp_state_cookie' may result in an unaligned pointer value
taking address of packed member 'addr' of class or structure 'sctp_ipv4addr_param' may result in an unaligned pointer value
taking address of packed member 'addr' of class or structure 'sctp_ipv6addr_param' may result in an unaligned pointer value
taking address of packed member 'aph' of class or structure 'sctp_asconf_addr_param' may result in an unaligned pointer value
taking address of packed member 'cookie' of class or structure 'sctp_cookie_echo_chunk' may result in an unaligned pointer value
taking address of packed member 'init' of class or structure 'sctp_init_chunk' may result in an unaligned pointer value
taking address of packed member 'laddress' of class or structure 'sctp_state_cookie' may result in an unaligned pointer value
taking address of packed member 'ph' of class or structure 'sctp_ipv4addr_param' may result in an unaligned pointer value
taking address of packed member 'ph' of class or structure 'sctp_ipv6addr_param' may result in an unaligned pointer value
taking address of packed member 'time_entered' of class or structure 'sctp_state_cookie' may result in an unaligned pointer value

which leads me to think that there might be cases where the field of a packed struct happens to be aligned for the type of the field itself. I'll add a check for this. I'll give it a spin.

[rogfer01]

Forget my wrong comment about packed fields that might actually be aligned. It does not make sense.

Regarding the way to selectively disable this, there is little syntax available at this point that we can leverage, so I propose to use parentheses as a way to disable the warning, e.g. &a.b may warn but &(a.b) will never warn. This may not be ideal but is in practice not that far from the classical ( (x = b) ) idiom used to silence the assignment-vs-equality warning.

Closer examination of Firefox code shows that memcpy/memcmp are used with packed members in the networking code. Being able to disable those safe uses by means of parentheses seems sensible to me.

I will upload a new patch with this change.

[rogfer01]

Only warn if the expression is of the form &a.x this way &(a.x) can be used to silence the warning.

[rogfer01]

Ping?

Thank you very much

[aaron.ballman]

In D20561#442051, @rogfer01 wrote:

Only warn if the expression is of the form &a.x this way &(a.x) can be used to silence the warning.

I think this is a reasonable idea, but would still like to make sure we have a low false-positive rate by default.

lib/Sema/SemaExpr.cpp

10519

The * binds to the identifier instead of the type; you should run the patch through clang-format (formatting issues are also in the test files).

10527

Ah, I forgot that the attribute also affected the alignment of the struct itself. Amending my false-positive idea, the following should be fine, shouldn't it?:

struct __attribute__((packed)) S {
  char c;
  int i;
  char c2;
};

void f(S s) {
  char *cp = &s.c;
  char *cp2 = &s.c2;
}

I think perhaps we should not diagnose if the member's type also has a one-byte alignment (or, for instance, uses alignas(1) to achieve that). What do you think?

test/SemaCXX/address-packed.cpp

2

This test should only really test the parts that are different from the C test (such as inheritance). The goal is to have the maximum test coverage with the minimum amount of testing code.

[rogfer01]

lib/Sema/SemaExpr.cpp

10527

Yes. I like the idea since the packed attribute has no effect on already 1-aligned data (like char). But note that C++11 does not allow reducing the alignment through alignas.

[aaron.ballman]

lib/Sema/SemaExpr.cpp

10527

Hmm, okay, I was not remembering properly; I thought one of alignas, __declspec(align), or __attribute__((aligned)) allowed for decreasing the alignment, but documentation implies otherwise.

[rogfer01]

Do not warn if the alignment of the type of the field is already 1 as packed does not have any effect on those fields.

[aaron.ballman]

This is getting really close, I mostly only have nits left.

lib/Sema/SemaExpr.cpp

10529

Comments should be complete sentences including capitalization and punctuation, so perhaps instead: // Packed does not have any effect.

test/Sema/address-packed.c

22

This is unused and can be removed.

28

Function should take (void) since this is C.

48

Extra space before }}.

52

Should put in a comment explaining that this produces no diagnostic because of the parens.

test/SemaCXX/address-packed.cpp

14

This is unused and can be removed.

78

Can we get one further test?

template <typename Ty>
class __attribute__((packed)) S {
  Ty X;
public:
  Ty *get() const {
    return &X; // no warning?
  }
};

I am fine if this does not warn. I am also fine if it only warns when there are instantiations of S for which Ty should warn (e.g., do an explicit instantiation with char that does not warn and another one with int that does).

[rogfer01]

It came to my mind that might be good idea adding one of those "fix-it" suggestions so the user knows it can silence the warning by using parentheses. What do you think?

[aaron.ballman]

In D20561#445730, @rogfer01 wrote:

It came to my mind that might be good idea adding one of those "fix-it" suggestions so the user knows it can silence the warning by using parentheses. What do you think?

I don't think that would be appropriate here. We usually only use fix-it hints when we know the code is incorrect and the hint is the proper way to fix the problem. In this case, when we trigger the warning, we want users to fix the underlying problem of taking the address of something that will result in an unaligned pointer, but the fix-it you're thinking of will merely silence the warning without fixing the underlying problem.

[rogfer01]

Well, the assignment-vs-comparison warning does emit a fix-it in Sema::DiagnoseAssignmentAsCondition(Expr *E)

Diag(Loc, diag::note_condition_assign_silence)
      << FixItHint::CreateInsertion(Open, "(")
      << FixItHint::CreateInsertion(Close, ")");

def note_condition_assign_silence : Note<
  "place parentheses around the assignment to silence this warning">;

so I thought it could be appropiate.

[rogfer01]

I think I wasn't clear with the purpose of the fix-it: there are a few cases where getting the address of an unaligned pointer is safe (i.e. false positives).

For instance, when I checked Firefox and Chromium there are cases where getting the address of an unaligned pointer is fine. For the particular case of these two browsers, they both use a library (usrsctp) that represents protocol data as packed structs. That library passes addresses of packed fields to memcpy and memmove which is OK.

The fix-it can help silencing the warning for those cases once deemed safe. This is the reason why I keep comparing the new warning to the assignment-vs-comparison warning: there may not be an error, just extra syntax (parentheses) can be used to silence the warning.

[rogfer01]

This change adds a fix-it suggesting parentheses to silence the warning.

[aaron.ballman]

In D20561#445824, @rogfer01 wrote:

I think I wasn't clear with the purpose of the fix-it: there are a few cases where getting the address of an unaligned pointer is safe (i.e. false positives).

For instance, when I checked Firefox and Chromium there are cases where getting the address of an unaligned pointer is fine. For the particular case of these two browsers, they both use a library (usrsctp) that represents protocol data as packed structs. That library passes addresses of packed fields to memcpy and memmove which is OK.

I think this is a false-positive that should be fixed.

The fix-it can help silencing the warning for those cases once deemed safe. This is the reason why I keep comparing the new warning to the assignment-vs-comparison warning: there may not be an error, just extra syntax (parentheses) can be used to silence the warning.

Assignment vs comparison is not a valuable comparison because that is a case relying on programmer intent (could be assignment, could be comparison, but there's no way to tell what's more likely, so use parens to disambiguate). In this case, there is code that could be dangerous (more likely) or benign (less likely), but adding parens does not disambiguate between two choices, it simply disables the diagnostic while leaving the semantics the same.

Fix-its are meant to be used when we can reasonably fix the user's code. Adding parens does *not* fix the user's code for the true positive case and is actually a dangerous code transformation (because it hides a true positive). I do not think this check should have a fix-it simply to silence false positives. If the false positive rate is that high, this should become a clang-tidy check instead (and I would still be opposed to the fix-it hint there as well).

include/clang/Basic/DiagnosticSemaKinds.td

5394

I don't think this note adds value. Placing parens around the expression does silence the warning for false-positives, but that seems more like a documentation issue than a diagnostic.

lib/Sema/SemaExpr.cpp

10537–10538

I do not think this diagnostic should have a FixIt hint. This isn't actually *fixing* the problem, it is simply a way to silence the diagnostic while retaining the same behavior.

test/SemaCXX/address-packed.cpp

93–94

Why this->X in the diagnostic when that's not what the user wrote?

[rogfer01]

include/clang/Basic/DiagnosticSemaKinds.td

5394

Ditto.

lib/Sema/SemaExpr.cpp

10537–10538

OK. I will remove it.

test/SemaCXX/address-packed.cpp

93–94

Probably clang introduces it internally when parsing an id-expression that happens to be a nonstatic data member because the diagnostic does include it.

That said this is going away as I'm removing the fix-it.

[rogfer01]

Remove fix-it as it is not suitable for this diagnostic.

[aaron.ballman]

I think the patch LGTM (with a minor formatting nit). @rsmith, what are your thoughts?

test/SemaCXX/address-packed.cpp

93

Formatting (may want to clang-format one last time before committing).

[rogfer01]

Ping?

Thanks a lot.

[aaron.ballman]

I think Richard has had enough time to comment. You can go ahead and commit, and if there are any issues, they can be handled post-commit. Thanks!

[rogfer01]

Thank you Aaron. I will commit ASAP.

[eugenis]

In D20561#446031, @aaron.ballman wrote:

In D20561#445824, @rogfer01 wrote:

I think I wasn't clear with the purpose of the fix-it: there are a few cases where getting the address of an unaligned pointer is safe (i.e. false positives).

For instance, when I checked Firefox and Chromium there are cases where getting the address of an unaligned pointer is fine. For the particular case of these two browsers, they both use a library (usrsctp) that represents protocol data as packed structs. That library passes addresses of packed fields to memcpy and memmove which is OK.

I think this is a false-positive that should be fixed.

This patch was committed without fixing the false positive case, why?

Could this warning be excluded from -Wall?

[rogfer01]

I've reverted the commit.

During implementation of the white list I felt the changes were too much for a follow up commit, so I prefer some feedback first.

[rogfer01]

This new patch adds a whitelist for memcpy, memmove, memset and memcmp.

I'm not entirely happy with the strategy that I'm using, where Parser communicates to Sema which is the LHS of a direct function call being parsed. Better approaches are of course welcome.

[rogfer01]

A build of Firefox does not show any false positive now.

[rsmith]

This still looks like it will have has lots of false positives for cases like:

struct __attribute__((packed)) A {
  char c;
  int n;
} a;
void *p = &a.n;

It also looks like it will now have false negatives for cases like:

memcpy(x, y, *&a.n);

I think whitelisting specific functions is not a reasonable approach here; instead, how about deferring the check until you see how the misaligned pointer is used? A couple of approaches seem feasible:

• you could extend the conversion-checking code in SemaChecking to look for such misaligned operations that are not immediately converted to a pointer type with suitable (lower) alignment requirements
• you could build a list in Sema of the cases that are pending a diagnostic, produce diagnostics at the end of the full-expression, and remove items from the list when you see a suitable conversion applied to them

In any case, this diagnostic should apply to reference binding as well as pointers.

GCC appears to check this as a separate step, at least after it applies its fold; for example:

struct __attribute__((packed, aligned(4))) S { char c[4]; int n; } s;
int k;
int &r = true ? s.n : k; // gcc rejects, "cannot bind paced field 's.S::n' to 'int&'
int &s = false ? s.n : k; // gcc accepts

[rogfer01]

Thanks @rsmith for the suggestions. I removed the whitelisting totally and changed the strategy. This patch implements your second suggestion which seemed more obvious to me. Since I expect the set of gathered misaligned member designations be small, I am using a plain SmallVector but maybe there is a more suitable structure.

I am now building Firefox and will post an update with the results.

[rogfer01]

Firefox build shows a couple of warnings in the sctp library that already gave warnings with the earlier patches.

That code has the following structure.

#define SCTP_PACKED __attribute__((packed))
#define SCTP_IDENTIFICATION_SIZE 16
// ...
/* state cookie header */
struct sctp_state_cookie {	/* this is our definition... */
	uint8_t identification[SCTP_IDENTIFICATION_SIZE];/* id of who we are */
	struct timeval time_entered;	/* the time I built cookie */
        // other fields
} SCTP_PACKED;

The warning is triggered by the following code (the other occurence is almost exact).

net->RTO = sctp_calculate_rto(stcb, asoc, net,
				      &cookie->time_entered,   // ← warning here!
				      sctp_align_unsafe_makecopy,
				      SCTP_RTT_FROM_NON_DATA);

the called function being declared as

uint32_t
sctp_calculate_rto(struct sctp_tcb *stcb,
		   struct sctp_association *asoc,
		   struct sctp_nets *net,
		   struct timeval *told,
		   int safe, int rtt_from_sack)
{
  ...

So I think that is a legitimate warning.

But the code is working (assuming this function is ever called that I didn't check). Checking the code, though, reveals a curious usage of told.

	/* Copy it out for sparc64 */
	if (safe == sctp_align_unsafe_makecopy) {
		old = &then;
		memcpy(&then, told, sizeof(struct timeval));
	} else if (safe == sctp_align_safe_nocopy) {
		old = told;
	} else {
		/* error */
		SCTP_PRINTF("Huh, bad rto calc call\n");
		return (0);
	}

which suggests that the code somehow knows that the pointer is unaligned and cannot be copied straightforwardly.

In order to silence the warning we can cast to void* and back to the original pointer type.

net->RTO = sctp_calculate_rto(stcb, asoc, net,
				      (struct timeval*)(void*)&cookie->time_entered, // note: the cast is because this pointer is unaligned
				      sctp_align_unsafe_makecopy,
				      SCTP_RTT_FROM_NON_DATA);

Which looks a bit ugly to me but clearly pinpoints a problem and can be wrapped in a macro.

#define UNALIGNED_ADDRESS(x) ((__typeof__(x))(void*)(x))

...

net->RTO = sctp_calculate_rto(stcb, asoc, net,
				      UNALIGNED_ADDRESS(&cookie->time_entered),
				      sctp_align_unsafe_makecopy,
				      SCTP_RTT_FROM_NON_DATA);

What do you think?

[eugenis]

This timeval thing looks like a legitimate warning to me.
I don't think the analysis should go beyond the function boundaries. If a callee expects timeval * as part of its signature it should get a properly aligned timeval *.

[rsmith]

I agree, that looks like correct behavior for the warning. Thanks!

lib/Sema/SemaCast.cpp

259–260 ↗

(On Diff #60951)

More generally, I think we should discard it if the destination of the cast is a suitably-aligned pointer type. (For instance, casting to char* should be OK, and casting an attribute((packed,aligned(2))) to a 2-byte-aligned pointer type should be OK.)

[rogfer01]

Following @rsmith suggestion, the diagnostic is silenced if the address is converted to a pointer with lower or equal alignment requirements.

[rogfer01]

Ping?

Thank you very much.

[rogfer01]

Ping? Any further comments, thoughts?

Thank you very much.

[aaron.ballman]

In D20561#466545, @rogfer01 wrote:

Ping? Any further comments, thoughts?

Thank you very much.

The usual practice is to ping after a week has gone by, btw. There were standards meetings this week, so that may be delaying some of the comments on this thread.

[rogfer01]

Ping?

Any comment on this?

Thank you very much.

[rogfer01]

Ping?

Looking forward any further comments before committing.

Thank you very much.

[rogfer01]

If there are not any objections I will commit this tomorrow.

[aaron.ballman]

In D20561#482963, @rogfer01 wrote:

If there are not any objections I will commit this tomorrow.

If @rsmith doesn't comment by tomorrow, then I think any feedback can be handled post-commit instead.

[jyknight]

This seems to trigger even for the implicitly generated copier of a packed struct. E.g.

#include <sys/epoll.h>

void copyit(epoll_event&out, const epoll_event &in) {
  out = in;
}

Is that as intended?

[jyknight]

Regardless, I think this should not have added a new un-disableable error, but instead only a default-on warning.

The ""binding reference to packed member" error is firing on some of our code, and even if it's not a false-positive, it should be possible to disable it until the code is modified.

[rogfer01]

Reverted.

[rogfer01]

I suggest that we handle the reference binding diagnostic in another change and leave this one only for the address taking diagnostic.

[rogfer01]

In D20561#484421, @jyknight wrote:

This seems to trigger even for the implicitly generated copier of a packed struct. E.g.

#include <sys/epoll.h>

void copyit(epoll_event&out, const epoll_event &in) {
  out = in;
}

Is that as intended?

No, it wasn't. It seems to happen as well for the implicit copy constructor as well.

#include <sys/epoll.h>

void copyit2(epoll_event foo);
void copyit(epoll_event &out) { copyit2(out); }

clang++ -c test.cc
In file included from test.cc:1:
/usr/include/x86_64-linux-gnu/sys/epoll.h:87:8: error: binding reference to packed member 'data' of class or structure 'epoll_event'
struct epoll_event
       ^~~~~~~~~~~
test.cc:4:41: note: implicit copy constructor for 'epoll_event' first required here
void copyit(epoll_event &out) { copyit2(out); }
                                        ^
1 error generated.

[rogfer01]

I've opened https://llvm.org/bugs/show_bug.cgi?id=28571 to track the reference binding issue.

[rogfer01]

Remove the diagnostic for the binding of references.

[rogfer01]

I plan to commit this today. It only diagnoses address-taking of packed fields.

Reference binding to packed fields will be addressed in another change as proper support may involve codegen changes which are out of the scope of this work.

Any further comments are welcome, as usual.

[aaron.ballman]

In D20561#486994, @rogfer01 wrote:

I plan to commit this today. It only diagnoses address-taking of packed fields.

Please wait until someone has had the chance to review before committing (the fix does not appear trivial and the original code broke code). I'm on vacation today (in theory), but perhaps @rsmith will have a chance to review.

~Aaron

Reference binding to packed fields will be addressed in another change as proper support may involve codegen changes which are out of the scope of this work.

Any further comments are welcome, as usual.

[rogfer01]

Please wait until someone has had the chance to review before committing (the fix does not appear trivial and the original code broke code). I'm on vacation today (in theory), but perhaps @rsmith will have a chance to review.

Sure. Thanks.

[rogfer01]

Hi, friendly ping.

@rsmith any further comment on this?

Thank you very much.

[MatzeB]

The sanitizer code triggers this warning for code that looks conceptually like this:

typedef struct Bla { char bar; int foo; } __attribute__((packed));

uintptr_t getu(struct Bla *b) { return (uintptr_t)&b->foo; }

Resulting in:

taking address of packed member 'foo' of class or structure
      'Bla' may result in an unaligned pointer value
      [-Waddress-of-packed-member]

Of course the warning can be silenced with return (uintptr_t)(char*)&b->foo; still casting to an int type seems like a benign case for this warning so maybe we should exclude that?

[royger]

Hello,

This addition is silly, and too noisy for real world usage.

For once, it doesn't really check whether the access is actually unaligned or not, and then on x86 for example unaligned accesses are allowed (although with a performance penalty). Please either make this only complain about real unaligned accesses, or remove it. I don't think it's going to be used by any real-world projects, since it's too noise and gives false positives.

[rogfer01]

In D20561#662922, @royger wrote:

Hi,

some targets do not support unaligned accesses so this warning is valuable in terms of portability. In platforms where unaligned accesses are allowed, it may happen that such accesses incur in a performance penalty. So there is also value in terms of performance.

We fixed all identified false positives in later patches to this one. So maybe you want to check against trunk clang. If trunk still diagnoses false positives, please report them to me and I will be more than happy to fix them.

Kind regards.

[royger]

In D20561#662959, @rogfer01 wrote:

We fixed all identified false positives in later patches to this one. So maybe you want to check against trunk clang. If trunk still diagnoses false positives, please report them to me and I will be more than happy to fix them.

Can you tell me which revisions are those? It seems like they are not contained in clang 4.0 AFAICT.

[rogfer01]

In D20561#662963, @royger wrote:

In D20561#662959, @rogfer01 wrote:

We fixed all identified false positives in later patches to this one. So maybe you want to check against trunk clang. If trunk still diagnoses false positives, please report them to me and I will be more than happy to fix them.

Can you tell me which revisions are those? It seems like they are not contained in clang 4.0 AFAICT.

https://reviews.llvm.org/rL283304
https://reviews.llvm.org/rL286798

The fixes for the false positives are in the second change but the first is required for the second to apply cleanly.

Regards

[kimgr]

... and both revisions should be in the 4.0 branch (taken from r291814)

I was looking forward to this warning to help iron out alignment issues at compile-time instead of runtime (our ARM CPUs don't like unaligned access). @royger, can you expand a little on what you mean by

For once, it doesn't really check whether the access is actually unaligned or no

? Isn't that exactly what it does? Or do you mean in relation to whether the target allows unaligned access or not?

[royger]

In D20561#663006, @kimgr wrote:

... and both revisions should be in the 4.0 branch (taken from r291814)

I was looking forward to this warning to help iron out alignment issues at compile-time instead of runtime (our ARM CPUs don't like unaligned access). @royger, can you expand a little on what you mean by

For once, it doesn't really check whether the access is actually unaligned or no

? Isn't that exactly what it does? Or do you mean in relation to whether the target allows unaligned access or not?

Take the following example structure:

typedef union __attribute__((__packed__)) segment_attributes {
    uint16_t bytes;
    struct
    {
        uint16_t type:4;    /* 0;  Bit 40-43 */
        uint16_t s:   1;    /* 4;  Bit 44 */
        uint16_t dpl: 2;    /* 5;  Bit 45-46 */
        uint16_t p:   1;    /* 7;  Bit 47 */
        uint16_t avl: 1;    /* 8;  Bit 52 */
        uint16_t l:   1;    /* 9;  Bit 53 */
        uint16_t db:  1;    /* 10; Bit 54 */
        uint16_t g:   1;    /* 11; Bit 55 */
        uint16_t pad: 4;
    } fields;
} segment_attributes_t;


struct __attribute__((__packed__)) segment_register {
    uint16_t   sel;
    segment_attributes_t attr;
    uint32_t   limit;
    uint64_t   base;
};

clang is complaining about:

vmx.c:973:38: error: taking address of packed member 'base' of class or structure 'segment_register' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        __vmread(GUEST_CS_BASE,     &reg->base);
                                     ^~~~~~~~~

But "base" is indeed aligned (it would be at the same offset without the packed attribute), so I'm not sure why it complains.

[joerg]

It is not true that all false positives have been fixed, some of the more complex cases involving nested data structures are still open.

@royger: Your example is missing explicit alignment. packed has two side effects: remove internal padding and set the alignment to 1. That means that the offset of base doesn't matter so much because reg itself is not necessarily aligned.

[rogfer01]

In D20561#663069, @joerg wrote:

It is not true that all false positives have been fixed, some of the more complex cases involving nested data structures are still open.

I could not find any in bugzilla after a quick search. So please feel free to point me to them (or CC) if you identify more false positives. I'm interested in fixing them.

Thanks!

[royger]

In D20561#663069, @joerg wrote:

@royger: Your example is missing explicit alignment. packed has two side effects: remove internal padding and set the alignment to 1. That means that the offset of base doesn't matter so much because reg itself is not necessarily aligned.

Does this means that it's "working as intended" then? I could expect this to complain when doing something like:

struct __attribute__((__packed__)) bar {
    uint64_t x1;
    uint16_t x2;
    uint64_t x3;
};

&bar->x3;

But not in the previous case.

[joerg]

The last review left out the case of naturally aligned packed members, IIRC. I have to go over the warning list in NetBSD again, but I'm moderately sure it is not fixed.

The better example is:

struct __attribute__((__packed__)) bar {
    uint16_t x1;
    uint16_t x2;
};

&bar->x2;

...which is not aligned.

[royger]

Ping?

It's not clear to me whether upstream is going to do something about this or not. I would like to know in case I need to start passing "-Wno-address-of-packed-member" around.

[joerg]

Yes, the project is interested on reducing the number of false positives. The example you gave is *not* a FP, but exactly the kind of situation the warning is supposed to trigger on.

[leonardchan]

I think I found a false negative with this where if the member is accessed from a packed struct type returned from a function, the warning does not appear:

typedef struct {
  uint8_t a;
  uint32_t b; 
} __attribute__((packed)) registers_t;

registers_t *Class::method() { return registers_; }

void Class::func() {
  other_func(&method()->b);  // I think the warning should appear here?
}

include/clang/Basic/DiagnosticSemaKinds.td

def warn_binding_null_to_reference : Warning<
   "binding dereferenced null pointer to reference has undefined behavior">,
   InGroup<NullDereference>;
 def note_indirection_through_null : Note<
   "consider using __builtin_trap() or qualifying pointer with 'volatile'">;
 def warn_pointer_indirection_from_incompatible_type : Warning<
   "dereference of type %1 that was reinterpret_cast from type %0 has undefined "
   "behavior">,
   InGroup<UndefinedReinterpretCast>, DefaultIgnore;
+def warn_taking_address_of_packed_member : Warning<
+  "taking address of packed member %0 of class or structure %q1 may result in an unaligned pointer value">,
+  InGroup<DiagGroup<"address-of-packed-member">>;
 
 def err_objc_object_assignment : Error<
   "cannot assign to class object (%0 invalid)">;
 def err_typecheck_invalid_operands : Error<
   "invalid operands to binary expression (%0 and %1)">;
 def err_typecheck_sub_ptr_compatible : Error<
   "%diff{$ and $ are not pointers to compatible types|"
   "pointers to incompatible types}0,1">;
 def ext_typecheck_ordered_comparison_of_pointer_integer : ExtWarn<
   "ordered comparison between pointer and integer (%0 and %1)">;

include/clang/Sema/Sema.h

public:
   /// \brief Stack containing information about each of the nested
   /// function, block, and method scopes that are currently active.
   ///
   /// This array is never empty.  Clients should ignore the first
   /// element, which is used to cache a single FunctionScopeInfo
   /// that's used to parse every top-level function.
   SmallVector<sema::FunctionScopeInfo *, 4> FunctionScopes;
 
+  /// \brief Stack containing the current LHS of a direct function call being
+  /// parsed. It is empty if we are not parsing an expression of the argument
+  /// list of a function call.
+  SmallVector<Expr *, 4> FunctionCallLHSStack;
+
   typedef LazyVector<TypedefNameDecl *, ExternalSemaSource,
                      &ExternalSemaSource::ReadExtVectorDecls, 2, 2>
     ExtVectorDeclsType;
 
   /// ExtVectorDecls - This is a list all the extended vector types. This allows
   /// us to associate a raw vector type with one of the ext_vector type names.
   /// This is only necessary for issuing pretty diagnostics.
   ExtVectorDeclsType ExtVectorDecls;
public:
   void recordUseOfEvaluatedWeak(const ExprT *E, bool IsRead=true) {
     if (!isUnevaluatedContext())
       getCurFunction()->recordUseOfWeak(E, IsRead);
   }
   
   void PushCompoundScope();
   void PopCompoundScope();
 
+  void PushFunctionCallLHS(Expr*);
+  void PopFunctionCallLHS();
+  Expr* CurrentFunctionCallLHS() const;
+
   sema::CompoundScopeInfo &getCurCompoundScope() const;
 
   bool hasAnyUnrecoverableErrorsInThisFunction() const;
 
   /// \brief Retrieve the current block, if any.
   sema::BlockScopeInfo *getCurBlock();
 
   /// \brief Retrieve the current lambda scope info, if any.
private:
   std::unique_ptr<llvm::DenseMap<TypeTagMagicValue, TypeTagData>>
       TypeTagForDatatypeMagicValues;
 
   /// \brief Peform checks on a call of a function with argument_with_type_tag
   /// or pointer_with_type_tag attributes.
   void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
                                 const Expr * const *ExprArgs);
 
+  /// \brief Diagnose if we are taking the address of a packed field
+  /// as this may be a problem if the pointer value is dereferenced.
+  void CheckAddressOfPackedMember(Expr *rhs, clang::SourceLocation &OpLoc);
+
   /// \brief The parser's current scope.
   ///
   /// The parser maintains this state here.
   Scope *CurScope;
 
   mutable IdentifierInfo *Ident_super;
   mutable IdentifierInfo *Ident___float128;
 

lib/Parse/ParseExpr.cpp

case tok::lesslessless: {  // p-e: p-e '<<<' argument-expression-list '>>>'
       if (Tok.is(tok::code_completion)) {
         Actions.CodeCompleteCall(getCurScope(), LHS.get(), None);
         cutOffParsing();
         return ExprError();
       }
 
       if (OpKind == tok::l_paren || !LHS.isInvalid()) {
         if (Tok.isNot(tok::r_paren)) {
+          Actions.PushFunctionCallLHS(LHS.get());
           if (ParseExpressionList(ArgExprs, CommaLocs, [&] {
                 Actions.CodeCompleteCall(getCurScope(), LHS.get(), ArgExprs);
               })) {
             (void)Actions.CorrectDelayedTyposInExpr(LHS);
             LHS = ExprError();
           } else if (LHS.isInvalid()) {
             for (auto &E : ArgExprs)
               Actions.CorrectDelayedTyposInExpr(E);
           }
+          Actions.PopFunctionCallLHS();
         }
       }
 
       // Match the ')'.
       if (LHS.isInvalid()) {
         SkipUntil(tok::r_paren, StopAtSemi);
       } else if (Tok.isNot(tok::r_paren)) {
         bool HadDelayedTypo = false;

lib/Sema/SemaChecking.cpp

void Sema::CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
 
   if (mismatch)
     Diag(ArgumentExpr->getExprLoc(), diag::warn_type_safety_type_mismatch)
         << ArgumentType << ArgumentKind
         << TypeInfo.LayoutCompatible << RequiredType
         << ArgumentExpr->getSourceRange()
         << TypeTagExpr->getSourceRange();
 }
+
+void Sema::CheckAddressOfPackedMember(Expr *rhs, clang::SourceLocation &OpLoc) {
+  Expr *FunctionCallLHS = CurrentFunctionCallLHS();
+  if (FunctionCallLHS)
+      FunctionCallLHS = FunctionCallLHS->IgnoreParens();
+
+  auto WhiteListedFunction = [](FunctionDecl *FDecl) -> bool {
+    if (FDecl) {
+      unsigned CMId = FDecl->getMemoryFunctionKind();
+      switch (CMId) {
+      case Builtin::BImemcmp:
+      case Builtin::BImemcpy:
+      case Builtin::BImemmove:
+      case Builtin::BImemset:
+        // We skip the diagnostic if we are calling these functions since they
+        // are a source of false positives.
+        return true;
+      default:
+        break;
+      }
+    }
+    return false;
+  };
+
+  if (auto *ULE = dyn_cast_or_null<UnresolvedLookupExpr>(FunctionCallLHS)) {
+    for (NamedDecl *D : ULE->decls()) {
+      if (auto *FDecl = dyn_cast<FunctionDecl>(D))
+        if (WhiteListedFunction(FDecl))
+          return;
+    }
+  } else if (auto *DRE = dyn_cast_or_null<DeclRefExpr>(FunctionCallLHS)) {
+    FunctionDecl *FDecl = nullptr;
+    if (DRE)
+      FDecl = dyn_cast_or_null<FunctionDecl>(DRE->getDecl());
+    if (FDecl && WhiteListedFunction(FDecl))
+      return;
+  }
+
+  const auto *ME = dyn_cast<MemberExpr>(rhs);
+  while (ME && isa<FieldDecl>(ME->getMemberDecl())) {
+    QualType BaseType = ME->getBase()->getType();
+    if (ME->isArrow())
+      BaseType = BaseType->getPointeeType();
+    RecordDecl *RD = BaseType->getAs<RecordType>()->getDecl();
+
+    ValueDecl *MD = ME->getMemberDecl();
+    bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
+    if (ByteAligned) // Attribute packed does not have any effect.
+      break;
+
+    if (!ByteAligned &&
+        (RD->hasAttr<PackedAttr>() || (MD->hasAttr<PackedAttr>()))) {
+      Diag(OpLoc, diag::warn_taking_address_of_packed_member)
+          << MD << RD << rhs->getSourceRange();
+      break;
+    }
+    ME = dyn_cast<MemberExpr>(ME->getBase());
+  }
+}
+

lib/Sema/SemaExpr.cpp

QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) {
 
   // OpenCL v2.0 s6.12.5 - The unary operators & cannot be used with a block.
   if (getLangOpts().OpenCL && OrigOp.get()->getType()->isBlockPointerType()) {
     Diag(OpLoc, diag::err_typecheck_unary_expr) << OrigOp.get()->getType()
                                                 << op->getSourceRange();
     return QualType();
   }
 
+  CheckAddressOfPackedMember(OrigOp.get(), OpLoc);
+
   return Context.getPointerType(op->getType());
 }
 
 static void RecordModifiableNonNullParam(Sema &S, const Expr *Exp) {
   const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp);
   if (!DRE)
     return;
   const Decl *D = DRE->getDecl();
   if (!D)
     return;
   const ParmVarDecl *Param = dyn_cast<ParmVarDecl>(D);
   if (!Param)
     return;
   if (const FunctionDecl* FD = dyn_cast<FunctionDecl>(Param->getDeclContext()))
     if (!FD->hasAttr<NonNullAttr>() && !Param->hasAttr<NonNullAttr>())
       return;
   if (FunctionScopeInfo *FD = S.getCurFunction())
     if (!FD->ModifiedNonNullParams.count(Param))
       FD->ModifiedNonNullParams.insert(Param);
 }
 
 /// CheckIndirectionOperand - Type check unary indirection (prefix '*').
 static QualType CheckIndirectionOperand(Sema &S, Expr *Op, ExprValueKind &VK,
                                         SourceLocation OpLoc) {
   if (Op->isTypeDependent())
     return S.Context.DependentTy;
 
   ExprResult ConvResult = S.UsualUnaryConversions(Op);
if (LookupName(Result, getCurScope()) && Result.isSingleResult()) {
         Context.setBOOLDecl(TD);
     }
   }
   if (Context.getBOOLDecl())
     BoolT = Context.getBOOLType();
   return new (Context)
       ObjCBoolLiteralExpr(Kind == tok::kw___objc_yes, BoolT, OpLoc);
 }
+
+void Sema::PushFunctionCallLHS(Expr *e) { FunctionCallLHSStack.push_back(e); }
+
+void Sema::PopFunctionCallLHS() {
+  assert(!FunctionCallLHSStack.empty() && "Empty stack");
+  FunctionCallLHSStack.pop_back();
+}
+
+Expr *Sema::CurrentFunctionCallLHS() const {
+  return !FunctionCallLHSStack.empty() ? FunctionCallLHSStack.back() : nullptr;
+}

test/Sema/address-packed-member-whitelist.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct B {
+  int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+  struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+
+void *my_memcpy(void *dest, const void *src, size_t n);
+int my_memcmp(const void *s1, const void *s2, size_t n);
+void *my_memmove(void *dest, const void *src, size_t n);
+void *my_memset(void *s, int c, size_t n);
+
+int x;
+
+void foo(void) {
+  memcpy(&a.b, &b, sizeof(b));     // no-warning
+  memmove(&a.b, &b, sizeof(b));    // no-warning
+  memset(&a.b, 0, sizeof(b));      // no-warning
+  x = memcmp(&a.b, &b, sizeof(b)); // no-warning
+
+  (memcpy)(&a.b, &b, sizeof(b));     // no-warning
+  (memmove)(&a.b, &b, sizeof(b));    // no-warning
+  (memset)(&a.b, 0, sizeof(b));      // no-warning
+  x = (memcmp)(&a.b, &b, sizeof(b)); // no-warning
+
+  my_memcpy(&a.b, &b, sizeof(b));     // expected-warning {{packed member 'b' of class or structure 'A'}}
+  my_memmove(&a.b, &b, sizeof(b));    // expected-warning {{packed member 'b' of class or structure 'A'}}
+  my_memset(&a.b, 0, sizeof(b));      // expected-warning {{packed member 'b' of class or structure 'A'}}
+  x = my_memcmp(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+}

test/Sema/address-packed.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+};
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void g0(void) {
+  {
+    struct Ok ok;
+    f1(&ok.x); // no-warning
+    f2(&ok.c); // no-warning
+  }
+  {
+    struct Arguable arguable;
+    f2(&arguable.c0); // no-warning
+    f1(&arguable.x);  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+    f2(&arguable.c1); // no-warning
+  }
+  {
+    union UnionArguable arguable;
+    f2(&arguable.c); // no-warning
+    f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
+  }
+  {
+    ArguableT arguable;
+    f2(&arguable.c0); // no-warning
+    f1(&arguable.x);  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+    f2(&arguable.c1); // no-warning
+  }
+  {
+    struct Arguable *arguable = get_arguable();
+    // These do not produce any warning because of the parentheses.
+    f2(&(arguable->c0)); // no-warning
+    f1(&(arguable->x));  // no-warning
+    f2(&(arguable->c1)); // no-warning
+  }
+  {
+    ArguableT *arguable = get_arguable();
+    // These do not produce any warning because of the parentheses.
+    f2(&(arguable->c0)); // no-warning
+    f1(&(arguable->x));  // no-warning
+    f2(&(arguable->c1)); // no-warning
+  }
+}
+
+struct S1 {
+  char c;
+  int i __attribute__((packed));
+};
+
+int *g1(struct S1 *s1) {
+  return &s1->i; // expected-warning {{packed member 'i' of class or structure 'S1'}}
+}
+
+struct S2_i {
+  int i;
+};
+struct __attribute__((packed)) S2 {
+  char c;
+  struct S2_i inner;
+};
+
+int *g2(struct S2 *s2) {
+  return &s2->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2'}}
+}
+
+struct S2_a {
+  char c;
+  struct S2_i inner __attribute__((packed));
+};
+
+int *g2_a(struct S2_a *s2_a) {
+  return &s2_a->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2_a'}}
+}
+
+struct __attribute__((packed)) S3 {
+  char c;
+  struct {
+    int i;
+  } inner;
+};
+
+int *g3(struct S3 *s3) {
+  return &s3->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S3'}}
+}
+
+struct S4 {
+  char c;
+  struct __attribute__((packed)) {
+    int i;
+  } inner;
+};
+
+int *g4(struct S4 *s4) {
+  return &s4->inner.i; // expected-warning {{packed member 'i' of class or structure 'S4::(anonymous)'}}
+}
+
+struct S5 {
+  char c;
+  struct {
+    char c1;
+    int i __attribute__((packed));
+  } inner;
+};
+
+int *g5(struct S5 *s5) {
+  return &s5->inner.i; // expected-warning {{packed member 'i' of class or structure 'S5::(anonymous)'}}
+}

test/SemaCXX/address-packed-member-whitelist.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct B {
+  int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+  struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+extern "C" {
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+}
+
+void *my_memcpy(void *dest, const void *src, size_t n);
+int my_memcmp(const void *s1, const void *s2, size_t n);
+void *my_memmove(void *dest, const void *src, size_t n);
+void *my_memset(void *s, int c, size_t n);
+
+int x;
+
+void foo() {
+  memcpy(&a.b, &b, sizeof(b));     // no-warning
+  memmove(&a.b, &b, sizeof(b));    // no-warning
+  memset(&a.b, 0, sizeof(b));      // no-warning
+  x = memcmp(&a.b, &b, sizeof(b)); // no-warning
+
+  (memcpy)(&a.b, &b, sizeof(b));     // no-warning
+  (memmove)(&a.b, &b, sizeof(b));    // no-warning
+  (memset)(&a.b, 0, sizeof(b));      // no-warning
+  x = (memcmp)(&a.b, &b, sizeof(b)); // no-warning
+
+  ::memcpy(&a.b, &b, sizeof(b));     // no-warning
+  ::memmove(&a.b, &b, sizeof(b));    // no-warning
+  ::memset(&a.b, 0, sizeof(b));      // no-warning
+  x = ::memcmp(&a.b, &b, sizeof(b)); // no-warning
+
+  (::memcpy)(&a.b, &b, sizeof(b));     // no-warning
+  (::memmove)(&a.b, &b, sizeof(b));    // no-warning
+  (::memset)(&a.b, 0, sizeof(b));      // no-warning
+  x = (::memcmp)(&a.b, &b, sizeof(b)); // no-warning
+
+  my_memcpy(&a.b, &b, sizeof(b));     // expected-warning {{packed member 'b' of class or structure 'A'}}
+  my_memmove(&a.b, &b, sizeof(b));    // expected-warning {{packed member 'b' of class or structure 'A'}}
+  my_memset(&a.b, 0, sizeof(b));      // expected-warning {{packed member 'b' of class or structure 'A'}}
+  x = my_memcmp(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+}

test/SemaCXX/address-packed.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void g0() {
+  {
+    Foo::Arguable arguable;
+    f1(&arguable.x);   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+    f2(&arguable.c);   // no-warning
+    f3(&arguable.foo); // no-warning
+  }
+  {
+    Arguable arguable1;
+    Arguable &arguable(arguable1);
+    f1(&arguable.x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+    f2(&arguable.c);   // no-warning
+    f3(&arguable.foo); // no-warning
+  }
+  {
+    Arguable *arguable1;
+    Arguable *&arguable(arguable1);
+    f1(&arguable->x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+    f2(&arguable->c);   // no-warning
+    f3(&arguable->foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+    return &this->x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+    return &x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+    return &c; // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+    return &this->x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+    return &x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+    return &c; // no-warning
+  }
+};
+
+template <typename Ty>
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+    return &X; // expected-warning {{packed member 'X' of class or structure 'S<int>'}}
+               // expected-warning@-1 {{packed member 'X' of class or structure 'S<float>'}}
+  }
+};
+
+template <typename Ty>
+void h(Ty *);
+
+void g1() {
+  S<int> s1;
+  s1.get(); // expected-note {{in instantiation of member function 'S<int>::get'}}
+
+  S<char> s2;
+  s2.get();
+
+  S<float> s3;
+  s3.get(); // expected-note {{in instantiation of member function 'S<float>::get'}}
+}