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