Re: [PATCH] Fix gdb.cp/no-dmgl-verbose.exp test

[Pedro Alves <pedro@palves.net>]

On 2022-04-29 18:20, Pedro Alves wrote:
> On 2022-04-29 18:09, Keith Seitz wrote:
>> On 4/29/22 09:57, Pedro Alves wrote:
>>> On 2022-04-29 16:48, Carl Love via Gdb-patches wrote:
>>>> On Fri, 2022-04-29 at 09:14 +0000, Lancelot SIX wrote:
>>>
>>> So the file is called gdb.cp/no-dmgl-verbose.exp.  "no-dmgl" most certainly means "no demangle".
>>>
>>> How is that related to "no demangle verbose" ?  A mystery.
>>
>> I believe I remember some history of this...
>>
>> When I did the physname work years ago, a maintainer objected that the
>> recorded physname for a function which takes a std::string was
>> "reduced" to "std::string<blah blah blah>" instead of recording (and
>> thus subsequently printing) "std::string" like other tools do. [He
>> speciifcally mentioned "nm".]
>>
>> The "no-dmgl-verbose" refers to the demangler option, DMGL_VERBOSE,
>> which I originally used when computing physnames. I believe this test's
>> intention was to make sure that DMGL_VERBOSE didn't creep back into the
>> code.
>>
>> [Background: At the time, the compiler did not output sufficient debuginfo
>> for a bunch of symbols, such as ctors. Thus the physname computation
>> was a way to "fill-in" this missing/necessary information.]
>>
>> There's a number of other workarounds for this "std::string"
>> vs "std::string<blah blah blah>" (and others) in cp-support.c.
>> See "ignore_typedefs". [Pardon if my explanation is imprecise.
>> This was a looong time ago.]
> 
> Thanks Keith!
> 
> That leads to this:
> 
>  https://sourceware.org/pipermail/gdb-patches/2011-July/thread.html
> 
> The "Test loading symbols from unrelocated C++ object files." intro is found in
> 
>  gdb.cp/cp-relocate.exp:# Test loading symbols from unrelocated C++ object files.
> 
> so I guess that Jan copied that testcase, and didn't update the intro.  The "unrelocated"
> aspect seems like not important for the test.
> 
> Note how Jan says:
> 
> "After Keith's fix of GDB PR 12266 GDB should resolve mostly any form
> (typedefed/untypedefed etc.) of the user entered function parameters."
> 
> I believe that should be true today, and GDB should be able to set a breakpoint
> on the typedef'ed f(std::string), no?  I find it very hard to believe that Jan
> didn't notice that the
> 
>  gdb_breakpoint {'f(std::string)'}
> 
> call was failing back then.  From the message, it very much seems like it was
> passing back then.  

Thanks for pointing at "ignore_typedefs" in cp-support.c, Keith.  So if I hack out
that special casing for std::string, like in the patch below, then GDB replaces the
typedef, and setting the breakpoint works.  However, that only works if we debug
a fully linked binary for some reason...  If we debug just the .o file, then the symbol
still isn't found...  That seems yet another bug somewhere.

To be clear, with this hack patch below, the testcase passes for me.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From 6154b784240703f2b7be1aa05c2b813aa2511435 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 29 Apr 2022 18:41:08 +0100
Subject: [PATCH] don't mask out std::string

Change-Id: Ief1da58246b859228fbe318d514cb9ce34629990
---
 gdb/cp-support.c                         | 3 ++-
 gdb/testsuite/gdb.cp/no-dmgl-verbose.cc  | 5 +++++
 gdb/testsuite/gdb.cp/no-dmgl-verbose.exp | 4 ++--
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index f52055893d2..78c302abaef 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -146,13 +146,14 @@ inspect_type (struct demangle_parse_info *info,
   memcpy (name, ret_comp->u.s_name.s, ret_comp->u.s_name.len);
   name[ret_comp->u.s_name.len] = '\0';
 
+#if 0
   /* Ignore any typedefs that should not be substituted.  */
   for (const char *ignorable : ignore_typedefs)
     {
       if (strcmp (name, ignorable) == 0)
 	return 0;
     }
-
+#endif
   sym = NULL;
 
   try
diff --git a/gdb/testsuite/gdb.cp/no-dmgl-verbose.cc b/gdb/testsuite/gdb.cp/no-dmgl-verbose.cc
index 454ab4c42ea..d9a666f2c48 100644
--- a/gdb/testsuite/gdb.cp/no-dmgl-verbose.cc
+++ b/gdb/testsuite/gdb.cp/no-dmgl-verbose.cc
@@ -21,3 +21,8 @@ void
 f (std::string s)
 {
 }
+
+int
+main ()
+{
+}
diff --git a/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp b/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
index 14f11ddcf04..c54daf33c8b 100644
--- a/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
+++ b/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
@@ -19,12 +19,12 @@ standard_testfile .cc
 
 if { [skip_cplus_tests] } { continue }
 
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}.o" object {c++ debug}] != "" } {
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug}] != "" } {
      untested "failed to compile"
      return -1
 }
 
-clean_restart ${testfile}.o
+clean_restart ${testfile}
 
 gdb_test_no_output "set breakpoint pending off"
 

base-commit: 225170409b47bc96b62b2ecfcc0d9d5ae1ef8949
prerequisite-patch-id: be6afcdb35fd6ed141be7d568afbcd1ae4f082ee
prerequisite-patch-id: be2d0109aea479ee1973cf5b6d6454b5811cfce4
-- 
2.36.0
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

So that seems like something that should be fixed in GDB, somehow.



Now, I'm wondering what is it that DMGL_VERBOSE outputs differently than not
having it.  The full name of std::string in libstd++ is nowadays:

  "std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >",

and the only difference compared to what we have in the test is the __cxx11 namespace,
which has meanwhile been added for the new C++11 ABI.

Here's today, vs testcase's, with extra space added for alignment:

 today:  "std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >"
 test:   "std::         basic_string<char, std::char_traits<char>, std::allocator<char> >"

Hmm.  If I force the testcase to use the old ABI to emulate what was being seeing
back then, with:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git c/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp w/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
index c54daf33c8b..4718191b471 100644
--- c/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
+++ w/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
@@ -19,7 +19,7 @@ standard_testfile .cc
 
 if { [skip_cplus_tests] } { continue }
 
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug}] != "" } {
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug additional_flags=-D_GLIBCXX_USE_CXX11_ABI=0}] != "" } {
      untested "failed to compile"
      return -1
 }
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'd expect that the breakpoint at the non-typedef'd name, like in:

 gdb_test {break 'f(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'} \
 	 {Function ".*" not defined\.} \
 	 "DMGL_VERBOSE-demangled f(std::string) is not defined"

... would actually manage to be set, and thus that test would fail.  Obviously it didn't
back then.  But why?

Turns out that with the old ABI we get this mangled name for the function:

 $ nm -A testsuite/outputs/gdb.cp/no-dmgl-verbose/no-dmgl-verbose |grep _Z 
 testsuite/outputs/gdb.cp/no-dmgl-verbose/no-dmgl-verbose:0000000000001129 T _Z1fSs

and running through c++filt does give us:

 $ echo _Z1fSs | c++filt
 f(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)

which is the demangled name Jan used in the test.

Interestingly, _Z1fSs seems very short.  That "S" seems like a shortcut for std::string.  Here's what
the mangled name looks like WITHOUT -D_GLIBCXX_USE_CXX11_ABI=0, thus with modern ABI:

 testsuite/outputs/gdb.cp/no-dmgl-verbose/no-dmgl-verbose:0000000000001129 T _Z1fNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE


If I run the gdb.cp/no-dmgl-verbose.exp test with -D_GLIBCXX_USE_CXX11_ABI=0, plus the hack in cp-support.c
to disable the std::string typedef thing, the breakpoint at f(std::string) surprisingly fails again!

 (gdb) PASS: gdb.cp/no-dmgl-verbose.exp: set breakpoint pending off
 break 'f(std::string)'
 Function "f(std::string)" not defined.
 (gdb) FAIL: gdb.cp/no-dmgl-verbose.exp: gdb_breakpoint: set breakpoint at 'f(std::string)'
 break 'f(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
 Function "f(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)" not defined.
 (gdb) PASS: gdb.cp/no-dmgl-verbose.exp: DMGL_VERBOSE-demangled f(std::string) is not defined

But note, if I debug it manually, I see this:

  (gdb) b f( [TAB]
  (gdb) b f( std::string) 

Ah!  That's different with the modern ABI, where TAB completion gives you the
whole full basic_string non-typedefed name.   Ah!  That's because GDB demangles the old
ABI's _Z1fSs as f(std::string):

 (gdb) demangle _Z1fSs
 f(std::string)

 (gdb) demangle _Z1fNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
 f(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)

And note this:

 $ echo "_Z1fSs" | c++filt
 f(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)

 $ echo "_Z1fSs" | c++filt --no-verbose
 f(std::string)

Bingo.  c++filt uses DMGL_VERBOSE by default, unless you pass it --no-verbose.

I don't know why the test would fail if I force the old ABI, and I hack out the 
std::string typedef replacement stuff in cp-support.c.  That's weird.


And now the fun thing -- reverting the cp-support.c hack, and just adding the -D_GLIBCXX_USE_CXX11_ABI=0
makes the testcase behave like it used to when it was written, and it passes cleanly
for me, as below.

GDB should be fixed so that setting a breakpoint at "f(std::string)" should work
with new ABI too, but that can be a separate thing.

From 4a9a9b0bcff56254b7897a3820bc06d9987aeac3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 29 Apr 2022 19:28:13 +0100
Subject: [PATCH] Fix gdb.cp/no-dmgl-verbose.exp

Change-Id: I196568fd92c2a6747a0467f12510714f549be2b7
---
 gdb/testsuite/gdb.cp/no-dmgl-verbose.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp b/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
index 14f11ddcf04..fd4e56f40b4 100644
--- a/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
+++ b/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
@@ -19,7 +19,7 @@ standard_testfile .cc
 
 if { [skip_cplus_tests] } { continue }
 
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}.o" object {c++ debug}] != "" } {
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}.o" object {c++ debug additional_flags=-D_GLIBCXX_USE_CXX11_ABI=0}] != "" } {
      untested "failed to compile"
      return -1
 }

-- 
2.36.0