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

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Pedro,

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

Thanks for having a look at this!

I only had a quick look and have minor comments/questions below.

> 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

Not sure about comment conventions, but shouldn't type be upper case
here?

> +		 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.

s/full-qualified/fully-qualified/ maybe?

> +# 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
                                                          ^
One ':' is missing.

> +# 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 

Extra space at the end of the line.

> +#  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.

I guess that this option is libstdc++ only, right?  When using another
standard library (libc++ for example), does this option have an impact?

It is probably not really an issue, though.  If this has no effect, we
just end up running the test twice with whatever ABI is used.  We still
want the tested behavior to be the same, it is just that we have test
names which do not really reflect what is tested.

Or maybe we assume libstdc++ in gdb.cp, I do not know.

> +
> +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"

Is this forced to OFF just in case `gdb_breakpoint` change in the future
to have the "allow-pending" behavior by default?  In the current state
of gdb_breakpoint, this is not necessary.

> +
> +    # So that "whatis" expands the C++ typedef.  Since we're debugging
> +    # an .o file, GDB doesn't figure out were debugging C++ code and

s/were/we’re/

Best,
Lancelot.

> +    # 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
>