This is an archive of the discontinued LLVM Phabricator instance.

Warn when taking address of packed member
ClosedPublic

Authored by rogfer01 on May 24 2016, 12:47 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

rogfer01 updated this revision to Diff 58198.May 24 2016, 12:47 AM
rogfer01 retitled this revision from to Warn when taking address of packed member.
rogfer01 updated this object.
rogfer01 added a reviewer: rsmith.
rogfer01 added a subscriber: cfe-commits.

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

lib/Sema/SemaExpr.cpp
10518 ↗(On Diff #58198)

Missing a full stop at the end of the sentence.

10519 ↗(On Diff #58198)

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

10527 ↗(On Diff #58198)

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
1 ↗(On Diff #58198)

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

2 ↗(On Diff #58198)

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

rogfer01 marked 4 inline comments as done.May 26 2016, 8:15 AM
rogfer01 added inline comments.
lib/Sema/SemaExpr.cpp
10527 ↗(On Diff #58198)

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.

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.

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 updated this revision to Diff 58760.May 27 2016, 2:14 AM

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

Ping?

Thank you very much

aaron.ballman edited edge metadata.May 31 2016, 6:22 AM

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 ↗(On Diff #58760)

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 ↗(On Diff #58760)

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
1 ↗(On Diff #58760)

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 marked 3 inline comments as done.May 31 2016, 7:47 AM
rogfer01 added inline comments.
lib/Sema/SemaExpr.cpp
10527 ↗(On Diff #58760)

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 added inline comments.May 31 2016, 1:56 PM
lib/Sema/SemaExpr.cpp
10527 ↗(On Diff #58760)

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 updated this revision to Diff 59178.Jun 1 2016, 1:07 AM
rogfer01 edited edge metadata.
rogfer01 marked an inline comment as done.

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.

rogfer01 marked an inline comment as done.Jun 1 2016, 1:08 AM

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

lib/Sema/SemaExpr.cpp
10529 ↗(On Diff #59178)

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

test/Sema/address-packed.c
21 ↗(On Diff #59178)

This is unused and can be removed.

27 ↗(On Diff #59178)

Function should take (void) since this is C.

47 ↗(On Diff #59178)

Extra space before }}.

51 ↗(On Diff #59178)

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

test/SemaCXX/address-packed.cpp
13 ↗(On Diff #59178)

This is unused and can be removed.

77 ↗(On Diff #59178)

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).

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?

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 marked 2 inline comments as done.Jun 1 2016, 8:59 AM

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.

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 marked 5 inline comments as done.Jun 1 2016, 9:40 AM
rogfer01 updated this revision to Diff 59241.Jun 1 2016, 9:42 AM

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

aaron.ballman requested changes to this revision.Jun 1 2016, 12:06 PM
aaron.ballman edited edge metadata.

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
5392 ↗(On Diff #59241)

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 ↗(On Diff #59241)

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
92–93 ↗(On Diff #59241)

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

This revision now requires changes to proceed.Jun 1 2016, 12:06 PM
rogfer01 marked 3 inline comments as done.Jun 2 2016, 12:11 AM
rogfer01 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5392 ↗(On Diff #59241)

Ditto.

lib/Sema/SemaExpr.cpp
10537–10538 ↗(On Diff #59241)

OK. I will remove it.

test/SemaCXX/address-packed.cpp
92–93 ↗(On Diff #59241)

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 updated this revision to Diff 59342.Jun 2 2016, 12:18 AM
rogfer01 edited edge metadata.
rogfer01 marked 3 inline comments as done.

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

aaron.ballman accepted this revision.Jun 2 2016, 6:19 AM
aaron.ballman edited edge metadata.

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

test/SemaCXX/address-packed.cpp
92 ↗(On Diff #59342)

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

This revision is now accepted and ready to land.Jun 2 2016, 6:19 AM
rogfer01 marked an inline comment as done.Jun 2 2016, 6:24 AM

Ping?

Thanks a lot.

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!

Thank you Aaron. I will commit ASAP.

This revision was automatically updated to reflect the committed changes.

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 reopened this revision.Jun 14 2016, 4:28 AM

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.

This revision is now accepted and ready to land.Jun 14 2016, 4:28 AM
rogfer01 updated this revision to Diff 60664.Jun 14 2016, 4:34 AM
rogfer01 edited edge metadata.
rogfer01 removed rL LLVM as the repository for this revision.

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.

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

rsmith edited edge metadata.Jun 14 2016, 11:50 AM

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 updated this revision to Diff 60951.Jun 16 2016, 2:09 AM
rogfer01 updated this object.
rogfer01 edited edge metadata.

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 added a comment.EditedJun 16 2016, 4:56 AM

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?

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 *.

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 marked an inline comment as done.Jun 17 2016, 6:21 AM
rogfer01 updated this revision to Diff 61089.Jun 17 2016, 6:23 AM

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

Ping?

Thank you very much.

Ping? Any further comments, thoughts?

Thank you very much.

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.

Ping?

Any comment on this?

Thank you very much.

Ping?

Looking forward any further comments before committing.

Thank you very much.

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

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.

This revision was automatically updated to reflect the committed changes.

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?

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.

trybka added a subscriber: trybka.Jul 14 2016, 11:26 AM
rogfer01 reopened this revision.Jul 15 2016, 12:19 AM

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

This revision is now accepted and ready to land.Jul 15 2016, 12:19 AM

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.

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

rogfer01 updated this revision to Diff 64114.Jul 15 2016, 2:52 AM
rogfer01 updated this object.
rogfer01 removed rL LLVM as the repository for this revision.

Remove the diagnostic for the binding of references.

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.

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.

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.

Hi, friendly ping.

@rsmith any further comment on this?

Thank you very much.

This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Aug 15 2016, 3:20 PM

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 reopened this revision.Feb 1 2017, 2:40 AM
royger added a subscriber: 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.

This revision is now accepted and ready to land.Feb 1 2017, 2:40 AM

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 added a comment.Feb 1 2017, 3:27 AM

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.

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 added a subscriber: kimgr.Feb 1 2017, 3:59 AM

... 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 added a comment.Feb 1 2017, 4:15 AM

... 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 added a subscriber: joerg.Feb 1 2017, 5:14 AM

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.

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 added a comment.Feb 1 2017, 5:29 AM

@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 added a comment.Feb 1 2017, 1:30 PM

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 added a comment.EditedFeb 2 2017, 8:18 AM

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 added a comment.Feb 2 2017, 11:07 AM

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.

asl added a subscriber: asl.Mar 24 2017, 2:02 AM

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?
}
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 10:06 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:49 AM