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 ↗

(On Diff #68517)

Spurious comment.

11049 ↗

(On Diff #68517)

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

11053 ↗

(On Diff #68517)

Don't use auto here either.

[rsmith]

lib/Sema/SemaChecking.cpp

11054 ↗

(On Diff #68517)

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 ↗

(On Diff #69968)

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 ↗

(On Diff #69968)

Too much indentation.

11072 ↗

(On Diff #69968)

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

11356–11357 ↗

(On Diff #73466)

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

11370 ↗

(On Diff #73466)

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

11348–11350 ↗

(On Diff #75395)

Move this to immediately after the loop.

11367 ↗

(On Diff #75395)

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

11383 ↗

(On Diff #75395)

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

cfe/trunk/include/clang/Sema/Sema.h

public:
   /// void*).
   void DiscardMisalignedMemberAddress(const Type *T, Expr *E);
 
   /// \brief This function calls Action when it determines that E designates a
   /// misaligned member due to the packed attribute. This is used to emit
   /// local diagnostics like in reference binding.
   void RefersToMemberWithReducedAlignment(
       Expr *E,
-      std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)> Action);
+      std::function<void(Expr *, RecordDecl *, FieldDecl *, CharUnits)> Action);
 };
 
 /// \brief RAII object that enters a new expression evaluation context.
 class EnterExpressionEvaluationContext {
   Sema &Actions;
   bool Entered = true;
 
 public:

cfe/trunk/lib/Sema/SemaChecking.cpp

for (MisalignedMember &m : MisalignedMembers) {
     }
     Diag(m.E->getLocStart(), diag::warn_taking_address_of_packed_member)
         << m.MD << ND << m.E->getSourceRange();
   }
   MisalignedMembers.clear();
 }
 
 void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
-  if (!T->isPointerType())
+  E = E->IgnoreParens();
+  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) {
+    std::function<void(Expr *, RecordDecl *, FieldDecl *, CharUnits)> Action) {
   const auto *ME = dyn_cast<MemberExpr>(E);
-  while (ME && isa<FieldDecl>(ME->getMemberDecl())) {
+  if (!ME)
+    return;
+
+  // For a chain of MemberExpr like "a.b.c.d" this list
+  // will keep FieldDecl's like [d, c, b].
+  SmallVector<FieldDecl *, 4> ReverseMemberChain;
+  const MemberExpr *TopME = nullptr;
+  bool AnyIsPacked = false;
+  do {
     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>()))) {
-      CharUnits Alignment = std::min(Context.getTypeAlignInChars(MD->getType()),
-                                     Context.getTypeAlignInChars(BaseType));
-      // Notify that this expression designates a member with reduced alignment
-      Action(E, RD, MD, Alignment);
+    auto *FD = dyn_cast<FieldDecl>(MD);
+    // We do not care about non-data members.
+    if (!FD || FD->isInvalidDecl())
+      return;
+
+    AnyIsPacked =
+        AnyIsPacked || (RD->hasAttr<PackedAttr>() || MD->hasAttr<PackedAttr>());
+    ReverseMemberChain.push_back(FD);
+
+    TopME = ME;
+    ME = dyn_cast<MemberExpr>(ME->getBase()->IgnoreParens());
+  } while (ME);
+  assert(TopME && "We did not compute a topmost MemberExpr!");
+
+  // Not the scope of this diagnostic.
+  if (!AnyIsPacked)
+    return;
+
+  const Expr *TopBase = TopME->getBase()->IgnoreParenImpCasts();
+  const auto *DRE = dyn_cast<DeclRefExpr>(TopBase);
+  // TODO: The innermost base of the member expression may be too complicated.
+  // For now, just disregard these cases. This is left for future
+  // improvement.
+  if (!DRE && !isa<CXXThisExpr>(TopBase))
+      return;
+
+  // Alignment expected by the whole expression.
+  CharUnits ExpectedAlignment = Context.getTypeAlignInChars(E->getType());
+
+  // No need to do anything else with this case.
+  if (ExpectedAlignment.isOne())
+    return;
+
+  // Synthesize offset of the whole access.
+  CharUnits Offset;
+  for (auto I = ReverseMemberChain.rbegin(); I != ReverseMemberChain.rend();
+       I++) {
+    Offset += Context.toCharUnitsFromBits(Context.getFieldOffset(*I));
+  }
+
+  // Compute the CompleteObjectAlignment as the alignment of the whole chain.
+  CharUnits CompleteObjectAlignment = Context.getTypeAlignInChars(
+      ReverseMemberChain.back()->getParent()->getTypeForDecl());
+
+  // The base expression of the innermost MemberExpr may give
+  // stronger guarantees than the class containing the member.
+  if (DRE && !TopME->isArrow()) {
+    const ValueDecl *VD = DRE->getDecl();
+    if (!VD->getType()->isReferenceType())
+      CompleteObjectAlignment =
+          std::max(CompleteObjectAlignment, Context.getDeclAlign(VD));
+  }
+
+  // Check if the synthesized offset fulfills the alignment.
+  if (Offset % ExpectedAlignment != 0 ||
+      // It may fulfill the offset it but the effective alignment may still be
+      // lower than the expected expression alignment.
+      CompleteObjectAlignment < ExpectedAlignment) {
+    // If this happens, we want to determine a sensible culprit of this.
+    // Intuitively, watching the chain of member expressions from right to
+    // left, we start with the required alignment (as required by the field
+    // type) but some packed attribute in that chain has reduced the alignment.
+    // It may happen that another packed structure increases it again. But if
+    // we are here such increase has not been enough. So pointing the first
+    // FieldDecl that either is packed or else its RecordDecl is,
+    // seems reasonable.
+    FieldDecl *FD = nullptr;
+    CharUnits Alignment;
+    for (FieldDecl *FDI : ReverseMemberChain) {
+      if (FDI->hasAttr<PackedAttr>() ||
+          FDI->getParent()->hasAttr<PackedAttr>()) {
+        FD = FDI;
+        Alignment = std::min(
+            Context.getTypeAlignInChars(FD->getType()),
+            Context.getTypeAlignInChars(FD->getParent()->getTypeForDecl()));
         break;
       }
-    ME = dyn_cast<MemberExpr>(ME->getBase());
+    }
+    assert(FD && "We did not find a packed FieldDecl!");
+    Action(E, FD->getParent(), FD, Alignment);
   }
 }
 
 void Sema::CheckAddressOfPackedMember(Expr *rhs) {
   using namespace std::placeholders;
   RefersToMemberWithReducedAlignment(
       rhs, std::bind(&Sema::AddPotentialMisalignedMembers, std::ref(*this), _1,
                      _2, _3, _4));
 }
 

cfe/trunk/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));
 };
 
union {
     char c;
     int x;
   } __attribute__((packed));
 };
 
 int *anonymousInnerUnion(struct S6 *s) {
   return &s->x; // expected-warning {{packed member 'x' of class or structure 'S6::(anonymous)'}}
 }
+
+struct S6a {
+    int a;
+    int _;
+    int c;
+    char __;
+    int d;
+} __attribute__((packed, aligned(16))) s6;
+
+void g8()
+{ 
+    f1(&s6.a); // no-warning
+    f1(&s6.c); // no-warning
+    f1(&s6.d); // expected-warning {{packed member 'd' of class or structure 'S6a'}}
+}
+
+struct __attribute__((packed, aligned(1))) MisalignedContainee { double d; };
+struct __attribute__((aligned(8))) AlignedContainer { struct MisalignedContainee b; };
+
+struct AlignedContainer *p;
+double* g9() {
+  return &p->b.d; // no-warning
+}
+
+union OneUnion
+{
+    uint32_t a;
+    uint32_t b:1;
+};
+
+struct __attribute__((packed)) S7 {
+    uint8_t length;
+    uint8_t stuff;
+    uint8_t padding[2];
+    union OneUnion one_union;
+};
+
+union AnotherUnion {
+    long data;
+    struct S7 s;
+} *au;
+
+union OneUnion* get_OneUnion(void)
+{
+    return &au->s.one_union; // no-warning
+}
+
+struct __attribute__((packed)) S8 {
+    uint8_t data1;
+    uint8_t data2;
+  uint16_t wider_data;
+};
+
+#define LE_READ_2(p)          \
+  ((uint16_t)         \
+   ((((const uint8_t *)(p))[0]      ) |   \
+    (((const uint8_t *)(p))[1] <<  8)))
+
+uint32_t get_wider_data(struct S8 *s)
+{
+    return LE_READ_2(&s->wider_data); // no-warning
+}
+
+struct S9 {
+  uint32_t x;
+  uint8_t y[2];
+  uint16_t z;
+} __attribute__((__packed__));
+
+typedef struct S9 __attribute__((__aligned__(16))) aligned_S9;
+
+void g10() {
+  struct S9 x;
+  struct S9 __attribute__((__aligned__(8))) y;
+  aligned_S9 z;
+
+  uint32_t *p32;
+  p32 = &x.x; // expected-warning {{packed member 'x' of class or structure 'S9'}}
+  p32 = &y.x; // no-warning
+  p32 = &z.x; // no-warning
+}
+
+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 g11(S10Header *hdr);
+void g12(S10 *s) {
+  g11(&s->hdr); // no-warning
+}
+
+struct S11 {
+  uint32_t x;
+} __attribute__((__packed__));
+
+void g13(void) {
+  struct S11 __attribute__((__aligned__(4))) a[4];
+  uint32_t *p32;
+  p32 = &a[0].x; // no-warning
+}