This is an archive of the discontinued LLVM Phabricator instance.

Remove some false positives when taking the address of packed members
ClosedPublic

Authored by rogfer01 on Aug 18 2016, 5:45 AM.

Details

Summary

This change remove some false positives when taking the address of packed members.

  • It silences the warning when a cast to uintptr_t/intptr_t happens.
  • If the field is in a packed record that is overaligned, the field may still be in a suitable offset for the required alignment of the field. We now check this as well.

Diff Detail

Event Timeline

rogfer01 updated this revision to Diff 68517.Aug 18 2016, 5:45 AM
rogfer01 retitled this revision from to Remove some false positives when taking the address of packed members.
rogfer01 updated this object.
rogfer01 added reviewers: aaron.ballman, rsmith.
rogfer01 added a subscriber: cfe-commits.

Hi, this is a friendly ping.

Thank you! :)

aaron.ballman edited edge metadata.Aug 30 2016, 2:43 PM

Some minor nits; @rsmith may have more substantial comments.

lib/Sema/SemaChecking.cpp
11039

Spurious comment.

11048

Please don't use auto here as the type is not spelled out in the initialization.

11052

Don't use auto here either.

rsmith added inline comments.Aug 30 2016, 5:42 PM
lib/Sema/SemaChecking.cpp
11053

Suppose I have this:

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

Here, on the second iteration through this loop, it looks like you'll compute RequiredAlignment == 4 (from A's alignment of 4), with AlignRecord == 1 (from the attribute), which would result in calling Action. This seems unnecessary, since we know we'll never diagnose this case, because the actual alignment of the field is (at least) the alignment of the original member c.

rogfer01 updated this revision to Diff 69968.Sep 1 2016, 3:04 AM
rogfer01 edited edge metadata.

Skip the check if the field is already aligned to 1.

Ping? :)

Thank you very much.

This is a friendly ping :)

Thank you very much!

rsmith added inline comments.Oct 3 2016, 3:48 PM
lib/Sema/SemaChecking.cpp
11053–11056

This doesn't seem to correctly handle the case where the base expression gives you some guaranteed alignment; it only handles the case where the base expression reduces the alignment. Suppose __alignof__(double) == 8 and I have this:

struct __attribute__((packed, aligned(1))) B { double d; };
struct __attribute__((aligned(8))) A { B b; };

A *p;
double *q = &p->b.d;

For the top MemberExpr here, we see RequiredAlignment == AlignField == 8, AlignRecord == 1, and we report that we have a reference to a field with reduced alignment. But we don't, because the reference is within an 8-byte-aligned B object within the enclosing A object.

Do you care about handling that case correctly? It seems like one way to handle all the nontrivial cases here would be to recurse to the innermost MemberExpr, accumulating an offset on the way, and then check that the innermost expression's type implies a correct alignment and offset for the type of E.

11069–11071

Too much indentation.

11072

You should at least IgnoreParens here, and maybe IgnoreParenNoopCasts.

joerg added a subscriber: joerg.Oct 3 2016, 4:12 PM

The following is from my comment on the original commit.

I'm trying this patch now. Two instances here show false positives. Some
others are misuse/overuse of __packed, it is time consuming to check all
of them.

  • sbin/route

https://nxr.netbsd.org/xref/src/sbin/route/route.c#1009 triggers the warning:

union mpls_shim *ms = &su->smpls.smpls_addr;

where su is defined as:

union sockunion {
    ...
    struct  sockaddr_mpls smpls;
};

and sockaddr_mpls in turn is https://nxr.netbsd.org/xref/src/sys/netmpls/mpls.h#66

struct sockaddr_mpls {
    uint8_t smpls_len;
    uint8_t smpls_family;
    uint8_t smpls_pad[2];
    union mpls_shim smpls_addr;
} __packed;

smpls_addr is a union of a uint32_t and matching bit field. The alignment of sockunion and the explicit padding ensures that all fields can be accessed correctly.

  • sbin/ifconfig

Member access is https://nxr.netbsd.org/xref/src/sbin/ifconfig/ieee80211.c#969
The macro is defined at https://nxr.netbsd.org/xref/src/sbin/ifconfig/ieee80211.c#928

Most importantly, this effective casts immediately to (unsigned) char and therefore doesn't care about any misalignment.

rogfer01 updated this revision to Diff 73466.Oct 4 2016, 6:00 AM

Change algorithm following @rsmith suggestions by computing the offset of the whole access and compare it against the expected alignment, so reduced aligned structs inside overaligned structs does not yield a warning.

Also ignore parentheses where necessary, which was effectively preventing silencing some false positives.

Includes testcases suggested by @rsmith and @joerg.

Thanks for working on this!

This patch fixes a couple of our internal warnings that shouldn't be presented. I included a code sample that reproduces our warnings, would you mind adding some code to the test case that's similar to the sample below?

typedef	struct {
  uint32_t msgh_bits;
  uint32_t msgh_size;
  int32_t msgh_voucher_port;
  int32_t msgh_id;
} S10Header;

typedef struct {
    uint32_t t;
    uint64_t m;
    uint32_t p;
    union {
        struct {
            uint32_t a;
            double z;
        } __attribute__ ((aligned (8), packed)) a;
        struct {
            uint32_t b;
            double z;
            uint32_t a;
        } __attribute__ ((aligned (8), packed)) b;
    };
} __attribute__ ((aligned (8), packed)) S10Data;

typedef struct {
  S10Header hdr;
  uint32_t size;
  uint8_t count;
  S10Data data[] __attribute__ ((aligned (8)));
} __attribute__ ((aligned (8), packed)) S10;

void foo(S10Header *hdr);
void bar(S10 *s) {
  foo(&s->hdr); // No warning expected.
}

Btw, slightly off-topic, but I noticed that the declaration DiagnoseMisalignedMembers in the header has a doc comment that violates the 80 chars rule. I committed a fix in r283228.

Btw, slightly off-topic, but I noticed that the declaration DiagnoseMisalignedMembers in the header has a doc comment that violates the 80 chars rule. I committed a fix in r283228.

I meant to say DiscardMisalignedMemberAddress, sorry.

@arphaman thanks for the testcase! Will do.

joerg added a comment.Oct 4 2016, 9:40 AM

Seems to work for the false positives I have identified so far.

joerg added a comment.Oct 11 2016, 6:52 AM

A simple case that still seems to fail:

#include <inttypes.h>

struct foo {
  uint32_t x;
  uint8_t y[2];
  uint16_t z;
} __attribute__((__packed__));

typedef struct foo __attribute__((__aligned__(16))) aligned_foo;

int main(void) {
  struct foo x;
  struct foo __attribute__((__aligned__(4))) y;
  aligned_foo z;

  uint32_t *p32;
  p32 = &x.x;
  p32 = &y.x;
  p32 = &z.x;
}

Only the first assignment to p32 should warn, the others are all correctly aligned.

rsmith added inline comments.Oct 11 2016, 2:59 PM
lib/Sema/SemaChecking.cpp
11076–11077

In order to fix Joerg's latest false positives, you need to look at the type of the base expression itself rather than the class type containing the innermost member. (The base expression's type might provide more alignment than the struct being accessed guarantees.)

joerg added inline comments.Oct 12 2016, 5:23 AM
lib/Sema/SemaChecking.cpp
11090

Missing space.

rogfer01 updated this revision to Diff 74638.Oct 14 2016, 2:55 AM

Updated patch, now we check if the innermost base of a MemberExpr is a DeclRefExpr and check for its declaration in case it provides stronger alignment guarantees.

joerg added a comment.Oct 14 2016, 2:42 PM

It seems like on-stack arrays still don't work?

#include <inttypes.h>

struct test {
  uint32_t x;
} __attribute__((__packed__));

int main(void) {
  struct test __attribute__((__aligned__(4))) a[4];
  uint32_t *p32;
  p32 = &a[0].x;
}
rogfer01 updated this revision to Diff 75007.Oct 18 2016, 8:15 AM

Ignore cases where the innermost base expression is too complicated for the scope of this patch.

joerg added a comment.Oct 18 2016, 9:11 AM

I agree that leaving out the really complicated parts is acceptable for now. Can you mark the comment as TODO?

rogfer01 updated this revision to Diff 75395.Oct 21 2016, 12:31 AM

Mark comment as TODO

joerg added a comment.Nov 3 2016, 11:20 AM

It works for me with the mentioned TODO item. But I'm not the right one to give a full OK.

rsmith accepted this revision.Nov 7 2016, 9:29 AM
rsmith edited edge metadata.

Some minor changes then this looks good to go.

lib/Sema/SemaChecking.cpp
11067–11069

Move this to immediately after the loop.

11086

I think this would be a lot clearer as CompleteObjectAlignment or something like that.

11102

|| should go on the previous line.

This revision is now accepted and ready to land.Nov 7 2016, 9:29 AM
rogfer01 updated this revision to Diff 77498.Nov 10 2016, 8:52 AM
rogfer01 edited edge metadata.
  • Address comments.
  • Rebase test with current trunk
  • Make the check resilient under errors

If no comments arise from this change I will be commiting it Monday next week.

This revision was automatically updated to reflect the committed changes.