Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Fri, Apr 13, 2018, 6:09 PM Alexandre Olivawrote: > On Apr 11, 2018, Jason Merrill wrote: > > > I raised this issue with the C++ committee, and it seems that nobody > > expects defining a type here to work. So let's go back to your first > > patch, removing the offending part of semicolon3.C. > > All right, here's the adjusted patch. Retested on i686- and > x86_64-linux-gnu. Ok to install? > Ok. Jason [PR c++/85039] no type definitions in builtin offsetof > > Types defined within a __builtin_offsetof argument don't always get > properly recorded as members of their context types, so if they're > anonymous, we may fail to assign them an anon type index for mangling > and ICE. > > We shouldn't allow types to be introduced in __builtin_offsetof, I > think, and Jason says the std committee agrees, so I've arranged for > us to reject them. > > Even then, we still parse the definitions and attempt to assign > mangled names to its member functions, so the ICE remains. Since > we've already reported an error, we might as well complete the name > assignment with an arbitrary index, thus avoiding the ICE. > > We used to have a test that expected to be able to define types in > __builtin_offsetof; this patch removes that specific test. > > > for gcc/cp/ChangeLog > > PR c++/85039 > * parser.c (cp_parser_builtin_offset): Reject type definitions. > * mangle.c (nested_anon_class_index): Avoid crash returning -1 > if we've seen errors. > > for gcc/testsuite/ChangeLog > > PR c++/85039 > * g++.dg/pr85039-1.C: New. > * g++.dg/pr85039-2.C: New. > * g++.dg/parse/semicolon3.C: Remove test_offsetof. > --- > gcc/cp/mangle.c |3 +++ > gcc/cp/parser.c |8 +++- > gcc/testsuite/g++.dg/parse/semicolon3.C |7 --- > gcc/testsuite/g++.dg/pr85039-1.C| 17 + > gcc/testsuite/g++.dg/pr85039-2.C| 10 ++ > 5 files changed, 37 insertions(+), 8 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/pr85039-1.C > create mode 100644 gcc/testsuite/g++.dg/pr85039-2.C > > diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c > index 94c4bed28486..a7f9d686345d 100644 > --- a/gcc/cp/mangle.c > +++ b/gcc/cp/mangle.c > @@ -1623,6 +1623,9 @@ nested_anon_class_index (tree type) > ++index; >} > > + if (seen_error ()) > +return -1; > + >gcc_unreachable (); > } > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index 8b1b271b53d1..bf46165f5ae1 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -9823,7 +9823,13 @@ cp_parser_builtin_offsetof (cp_parser *parser) >parens.require_open (parser); >/* Parse the type-id. */ >location_t loc = cp_lexer_peek_token (parser->lexer)->location; > - type = cp_parser_type_id (parser); > + { > +const char *saved_message = parser->type_definition_forbidden_message; > +parser->type_definition_forbidden_message > + = G_("types may not be defined within __builtin_offsetof"); > +type = cp_parser_type_id (parser); > +parser->type_definition_forbidden_message = saved_message; > + } >/* Look for the `,'. */ >cp_parser_require (parser, CPP_COMMA, RT_COMMA); >token = cp_lexer_peek_token (parser->lexer); > diff --git a/gcc/testsuite/g++.dg/parse/semicolon3.C > b/gcc/testsuite/g++.dg/parse/semicolon3.C > index 8a2b1ac46301..0d46be9ed654 100644 > --- a/gcc/testsuite/g++.dg/parse/semicolon3.C > +++ b/gcc/testsuite/g++.dg/parse/semicolon3.C > @@ -20,13 +20,6 @@ struct OK3 > } // no complaints >(s7); > > -__SIZE_TYPE__ > -test_offsetof (void) > -{ > - // no complaints about a missing semicolon > - return __builtin_offsetof (struct OK4 { int a; int b; }, b); > -} > - > struct OK5 > { >int a; > diff --git a/gcc/testsuite/g++.dg/pr85039-1.C > b/gcc/testsuite/g++.dg/pr85039-1.C > new file mode 100644 > index ..f57c8a261dee > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr85039-1.C > @@ -0,0 +1,17 @@ > +// { dg-do compile { target c++14 } } > + > +constexpr int a() { > + return > + __builtin_offsetof(struct { // { dg-error "types may not be defined" } > +int i; > +short b { > + __builtin_offsetof(struct { // { dg-error "types may not be > defined" } > + int j; > +struct c { // { dg-error "types may not be defined" } > + void d() { > + } > +}; > + }, j) > +}; > + }, i); > +} > diff --git a/gcc/testsuite/g++.dg/pr85039-2.C > b/gcc/testsuite/g++.dg/pr85039-2.C > new file mode 100644 > index ..e6d16325105b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr85039-2.C > @@ -0,0 +1,10 @@ > +// { dg-do compile } > + > +struct d { > + static d *b; > +} * d::b(__builtin_offsetof(struct { // { dg-error "types may not be > defined" } > + int i; > + struct a { // { dg-error "types may not be defined" } > +int c() { return .1f; } > + }; > +}, i)); > > >
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Apr 11, 2018, Jason Merrillwrote: > I raised this issue with the C++ committee, and it seems that nobody > expects defining a type here to work. So let's go back to your first > patch, removing the offending part of semicolon3.C. All right, here's the adjusted patch. Retested on i686- and x86_64-linux-gnu. Ok to install? [PR c++/85039] no type definitions in builtin offsetof Types defined within a __builtin_offsetof argument don't always get properly recorded as members of their context types, so if they're anonymous, we may fail to assign them an anon type index for mangling and ICE. We shouldn't allow types to be introduced in __builtin_offsetof, I think, and Jason says the std committee agrees, so I've arranged for us to reject them. Even then, we still parse the definitions and attempt to assign mangled names to its member functions, so the ICE remains. Since we've already reported an error, we might as well complete the name assignment with an arbitrary index, thus avoiding the ICE. We used to have a test that expected to be able to define types in __builtin_offsetof; this patch removes that specific test. for gcc/cp/ChangeLog PR c++/85039 * parser.c (cp_parser_builtin_offset): Reject type definitions. * mangle.c (nested_anon_class_index): Avoid crash returning -1 if we've seen errors. for gcc/testsuite/ChangeLog PR c++/85039 * g++.dg/pr85039-1.C: New. * g++.dg/pr85039-2.C: New. * g++.dg/parse/semicolon3.C: Remove test_offsetof. --- gcc/cp/mangle.c |3 +++ gcc/cp/parser.c |8 +++- gcc/testsuite/g++.dg/parse/semicolon3.C |7 --- gcc/testsuite/g++.dg/pr85039-1.C| 17 + gcc/testsuite/g++.dg/pr85039-2.C| 10 ++ 5 files changed, 37 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/g++.dg/pr85039-1.C create mode 100644 gcc/testsuite/g++.dg/pr85039-2.C diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c index 94c4bed28486..a7f9d686345d 100644 --- a/gcc/cp/mangle.c +++ b/gcc/cp/mangle.c @@ -1623,6 +1623,9 @@ nested_anon_class_index (tree type) ++index; } + if (seen_error ()) +return -1; + gcc_unreachable (); } diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 8b1b271b53d1..bf46165f5ae1 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -9823,7 +9823,13 @@ cp_parser_builtin_offsetof (cp_parser *parser) parens.require_open (parser); /* Parse the type-id. */ location_t loc = cp_lexer_peek_token (parser->lexer)->location; - type = cp_parser_type_id (parser); + { +const char *saved_message = parser->type_definition_forbidden_message; +parser->type_definition_forbidden_message + = G_("types may not be defined within __builtin_offsetof"); +type = cp_parser_type_id (parser); +parser->type_definition_forbidden_message = saved_message; + } /* Look for the `,'. */ cp_parser_require (parser, CPP_COMMA, RT_COMMA); token = cp_lexer_peek_token (parser->lexer); diff --git a/gcc/testsuite/g++.dg/parse/semicolon3.C b/gcc/testsuite/g++.dg/parse/semicolon3.C index 8a2b1ac46301..0d46be9ed654 100644 --- a/gcc/testsuite/g++.dg/parse/semicolon3.C +++ b/gcc/testsuite/g++.dg/parse/semicolon3.C @@ -20,13 +20,6 @@ struct OK3 } // no complaints (s7); -__SIZE_TYPE__ -test_offsetof (void) -{ - // no complaints about a missing semicolon - return __builtin_offsetof (struct OK4 { int a; int b; }, b); -} - struct OK5 { int a; diff --git a/gcc/testsuite/g++.dg/pr85039-1.C b/gcc/testsuite/g++.dg/pr85039-1.C new file mode 100644 index ..f57c8a261dee --- /dev/null +++ b/gcc/testsuite/g++.dg/pr85039-1.C @@ -0,0 +1,17 @@ +// { dg-do compile { target c++14 } } + +constexpr int a() { + return + __builtin_offsetof(struct { // { dg-error "types may not be defined" } +int i; +short b { + __builtin_offsetof(struct { // { dg-error "types may not be defined" } + int j; +struct c { // { dg-error "types may not be defined" } + void d() { + } +}; + }, j) +}; + }, i); +} diff --git a/gcc/testsuite/g++.dg/pr85039-2.C b/gcc/testsuite/g++.dg/pr85039-2.C new file mode 100644 index ..e6d16325105b --- /dev/null +++ b/gcc/testsuite/g++.dg/pr85039-2.C @@ -0,0 +1,10 @@ +// { dg-do compile } + +struct d { + static d *b; +} * d::b(__builtin_offsetof(struct { // { dg-error "types may not be defined" } + int i; + struct a { // { dg-error "types may not be defined" } +int c() { return .1f; } + }; +}, i)); -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Thu, Apr 5, 2018 at 9:33 AM, Jason Merrillwrote: > On Wed, Apr 4, 2018 at 12:25 PM, Alexandre Oliva wrote: >> On Apr 4, 2018, Jason Merrill wrote: >> >>> On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Oliva wrote: I still think we could attempt to retain the extension as it is, parsing types introduced in data member initializers within the scope of the class containing the data member, like we do, but, when the class is already complete, recording it if not in TYPE_FIELDS, in some additional field consulted for name mangling purposes and, in order to retain compatibility, if the type is not a closure or anonymous, also recording it in the enclosing namespace, so that it is found by lookup as in the quoted snippet. Is that a terrible idea? >> >>> It sounds like a lot of work to support a very questionable pattern. >> >> It's not so much work (the simple patch below does just that, and its >> testing is almost done); I agree it's questionable, and it's limited (it >> never worked in initializers of members of template classes, as the -4 >> testcase, so we don't have to worry about retaining temporary >> compatibility with that), but it's there, so I think we'd be better off >> deprecating it, if that's the direction we want to go. The patch below >> has just the right spot for a deprecation warning, even ;-) >> >> We could recommend users to use a closure that returns the offsetof >> instead of the unadorned offsetof. That would work portably, but we >> shouldn't make the transformation ourselves: it would change the >> ABI-defined numbering of closure types. >> >>> Perhaps we should just disallow defining a type in offsetof if the >>> current scope is a class. >> >> Even anonymous types? I suspect this could break a lot of existing >> code, with anonymous types hiding in macros. > > It seems unlikely to me that such a use of macros would occur at class > scope; there's no C compatibility issue there. I raised this issue with the C++ committee, and it seems that nobody expects defining a type here to work. So let's go back to your first patch, removing the offending part of semicolon3.C. Jason
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Wed, Apr 4, 2018 at 12:25 PM, Alexandre Olivawrote: > On Apr 4, 2018, Jason Merrill wrote: > >> On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Oliva wrote: >>> I still think we could attempt to retain the extension as it is, parsing >>> types introduced in data member initializers within the scope of the >>> class containing the data member, like we do, but, when the class is >>> already complete, recording it if not in TYPE_FIELDS, in some additional >>> field consulted for name mangling purposes and, in order to retain >>> compatibility, if the type is not a closure or anonymous, also recording >>> it in the enclosing namespace, so that it is found by lookup as in the >>> quoted snippet. >>> >>> Is that a terrible idea? > >> It sounds like a lot of work to support a very questionable pattern. > > It's not so much work (the simple patch below does just that, and its > testing is almost done); I agree it's questionable, and it's limited (it > never worked in initializers of members of template classes, as the -4 > testcase, so we don't have to worry about retaining temporary > compatibility with that), but it's there, so I think we'd be better off > deprecating it, if that's the direction we want to go. The patch below > has just the right spot for a deprecation warning, even ;-) > > We could recommend users to use a closure that returns the offsetof > instead of the unadorned offsetof. That would work portably, but we > shouldn't make the transformation ourselves: it would change the > ABI-defined numbering of closure types. > >> Perhaps we should just disallow defining a type in offsetof if the >> current scope is a class. > > Even anonymous types? I suspect this could break a lot of existing > code, with anonymous types hiding in macros. It seems unlikely to me that such a use of macros would occur at class scope; there's no C compatibility issue there. Jason
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Apr 4, 2018, Jason Merrillwrote: > On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Oliva wrote: >> I still think we could attempt to retain the extension as it is, parsing >> types introduced in data member initializers within the scope of the >> class containing the data member, like we do, but, when the class is >> already complete, recording it if not in TYPE_FIELDS, in some additional >> field consulted for name mangling purposes and, in order to retain >> compatibility, if the type is not a closure or anonymous, also recording >> it in the enclosing namespace, so that it is found by lookup as in the >> quoted snippet. >> >> Is that a terrible idea? > It sounds like a lot of work to support a very questionable pattern. It's not so much work (the simple patch below does just that, and its testing is almost done); I agree it's questionable, and it's limited (it never worked in initializers of members of template classes, as the -4 testcase, so we don't have to worry about retaining temporary compatibility with that), but it's there, so I think we'd be better off deprecating it, if that's the direction we want to go. The patch below has just the right spot for a deprecation warning, even ;-) We could recommend users to use a closure that returns the offsetof instead of the unadorned offsetof. That would work portably, but we shouldn't make the transformation ourselves: it would change the ABI-defined numbering of closure types. > Perhaps we should just disallow defining a type in offsetof if the > current scope is a class. Even anonymous types? I suspect this could break a lot of existing code, with anonymous types hiding in macros. Anyway, here's the patch. I'm not quite proposing it for inclusion (messing with TYPE_FIELDS directly was an experiment, and it worked, but it could use some polishing ;-) but it shows it can be done without too much trouble. Presumably we'll want a deprecation notice in the patch and in the tests. [PR c++/85039] adjust context of new types in member initializers Types defined in data member initializers of completed classes are defined inconsistently: the context of the type and decl are set up so that they seem to be members of the class containing the data member, but the type decl is recorded in the enclosing namespace. This is not a problem for named types, but anonymous types that have a classtype as their context need to be able to find themselves in the field list of the context type to be assigned an anon type count for its mangled name. This patch arranges for the context recorded in the type to match the context in which its definition is introduced, namely that of the class containing the data member, but preserving preexisting behavior of making named types introduced in data member initializers visible in the enclosing namespace. for gcc/cp/ChangeLog PR c++/85039 * name-lookup.c (do_pushtag): Use correct context and scope for types introduced in data member initializers of completed classes, but retain visibility in enclosing namespace. for gcc/testsuite/ChangeLog PR c++/85039 * g++.dg/pr85039-1.C: New. * g++.dg/pr85039-2.C: New. * g++.dg/pr85039-3.C: New. * g++.dg/pr85039-4.C: New. --- gcc/cp/name-lookup.c | 30 -- gcc/testsuite/g++.dg/pr85039-1.C | 17 + gcc/testsuite/g++.dg/pr85039-2.C | 10 ++ gcc/testsuite/g++.dg/pr85039-3.C | 15 +++ gcc/testsuite/g++.dg/pr85039-4.C | 12 5 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 gcc/testsuite/g++.dg/pr85039-1.C create mode 100644 gcc/testsuite/g++.dg/pr85039-2.C create mode 100644 gcc/testsuite/g++.dg/pr85039-3.C create mode 100644 gcc/testsuite/g++.dg/pr85039-4.C diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 9b5db3dc3aa7..02023b764324 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -6394,13 +6394,7 @@ do_pushtag (tree name, tree type, tag_scope scope) view of the language. */ || (b->kind == sk_template_parms && (b->explicit_spec_p || scope == ts_global)) -|| (b->kind == sk_class -&& (scope != ts_current -/* We may be defining a new type in the initializer - of a static member variable. We allow this when - not pedantic, and it is particularly useful for - type punning via an anonymous union. */ -|| COMPLETE_TYPE_P (b->this_entity +|| (b->kind == sk_class && scope != ts_current)) b = b->level_chain; gcc_assert (identifier_p (name)); @@ -6455,9 +6449,25 @@ do_pushtag (tree name, tree type, tag_scope scope) { if (!TYPE_BEING_DEFINED (current_class_type) && !LAMBDA_TYPE_P (type)) - return error_mark_node; - - if
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Olivawrote: > On Apr 3, 2018, Alexandre Oliva wrote: > >> On Apr 2, 2018, Jason Merrill wrote: >>> On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Oliva wrote: struct a { static int const z, i = __builtin_offsetof(struct b { int j; }, j); b c; }; int const a::z = __builtin_offsetof(struct d { int k; }, k); d e; > Since d is visible, I suppose the most conservative solution would be to name the global namespace as the context for type d, rather than defining it as a member of a. Right? > >>> The global namespace would be a rather arbitrary choice; it seems to >>> me that using the current scope is a natural interpretation. > >>> About d in the example, I'm not sure how you mean the global namespace >>> is the current scope; we should be pushed into a when parsing the >>> initializer for a::z. > >> I was just describing observed behavior. The code above compiles. > >> The explanation is in do_pushtag. It starts with a loop that, among >> other things, skips COMPLETE_TYPE_P class scopes. we then record the >> new type in the namespace named by the resulting scope. As the comment >> says, this is meant to allow for types to be introduced in initializers >> of static data members in spite of the class being already complete. > >> The problem, as I see it, is that we don't adjust the context to match, >> so we introduce the type in one scope, but claim it to belong to >> another. Which sort of works for named types, but comes down in flames >> (err, reenters like Tiangong-1? ;-) if the type is anonymous. > >>> But I would think we should reject the definition of d because a is >>> already complete, so it's too late to add members to it. > >> The existing code in GCC sort of disagrees with your proposal, so, for >> the sake of avoiding breaking code that we used to compile (like the >> definition 'd e' above), I'll insist on something along the lines of the >> following patch: > > That patch breaks various cases of lambdas in data member initializers > that reference members of the class containing the data member. > > This suggested to me that we already have infrastructure for and > expectations of introducing types within a class after the class is > complete. Using similar infrastructure to make types introduced within > __builtin_offsetof, or even in type-casts under the extension I > attempted to adjust in the proposed patch (the extension that Nathan > mentioned he also ran into). > > I still think we could attempt to retain the extension as it is, parsing > types introduced in data member initializers within the scope of the > class containing the data member, like we do, but, when the class is > already complete, recording it if not in TYPE_FIELDS, in some additional > field consulted for name mangling purposes and, in order to retain > compatibility, if the type is not a closure or anonymous, also recording > it in the enclosing namespace, so that it is found by lookup as in the > quoted snippet. > > Is that a terrible idea? It sounds like a lot of work to support a very questionable pattern. Perhaps we should just disallow defining a type in offsetof if the current scope is a class. Jason
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Apr 3, 2018, Alexandre Olivawrote: > On Apr 2, 2018, Jason Merrill wrote: >> On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Oliva wrote: >>> struct a { >>> static int const z, i = __builtin_offsetof(struct b { int j; }, j); >>> b c; >>> }; >>> int const a::z = __builtin_offsetof(struct d { int k; }, k); >>> d e; >>> Since d is visible, I suppose the most conservative solution would be to >>> name the global namespace as the context for type d, rather than >>> defining it as a member of a. Right? >> The global namespace would be a rather arbitrary choice; it seems to >> me that using the current scope is a natural interpretation. >> About d in the example, I'm not sure how you mean the global namespace >> is the current scope; we should be pushed into a when parsing the >> initializer for a::z. > I was just describing observed behavior. The code above compiles. > The explanation is in do_pushtag. It starts with a loop that, among > other things, skips COMPLETE_TYPE_P class scopes. we then record the > new type in the namespace named by the resulting scope. As the comment > says, this is meant to allow for types to be introduced in initializers > of static data members in spite of the class being already complete. > The problem, as I see it, is that we don't adjust the context to match, > so we introduce the type in one scope, but claim it to belong to > another. Which sort of works for named types, but comes down in flames > (err, reenters like Tiangong-1? ;-) if the type is anonymous. >> But I would think we should reject the definition of d because a is >> already complete, so it's too late to add members to it. > The existing code in GCC sort of disagrees with your proposal, so, for > the sake of avoiding breaking code that we used to compile (like the > definition 'd e' above), I'll insist on something along the lines of the > following patch: That patch breaks various cases of lambdas in data member initializers that reference members of the class containing the data member. This suggested to me that we already have infrastructure for and expectations of introducing types within a class after the class is complete. Using similar infrastructure to make types introduced within __builtin_offsetof, or even in type-casts under the extension I attempted to adjust in the proposed patch (the extension that Nathan mentioned he also ran into). I still think we could attempt to retain the extension as it is, parsing types introduced in data member initializers within the scope of the class containing the data member, like we do, but, when the class is already complete, recording it if not in TYPE_FIELDS, in some additional field consulted for name mangling purposes and, in order to retain compatibility, if the type is not a closure or anonymous, also recording it in the enclosing namespace, so that it is found by lookup as in the quoted snippet. Is that a terrible idea? -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On 04/02/2018 06:18 PM, Jason Merrill wrote: About d in the example, I'm not sure how you mean the global namespace is the current scope; we should be pushed into a when parsing the initializer for a::z. this may be related to the brokenness I encountered in 85046. We push member structs into the wrong binding level. And there's this in name-lookup.c: do_pushtag: && (scope != ts_current /* We may be defining a new type in the initializer of a static member variable. We allow this when not pedantic, and it is particularly useful for type punning via an anonymous union. */ || COMPLETE_TYPE_P (b->this_entity But I would think we should reject the definition of d because a is already complete, so it's too late to add members to it. Agreed. -- Nathan Sidwell
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Apr 2, 2018, Jason Merrillwrote: > On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Oliva wrote: >> struct a { >> static int const z, i = __builtin_offsetof(struct b { int j; }, j); >> b c; >> }; >> int const a::z = __builtin_offsetof(struct d { int k; }, k); >> d e; >> Since d is visible, I suppose the most conservative solution would be to >> name the global namespace as the context for type d, rather than >> defining it as a member of a. Right? > The global namespace would be a rather arbitrary choice; it seems to > me that using the current scope is a natural interpretation. > About d in the example, I'm not sure how you mean the global namespace > is the current scope; we should be pushed into a when parsing the > initializer for a::z. I was just describing observed behavior. The code above compiles. The explanation is in do_pushtag. It starts with a loop that, among other things, skips COMPLETE_TYPE_P class scopes. we then record the new type in the namespace named by the resulting scope. As the comment says, this is meant to allow for types to be introduced in initializers of static data members in spite of the class being already complete. The problem, as I see it, is that we don't adjust the context to match, so we introduce the type in one scope, but claim it to belong to another. Which sort of works for named types, but comes down in flames (err, reenters like Tiangong-1? ;-) if the type is anonymous. > But I would think we should reject the definition of d because a is > already complete, so it's too late to add members to it. The existing code in GCC sort of disagrees with your proposal, so, for the sake of avoiding breaking code that we used to compile (like the definition 'd e' above), I'll insist on something along the lines of the following patch: [PR c++/85039] adjust context of new types in member initializers Types defined in data member initializers of completed classes are defined inconsistently: the context of the type and decl are set up so that they seem to be members of the class containing the data member, but the type decl is recorded in the enclosing namespace. This is not a problem for named types, but anonymous types that have a classtype as their context need to be able to find themselves in the field list of the context type to be assigned an anon type count for its mangled name. This patch arranges for the context recorded in the type to match the context in which its definition is introduced. This preserves intentional preexisting behavior for named types introduced in data member initializers. for gcc/cp/ChangeLog PR c++/85039 * name-lookup.c (do_pushtag): Make context match scope for types introduced in data member initializers of completed classes. for gcc/testsuite/ChangeLog PR c++/85039 * g++.dg/pr85039-1.C: New. * g++.dg/pr85039-2.C: New. * g++.dg/pr85039-3.C: New. --- gcc/cp/name-lookup.c | 17 ++--- gcc/testsuite/g++.dg/pr85039-1.C | 17 + gcc/testsuite/g++.dg/pr85039-2.C | 10 ++ gcc/testsuite/g++.dg/pr85039-3.C | 13 + 4 files changed, 54 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/g++.dg/pr85039-1.C create mode 100644 gcc/testsuite/g++.dg/pr85039-2.C create mode 100644 gcc/testsuite/g++.dg/pr85039-3.C diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 061729a989b6..97618d414edd 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -6394,9 +6394,10 @@ do_pushtag (tree name, tree type, tag_scope scope) || (b->kind == sk_class && (scope != ts_current /* We may be defining a new type in the initializer - of a static member variable. We allow this when - not pedantic, and it is particularly useful for - type punning via an anonymous union. */ + of a static member variable. We allow this for + __builtin_offsetof, and when not pedantic, and it + is particularly useful for type punning via an + anonymous union. */ || COMPLETE_TYPE_P (b->this_entity b = b->level_chain; @@ -6422,6 +6423,16 @@ do_pushtag (tree name, tree type, tag_scope scope) containing the local class, not the namespace scope. */ context = decl_function_context (get_type_decl (cs)); + + /* We may be defining a new type in the initializer of a +static member variable. We allow this for +__builtin_offsetof, and also when not pedantic, and it is +particularly useful for type punning via an anonymous +union. */ + while (context && TYPE_P (context) +&& scope == ts_current +&& COMPLETE_TYPE_P (context)) + context =
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Olivawrote: > On Mar 30, 2018, Jason Merrill wrote: > >> On Fri, Mar 30, 2018 at 3:55 AM, Alexandre Oliva wrote: >>> Types defined within a __builtin_offsetof argument don't always get >>> properly recorded as members of their context types > >>> I suppose this means I should look for another solution that doesn't >>> involve rejecting these definitions, eh? > >> Hmm, I'm afraid so > > Ok, but... tricky... > > If I were to make the anonymous types members of the enclosing class, > their members would be promoted to members of the enclosing class, which > doesn't sound like the right thing to do. Agreed. The C definition talks in terms of a variable declaration static t; so treating the type as an anonymous aggregate member of the class would be wrong; it can be a type member, but not an anonymous aggregate. > But I wonder, should they even be regarded as members of the enclosing > class, when they do not appear inside the body of the class. Perhaps > their context should be something else, say the innermost enclosing > namespace, the global namespace, some anonymous namespace introduced for > the offsetof... > One thing that's not clear to me is whether types defined in offsetof > can be referenced outside their own definition. In C, that would be > natural, since the struct namespace is flat, but in C++, structs belong > to a context, and it's not obvious to me that offsetof should inject > names in an enclosing context, or in a separate invisible namespace. I > lean towards the latter, but then I tried: > > template class foo { }; > int j = __builtin_offsetof(struct a { int i; static foo x = foo(); }, > i); > > and found (through debug info) that foo is mangled as if struct a was > defined in the global namespace. And, indeed, a is recorded in the > global namespace: > > a b; > > which was slightly surprising to me. > > But it seems to be consistent: when the offsetof expression is within a > function, the type will be defined within the function; when it is used > in the initializer of a data member, the context is the class containing > the data member, even if the initializer is outside the class body, but > the type is defined in the global namespace: > > struct a { > static int const z, i = __builtin_offsetof(struct b { int j; }, j); > b c; > }; > int const a::z = __builtin_offsetof(struct d { int k; }, k); > d e; > b f; // b is not defined > a::b g; // ok > > So, the problem with anon types defined in offsetof appears to be the > inconsistency between the scope in which d is introduced, namely the > global namespace because that's the current scope, and the DECL_CONTEXT, > copied to TYPE_CONTEXT, that is taken as class a because we're parsing > the initializer for a::z. Since they disagree, we don't find the type > defined in the context named as enclosing it. > > Since d is visible, I suppose the most conservative solution would be to > name the global namespace as the context for type d, rather than > defining it as a member of a. Right? The global namespace would be a rather arbitrary choice; it seems to me that using the current scope is a natural interpretation. About d in the example, I'm not sure how you mean the global namespace is the current scope; we should be pushed into a when parsing the initializer for a::z. But I would think we should reject the definition of d because a is already complete, so it's too late to add members to it. > Now, I really don't want to think of offsetof appearing in default > arguments or array lengths in function prototypes or even in template > parameters. Types are not supposed to be defined in such contexts, > but... should offsetof make an exception? Currently, we reject them, > which is quite a relief, because otherwise it might appear in > expressions in templates and that would be really really hairy ;-) No, offsetof should not make an exception. >> Incidentally, it would be nice to replace all the >> type_definition_forbidden stuff with defining-type-specifier as per DR >> 2141... (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50169) > > *nod* > > Is this in scope for GCC 8? It's not marked as a regression, but I > guess I could try and tackle it if it's intended to make it anyway, > given that I've got some context on this issue. LMK about the plans, > and whether you envision any pitfalls. No, sorry, that's a relatively low priority cleanup. Jason
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Mar 30, 2018, Jason Merrillwrote: > On Fri, Mar 30, 2018 at 3:55 AM, Alexandre Oliva wrote: >> Types defined within a __builtin_offsetof argument don't always get >> properly recorded as members of their context types >> I suppose this means I should look for another solution that doesn't >> involve rejecting these definitions, eh? > Hmm, I'm afraid so Ok, but... tricky... If I were to make the anonymous types members of the enclosing class, their members would be promoted to members of the enclosing class, which doesn't sound like the right thing to do. But I wonder, should they even be regarded as members of the enclosing class, when they do not appear inside the body of the class. Perhaps their context should be something else, say the innermost enclosing namespace, the global namespace, some anonymous namespace introduced for the offsetof... One thing that's not clear to me is whether types defined in offsetof can be referenced outside their own definition. In C, that would be natural, since the struct namespace is flat, but in C++, structs belong to a context, and it's not obvious to me that offsetof should inject names in an enclosing context, or in a separate invisible namespace. I lean towards the latter, but then I tried: template class foo { }; int j = __builtin_offsetof(struct a { int i; static foo x = foo(); }, i); and found (through debug info) that foo is mangled as if struct a was defined in the global namespace. And, indeed, a is recorded in the global namespace: a b; which was slightly surprising to me. But it seems to be consistent: when the offsetof expression is within a function, the type will be defined within the function; when it is used in the initializer of a data member, the context is the class containing the data member, even if the initializer is outside the class body, but the type is defined in the global namespace: struct a { static int const z, i = __builtin_offsetof(struct b { int j; }, j); b c; }; int const a::z = __builtin_offsetof(struct d { int k; }, k); d e; b f; // b is not defined a::b g; // ok So, the problem with anon types defined in offsetof appears to be the inconsistency between the scope in which d is introduced, namely the global namespace because that's the current scope, and the DECL_CONTEXT, copied to TYPE_CONTEXT, that is taken as class a because we're parsing the initializer for a::z. Since they disagree, we don't find the type defined in the context named as enclosing it. Since d is visible, I suppose the most conservative solution would be to name the global namespace as the context for type d, rather than defining it as a member of a. Right? Now, I really don't want to think of offsetof appearing in default arguments or array lengths in function prototypes or even in template parameters. Types are not supposed to be defined in such contexts, but... should offsetof make an exception? Currently, we reject them, which is quite a relief, because otherwise it might appear in expressions in templates and that would be really really hairy ;-) > Incidentally, it would be nice to replace all the > type_definition_forbidden stuff with defining-type-specifier as per DR > 2141... (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50169) *nod* Is this in scope for GCC 8? It's not marked as a regression, but I guess I could try and tackle it if it's intended to make it anyway, given that I've got some context on this issue. LMK about the plans, and whether you envision any pitfalls. -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof
On Fri, Mar 30, 2018 at 3:55 AM, Alexandre Olivawrote: > Types defined within a __builtin_offsetof argument don't always get > properly recorded as members of their context types, so if they're > anonymous, we may fail to assign them an anon type index for mangling > and ICE. > > We shouldn't allow types to be introduced in __builtin_offsetof, I > think, so I've arranged for us to reject them. Even then, we still > parse the definitions and attempt to assign mangled names to its > member functions, so the ICE remains. Since we've already reported an > error, we might as well complete the name assignment with an arbitrary > index, thus avoiding the ICE. > > > Regstrapped on i686- and x86_64-linux-gnu, regressing > g++.dg/parse/semicolon3.C, which defines a (named) struct in > builtin_offsetof. I suppose this means I should look for another > solution that doesn't involve rejecting these definitions, eh? Hmm, I'm afraid so. The C standard defines offsetof in terms of a notional declaration static /type/ t; and a type definition is allowed in such a declaration. Incidentally, it would be nice to replace all the type_definition_forbidden stuff with defining-type-specifier as per DR 2141... (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50169) Jason