| Message ID | 1ae01cf1-3c10-84a8-418f-efa38ad78032@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,c++/87137] GCC-8 Fix | expand |
On August 29, 2018 7:36:15 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> 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
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
在 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 <stdio.h> 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.
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
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. <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 I don't have any objections.
On Thu, Aug 30, 2018 at 5:09 PM JonY <10walls@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. <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 > > I don't have any objections. Me neither if appropriately documented in changes.html. Richard. >
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. <aside> 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.
On Fri, Aug 31, 2018 at 10:35 AM, Michael Matz <matz@suse.de> 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. <aside> 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
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.
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 & 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. >