[PATCH] D23325: [WIP] Binding of references to packed fields

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/Expr.h:441
@@ +440,3 @@
+  /// Likewise bitfields, we model gl-values referring to packed-fields as
+  /// an aspect of the value-kind type system.
+  bool refersToPackedField() const { return getObjectKind() == OK_PackedField; }
----------------
value-kind -> object kind.

================
Comment at: include/clang/Basic/Specifiers.h:119-140
@@ -118,21 +118,24 @@
   /// l-value or x-value.
   enum ExprObjectKind {
     /// An ordinary object is located at an address in memory.
     OK_Ordinary,
 
     /// A bitfield object is a bitfield on a C or C++ record.
     OK_BitField,
 
+    /// A packed-field is a field on a C or C++ packed record.
+    OK_PackedField,
+
     /// A vector component is an element or range of elements on a vector.
     OK_VectorComponent,
 
     /// An Objective-C property is a logical field of an Objective-C
     /// object which is read and written via Objective-C method calls.
     OK_ObjCProperty,
     
     /// An Objective-C array/dictionary subscripting which reads an
     /// object or writes at the subscripted array/dictionary element via
     /// Objective-C method calls.
     OK_ObjCSubscript
   };
 
----------------
Wait a second, how did this fit into 2 bits before your change?!

================
Comment at: lib/AST/Decl.cpp:3523
@@ +3522,3 @@
+  return !isBitField() &&
+         (this->hasAttr<PackedAttr>() || getParent()->hasAttr<PackedAttr>()) &&
+         Context.getDeclAlign(this) <
----------------
Does this properly handle anonymous struct/union members:

  struct A __attribute__((packed)) {
    char a;
    union { int b; };
  } a;
  int &r = a.b;

================
Comment at: lib/CodeGen/CGCall.cpp:3278
@@ -3277,3 +3277,3 @@
   if (E->isGLValue()) {
-    assert(E->getObjectKind() == OK_Ordinary);
+    assert(E->getObjectKind() == OK_Ordinary || E->getObjectKind() == OK_PackedField);
     return args.add(EmitReferenceBindingToExpr(E), type);
----------------
This looks wrong: we shouldn't be emitting reference bindings to packed fields. We should have rejected them in Sema.

================
Comment at: lib/Sema/SemaCast.cpp:1912
@@ -1911,2 +1911,3 @@
     case OK_Ordinary:
+    case OK_PackedField:
       break;
----------------
Might be worth adding a comment here, "GCC allows a packed field to be explicitly cast to a reference type as a way of removing the 'packed' taint" or similar.

================
Comment at: lib/Sema/SemaExprMember.cpp:1772-1774
@@ -1771,4 +1771,5 @@
   if (!IsArrow) {
-    if (BaseExpr->getObjectKind() == OK_Ordinary)
+    if (BaseExpr->getObjectKind() == OK_Ordinary ||
+        BaseExpr->getObjectKind() == OK_PackedField)
       VK = BaseExpr->getValueKind();
     else
----------------
If the `BaseExpr` is an `OK_PackedField` then the resulting `OK` for the field should also be an `OK_PackedField`:

  struct Q { int k; };
  struct __attribute__((packed)) A { Q k; } a;
  int &r = a.k.k; // error, binding reference to packed field

You should handle this here...

================
Comment at: lib/Sema/SemaExprMember.cpp:1782
@@ +1781,3 @@
+      OK = OK_BitField;
+    else if (Field->isPackedField(S.Context) || BaseExpr->refersToPackedField())
+      OK = OK_PackedField;
----------------
... not here. This check does the wrong thing for the `IsArrow` case:

  struct Q { int k; };
  struct __attribute__((packed)) A { Q *k; } a;
  int &r = a.k->k; // should be valid, not a packed field

================
Comment at: lib/Sema/SemaInit.cpp:4186
@@ -4184,1 +4185,3 @@
 
+    if (IsLValueRef && Initializer->refersToPackedField() &&
+        Initializer->getType()->getAsCXXRecordDecl()) {
----------------
You don't need to special-case packed fields here. They just happen to be the only non-addressable object kind we have right now that can be a class type. If we had others, they should get this treatment too, so it seems better to remove the check.

================
Comment at: lib/Sema/SemaInit.cpp:4186
@@ -4184,1 +4185,3 @@
 
+    if (IsLValueRef && Initializer->refersToPackedField() &&
+        Initializer->getType()->getAsCXXRecordDecl()) {
----------------
rsmith wrote:
> You don't need to special-case packed fields here. They just happen to be the only non-addressable object kind we have right now that can be a class type. If we had others, they should get this treatment too, so it seems better to remove the check.
Why are you treating lvalue references specially here? GCC doesn't seem to, and it's not obvious to me why rvalue references should get special treatment.

================
Comment at: lib/Sema/SemaInit.cpp:4202-4204
@@ +4201,5 @@
+
+       It is not possible to bind w to a temporary of a.x because this
+       would require (recursively) invoking the copy constructor of
+       std::vector to obtain first a (properly aligned) temporary of a.x.
+      */
----------------
rogfer01 wrote:
> Rereading this makes no sense to me now. I'll adress this in a later update.
Perhaps replace this entire comment with "If the class doesn't have a trivial copy constructor, we can't create a copy of it for the reference binding because doing so would bind it to a reference in the class's own copy/move constructor, so just give up and allow the error to be diagnosed."

================
Comment at: test/SemaCXX/bind-packed-member.cpp:68-73
@@ +67,8 @@
+
+struct F
+{
+    char c;
+    NonTrivialCopy x __attribute__((packed));
+    int w __attribute__((packed));
+} f;
+
----------------
Another interesting case:

    struct __attribute__((packed)) G {
      char c;
      NonTrivialCopy x;
    };

Here, GCC ignores the `packed` attribute entirely (with a warning). For ABI compatibility, we should do the same.


https://reviews.llvm.org/D23325