[PATCH] D14872: PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin created this revision.
DmitryPolukhin added a reviewer: aaron.ballman.
DmitryPolukhin added a subscriber: cfe-commits.

This CL is for discussion how to better fix bit-filed layout compatibility issue with GCC (see PR25575 for test case and more details). Current clang behavior is compatible with GCC 4.1-4.3 series but it was fixed in 4.4+. Ignoring packed attribute looks very odd and because it was also fixed in GCC 4.4+, it makes sense also fix it in clang.

Open questions:

1. Should we add warning about changes in layout for packed bit-fileds of char type? GCC has it "note: offset of packed bit-field ‘b’ has changed in GCC 4.4" will add if needed.

2. This CL completely removes warning "packed attribute ignored for field of type char". Should we keep it but apply only to normal fields (i.e. not bit-fields)?

This CL removes the warning because IMHO it makes no sense if we fix bit-field case to match GCC 4.4+. Packed attribute was actually ignored only for bit-field. For all other cases it is not ignored but reported only when alignment is already fulfilled (i.e. attribute just is not required and doesn't change anything). But there are tons of cases when packed attribute doesn't change anything but warning is not emitted, for example:
struct S1 {
  char a;
  char b __attribute((packed)); // warning: 'packed' attribute ignored for field of type 'char'
  char c;
};

extern int a[sizeof(struct S1) == 3 ? 1 : -1];
extern int b[__alignof(struct S1) == 1 ? 1 : -1];

struct S2 {
  short a;
  short b __attribute((packed)); // no warning
  short c;
};

extern int c[sizeof(struct S2) == 6 ? 1 : -1];
extern int d[__alignof(struct S2) == 2 ? 1 : -1];

In both cases you can remove __attribute((packed)) and program behavior will not change. But warning is generated only in the first case.

http://reviews.llvm.org/D14872

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/struct-packed-align.c

Index: test/Sema/struct-packed-align.c
===================================================================
--- test/Sema/struct-packed-align.c
+++ test/Sema/struct-packed-align.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 // expected-no-diagnostics
 
 // Packed structs.
@@ -138,3 +139,23 @@
 extern int n1[sizeof(struct nS) == 9 ? 1 : -1];
 extern int n2[__alignof(struct nS) == 1 ? 1 : -1];
 #endif
+
+// Packed attribute shouldn't be ignored for bit-field of char types.
+// Note from GCC reference manual: The 4.1, 4.2 and 4.3 series of GCC ignore
+// the packed attribute on bit-fields of type char. This has been fixed in
+// GCC 4.4 but the change can lead to differences in the structure layout.
+// See the documentation of -Wpacked-bitfield-compat for more information.
+struct packed_chars {
+  char a:4;
+  char b:8 __attribute__ ((packed));
+  char c:4;
+};
+
+#if defined(_WIN32)
+// On Windows clang ignores uses MSVC compatible layout in this case.
+extern int o1[sizeof(struct packed_chars) == 3 ? 1 : -1];
+extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
+#else
+extern int o1[sizeof(struct packed_chars) == 2 ? 1 : -1];
+extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
+#endif
Index: lib/Sema/SemaDeclAttr.cpp
===================================================================
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -1032,22 +1032,10 @@
 }
 
 static void handlePackedAttr(Sema &S, Decl *D, const AttributeList &Attr) {
-  if (TagDecl *TD = dyn_cast<TagDecl>(D))
-    TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context,
-                                        Attr.getAttributeSpellingListIndex()));
-  else if (FieldDecl *FD = dyn_cast<FieldDecl>(D)) {
-    // If the alignment is less than or equal to 8 bits, the packed attribute
-    // has no effect.
-    if (!FD->getType()->isDependentType() &&
-        !FD->getType()->isIncompleteType() &&
-        S.Context.getTypeAlign(FD->getType()) <= 8)
-      S.Diag(Attr.getLoc(), diag::warn_attribute_ignored_for_field_of_type)
-        << Attr.getName() << FD->getType();
-    else
-      FD->addAttr(::new (S.Context)
-                  PackedAttr(Attr.getRange(), S.Context,
-                             Attr.getAttributeSpellingListIndex()));
-  } else
+  if (isa<TagDecl>(D) || isa<FieldDecl>(D))
+    D->addAttr(::new (S.Context) PackedAttr(
+        Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
+  else
     S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName();
 }
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2777,9 +2777,6 @@
   "cast to %1 from smaller integer type %0">,
   InGroup<IntToVoidPointerCast>;
 
-def warn_attribute_ignored_for_field_of_type : Warning<
-  "%0 attribute ignored for field of type %1">,
-  InGroup<IgnoredAttributes>;
 def warn_transparent_union_attribute_field_size_align : Warning<
   "%select{alignment|size}0 of field %1 (%2 bits) does not match the "
   "%select{alignment|size}0 of the first field in transparent union; "


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D14872.40772.patch
Type: text/x-patch
Size: 3309 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151120/f6d4daeb/attachment.bin>