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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
