Re: [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-30 22:11, Lancelot SIX wrote:

>> @@ -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?

Yes, fixed.

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

Also, here we can use the symbol's language instead of hardcoding C++.

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

...

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

Right, fixed.

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

Fixed.  And this should say "f(std::string)" instead of std::string.  Fix that
too.

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

Fixed.

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

Right.  I've now mentioned this in a comment, and changed the prefix used in the
test names, to be the full name of the define to make this a bit more obvious:

 ...
 PASS: gdb.cp/break-std-string.exp: _GLIBCXX_USE_CXX11_ABI=0: lang=c: gdb_breakpoint: set breakpoint at f(std::string)
 ...
 PASS: gdb.cp/break-std-string.exp: _GLIBCXX_USE_CXX11_ABI=1: lang=c: gdb_breakpoint: set breakpoint at f(std::string)
 ...

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

We don't.

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

I had just blindly copied this from gdb.cp/no-dmgl-verbose.exp.

I removed it, as indeed it's not necessary.  Actually, I stopped using gdb_breakpoint
too.  There's no real advantage to using it here, and gdb_breakpoint is a bit too
lax with expected regexps to my liking when we're actually testing a break command
rather than setting a breakpoint to run to it to set up for some other test.

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

Thanks, fixed.

I'll send a v2 with these fixes once I manage to write an actual commit log.