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

[Yao Qi]

On Tue, Oct 25, 2016 at 7:06 PM, Jakub Jelinek  wrote:
>
> I think this patch should fix it, will bootstrap/regtest it now:
>

Yes, the fails in gdb.cp/member-ptr.exp go away with the patched g++.
I run gdb.cp/member-ptr.exp with three different c++ variations,

Schedule of variations:
unix/-std=c++03
unix/-std=c++11
unix/-std=c++1z

Thanks for the fix.

-- 
Yao (齐尧)

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

[Jakub Jelinek]

On Tue, Oct 25, 2016 at 04:05:30PM +0100, Andre Vieira (lists) wrote:
> 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.

I think this patch should fix it, will bootstrap/regtest it now:

2016-10-25  Jakub Jelinek  

* dwarf2out.c (gen_member_die): Only reparent_child instead of
splice_child_die if child doesn't have DW_AT_specification attribute.

--- gcc/dwarf2out.c.jj  2016-10-25 19:49:28.0 +0200
+++ gcc/dwarf2out.c 2016-10-25 20:02:33.264639847 +0200
@@ -22624,7 +22624,8 @@ gen_member_die (tree type, dw_die_ref co
  /* Handle inline static data members, which only have in-class
 declarations.  */
  if (child->die_tag == DW_TAG_variable
- && child->die_parent == comp_unit_die ())
+ && child->die_parent == comp_unit_die ()
+ && get_AT (child, DW_AT_specification) == NULL)
{
  reparent_child (child, context_die);
  child->die_tag = DW_TAG_member;


Jakub

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

[Andre Vieira (lists)]

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)
>>  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

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

[Jakub Jelinek]

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)
>  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

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

[Yao Qi]

Hi Jakub,

On Thu, Oct 20, 2016 at 5:21 PM, Andre Vieira (lists)
 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.

-- 
Yao (齐尧)

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

[Eric Botcazou]

> Does this fix it?  It still works on Linux:
> 
> 2016-10-20  Jakub Jelinek  
> 
>   * g++.dg/cpp1z/inline-var1.C (w): Initialize to 64 + 2.

Yes, it does, thanks!

-- 
Eric Botcazou

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

[Andre Vieira (lists)]

On 13/10/16 20:34, Jason Merrill wrote:
> On Tue, Oct 11, 2016 at 9:39 AM, Jakub Jelinek  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 , 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 , 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 , 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 , 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

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

[Jakub Jelinek]

On Thu, Oct 20, 2016 at 01:22:16PM +0200, Eric Botcazou wrote:
> If fails because x == 16 and w == 0 on the first invocation to bar but, given 
> that w is not modified anywhere else, this seems to be expected.

Does this fix it?  It still works on Linux:

2016-10-20  Jakub Jelinek  

* g++.dg/cpp1z/inline-var1.C (w): Initialize to 64 + 2.

--- gcc/testsuite/g++.dg/cpp1z/inline-var1.C.jj 2016-10-14 12:31:44.0 
+0200
+++ gcc/testsuite/g++.dg/cpp1z/inline-var1.C2016-10-20 17:59:26.671358964 
+0200
@@ -13,7 +13,7 @@ inline int var22 = foo (7);
 extern inline int var23, var22;
 inline int var23 = foo (8);
 
-static int v, w;
+static int v, w = 64 + 2;
 
 int
 foo (int x)


Jakub

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

[Eric Botcazou]

> The test wants to ensure:
> 1) the foo calls in the ctors are invoked in increasing order (this is used
>for inline vars defined in both CUs)
> 2) ensure that the first bar call in the ctors is invoked after foo (5)
> 3) that bar used in ctors (it is used for inline vars defined in one or the
>other CU, but not both) is invoked either in the order
>bar (0); bar (1); /* optionally some foo calls here */ bar (16); bar
> (17); or in the order
>bar (16); bar (17); /* optionally some foo calls here */ bar (0); bar
> (1);
> 
> So, in what order is it run on Solaris and why does it fail?

The latter:

(gdb) run
Starting program: /sydney.a/users/botcazou/gcc-head/inline-var1 
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
[Switching to Thread 1 (LWP 1)]

Breakpoint 1, bar (x=16) at inline-var1.C:29
29if (v < 6)
(gdb) continue
Continuing.

Program received signal SIGABRT, Aborted.
0xfefcaa58 in _lwp_kill () from /lib/libc.so.1

If fails because x == 16 and w == 0 on the first invocation to bar but, given 
that w is not modified anywhere else, this seems to be expected.

-- 
Eric Botcazou

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

[Jakub Jelinek]

On Thu, Oct 20, 2016 at 12:50:56PM +0200, Eric Botcazou wrote:
> > testsuite/
> > * g++.dg/cpp1z/inline-var1.C: New test.
> > * g++.dg/cpp1z/inline-var1a.C: New test.
> > * g++.dg/cpp1z/inline-var1.h: New file.
> 
> This one fails on SPARC/Solaris:
> 
> 0xfefcaa58 in _lwp_kill () from /lib/libc.so.1
> (gdb) bt
> #0  0xfefcaa58 in _lwp_kill () from /lib/libc.so.1
> #1  0xfef65a64 in raise () from /lib/libc.so.1
> #2  0xfef41954 in abort () from /lib/libc.so.1
> #3  0x00011770 in bar (x=16) at inline-var1.C:34
> #4  0x00012984 in __static_initialization_and_destruction_0 
> (__initialize_p=1, 
> __priority=65535) at inline-var1a.C:6
> #5  0x00012b18 in _GLOBAL__sub_I_alt1 () at inline-var1a.C:44
> #6  0x00012b54 in __do_global_ctors_aux ()
> #7  0x00012b8c in _init ()
> #8  0x00011458 in _start ()
> 
> (gdb) frame 3
> #3  0x00011770 in bar (x=16) at inline-var1.C:34
> 34  __builtin_abort ();
> 
> (gdb) frame 4
> #4  0x00012984 in __static_initialization_and_destruction_0 
> (__initialize_p=1, 
> __priority=65535) at inline-var1a.C:6
> 6   static inline int var19 = bar (16);
> 
> (gdb) p w
> $2 = 0
> 
> The testcase is rather cryptic, in particular the logic in 'bar', so it's 
> hard 
> to figure out what doesn't work as expected, but does it require support for 
> constructor priorities for example?  Or does it assume an order of invocation 
> for the constructors of inline-var1.C vs those of inline-var1a.C?

The test wants to ensure:
1) the foo calls in the ctors are invoked in increasing order (this is used
   for inline vars defined in both CUs)
2) ensure that the first bar call in the ctors is invoked after foo (5)
3) that bar used in ctors (it is used for inline vars defined in one or the
   other CU, but not both) is invoked either in the order
   bar (0); bar (1); /* optionally some foo calls here */ bar (16); bar (17);
   or in the order
   bar (16); bar (17); /* optionally some foo calls here */ bar (0); bar (1);

So, in what order is it run on Solaris and why does it fail?

Jakub

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

[Eric Botcazou]

> testsuite/
>   * g++.dg/cpp1z/inline-var1.C: New test.
>   * g++.dg/cpp1z/inline-var1a.C: New test.
>   * g++.dg/cpp1z/inline-var1.h: New file.

This one fails on SPARC/Solaris:

0xfefcaa58 in _lwp_kill () from /lib/libc.so.1
(gdb) bt
#0  0xfefcaa58 in _lwp_kill () from /lib/libc.so.1
#1  0xfef65a64 in raise () from /lib/libc.so.1
#2  0xfef41954 in abort () from /lib/libc.so.1
#3  0x00011770 in bar (x=16) at inline-var1.C:34
#4  0x00012984 in __static_initialization_and_destruction_0 (__initialize_p=1, 
__priority=65535) at inline-var1a.C:6
#5  0x00012b18 in _GLOBAL__sub_I_alt1 () at inline-var1a.C:44
#6  0x00012b54 in __do_global_ctors_aux ()
#7  0x00012b8c in _init ()
#8  0x00011458 in _start ()

(gdb) frame 3
#3  0x00011770 in bar (x=16) at inline-var1.C:34
34  __builtin_abort ();

(gdb) frame 4
#4  0x00012984 in __static_initialization_and_destruction_0 (__initialize_p=1, 
__priority=65535) at inline-var1a.C:6
6   static inline int var19 = bar (16);

(gdb) p w
$2 = 0

The testcase is rather cryptic, in particular the logic in 'bar', so it's hard 
to figure out what doesn't work as expected, but does it require support for 
constructor priorities for example?  Or does it assume an order of invocation 
for the constructors of inline-var1.C vs those of inline-var1a.C?

-- 
Eric Botcazou

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

[Jason Merrill]

On Tue, Oct 11, 2016 at 9:39 AM, Jakub Jelinek  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 ();
> __cxa_thread_atexit (S::~S, , &__dso_handle);
>   }
>   if (*(char *)&_ZGV1t == 0) {
> *(char *)&_ZGV1t = 1;
> __cxa_thread_atexit (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.
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 % 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 % 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;