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

[Alexandre Oliva <aoliva at redhat dot com>]

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