[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added reviewers: rnk, rjmccall, rsmith.
dblaikie requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This matches GCC: https://godbolt.org/z/sM5q95PGY

I realize this is an API break for clang+clang - so I'm totally open to
discussing how we should deal with that. If Apple wants to keep the
Clang layout indefinitely, if we want to put a flag on this so non-Apple
folks can opt out of this fix/new behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117616

Files:
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/SemaCXX/address-packed.cpp


Index: clang/test/SemaCXX/address-packed.cpp
===================================================================
--- clang/test/SemaCXX/address-packed.cpp
+++ clang/test/SemaCXX/address-packed.cpp
@@ -121,3 +121,19 @@
   Incomplete *e() const;
 } __attribute__((packed));
 Incomplete *S2::e() const { return (Incomplete *)&d; } // no-warning
+
+
+namespace non_classic_pod {
+struct t1 { protected: int a; };
+static_assert(alignof(t1) == 4, "");
+// GCC prints warning: ignoring packed attribute because of unpacked non-POD 
field 't1 t2::f'`
+struct t2 { t1 v1; } __attribute__((packed));
+static_assert(alignof(t2) == 4, "");
+} // namespace non_classic_pod
+
+namespace non_classic_pod_packed {
+struct t1 { protected: int a; } __attribute__((packed));
+static_assert(alignof(t1) == 1, "");
+struct t2 { t1 v1; } __attribute__((packed));
+static_assert(alignof(t2) == 1, "");
+} // namespace non_classic_pod_packed
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1887,7 +1887,8 @@
   UnfilledBitsInLastUnit = 0;
   LastBitfieldStorageUnitSize = 0;
 
-  bool FieldPacked = Packed || D->hasAttr<PackedAttr>();
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD())) ||
+                     D->hasAttr<PackedAttr>();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;



Index: clang/test/SemaCXX/address-packed.cpp
===================================================================
--- clang/test/SemaCXX/address-packed.cpp
+++ clang/test/SemaCXX/address-packed.cpp
@@ -121,3 +121,19 @@
   Incomplete *e() const;
 } __attribute__((packed));
 Incomplete *S2::e() const { return (Incomplete *)&d; } // no-warning
+
+
+namespace non_classic_pod {
+struct t1 { protected: int a; };
+static_assert(alignof(t1) == 4, "");
+// GCC prints warning: ignoring packed attribute because of unpacked non-POD field 't1 t2::f'`
+struct t2 { t1 v1; } __attribute__((packed));
+static_assert(alignof(t2) == 4, "");
+} // namespace non_classic_pod
+
+namespace non_classic_pod_packed {
+struct t1 { protected: int a; } __attribute__((packed));
+static_assert(alignof(t1) == 1, "");
+struct t2 { t1 v1; } __attribute__((packed));
+static_assert(alignof(t2) == 1, "");
+} // namespace non_classic_pod_packed
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1887,7 +1887,8 @@
   UnfilledBitsInLastUnit = 0;
   LastBitfieldStorageUnitSize = 0;
 
-  bool FieldPacked = Packed || D->hasAttr<PackedAttr>();
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD())) ||
+                     D->hasAttr<PackedAttr>();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits