Remove some false positives when taking the address of packed members

[rogfer01]

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.

[rogfer01]

Hi, this is a friendly ping.

Thank you! :)

[aaron.ballman]

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]

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]

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

[rogfer01]

Ping? :)

Thank you very much.

[rogfer01]

This is a friendly ping :)

Thank you very much!

[rogfer01]

Ping?

[rogfer01]

Ping?

[rsmith]

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]

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]

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.

[arphaman]

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.

[arphaman]

In D23657#560938, @arphaman wrote:

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.

[rogfer01]

@arphaman thanks for the testcase! Will do.

[joerg]

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

[joerg]

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]

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]

lib/Sema/SemaChecking.cpp

11090

Missing space.

[rogfer01]

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]

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]

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

[joerg]

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

[rogfer01]

Mark comment as TODO

[rogfer01]

Ping?

[rogfer01]

Ping? :)

[joerg]

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

[rsmith]

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.

[rogfer01]

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

lib/Sema/SemaChecking.cpp

void Sema::DiagnoseMisalignedMembers() {
   for (MisalignedMember &m : MisalignedMembers) {
     Diag(m.E->getLocStart(), diag::warn_taking_address_of_packed_member)
         << m.MD << m.RD << m.E->getSourceRange();
   }
   MisalignedMembers.clear();
 }
 
 void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
-  if (!T->isPointerType())
+  if (!T->isPointerType() && !T->isIntegerType())
     return;
   if (isa<UnaryOperator>(E) &&
       cast<UnaryOperator>(E)->getOpcode() == UO_AddrOf) {
     auto *Op = cast<UnaryOperator>(E)->getSubExpr()->IgnoreParens();
     if (isa<MemberExpr>(Op)) {
       auto MA = std::find(MisalignedMembers.begin(), MisalignedMembers.end(),
                           MisalignedMember(Op));
       if (MA != MisalignedMembers.end() &&
-          Context.getTypeAlignInChars(T->getPointeeType()) <= MA->Alignment)
+          (T->isIntegerType() ||
+           (T->isPointerType() &&
+            Context.getTypeAlignInChars(T->getPointeeType()) <= MA->Alignment)))
         MisalignedMembers.erase(MA);
     }
   }
 }
 
 void Sema::RefersToMemberWithReducedAlignment(
     Expr *E,
     std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)> Action) {
   const auto *ME = dyn_cast<MemberExpr>(E);
+  CharUnits RequiredAlignment;
   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;
+    CharUnits AlignField = Context.getTypeAlignInChars(MD->getType());
 
-    if (!ByteAligned &&
-        (RD->hasAttr<PackedAttr>() || (MD->hasAttr<PackedAttr>()))) {
-      CharUnits Alignment = std::min(Context.getTypeAlignInChars(MD->getType()),
-                                     Context.getTypeAlignInChars(BaseType));
-      // Notify that this expression designates a member with reduced alignment
+    RequiredAlignment = std::max(RequiredAlignment, AlignField);
+    if (RD->hasAttr<PackedAttr>() || MD->hasAttr<PackedAttr>()) {
+      CharUnits AlignRecord = Context.getTypeAlignInChars(BaseType);
+      if ((RequiredAlignment > AlignRecord) ||
+          ((Context.toCharUnitsFromBits(
+                Context.getFieldOffset(cast<FieldDecl>(MD))) %
+            RequiredAlignment) != 0)) {
+        // If the struct or the field are packed
+        //   - and the required alignment for the field is larger than the
+        //   alignment of the record, or if not that
+        //   - the offset of the field is misaligned to its required alignment
+        // then notify that this expression designates a member with reduced
+        // alignment.
+        CharUnits Alignment = std::min(AlignField, AlignRecord);
         Action(E, RD, MD, Alignment);
         break;
       }
+    }
+    else if (AlignField.isOne())
+        // If this field already has the least alignment possible it will never
+        // yield a misaligned pointer.
+        break;
     ME = dyn_cast<MemberExpr>(ME->getBase());
   }
 }
 
 void Sema::CheckAddressOfPackedMember(Expr *rhs) {
   using namespace std::placeholders;
   RefersToMemberWithReducedAlignment(
       rhs, std::bind(&Sema::AddPotentialMisalignedMembers, std::ref(*this), _1,
                      _2, _3, _4));
 }
 

test/Sema/address-packed.c

union __attribute__((packed)) UnionArguable {
   int x;
 };
 
 typedef struct Arguable ArguableT;
 
 struct Arguable *get_arguable();
 
 void to_void(void *);
+void to_intptr(intptr_t);
 
 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
 
     f1((int *)(void *)&arguable.x); // no-warning
     to_void(&arguable.x);           // no-warning
-    void *p = &arguable.x;          // no-warning;
+    void *p = &arguable.x;          // no-warning
     to_void(p);
+    to_intptr((intptr_t)p);         // no-warning
   }
   {
     union UnionArguable arguable;
     f2(&arguable.c); // no-warning
     f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
 
-    f1((int *)(void *)&arguable.x); // no-warning
-    to_void(&arguable.x);           // no-warning
+    f1((int *)(void *)&arguable.x);   // no-warning
+    to_void(&arguable.x);             // no-warning
+    to_intptr((intptr_t)&arguable.x); // no-warning
   }
   {
     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
 
-    f1((int *)(void *)&arguable.x); // no-warning
-    to_void(&arguable.x);           // no-warning
+    f1((int *)(void *)&arguable.x);   // no-warning
+    to_void(&arguable.x);             // no-warning
+    to_intptr((intptr_t)&arguable.x); // no-warning
   }
   {
     struct Arguable *arguable = get_arguable();
     f2(&arguable->c0); // no-warning
     f1(&arguable->x);  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
     f2(&arguable->c1); // no-warning
 
-    f1((int *)(void *)&arguable->x); // no-warning
-    to_void(&arguable->c1);          // no-warning
+    f1((int *)(void *)&arguable->x);    // no-warning
+    to_void(&arguable->c1);             // no-warning
+    to_intptr((intptr_t)&arguable->c1); // no-warning
   }
   {
     ArguableT *arguable = get_arguable();
     f2(&(arguable->c0)); // no-warning
     f1(&(arguable->x));  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
     f2(&(arguable->c1)); // no-warning
 
-    f1((int *)(void *)&(arguable->x)); // no-warning
-    to_void(&(arguable->c1));          // no-warning
+    f1((int *)(void *)&(arguable->x));      // no-warning
+    to_void(&(arguable->c1));               // no-warning
+    to_intptr((intptr_t)&(arguable->c1));   // no-warning
   }
 }
 
 struct S1 {
   char c;
   int i __attribute__((packed));
 };
 

 struct __attribute__((packed, aligned(2))) AlignedTo2Bis {
   int x;
 };
 
 struct AlignedTo2Bis* g7(struct AlignedTo2 *s)
 {
     return (struct AlignedTo2Bis*)&s->x; // no-warning
 }
+
+struct S6 {
+    int a;
+    int _;
+    int c;
+    char __;
+    int d;
+} __attribute__((packed, aligned(16))) s6;
+
+void foo()
+{ 
+    f1(&s6.a); // no-warning
+    f1(&s6.c); // no-warning
+    f1(&s6.d); // expected-warning {{packed member 'd' of class or structure 'S6'}}
+}