Re: [PATCH] [PR c++/85039] no type definitions in builtin offsetof

[Alexandre Oliva <aoliva at redhat dot com>]

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