[PATCH] Fix "b func(std::string)", use DMGL_VERBOSE (was: Re: [PATCH] Fix gdb.cp/no-dmgl-verbose.exp test)

[Pedro Alves <pedro@palves.net>]

On 2022-04-29 20:13, Carl Love wrote:
> GDB maintaners, Pedro, Lancelot:
> 
> I am not much of a C++ programer.  I will have to admit that there is a
> fair bit of the deep C++ behaviour with old/new APIs in Pedro's post
> that is beyond me.  :-)  Sorry.  Additionally, there is concern that
> the name of the test is not accurate, etc.
> 
> So at this point, not sure where to go with fixing this test.  I seem
> to have opened a real can of worms.  Suggestions on how to move
> forward, remove the test, go with a minimal change to get the test to
> pass, do a rewrite/name change, .... ?
> 

I knew I wouldn't be able to sleep if I didn't give this a try, so here it goes...

I think we should fix it properly such that "b f(std::string)" also works
with the modern C++11 ABI.  Given with the modern ABI, we don't even
end up with standard substitutions in the mangled name, avoiding DMGL_VERBOSE
seems pointless, as everyone who doesn't force old ABI in their compilations is getting
expanded std::string typedefs anyhow.  I think we should instead enforce usage of
DMGL_VERBOSE throughout GDB, and remove the special treatment for std::string and
others from cp-support.c.  This way, old ABI behaves as new ABI.  If we do this, then
gdb.cp/no-dmgl-verbose.exp loses it's main purpose (making sure DMGL_VERBOSE doesn't
creep back in), and it should be renamed/adjusted to test that setting a
breakpoint at f(std::string) works, _and_ that setting a breakpoint at
the expanded non-typedefed version of that function works too, similar to what
Lancelot had originally suggested.  (I think we should be able to use whatis instead
of info types, though.)

I'll have to leave writing a proper commit log for later.  Meanwhile, here's a patch
for comments.

No regressions on x86-64 GNU/Linux, and setting a breakpoint at "f(std::string)"
now works with either old or new ABI.

From d298d3c78f2d19bb70b312674b5ef9e5dbce2fde Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 29 Apr 2022 23:21:18 +0100
Subject: [PATCH] Fix "b func(std::string)", use DMGL_VERBOSE

Change-Id: Ib54fab4cf0fd307bfd55bf1dd5056830096a653b
---
 gdb/cp-name-parser.y                          |   5 +-
 gdb/cp-support.c                              |  52 ++++++---
 gdb/cp-support.h                              |  10 ++
 ...no-dmgl-verbose.cc => break-std-string.cc} |   0
 gdb/testsuite/gdb.cp/break-std-string.exp     | 108 ++++++++++++++++++
 gdb/testsuite/gdb.cp/no-dmgl-verbose.exp      |  35 ------
 6 files changed, 156 insertions(+), 54 deletions(-)
 rename gdb/testsuite/gdb.cp/{no-dmgl-verbose.cc => break-std-string.cc} (100%)
 create mode 100644 gdb/testsuite/gdb.cp/break-std-string.exp
 delete mode 100644 gdb/testsuite/gdb.cp/no-dmgl-verbose.exp

diff --git a/gdb/cp-name-parser.y b/gdb/cp-name-parser.y
index 6410363c87f..ceb4c968935 100644
--- a/gdb/cp-name-parser.y
+++ b/gdb/cp-name-parser.y
@@ -1959,8 +1959,7 @@ cp_comp_to_string (struct demangle_component *result, int estimated_len)
 {
   size_t err;
 
-  char *res = cplus_demangle_print (DMGL_PARAMS | DMGL_ANSI,
-				    result, estimated_len, &err);
+  char *res = gdb_cplus_demangle_print (result, estimated_len, &err);
   return gdb::unique_xmalloc_ptr<char> (res);
 }
 
@@ -2060,7 +2059,7 @@ cp_print (struct demangle_component *result)
   char *str;
   size_t err = 0;
 
-  str = cplus_demangle_print (DMGL_PARAMS | DMGL_ANSI, result, 64, &err);
+  str = gdb_cplus_demangle_print (result, 64, &err);
   if (str == NULL)
     return;
 
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index f52055893d2..b3eb775f717 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -70,18 +70,16 @@ static void add_symbol_overload_list_qualified
 
 struct cmd_list_element *maint_cplus_cmd_list = NULL;
 
-/* A list of typedefs which should not be substituted by replace_typedefs.  */
-static const char * const ignore_typedefs[] =
-  {
-    "std::istream", "std::iostream", "std::ostream", "std::string"
-  };
-
 static void
   replace_typedefs (struct demangle_parse_info *info,
 		    struct demangle_component *ret_comp,
 		    canonicalization_ftype *finder,
 		    void *data);
 
+static struct demangle_component *
+  gdb_cplus_demangle_v3_components (const char *mangled,
+				    int options, void **mem);
+
 /* A convenience function to copy STRING into OBSTACK, returning a pointer
    to the newly allocated string and saving the number of bytes saved in LEN.
 
@@ -146,13 +144,6 @@ 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';
 
-  /* Ignore any typedefs that should not be substituted.  */
-  for (const char *ignorable : ignore_typedefs)
-    {
-      if (strcmp (name, ignorable) == 0)
-	return 0;
-    }
-
   sym = NULL;
 
   try
@@ -238,6 +229,14 @@ inspect_type (struct demangle_parse_info *info,
 	  string_file buf;
 	  try
 	    {
+	      /* If the current language is C, and type is a
+		 struct/class, type_print prefixes the type name with
+		 "struct" or "class", which we don't want when we're
+		 expanding a C++ typedef.  Force language temporarily
+		 to avoid it.  */
+	      scoped_restore_current_language restore_language;
+	      set_language (language_cplus);
+
 	      type_print (type, "", &buf, -1);
 	    }
 	  /* If type_print threw an exception, there is little point
@@ -670,8 +669,8 @@ mangled_name_to_comp (const char *mangled_name, int options,
     {
       struct demangle_component *ret;
 
-      ret = cplus_demangle_v3_components (mangled_name,
-					  options, memory);
+      ret = gdb_cplus_demangle_v3_components (mangled_name,
+					      options, memory);
       if (ret)
 	{
 	  std::unique_ptr<demangle_parse_info> info (new demangle_parse_info);
@@ -1635,7 +1634,7 @@ gdb_demangle (const char *name, int options)
 #endif
 
   if (crash_signal == 0)
-    result.reset (bfd_demangle (NULL, name, options));
+    result.reset (bfd_demangle (NULL, name, options | DMGL_VERBOSE));
 
 #ifdef HAVE_WORKING_FORK
   if (catch_demangler_crashes)
@@ -1670,6 +1669,27 @@ gdb_demangle (const char *name, int options)
 
 /* See cp-support.h.  */
 
+char *
+gdb_cplus_demangle_print (struct demangle_component *tree,
+			  int estimated_length,
+			  size_t *p_allocated_size)
+{
+  return cplus_demangle_print (DMGL_PARAMS | DMGL_ANSI | DMGL_VERBOSE,
+			       tree, estimated_length, p_allocated_size);
+}
+
+/* A wrapper for cplus_demangle_v3_components that forces
+   DMGL_VERBOSE.  */
+
+static struct demangle_component *
+gdb_cplus_demangle_v3_components (const char *mangled,
+				  int options, void **mem)
+{
+  return cplus_demangle_v3_components (mangled, options | DMGL_VERBOSE, mem);
+}
+
+/* See cp-support.h.  */
+
 unsigned int
 cp_search_name_hash (const char *search_name)
 {
diff --git a/gdb/cp-support.h b/gdb/cp-support.h
index 4fbd53c8923..0a30e59d36d 100644
--- a/gdb/cp-support.h
+++ b/gdb/cp-support.h
@@ -186,10 +186,20 @@ extern void cp_merge_demangle_parse_infos (struct demangle_parse_info *,
 
 extern struct cmd_list_element *maint_cplus_cmd_list;
 
+/* Wrappers for bfd and libiberty demangling entry points.  Note they
+   all force DMGL_VERBOSE so that callers don't need to.  This is so
+   that GDB consistently uses DMGL_VERBOSE throughout.  */
+
 /* A wrapper for bfd_demangle.  */
 
 gdb::unique_xmalloc_ptr<char> gdb_demangle (const char *name, int options);
 
+/* A wrapper for cplus_demangle_print.  Uses DMGL_PARAMS and
+   DMGL_ANSI, in addition to DMGL_VERBOSE.  */
+extern char *gdb_cplus_demangle_print (struct demangle_component *tree,
+				       int estimated_length,
+				       size_t *p_allocated_size);
+
 /* Find an instance of the character C in the string S that is outside
    of all parenthesis pairs, single-quoted strings, and double-quoted
    strings.  Also, ignore the char within a template name, like a ','
diff --git a/gdb/testsuite/gdb.cp/no-dmgl-verbose.cc b/gdb/testsuite/gdb.cp/break-std-string.cc
similarity index 100%
rename from gdb/testsuite/gdb.cp/no-dmgl-verbose.cc
rename to gdb/testsuite/gdb.cp/break-std-string.cc
diff --git a/gdb/testsuite/gdb.cp/break-std-string.exp b/gdb/testsuite/gdb.cp/break-std-string.exp
new file mode 100644
index 00000000000..edfe4fb92ae
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/break-std-string.exp
@@ -0,0 +1,108 @@
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# Test setting a breakpoint at "f(std::string)".
+#
+# GDB should be able to expand the std::string typedef, and then set
+# the breakpoint using the full-qualified and expanded parameter name.
+# std::string, std::istream, std::iostream, std::ostream are special
+# for the demangler, though, they have corresponding "standard
+# substitutions" elements.  The libiberty demangler only expands the
+# standard substitutions to their full non-typedef underlying type if
+# the DMGL_VERBOSE option is requested.  GDB didn't use to use that
+# option, and would instead prevent expansion of the std::string
+# typedefs at breakpoint-set type, such that the function name used
+# for function lookup would match with the "std::string" present in
+# the function's non-DMGL_VERBOSE demangled name.
+#
+# For example (DMGL_VERBOSE):
+#
+#  $ echo "_Z1fSs" | c++filt
+#  f(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)
+#
+# vs (no DMGL_VERBOSE):
+#
+#  $ echo "_Z1fSs" | c++filt --no-verbose
+#  f(std::string)
+#
+# This design however broke setting a breakpoint at std:string when
+# the new libstdc++ C++11 ABI was introduced, as the "f(std::string)"
+# function's mangled name no longer uses a standard substitution for
+# std::string...
+#
+# I.e., with the libstdc++ C++11 ABI, we now have (and DMGL_VERBOSE
+# makes no difference):
+#
+#  $ echo _Z1fNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE | c++filt 
+#  f(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)
+#
+# So nowadays, GDB always uses DMGL_VERBOSE and no longer prevents
+# std::string (etc.) typedef expansion.  This test exercises both old
+# and new C++11 ABI for this reason.
+
+standard_testfile .cc
+
+if { [skip_cplus_tests] } { continue }
+
+# CXX11_ABI specifies the value to define _GLIBCXX_USE_CXX11_ABI as.
+
+proc test {cxx11_abi} {
+    global srcdir subdir srcfile binfile testfile
+
+    set options \
+	[list c++ debug additional_flags=-D_GLIBCXX_USE_CXX11_ABI=$cxx11_abi]
+    if { [gdb_compile \
+	      "${srcdir}/${subdir}/${srcfile}" "${binfile}-${cxx11_abi}.o" \
+	      object $options] != "" } {
+	untested "failed to compile"
+	return -1
+    }
+
+    clean_restart ${testfile}-${cxx11_abi}.o
+
+    gdb_test_no_output "set breakpoint pending off"
+
+    # So that "whatis" expands the C++ typedef.  Since we're debugging
+    # an .o file, GDB doesn't figure out were debugging C++ code and
+    # the current language when auto, is guessed as C.
+    gdb_test_no_output "set language c++"
+
+    # Get the type std::string is a typedef for.
+    set realtype ""
+    set type "std::string"
+    gdb_test_multiple "whatis /r $type" "" {
+	-re -wrap "type = (\[^\r\n\]+)" {
+	    set realtype $expect_out(1,string)
+	    gdb_assert \
+		{![string eq "$realtype" "$type"] && ![string eq "$realtype" ""]} \
+		$gdb_test_name
+	}
+    }
+
+    # Setting the breakpoint should still work even if GDB guesses the
+    # current language is C.  Make sure to exercise C and C++ even if
+    # GDB someday changes to autodetect the current language as C++.
+    foreach_with_prefix lang {"c" "c++" "auto"} {
+	gdb_breakpoint "f($type)" message
+
+	if { $realtype != "" } {
+	    gdb_breakpoint "f($realtype)" message
+	}
+    }
+}
+
+foreach_with_prefix cxx11_abi {0 1} {
+    test $cxx11_abi
+}
diff --git a/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp b/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
deleted file mode 100644
index 14f11ddcf04..00000000000
--- a/gdb/testsuite/gdb.cp/no-dmgl-verbose.exp
+++ /dev/null
@@ -1,35 +0,0 @@
-# Copyright 2011-2022 Free Software Foundation, Inc.
-
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
-
-# Test loading symbols from unrelocated C++ object files.
-
-standard_testfile .cc
-
-if { [skip_cplus_tests] } { continue }
-
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}.o" object {c++ debug}] != "" } {
-     untested "failed to compile"
-     return -1
-}
-
-clean_restart ${testfile}.o
-
-gdb_test_no_output "set breakpoint pending off"
-
-gdb_breakpoint {'f(std::string)'}
-
-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"

base-commit: 2e920d702b43c6d21ebd1e8a49c9e976a0d2cde6
-- 
2.36.0