[PR,c++/85039] no type definitions in builtin offsetof

Message ID or1sg26lr4.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • [PR,c++/85039] no type definitions in builtin offsetof
Related show

Commit Message

Alexandre Oliva March 30, 2018, 7:55 a.m.
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?


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.
---
 gcc/cp/mangle.c                  |    3 +++
 gcc/cp/parser.c                  |    8 +++++++-
 gcc/testsuite/g++.dg/pr85039-1.C |   17 +++++++++++++++++
 gcc/testsuite/g++.dg/pr85039-2.C |   10 ++++++++++
 4 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/pr85039-1.C
 create mode 100644 gcc/testsuite/g++.dg/pr85039-2.C



-- 
Alexandre Oliva, freedom fighter    http://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

Comments

Jason Merrill March 30, 2018, 3:05 p.m. | #1
On Fri, Mar 30, 2018 at 3:55 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> 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
Alexandre Oliva March 31, 2018, 11:12 a.m. | #2
On Mar 30, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Fri, Mar 30, 2018 at 3:55 AM, Alexandre Oliva <aoliva@redhat.com> 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 <typename T> class foo { };
int j = __builtin_offsetof(struct a { int i; static foo<a> x = foo<a>(); }, i);

and found (through debug info) that foo<a> 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 fighter    http://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
Jason Merrill April 2, 2018, 10:18 p.m. | #3
On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 30, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Fri, Mar 30, 2018 at 3:55 AM, Alexandre Oliva <aoliva@redhat.com> 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 <type> 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 <typename T> class foo { };

> int j = __builtin_offsetof(struct a { int i; static foo<a> x = foo<a>(); }, i);

>

> and found (through debug info) that foo<a> 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
Alexandre Oliva April 3, 2018, 8:03 a.m. | #4
On Apr  2, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Oliva <aoliva@redhat.com> 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 = TYPE_CONTEXT (context);
 	}
       if (!context)
 	context = current_namespace;
diff --git a/gcc/testsuite/g++.dg/pr85039-1.C b/gcc/testsuite/g++.dg/pr85039-1.C
new file mode 100644
index 000000000000..8ff0352f11f7
--- /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 {
+    int i;
+    short b {
+      __builtin_offsetof(struct {
+	int j;
+        struct c {
+          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 000000000000..e546732a790d
--- /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 {
+  int i;
+  struct a {
+    int c() { return .1f; }
+  };
+}, i));
diff --git a/gcc/testsuite/g++.dg/pr85039-3.C b/gcc/testsuite/g++.dg/pr85039-3.C
new file mode 100644
index 000000000000..117cba792fd5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr85039-3.C
@@ -0,0 +1,13 @@
+// { dg-do compile }
+
+// Check that the type defined in the initializer of a member of a
+// completed class is defined remains visible in the enclosing
+// namespace scope.
+
+struct a {
+  struct b {
+    static int c;
+  };
+};
+int a::b::c = __builtin_offsetof(struct d { int k; }, k);
+d e;


-- 
Alexandre Oliva, freedom fighter    http://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
Nathan Sidwell April 3, 2018, 11:04 a.m. | #5
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
Alexandre Oliva April 4, 2018, 3:25 a.m. | #6
On Apr  3, 2018, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Apr  2, 2018, Jason Merrill <jason@redhat.com> wrote:

>> On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Oliva <aoliva@redhat.com> 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 fighter    http://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
Jason Merrill April 4, 2018, 2:45 p.m. | #7
On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Apr  3, 2018, Alexandre Oliva <aoliva@redhat.com> wrote:

>

>> On Apr  2, 2018, Jason Merrill <jason@redhat.com> wrote:

>>> On Sat, Mar 31, 2018 at 7:12 AM, Alexandre Oliva <aoliva@redhat.com> 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
Alexandre Oliva April 4, 2018, 4:25 p.m. | #8
On Apr  4, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Oliva <aoliva@redhat.com> 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 (!PROCESSING_REAL_TEMPLATE_DECL_P ())
+	    {
+	      TYPE_FIELDS (current_class_type)
+		= chainon (TYPE_FIELDS (current_class_type), decl);
+	      if (!ANON_AGGR_TYPE_P (type)
+		  && !PROCESSING_REAL_TEMPLATE_DECL_P ())
+		{
+		  in_class = 0;
+		  /* We may be defining a new type in the initializer
+		     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.  */
+		  while (b->kind == sk_class
+			 && scope == ts_current
+			 && COMPLETE_TYPE_P (b->this_entity))
+		    b = b->level_chain;
+		}
+	    }
+	  else if (!PROCESSING_REAL_TEMPLATE_DECL_P ())
 	    /* Put this TYPE_DECL on the TYPE_FIELDS list for the
 	       class.  But if it's a member template class, we want
 	       the TEMPLATE_DECL, not the TYPE_DECL, so this is done
diff --git a/gcc/testsuite/g++.dg/pr85039-1.C b/gcc/testsuite/g++.dg/pr85039-1.C
new file mode 100644
index 000000000000..8ff0352f11f7
--- /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 {
+    int i;
+    short b {
+      __builtin_offsetof(struct {
+	int j;
+        struct c {
+          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 000000000000..e546732a790d
--- /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 {
+  int i;
+  struct a {
+    int c() { return .1f; }
+  };
+}, i));
diff --git a/gcc/testsuite/g++.dg/pr85039-3.C b/gcc/testsuite/g++.dg/pr85039-3.C
new file mode 100644
index 000000000000..17254584ff3c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr85039-3.C
@@ -0,0 +1,15 @@
+// { dg-do compile }
+
+// Check that the type defined in the initializer of a member of a
+// completed class is defined remains visible in the enclosing
+// namespace scope.
+
+struct a {
+  struct b {
+    static int c;
+  };
+};
+int a::b::c = 0
++ __builtin_offsetof(struct d { int k; }, k);
+d e;
+a::b::d f;
diff --git a/gcc/testsuite/g++.dg/pr85039-4.C b/gcc/testsuite/g++.dg/pr85039-4.C
new file mode 100644
index 000000000000..a45a36dcd6d5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr85039-4.C
@@ -0,0 +1,12 @@
+// { dg-do compile }
+
+template <typename T>
+struct a {
+  struct b {
+    static int c;
+  };
+};
+template <typename T>
+int a<T>::b::c = 0
++ __builtin_offsetof(struct d { int k; }, k); // { dg-error "non-template" }
+template struct a<void>;


-- 
Alexandre Oliva, freedom fighter    http://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
Jason Merrill April 5, 2018, 1:33 p.m. | #9
On Wed, Apr 4, 2018 at 12:25 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Apr  4, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Oliva <aoliva@redhat.com> 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
Jason Merrill April 11, 2018, 4:47 p.m. | #10
On Thu, Apr 5, 2018 at 9:33 AM, Jason Merrill <jason@redhat.com> wrote:
> On Wed, Apr 4, 2018 at 12:25 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> On Apr  4, 2018, Jason Merrill <jason@redhat.com> wrote:

>>

>>> On Tue, Apr 3, 2018 at 11:25 PM, Alexandre Oliva <aoliva@redhat.com> 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
Alexandre Oliva April 13, 2018, 10:09 p.m. | #11
On Apr 11, 2018, Jason Merrill <jason@redhat.com> 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?


[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 000000000000..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 000000000000..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 fighter    http://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
Jason Merrill April 14, 2018, 12:12 a.m. | #12
On Fri, Apr 13, 2018, 6:09 PM Alexandre Oliva <aoliva@redhat.com> wrote:

> On Apr 11, 2018, Jason Merrill <jason@redhat.com> 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 000000000000..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 000000000000..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 fighter    http://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

>

Patch

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 e946d0b72292..135efb7eb2da 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -9833,7 +9833,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/pr85039-1.C b/gcc/testsuite/g++.dg/pr85039-1.C
new file mode 100644
index 000000000000..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 000000000000..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));