This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


This defect concerns bitfield layout in the Microsoft ABI. This is a fix for gcc-8.

As well as MINGW targets, MS-ABI can be enabled on PowerPC & SuperH by suitable use of attributes or options.

When I folded TYPE_METHODS into TYPE_FIELDS, the 'am I the last field' check of place_field could give a false positive in more cases. Specifically if two bitfields were separated by a member function declaration, they'd now be placed in separate allocation units.

The place_field code was working under the assumption that the only things on the TYPE_FIELDS list could be FIELD_DECLs possibly followed by TYPE_DECLs. That stopped being true some time ago, and as such we already had a layout bug.

But, it would be bad to make that particular ABI fix in a point release, so this patch just reverts the regression I caused. Sadly, because it requires understanding TEMPLATE_DECL, we can't simply update place_field. Instead I temporarily stitch out undesired DECLs around the call to place_field. This seems the least intrusive, and happens only when ms_bitfield_layout_p is in effect.

I have manually checked the new testcase behaves the same in gcc-7 and patched gcc-8 for an x86_64-mingw32 target. Liu Hao has verified that the microsoft compiler gives the same results.

I also attach a change to wwwdocs describing this change.

Thoughts?

nathan

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

	PR c++/87137
	gcc/
	* stor-layout.c (place_field): Comment about
	layout_nonempty_base_or_field behaviour.
	gcc/cp/
	* class.c (layout_nonempty_base_or_field): Stitch out member
	functions during place_field call when ms_bitfield_layout_p is in
	effect.
	gcc/testsuite/
	* g++.dg/abi/pr87137.C: New.

Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c	(revision 263959)
+++ gcc/cp/class.c	(working copy)
@@ -4041,6 +4041,32 @@ layout_nonempty_base_or_field (record_la
       field_p = true;
     }
 
+  /* PR c++/87137 When ms_bitfield_layout_p is in effect, place_field
+     used to just look at the DECL's TREE_CHAIN to see if it ended the
+     struct.  That was ok when only FIELD_DECLs and TYPE_DECLs were on
+     the chain, AND the TYPE_DECLs were known to be last.  That
+     stopped working when other things, such as static members were
+     also placed there.  However, in GCC 8 onwards all members are on
+     the chain (adding member functions).  We want to restore the
+     pre-GCC-8 behaviour, so the ABI doesn't change in a point
+     release.  Because the middle-end doesn't know about
+     TEMPLATE_DECL, we have to do nastiness here, to make DECL_CHAIN
+     look like it used to before calling place_field.  THIS IS STILL
+     WRONG, but it's the same wrongness ass gcc-7 and earier.  */
+  tree chain = NULL_TREE;
+  if (targetm.ms_bitfield_layout_p (rli->t))
+    {
+      tree probe = decl;
+      while ((probe = TREE_CHAIN (probe)))
+	if (TREE_CODE (STRIP_TEMPLATE (probe)) != FUNCTION_DECL)
+	  break;
+      if (probe != TREE_CHAIN (decl))
+	{
+	  chain = TREE_CHAIN (decl);
+	  TREE_CHAIN (decl) = probe;
+	}
+    }
+
   /* Try to place the field.  It may take more than one try if we have
      a hard time placing the field without putting two objects of the
      same type at the same address.  */
@@ -4111,6 +4137,11 @@ layout_nonempty_base_or_field (record_la
 	break;
     }
 
+  if (chain)
+    /* Restore the original chain our ms-bifield-offset shenanigans
+       above overwrote.  */
+    TREE_CHAIN (decl) = chain;
+  
   /* Now that we know where it will be placed, update its
      BINFO_OFFSET.  */
   if (binfo && CLASS_TYPE_P (BINFO_TYPE (binfo)))
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c	(revision 263959)
+++ gcc/stor-layout.c	(working copy)
@@ -1685,6 +1685,9 @@ place_field (record_layout_info rli, tre
     {
       rli->bitpos = size_binop (PLUS_EXPR, rli->bitpos, DECL_SIZE (field));
 
+      /* PR c++/87137, see note in cp/class.c
+	 layout_nonempty_base_or_field about why this is wrong and
+	 why we can't fix it in this release.  */
       /* 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
Index: gcc/testsuite/g++.dg/abi/pr87137.C
===================================================================
--- gcc/testsuite/g++.dg/abi/pr87137.C	(nonexistent)
+++ gcc/testsuite/g++.dg/abi/pr87137.C	(working copy)
@@ -0,0 +1,39 @@
+// 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.
+
+#define DEFINE(NAME,BASE,THING) \
+  struct NAME BASE {		\
+    unsigned one : 12;		\
+    THING			\
+      unsigned two : 4;		\
+  }
+
+#define BOTH(NAME,BASE,THING)			\
+  DEFINE(NAME##_WITH,BASE,THING);		\
+  DEFINE(NAME##_WITHOUT,BASE,)
+
+template<unsigned I, unsigned J> struct Test;
+template<unsigned I> struct Test<I,I> {};
+
+#define TEST(NAME,BASE,THING)			\
+  BOTH(NAME,BASE,THING);			\
+  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(MType,,struct X{int bob;};);
+TEST(TmplFN,,template<unsigned> void fu (););
+
+// Following already failed in GCC-7
+#if 0
+TEST(Var,,static int var;);
+struct base { int f; };
+TEST(Use,:base,using base::f;);
+TEST(Tmpl,,template<unsigned> class X {};);
+#endif
Index: htdocs/gcc-8/changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-8/changes.html,v
retrieving revision 1.93
diff -r1.93 changes.html
1350a1351,1374
> <!-- .................................................................. -->
> <h2 id="GCC8.3">GCC 8.3</h2>
> 
> GCC 8.3 is <em>not</em> yet released.
> 
> <h3>Structure Layout for Windows, PowerPC &amp; SuperH targets</h4> A
>   C++ Microsoft ABI bitfield layout
>   regression, <a href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87137";>PR87137</a>
>   introduced in GCC 8.1, has been fixed.  A declaration of a member
>   function or member function templare 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 targets when <code>__attribute__((ms_struct))</code>
>       is used
>     <li>SuperH targets when the <code>-mhitachi</code> option is
>       specified, or the <code>__attribute__((renesas))</code>
>       attribute is used
>   </ul>
>   The layout bug could also be triggered by other intermediate
>   declarations.  This pre-existing issue will be fixed in GCC 9.1.
> 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]