diff mbox

[C++] RFC: implement P0386R2 - C++17 inline variables

Message ID CADzB+2npzVw4WmehJUQpemOrVkZjXomxywiqMGcB22N7ZvQsPw@mail.gmail.com
State New
Headers show

Commit Message

Jason Merrill Oct. 13, 2016, 7:34 p.m. UTC 
On Tue, Oct 11, 2016 at 9:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Here is an attempt to implement C++17 inline variables.
> Bootstrapped/regtested on x86_64-linux and i686-linux.
>
> The main question is if the inline variables, which are vague linkage,
> should be !DECL_EXTERNAL or DECL_EXTERNAL DECL_NOT_REALLY_EXTERN while
> in the FE.  In the patch, they are !DECL_EXTERNAL, except for inline
> static data members in instantiated templates.  As the inline-var3.C
> testcase shows, even if they are !DECL_EXTERNAL (except for the instantiated
> static data members), even at -O0 we do not actually emit them unless
> odr-used, so to some extent I don't see the point in forcing them to
> be DECL_EXTERNAL DECL_NOT_REALLY_EXTERN.

Yeah, I ended up agreeing with you.  There's no need to work hard to
make them work with an obsolete system.  So I've checked in the patch
with a few minor tweaks, attached.

> Another thing is I've noticed (with Jonathan's help to look it up) that
> we aren't implementing DR1511, I'm willing to try to implement that, but
> as it will need to touch the same spots as the patch, I think it should be
> resolved incrementally.

Sounds good.

> Yet another thing are thread_local inline vars.  E.g. on:
> struct S { S (); ~S (); };
> struct T { ~T (); };
> int foo ();
> thread_local inline S s;
> inline thread_local T t;
> inline thread_local int u = foo ();
> it seems both clang++ (~2 months old) and g++ with the patch emits:
> 8 byte TLS _ZGV1{stu] variables
> 1 byte TLS __tls_guard variable
> and _ZTH1[stu] being aliases to __tls_init, which does essentially:
>   if (__tls_guard) return;
>   __tls_guard = 1;
>   if (*(char *)&_ZGV1s == 0) {
>     *(char *)&_ZGV1s = 1;
>     S::S (&s);
>     __cxa_thread_atexit (S::~S, &s, &__dso_handle);
>   }
>   if (*(char *)&_ZGV1t == 0) {
>     *(char *)&_ZGV1t = 1;
>     __cxa_thread_atexit (T::~T, &t, &__dso_handle);
>   }
>   if (*(char *)&_ZGV1u == 0) {
>     *(char *)&_ZGV1u = 1;
>     u = foo ();
>   }
> Is that what we want to emit?  At first I doubted this could work properly,
> now thinking about it more, perhaps it can.

I think so; I don't see a reason for inline vars to work differently
from templates here.

> And, do we really want all the
> _ZGV* vars for the TLS inline vars (and other TLS comdats) to be 8 byte,
> even when we are using just a single byte?  Or is it too late to change (ABI
> break)?

Right, the ABI specifies that the guard variable is 8 bytes.  A
comment says, "The intent of specifying an 8-byte structure for the
guard variable, but only describing one byte of its contents, is to
allow flexibility in the implementation of the API above. On systems
with good small lock support, the second word might be used for a
mutex lock. On others, it might identify (as a pointer or index) a
more complex lock structure to use."  This seems unnecessary for
systems with byte atomic instructions, and we might be able to get
away with changing it without breaking any actual usage, but it would
indeed be an ABI change.

> And, as mentioned in the DWARF mailing list, I think we should emit
> DW_AT_inline on the inline vars (both explicit and implicit - static
> constexpr data members in C++17 mode).  I hope that can be done as a
> follow-up.

Makes sense.

Comments

Andre Vieira (lists) Oct. 20, 2016, 4:21 p.m. UTC | #1 
On 13/10/16 20:34, Jason Merrill wrote:
> On Tue, Oct 11, 2016 at 9:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:

> 
>> And, as mentioned in the DWARF mailing list, I think we should emit
>> DW_AT_inline on the inline vars (both explicit and implicit - static
>> constexpr data members in C++17 mode).  I hope that can be done as a
>> follow-up.
> 
> Makes sense.
> 

As I write this I have been waiting for my registration email for the
DWARF mailing list, so I havent seen the email you sent.

I was wondering whether the regressions I'm picking up for
arm-none-eabi-gdb are related to the change in behavior you are
suggesting. These are the fails:

$9 = {static s = 10, _vptr.A = 0xbb14 <vtable for A+8>, c = 120 'x', j =
33, jj = 1331, s = 47892}^M
(gdb) FAIL: gdb.cp/member-ptr.exp: print a (j = 33)
...
$12 = {static s = 10, _vptr.A = 0xbb14 <vtable for A+8>, c = 120 'x', j
= 44, jj = 1331, s = 47892}^M
(gdb) FAIL: gdb.cp/member-ptr.exp: print a (j = 44)

The logs used to read:
$9 = {_vptr.A = 0xbb44 <vtable for A+8>, c = 120 'x', j = 33, jj = 1331,
static s = 10}^M
(gdb) PASS: gdb.cp/member-ptr.exp: print a (j = 33)
...
$12 = {_vptr.A = 0xbb44 <vtable for A+8>, c = 120 'x', j = 44, jj =
1331, static s = 10}^M
(gdb) PASS: gdb.cp/member-ptr.exp: print a (j = 44)

As you see the Classe's structure has changed.

After bisecting GCC it does seem it's this patch that causes the change
in debug information.

I did the following to inspect the debug information (renaming A to
BANANA, to make it easier to search):

$ cp src/binutils-gdb/gdb/testsuite/gdb.cp/member-ptr.cc t.c
$ sed -i "s/A/BANANA/g" t.c
$ arm-none-eabi-g++ -c -g -mthumb -mcpu=cortex-m3  t.c
$ arm-none-eabi-g++ t.o -g  -lm -specs=rdimon.specs -lc -lg -lrdimon
-mthumb  -mcpu=cortex-m3
$ arm-none-eabi-readelf -wi a.out

Before this patch you see BANANA's first DW_TAG_MEMBER is '_vptr.BANANA'
and there is only one DW_TAG_MEMBER entry for the 'static s'.
Whereas after this patch you see the first DW_TAG_MEMBER is:
 <2><8be>: Abbrev Number: 34 (DW_TAG_member)
    <8bf>   DW_AT_name        : s
    <8c1>   DW_AT_decl_file   : 1
    <8c2>   DW_AT_decl_line   : 40
    <8c3>   DW_AT_type        : <0x5d>
    <8c7>   DW_AT_external    : 1
    <8c7>   DW_AT_accessibility: 1      (public)
    <8c8>   DW_AT_declaration : 1

the 'static s' that used to be the last, followed by '_vptr.BANANA' and
its last DW_TAG_MEMBER is:
 <2><8f5>: Abbrev Number: 38 (DW_TAG_member)
    <8f6>   DW_AT_specification: <0x8be>
    <8fa>   DW_AT_linkage_name: (indirect string, offset: 0x4a0):
_ZN6BANANA1sE
    <8fe>   DW_AT_location    : 5 byte block: 3 64 bf 1 0
(DW_OP_addr: 1bf64)

I haven't tested it on other targets.

Is this expected behavior?

Regards,
Andre
Yao Qi Oct. 21, 2016, 2:57 p.m. UTC | #2 
Hi Jakub,

On Thu, Oct 20, 2016 at 5:21 PM, Andre Vieira (lists)
<Andre.SimoesDiasVieira@arm.com> wrote:
>  <2><8f5>: Abbrev Number: 38 (DW_TAG_member)
>     <8f6>   DW_AT_specification: <0x8be>
>     <8fa>   DW_AT_linkage_name: (indirect string, offset: 0x4a0):
> _ZN6BANANA1sE
>     <8fe>   DW_AT_location    : 5 byte block: 3 64 bf 1 0
> (DW_OP_addr: 1bf64)
>
> I haven't tested it on other targets.

I can reproduce it on x86_64 as well.

 <1><328>: Abbrev Number: 20 (DW_TAG_class_type)
    <329>   DW_AT_name        : A
    <32b>   DW_AT_byte_size   : 24
    <32c>   DW_AT_decl_file   : 1
    <32d>   DW_AT_decl_line   : 23
    <32e>   DW_AT_containing_type: <0x328>
    <332>   DW_AT_sibling     : <0x458>

 <2><336>: Abbrev Number: 19 (DW_TAG_member)
    <337>   DW_AT_name        : s
    <339>   DW_AT_decl_file   : 1
    <33a>   DW_AT_decl_line   : 40
    <33b>   DW_AT_type        : <0x5e>
    <33f>   DW_AT_external    : 1
    <33f>   DW_AT_accessibility: 1      (public)
    <340>   DW_AT_declaration : 1
 <2><36d>: Abbrev Number: 23 (DW_TAG_member)
    <36e>   DW_AT_specification: <0x336>
    <372>   DW_AT_linkage_name: (indirect string, offset: 0x447): _ZN1A1sE
    <376>   DW_AT_location    : 9 byte block: 3 10 15 60 0 0 0 0 0
 (DW_OP_addr: 601510)

We have two DIEs for member 's'.  GDB adds both of them as two fields,
the first one as static member (because of DW_AT_declaration), and the
second one as a non-static member.  GDB doesn't understand the
relationship between these two DIEs by DW_AT_specification.

Is attribute DW_AT_specification applicable to DW_TAG_member?
This is not documented in DWARF5 Appendix A "Attribute by Tage Value",
Page 258.
Jakub Jelinek Oct. 21, 2016, 3:14 p.m. UTC | #3 
On Fri, Oct 21, 2016 at 03:57:34PM +0100, Yao Qi wrote:
> Hi Jakub,
> 
> On Thu, Oct 20, 2016 at 5:21 PM, Andre Vieira (lists)
> <Andre.SimoesDiasVieira@arm.com> wrote:
> >  <2><8f5>: Abbrev Number: 38 (DW_TAG_member)
> >     <8f6>   DW_AT_specification: <0x8be>
> >     <8fa>   DW_AT_linkage_name: (indirect string, offset: 0x4a0):
> > _ZN6BANANA1sE
> >     <8fe>   DW_AT_location    : 5 byte block: 3 64 bf 1 0
> > (DW_OP_addr: 1bf64)
> >
> > I haven't tested it on other targets.
> 
> I can reproduce it on x86_64 as well.

First of all, I have a pending patch for this area:
http://gcc.gnu.org/ml/gcc-patches/2016-10/msg01183.html
so I think it doesn't really make much sense to discuss it until it gets in.
But unless you are talking about -std=c++17 or code with explicit inline
vars, I don't think anything has really changed in the debug representation
with that patch.

	Jakub
Andre Vieira (lists) Oct. 25, 2016, 3:05 p.m. UTC | #4 
On 21/10/16 16:14, Jakub Jelinek wrote:
> On Fri, Oct 21, 2016 at 03:57:34PM +0100, Yao Qi wrote:
>> Hi Jakub,
>>
>> On Thu, Oct 20, 2016 at 5:21 PM, Andre Vieira (lists)
>> <Andre.SimoesDiasVieira@arm.com> wrote:
>>>  <2><8f5>: Abbrev Number: 38 (DW_TAG_member)
>>>     <8f6>   DW_AT_specification: <0x8be>
>>>     <8fa>   DW_AT_linkage_name: (indirect string, offset: 0x4a0):
>>> _ZN6BANANA1sE
>>>     <8fe>   DW_AT_location    : 5 byte block: 3 64 bf 1 0
>>> (DW_OP_addr: 1bf64)
>>>
>>> I haven't tested it on other targets.
>>
>> I can reproduce it on x86_64 as well.
> 
> First of all, I have a pending patch for this area:
> http://gcc.gnu.org/ml/gcc-patches/2016-10/msg01183.html
> so I think it doesn't really make much sense to discuss it until it gets in.
> But unless you are talking about -std=c++17 or code with explicit inline
> vars, I don't think anything has really changed in the debug representation
> with that patch.
> 
> 	Jakub
> 

Hi Jakub,

I built gcc for the following:
1) revision r241135
2) revision r241135  + cherry-picked your patch in revision r241137
(skipped the patch in revision r241136 because that gives a build failure).
3) trunk + patch in http://gcc.gnu.org/ml/gcc-patches/2016-10/msg01183.html

And compiling the member-ptr.cc file in the gdb testsuite without
-std=c17 (see
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/testsuite/gdb.cp/member-ptr.cc;h=4b7da34d3a77e3b5c045bd76d1f0a01514a039d7;hb=HEAD)
leads to the following behavior:

1) expected behavior, debug of information of objects of 'class A' looks
fine.
2) new debug information for objects of 'class A' breaking the test.
3) same as 2)

As you can see the file has no explicit inline vars and I do not compile
it with -std=c++17.

So I'm suspecting your patch changes this behavior in an unexpected way.

Regards,
Andre
diff mbox

Patch

diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index 94af585..06b5aa3 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -935,6 +935,7 @@  c_cpp_builtins (cpp_reader *pfile)
 	  cpp_define (pfile, "__cpp_constexpr=201603");
 	  cpp_define (pfile, "__cpp_if_constexpr=201606");
 	  cpp_define (pfile, "__cpp_capture_star_this=201603");
+	  cpp_define (pfile, "__cpp_inline_variables=201606");
 	}
       if (flag_concepts)
 	/* Use a value smaller than the 201507 specified in
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 7670162..f761d0d 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -2917,9 +2917,11 @@  redeclaration_error_message (tree newdecl, tree olddecl)
 	  && !DECL_INITIAL (newdecl))
 	{
 	  DECL_EXTERNAL (newdecl) = 1;
-	  if (warning_at (DECL_SOURCE_LOCATION (newdecl), OPT_Wdeprecated,
-			  "redundant redeclaration of %<constexpr%> static "
-			  "data member %qD", newdecl))
+	  /* For now, only warn with explicit -Wdeprecated.  */
+	  if (global_options_set.x_warn_deprecated
+	      && warning_at (DECL_SOURCE_LOCATION (newdecl), OPT_Wdeprecated,
+			     "redundant redeclaration of %<constexpr%> static "
+			     "data member %qD", newdecl))
 	    inform (DECL_SOURCE_LOCATION (olddecl),
 		    "previous declaration of %qD", olddecl);
 	  return NULL;
@@ -5479,18 +5481,19 @@  maybe_commonize_var (tree decl)
 		 be merged.  */
 	      TREE_PUBLIC (decl) = 0;
 	      DECL_COMMON (decl) = 0;
+	      const char *msg;
 	      if (DECL_INLINE_VAR_P (decl))
-		warning_at (DECL_SOURCE_LOCATION (decl), 0,
-			    "sorry: semantics of inline variable "
-			    "%q#D are wrong (you%'ll wind up with "
-			    "multiple copies)", decl);
-	      else if (warning_at (DECL_SOURCE_LOCATION (decl), 0,
-				   "sorry: semantics of inline function "
-				   "static data %q#D are wrong (you%'ll wind "
-				   "up with multiple copies)", decl))
+		msg = G_("sorry: semantics of inline variable "
+			 "%q#D are wrong (you%'ll wind up with "
+			 "multiple copies)");
+	      else
+		msg = G_("sorry: semantics of inline function "
+			 "static data %q#D are wrong (you%'ll wind "
+			 "up with multiple copies)");
+	      if (warning_at (DECL_SOURCE_LOCATION (decl), 0,
+			      msg, decl))
 		inform (DECL_SOURCE_LOCATION (decl),
-			"you can work around this by removing the "
-			"initializer");
+			"you can work around this by removing the initializer");
 	    }
 	}
     }
@@ -9300,6 +9303,29 @@  check_var_type (tree identifier, tree type)
   return type;
 }
 
+/* Handle declaring DECL as an inline variable.  */
+
+static void
+mark_inline_variable (tree decl)
+{
+  bool inlinep = true;
+  if (! toplevel_bindings_p ())
+    {
+      error ("%<inline%> specifier invalid for variable "
+	     "%qD declared at block scope", decl);
+      inlinep = false;
+    }
+  else if (cxx_dialect < cxx1z)
+    pedwarn (DECL_SOURCE_LOCATION (decl), 0,
+	     "inline variables are only available "
+	     "with -std=c++1z or -std=gnu++1z");
+  if (inlinep)
+    {
+      retrofit_lang_decl (decl);
+      SET_DECL_VAR_DECLARED_INLINE_P (decl);
+    }
+}
+
 /* Given declspecs and a declarator (abstract or otherwise), determine
    the name and type of the object declared and construct a DECL node
    for it.
@@ -11427,20 +11453,7 @@  grokdeclarator (const cp_declarator *declarator,
 		  }
 
 		if (inlinep)
-		  {
-		    if (! toplevel_bindings_p ())
-		      {
-			error ("%<inline%> specifier invalid for variable "
-			       "%qD declared at block scope", decl);
-			inlinep = false;
-		      }
-		    else if (cxx_dialect < cxx1z)
-		      pedwarn (DECL_SOURCE_LOCATION (decl), 0,
-			       "inline variables are only available "
-			       "with -std=c++1z or -std=gnu++1z");
-		    if (inlinep)
-		      SET_DECL_VAR_DECLARED_INLINE_P (decl);
-		  }
+		  mark_inline_variable (decl);
 
 		if (!DECL_VAR_DECLARED_INLINE_P (decl)
 		    && !(cxx_dialect >= cxx1z && constexpr_p))
@@ -11655,23 +11668,7 @@  grokdeclarator (const cp_declarator *declarator,
 	  }
 
 	if (inlinep)
-	  {
-	    if (! toplevel_bindings_p ())
-	      {
-		error ("%<inline%> specifier invalid for variable %qs "
-		       "declared at block scope", name);
-		inlinep = false;
-	      }
-	    else if (cxx_dialect < cxx1z)
-	      pedwarn (DECL_SOURCE_LOCATION (decl), 0,
-		       "inline variables are only available "
-		       "with -std=c++1z or -std=gnu++1z");
-	    if (inlinep)
-	      {
-		retrofit_lang_decl (decl);
-		SET_DECL_VAR_DECLARED_INLINE_P (decl);
-	      }
-	  }
+	  mark_inline_variable (decl);
       }
 
     if (VAR_P (decl) && !initialized)
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 661853e..541faf7 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -22654,7 +22654,8 @@  gen_member_die (tree type, dw_die_ref context_die)
       child = lookup_decl_die (member);
       if (child)
 	{
-	  /* Handle inline static data members.  */
+	  /* Handle inline static data members, which only have in-class
+	     declarations.  */
 	  if (child->die_tag == DW_TAG_variable
 	      && child->die_parent == comp_unit_die ())
 	    {
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-ice10.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-ice10.C
index 5161273..381b8a4 100644
--- a/gcc/testsuite/g++.dg/cpp0x/constexpr-ice10.C
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-ice10.C
@@ -6,3 +6,5 @@  struct A
   constexpr A() {}
   static constexpr A a[2] = {};  // { dg-error "22:elements of array 'constexpr const A A::a \\\[2\\\]' have incomplete type" }
 };
+
+// { dg-prune-output "storage size" }
diff --git a/gcc/testsuite/g++.dg/cpp1z/feat-cxx1z.C b/gcc/testsuite/g++.dg/cpp1z/feat-cxx1z.C
index eeeae45..c86dbe2 100644
--- a/gcc/testsuite/g++.dg/cpp1z/feat-cxx1z.C
+++ b/gcc/testsuite/g++.dg/cpp1z/feat-cxx1z.C
@@ -356,6 +356,12 @@ 
 #  error "__cpp_aligned_new != 201606"
 #endif
 
+#ifndef __cpp_inline_variables
+#  error "__cpp_inline_variables"
+#elif __cpp_inline_variables != 201606
+#  error "__cpp_inline_variables != 201606"
+#endif
+
 #ifndef __cpp_capture_star_this
 #  error "__cpp_capture_star_this"
 #elif __cpp_capture_star_this != 201603
diff --git a/gcc/testsuite/g++.old-deja/g++.brendan/misc6.C b/gcc/testsuite/g++.old-deja/g++.brendan/misc6.C
index 93b241b..c7d35b7 100644
--- a/gcc/testsuite/g++.old-deja/g++.brendan/misc6.C
+++ b/gcc/testsuite/g++.old-deja/g++.brendan/misc6.C
@@ -1,7 +1,7 @@ 
 // { dg-do assemble  }
 // GROUPS passed miscellaneous
 // test that use of `inline' is forbidden when it should be
-inline int i;// { dg-error "" } .*
+inline int i;// { dg-error "" "" { target c++14_down } } .*
 struct c { inline int i; };// { dg-error "" } .*
 int foo (inline int i);// { dg-error "" } .*
 inline class c; // { dg-error "'inline' can only be specified for functions" } inline