Re: [PR c++/87137] GCC-8 Fix

[Nathan Sidwell <nathan at acm dot org>]

On 08/29/2018 11:06 PM, Liu Hao wrote:

> It is strictly an ABI break but I doubt whether code in real world that 
> got broken by this bug ever exists. Usually when people expect 
> consecutive bitfields to be packed into a single word they wouldn't put 
> irrelevant declarations between them.

You're probably right.  I'm guessing this bug was found because:

   int bob : 1;
   int get_bob () const {return bob;}
   void set_bob (bool v) {bob=v;}
   int bill : 1;
   ...

might be a useful style.

> An important reason why such code could be rare is that the following 
> code compiles differently as C and C++:

> struct foo
>    {
>      unsigned a : 21;
>      union x1 { char x; };
>      unsigned b : 11;
>      union x1 u;
>    };

(for C++) this happens to be a case we already get right.  <aside> I 
think that'd be a C vendor extension, I don't think unnamed fields are 
permitted there?

Here's a version of the patch to completely resolve the issue, tested on 
trunk.  I noticed regular x86 targets support the ms_struct attribute, 
so we can give this wider testing.  I did consider trying to reorder the 
field decls before struct layour, but that seemed a more invasive change 
-- besides it might be nice to be able to remove the template-specific 
CLASSTYPE_DECL_LIST which almost but not quite mirrors TYPE_FIELDS.

Ok for trunk, ok for 8?

nathan
--
Nathan Sidwell

2018-08-30  Nathan Sidwell  <nathan@acm.org>

	PR c++/87137
	* stor-layout.c (place_field): Scan forwards to check last
	bitfield when ms_bitfield_placement is in effect.
	gcc/testsuite/
	* g++.dg/abi/pr87137.C: New.

Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 263956)
+++ stor-layout.c	(working copy)
@@ -1686,14 +1686,21 @@ place_field (record_layout_info rli, tre
     {
       rli->bitpos = size_binop (PLUS_EXPR, rli->bitpos, DECL_SIZE (field));
 
-      /* If we ended a bitfield before the full length of the type then
-	 pad the struct out to the full length of the last type.  */
-      if ((DECL_CHAIN (field) == NULL
-	   || TREE_CODE (DECL_CHAIN (field)) != FIELD_DECL)
-	  && DECL_BIT_FIELD_TYPE (field)
+      /* If FIELD is the last field and doesn't end at the full length
+	 of the type then pad the struct out to the full length of the
+	 last type.  */
+      if (DECL_BIT_FIELD_TYPE (field)
 	  && !integer_zerop (DECL_SIZE (field)))
-	rli->bitpos = size_binop (PLUS_EXPR, rli->bitpos,
-				  bitsize_int (rli->remaining_in_alignment));
+	{
+	  /* We have to scan, because non-field DECLS are also here.  */
+	  tree probe = field;
+	  while ((probe = DECL_CHAIN (probe)))
+	    if (TREE_CODE (probe) == FIELD_DECL)
+	      break;
+	  if (!probe)
+	    rli->bitpos = size_binop (PLUS_EXPR, rli->bitpos,
+				      bitsize_int (rli->remaining_in_alignment));
+	}
 
       normalize_rli (rli);
     }
Index: testsuite/g++.dg/abi/pr87137.C
===================================================================
--- testsuite/g++.dg/abi/pr87137.C	(nonexistent)
+++ testsuite/g++.dg/abi/pr87137.C	(working copy)
@@ -0,0 +1,40 @@
+// PR c++/87137
+
+// Empty macro args are undefined in C++ 98
+// { dg-do compule { target c++11 } }
+
+// We got confused by non-field decls separating bitfields when
+// ms_bitfield_layout was in effect.
+
+#if defined (__x86_64__) || defined (__i686__) || defined (__powerpc__)
+#define ATTRIB  __attribute__((ms_struct))
+#elif defined (__superh__)
+#define ATTRIB __attribute__((renesas))
+#else
+#define ATTRIB
+#endif
+
+#define DEFINE(NAME,BASE,THING)		\
+  struct ATTRIB NAME BASE {		\
+    unsigned one : 12;			\
+    THING				\
+    unsigned two : 4;			\
+  }
+
+template<unsigned I, unsigned J> struct Test;
+template<unsigned I> struct Test<I,I> {};
+
+#define TEST(NAME,BASE,THING)			\
+  DEFINE(NAME##_WITH,BASE,THING);		\
+  DEFINE(NAME##_WITHOUT,BASE,);			\
+  int NAME = sizeof (Test<sizeof(NAME##_WITH),sizeof (NAME##_WITHOUT)>)
+
+TEST(NSFun,,int fun (););
+TEST(SFun,,static int fun (););
+TEST(Tdef,,typedef int tdef;);
+
+TEST(Var,,static int var;);
+struct base { int f; };
+TEST(Use,:base,using base::f;);
+TEST(Tmpl,,template<unsigned> class X {};);
+TEST(TmplFN,,template<unsigned> void fu (););

Index: htdocs/gcc-9/changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-9/changes.html,v
retrieving revision 1.18
diff -r1.18 changes.html
211c211
< <!-- <h3 id="windows">Windows</h3> -->
---
> <h3 id="windows">Windows</h3>
212a213,227
> <ul>
>   <li>A C++ Microsoft ABI bitfield layout
>   bug, <a href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87137";>PR87137</a>
>   has been fixed.  A non-field declaration could cause the current
>   bitfield allocation unit to be completed, incorrectly placing a
>   following bitfield into a new allocation unit.  Microsoft ABI is
>   selected for:
>   <ul>
>     <li>Mingw targets
>     <li>PowerPC, IA-32 or x86-64 targets
>       when <code>-mms-bitfields</code> option is specified
>       or <code>__attribute__((ms_struct))</code> is used
>     <li>SuperH targets when the <code>-mhitachi</code> option is
>       specified, or <code>__attribute__((renesas))</code> is used
>   </ul>