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

[Alexey Bataev via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL254596: PR25575: Make GCC 4.4+ comatible layout for packed 
bit-fileds of char type… (authored by ABataev).

Changed prior to commit:
  http://reviews.llvm.org/D14872?vs=41648=41724#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D14872

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

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2784,9 +2784,10 @@
   "cast to %1 from smaller integer type %0">,
   InGroup;
 
-def warn_attribute_ignored_for_field_of_type : Warning<
-  "%0 attribute ignored for field of type %1">,
-  InGroup;
+def warn_attribute_packed_for_bitfield : Warning<
+  "'packed' attribute was ignored on bit-fields with single-byte alignment "
+  "in older versions of GCC and Clang">,
+  InGroup>;
 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; "
Index: cfe/trunk/test/Sema/struct-packed-align.c
===
--- cfe/trunk/test/Sema/struct-packed-align.c
+++ cfe/trunk/test/Sema/struct-packed-align.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 
 // Packed structs.
 struct s {
@@ -138,3 +138,24 @@
 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));
+  // expected-warning@-1 {{'packed' attribute was ignored on bit-fields with 
single-byte alignment in older versions of GCC and Clang}}
+  char c:4;
+};
+
+#if defined(_WIN32)
+// On Windows clang 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: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -1073,17 +1073,14 @@
 TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context,
 Attr.getAttributeSpellingListIndex()));
   else if (FieldDecl *FD = dyn_cast(D)) {
-// If the alignment is less than or equal to 8 bits, the packed attribute
-// has no effect.
+// Report warning about changed offset in the newer compiler versions.
 if (!FD->getType()->isDependentType() &&
-!FD->getType()->isIncompleteType() &&
+!FD->getType()->isIncompleteType() && FD->isBitField() &&
 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()));
+  S.Diag(Attr.getLoc(), diag::warn_attribute_packed_for_bitfield);
+
+FD->addAttr(::new (S.Context) PackedAttr(
+Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
   } else
 S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName();
 }


Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2784,9 +2784,10 @@
   "cast to %1 from smaller integer type %0">,
   InGroup;
 
-def warn_attribute_ignored_for_field_of_type : Warning<
-  "%0 attribute ignored for field of type %1">,
-  InGroup;
+def warn_attribute_packed_for_bitfield : Warning<
+  "'packed' attribute was ignored on bit-fields with single-byte alignment "
+  "in older versions of GCC and Clang">,
+  InGroup>;
 def warn_transparent_union_attribute_field_size_align : Warning<
   

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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2788
@@ +2787,3 @@
+def warn_attribute_packed_for_bitfield : Warning<
+  "'packed' attribute was ignored on bit-fields with alignment 1 "
+  "in older versions of GCC and Clang">,

Would this read better as "on bit-fields with single-byte alignment" instead of 
"on bit-fields with alignment 1"?


Comment at: test/Sema/struct-packed-align.c:155
@@ +154,3 @@
+#if defined(_WIN32)
+// On Windows clang ignores uses MSVC compatible layout in this case.
+extern int o1[sizeof(struct packed_chars) == 3 ? 1 : -1];

"ignores uses" -> "uses"


http://reviews.llvm.org/D14872



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

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

[Aaron Ballman via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM! Thank you!


http://reviews.llvm.org/D14872



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

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

[John McCall via cfe-commits]

rjmccall added a comment.

LGTM, thanks!


http://reviews.llvm.org/D14872



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

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin updated this revision to Diff 41648.
DmitryPolukhin marked 2 inline comments as done.
DmitryPolukhin added a comment.

Fixed warning and comment.


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,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 
 // Packed structs.
 struct s {
@@ -138,3 +138,24 @@
 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));
+  // expected-warning@-1 {{'packed' attribute was ignored on bit-fields with 
alignment 1 in older versions of GCC and Clang}}
+  char c:4;
+};
+
+#if defined(_WIN32)
+// On Windows clang 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
@@ -1036,17 +1036,14 @@
 TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context,
 Attr.getAttributeSpellingListIndex()));
   else if (FieldDecl *FD = dyn_cast(D)) {
-// If the alignment is less than or equal to 8 bits, the packed attribute
-// has no effect.
+// Report warning about changed offset in the newer compiler versions.
 if (!FD->getType()->isDependentType() &&
-!FD->getType()->isIncompleteType() &&
+!FD->getType()->isIncompleteType() && FD->isBitField() &&
 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()));
+  S.Diag(Attr.getLoc(), diag::warn_attribute_packed_for_bitfield);
+
+FD->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
@@ -2784,9 +2784,10 @@
   "cast to %1 from smaller integer type %0">,
   InGroup;
 
-def warn_attribute_ignored_for_field_of_type : Warning<
-  "%0 attribute ignored for field of type %1">,
-  InGroup;
+def warn_attribute_packed_for_bitfield : Warning<
+  "'packed' attribute was ignored on bit-fields with single-byte alignment "
+  "in older versions of GCC and Clang">,
+  InGroup>;
 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; "


Index: test/Sema/struct-packed-align.c
===
--- test/Sema/struct-packed-align.c
+++ test/Sema/struct-packed-align.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 
 // Packed structs.
 struct s {
@@ -138,3 +138,24 @@
 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));
+  // expected-warning@-1 {{'packed' attribute was ignored on bit-fields 

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

[John McCall via cfe-commits]

rjmccall added inline comments.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2790
@@ -2790,1 +2789,3 @@
+  "version 4.4 - the newer offset is used here">,
+  InGroup>;
 def warn_transparent_union_attribute_field_size_align : Warning<

No, this diagnostic shouldn't make such an affirmative statement about the 
offset changing.  I would instead suggest:
  warning: 'packed' attribute was ignored on bit-fields with alignment 1 in 
older versions of GCC and Clang


http://reviews.llvm.org/D14872



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

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin updated this revision to Diff 41510.
DmitryPolukhin marked an inline comment as done.
DmitryPolukhin added a comment.

Don't call getDeclName() that it is not required. PTAL


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,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 
 // Packed structs.
 struct s {
@@ -138,3 +138,24 @@
 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));
+  // expected-warning@-1 {{the offset assigned to packed bit-field member 'b' 
changed with GCC version 4.4 - the newer offset is used here}}
+  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
@@ -1036,17 +1036,15 @@
 TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context,
 Attr.getAttributeSpellingListIndex()));
   else if (FieldDecl *FD = dyn_cast(D)) {
-// If the alignment is less than or equal to 8 bits, the packed attribute
-// has no effect.
+// Report warning about changed offset in the newer compiler versions.
 if (!FD->getType()->isDependentType() &&
-!FD->getType()->isIncompleteType() &&
+!FD->getType()->isIncompleteType() && FD->isBitField() &&
 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()));
+  S.Diag(Attr.getLoc(),
+ diag::note_attribute_packed_for_bitfield_offset_changed) << FD;
+
+FD->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
@@ -2784,9 +2784,10 @@
   "cast to %1 from smaller integer type %0">,
   InGroup;
 
-def warn_attribute_ignored_for_field_of_type : Warning<
-  "%0 attribute ignored for field of type %1">,
-  InGroup;
+def note_attribute_packed_for_bitfield_offset_changed : Warning<
+  "the offset assigned to packed bit-field member %0 changed with GCC "
+  "version 4.4 - the newer offset is used here">,
+  InGroup>;
 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; "


Index: test/Sema/struct-packed-align.c
===
--- test/Sema/struct-packed-align.c
+++ test/Sema/struct-packed-align.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 
 // Packed structs.
 struct s {
@@ -138,3 +138,24 @@
 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 {
+  

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

This is another GCC ABI compatibility issue. If there is no more comments, 
could someone please approve this CL?


http://reviews.llvm.org/D14872



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

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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: lib/Sema/SemaDeclAttr.cpp:1046
@@ +1045,3 @@
+ diag::note_attribute_packed_for_bitfield_offset_changed)
+<< FD->getDeclName();
+

No need to call getDeclName(), the diagnostic can accept a NamedDecl directly 
(which properly quotes the name in the diagnostic).


http://reviews.llvm.org/D14872



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

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin updated this revision to Diff 41222.
DmitryPolukhin marked 2 inline comments as done.
DmitryPolukhin added a comment.

Changed note text message + fixed outdated comment.


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,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 
 // Packed structs.
 struct s {
@@ -138,3 +138,24 @@
 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));
+  // expected-warning@-1 {{the offset assigned to packed bit-field member 'b' 
changed with GCC version 4.4 - the newer offset is used here}}
+  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
@@ -1036,17 +1036,17 @@
 TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context,
 Attr.getAttributeSpellingListIndex()));
   else if (FieldDecl *FD = dyn_cast(D)) {
-// If the alignment is less than or equal to 8 bits, the packed attribute
-// has no effect.
+// Report warning about changed offset in the newer compiler versions.
 if (!FD->getType()->isDependentType() &&
 !FD->getType()->isIncompleteType() &&
+FD->isBitField() &&
 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()));
+  S.Diag(Attr.getLoc(),
+ diag::note_attribute_packed_for_bitfield_offset_changed)
+<< FD->getDeclName();
+
+FD->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
@@ -2780,9 +2780,10 @@
   "cast to %1 from smaller integer type %0">,
   InGroup;
 
-def warn_attribute_ignored_for_field_of_type : Warning<
-  "%0 attribute ignored for field of type %1">,
-  InGroup;
+def note_attribute_packed_for_bitfield_offset_changed : Warning<
+  "the offset assigned to packed bit-field member %0 changed with GCC "
+  "version 4.4 - the newer offset is used here">,
+  InGroup>;
 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; "


Index: test/Sema/struct-packed-align.c
===
--- test/Sema/struct-packed-align.c
+++ test/Sema/struct-packed-align.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
 
 // Packed structs.
 struct s {
@@ -138,3 +138,24 @@
 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 

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

[Richard Smith via cfe-commits]

On Nov 25, 2015 1:53 PM, "hfin...@anl.gov via cfe-commits" <
cfe-commits@lists.llvm.org> wrote:
>
> hfinkel added inline comments.
>
> 
> Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2783
> @@ -2783,1 +2782,3 @@
> +  "the newer semantic is provided here">,
> +  InGroup>;
>  def warn_transparent_union_attribute_field_size_align : Warning<
> 
> Calling this "a semantic" reads oddly to me. This sounds better to me:
>
>   def note_attribute_packed_for_bitfield_offset_changed : Warning<
> "the offset assigned to packed bit-field member %0 has changed with
GCC version 4.4 - "

Please also remove the "has" here.

> "the newer offset is used here">,
> InGroup>;
>
> 
> Comment at: lib/Sema/SemaDeclAttr.cpp:1040
> @@ -1039,3 +1039,3 @@
>  // If the alignment is less than or equal to 8 bits, the packed
attribute
>  // has no effect.
>  if (!FD->getType()->isDependentType() &&
> 
> This comment is now out of date?
>
>
> http://reviews.llvm.org/D14872
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[hfin...@anl.gov via cfe-commits]

hfinkel added inline comments.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2783
@@ -2783,1 +2782,3 @@
+  "the newer semantic is provided here">,
+  InGroup>;
 def warn_transparent_union_attribute_field_size_align : Warning<

Calling this "a semantic" reads oddly to me. This sounds better to me:

  def note_attribute_packed_for_bitfield_offset_changed : Warning<
"the offset assigned to packed bit-field member %0 has changed with GCC 
version 4.4 - "
"the newer offset is used here">,
InGroup>;


Comment at: lib/Sema/SemaDeclAttr.cpp:1040
@@ -1039,3 +1039,3 @@
 // If the alignment is less than or equal to 8 bits, the packed attribute
 // has no effect.
 if (!FD->getType()->isDependentType() &&

This comment is now out of date?


http://reviews.llvm.org/D14872



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

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

[hfin...@anl.gov via cfe-commits]

hfinkel added a subscriber: hfinkel.
hfinkel added a comment.

Please upload this patch with full context, see: 
http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

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


Warning about this is a good idea. We did something similar when we 
(re-)introduced sync_fetch_and_nand, see:

  def warn_sync_fetch_and_nand_semantics_change : Warning<
"the semantics of this intrinsic changed with GCC "
"version 4.4 - the newer semantics are provided here">,
InGroup>;


http://reviews.llvm.org/D14872



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

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

It seems that check for type alignment <= 8 was there practically forever 
http://llvm.org/viewvc/llvm-project/cfe/trunk/Sema/SemaDecl.cpp?r1=47197=47196=47197
 and there is no good explanation why it was implemented. Subsequent changes 
only add more condition to exclude cases when it shouldn't be reported.


http://reviews.llvm.org/D14872



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

[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 , Decl *D, const AttributeList ) {
-  if (TagDecl *TD = dyn_cast(D))
-TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context,
-Attr.getAttributeSpellingListIndex()));
-  else if (FieldDecl *FD = dyn_cast(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(D) || isa(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;
 
-def warn_attribute_ignored_for_field_of_type : Warning<
-  "%0 attribute ignored for field of type %1">,
-  InGroup;