Re: [PR c++/87137] GCC-8 Fix
On Fri, 31 Aug 2018, Jason Merrill wrote: > > Anonymous structures and unions are in C11 (and before that a widely > > accepted extension). > > This isn't an anonymous union, it's named "x1". I don't think that > line declares a field in C, either. Indeed, the case where the field has a tag is one of the extensions from -fms-extensions / -fplan9-extensions, not part of the C11 anonymous struct / union feature which requires a struct or union without a tag as the type of the unnamed field. -- Joseph S. Myers jos...@codesourcery.com
Re: [PR c++/87137] GCC-8 Fix
On Fri, Aug 31, 2018 at 10:35 AM, Michael Matz wrote:
> Hi,
>
> On Thu, 30 Aug 2018, Nathan Sidwell wrote:
>
>> > 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. I think
>> that'd be a C vendor extension, I don't think unnamed fields are permitted
>> there?
>
> Anonymous structures and unions are in C11 (and before that a widely
> accepted extension).
This isn't an anonymous union, it's named "x1". I don't think that
line declares a field in C, either.
Jason
Re: [PR c++/87137] GCC-8 Fix
Hi,
On Thu, 30 Aug 2018, Nathan Sidwell wrote:
> > 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. I think
> that'd be a C vendor extension, I don't think unnamed fields are permitted
> there?
Anonymous structures and unions are in C11 (and before that a widely
accepted extension).
Ciao,
Michael.
Re: [PR c++/87137] GCC-8 Fix
On Thu, Aug 30, 2018 at 5:09 PM JonY <10wa...@gmail.com> wrote:
>
> On 08/30/2018 11:59 AM, Nathan Sidwell wrote:
> > 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. 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
>
> I don't have any objections.
Me neither if appropriately documented in changes.html.
Richard.
>
Re: [PR c++/87137] GCC-8 Fix
On 08/30/2018 11:59 AM, Nathan Sidwell wrote:
> 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. 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
I don't have any objections.
signature.asc
Description: OpenPGP digital signature
Re: [PR c++/87137] GCC-8 Fix
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. 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
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 struct Test;
+template struct Test {};
+
+#define TEST(NAME,BASE,THING) \
+ DEFINE(NAME##_WITH,BASE,THING); \
+ DEFINE(NAME##_WITHOUT,BASE,); \
+ int NAME = sizeof (Test)
+
+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 class X {};);
+TEST(TmplFN,,template 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
<
---
> Windows
212a213,227
>
> A C++ Microsoft ABI bitfield layout
> bug, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87137;>PR87137
> 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:
>
> Mingw targets
> PowerPC, IA-32 or x86-64 targets
> when -mms-bitfields option is specified
> or __attribute__((ms_struct)) is used
> SuperH targets when the -mhitachi option is
> specified, or __attribute__((renesas)) is used
>
Re: [PR c++/87137] GCC-8 Fix
在 2018-08-30 01:36, Nathan Sidwell 写道:
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.
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.
An important reason why such code could be rare is that the following
code compiles differently as C and C++:
```
E:\Desktop>cat test.cc
#include
struct foo
{
unsigned a : 21;
union x1 { char x; };
unsigned b : 11;
union x1 u;
};
struct bar
{
union x2 { char x; };
unsigned a : 21;
unsigned b : 11;
union x2 u;
};
int main(void)
{
printf("%d\n", (int)sizeof(struct foo));
printf("%d\n", (int)sizeof(struct bar));
}
```
When compiled as C the output is:
```
E:\Desktop>cl /nologo /Tctest.cc /Fe:a.exe && a.exe
test.cc
16
12
```
while as C++ the output is:
```
E:\Desktop>cl /nologo /Tptest.cc /Fe:a.exe && a.exe
test.cc
8
8
```
Basing on the fact that such code is rare I think it should be safe to
trade tolerance (or 'compatibility for this bug') for the correct behavior.
--
Best regards,
LH_Mouse
Re: [PR c++/87137] GCC-8 Fix
On Wed, Aug 29, 2018 at 01:36:15PM -0400, Nathan Sidwell wrote:
> --- 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. */
s/ass gcc-7 and earier/as gcc-7 and earlier/ ?
> + tree chain = NULL_TREE;
> + if (targetm.ms_bitfield_layout_p (rli->t))
> +{
> + tree probe = decl;
> + while ((probe = TREE_CHAIN (probe)))
Any reason you are using TREE_CHAIN rather than DECL_CHAIN everywhere (in
comments as well as here and below?
Shouldn't all chain elts be decls?
> + 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)))
Jakub
Re: [PR c++/87137] GCC-8 Fix
On August 29, 2018 7:36:15 PM GMT+02:00, Nathan Sidwell wrote: >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? If it is that cumbersome to fix just the regression part we might also consider to fix the latent problem as well for GCC 8? We're already breaking the GCC 8 ABI and will be breaking the ABI again for GCC 9. Richard. > >nathan
[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
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); \
+
