GDB crash due to infinite recursion in typedef substitution (Re: [PATCHv2 3/3] gdb: Remove C++ symbol aliases from completion list)

Message ID 70063d9a-3bc0-c56f-49dd-0060a6d214b0@redhat.com
State New
Headers show
Series
  • GDB crash due to infinite recursion in typedef substitution (Re: [PATCHv2 3/3] gdb: Remove C++ symbol aliases from completion list)
Related show

Commit Message

Adding Keith, who I believe is the original author of this
whole replace-typedefs machinery.  [ Help? :-) ]

On 5/24/20 12:35 PM, Pedro Alves via Gdb-patches wrote:
> Hi,

> 

>> gdb/ChangeLog:

>>

>> 	* completer.c (completion_tracker::remove_completion): Define new

>> 	function.

>> 	* completer.h (completion_tracker::remove_completion): Declare new

>> 	function.

>> 	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols

>> 	when adding a C++ function symbol.

>>

>> gdb/testsuite/ChangeLog:

>>

>> 	* gdb.linespec/cp-completion-aliases.cc: New file.

>> 	* gdb.linespec/cp-completion-aliases.exp: New file.

> 

> This causes GDB to crash for me, when debugging GDB itself, and then

> doing "b main<TAB>".

> 

> $ ./gdb -data-directory=data-directory ./gdb -ex "complete b main"

> GNU gdb (GDB) 10.0.50.20200319-git

> ...

> Reading symbols from ./gdb...

> Setting up the environment for debugging gdb.

> During symbol reading: unsupported tag: 'DW_TAG_unspecified_type'

> During symbol reading: debug info gives source 55 included from file at zero line 0

> During symbol reading: debug info gives command-line macro definition with non-zero line 19: _STDC_PREDEF_H 1

> Breakpoint 1 at 0xaf48c3: file /home/pedro/gdb/mygit/src/gdbsupport/errors.cc, line 54.

> During symbol reading: Member function "~_Sp_counted_base" (offset 0x683070) is virtual but the vtable offset is not specified

> During symbol reading: const value length mismatch for 'std::ratio<1, 1000000000>::num', got 8, expected 0

> During symbol reading: cannot get low and high bounds for subprogram DIE at 0x694d73

> During symbol reading: Child DIE 0x6afc31 and its abstract origin 0x6afbcd have different parents

> During symbol reading: Multiple children of DIE 0x6b2668 refer to DIE 0x6b2657 as their abstract origin

> Breakpoint 2 at 0x4f6775: file /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c, line 201.

> Aborted (core dumped)

> 

> GDB seemingly crashes due to infinite recursion.  

> 

> Here's the top of the stack, showing the recursion starting:

> 

> $ gdb ./gdb -c core.4577

> ...

> Program terminated with signal SIGABRT, Aborted.

> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51

> 51      }

> [Current thread is 1 (Thread 0x7f9d6f25ecc0 (LWP 4577))]

> (gdb) bt -44

> #51229 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267

> #51230 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475

> #51231 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> #51232 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389

> #51233 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479

> #51234 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267

> #51235 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475

> #51236 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e80, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> #51237 0x0000000000555696 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:357

> #51238 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479

> #51239 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c8400, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> #51240 0x0000000000555bd0 in cp_canonicalize_string_full[abi:cxx11](char const*, char const* (*)(type*, void*), void*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:527

> #51241 0x0000000000555d28 in cp_canonicalize_string_no_typedefs[abi:cxx11](char const*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"...) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:550

> #51242 0x00000000008c4959 in completion_list_add_symbol (tracker=..., sym=0x980fde0, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5316

> #51243 0x00000000008c541b in add_symtab_completions (cust=0x673ba40, tracker=..., mode=complete_symbol_mode::LINESPEC, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5576

> #51244 0x00000000008c5496 in <lambda(compunit_symtab*)>::operator()(compunit_symtab *) const (__closure=0x7fff6159e230, symtab=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5697

> #51245 0x00000000008c8f14 in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::operator()(gdb::fv_detail::erased_callable, compunit_symtab *) const (__closure=0x0, ecall=..., args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:263

> #51246 0x00000000008c8f3c in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::_FUN(gdb::fv_detail::erased_callable, compunit_symtab *) () at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:257

> #51247 0x0000000000603da8 in gdb::function_view<void (compunit_symtab*)>::operator()(compunit_symtab*) const (this=0x7fff6159df00, args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:247

> #51248 0x00000000007c8483 in psym_expand_symtabs_matching(objfile *, gdb::function_view<bool(char const*, bool)>, const lookup_name_info &, gdb::function_view<bool(char const*)>, gdb::function_view<void(compunit_symtab*)>, search_domain) (objfile=0x232c6a0, file_matcher=..., lookup_name_in=..., symbol_matcher=..., expansion_notify=..., domain=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/psymtab.c:1354

> #51249 0x00000000008ab6a7 in expand_symtabs_matching(gdb::function_view<bool (char const*, bool)>, lookup_name_info const&, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (file_matcher=..., lookup_name=..., symbol_matcher=..., expansion_notify=..., kind=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/symfile.c:3788

> #51250 0x00000000008c5ab6 in default_collect_symbol_completion_matches_break_on (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", break_on=0xc3468e "", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5692

> #51251 0x00000000008c5f79 in default_collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5795

> #51252 0x00000000008c5fc2 in collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5810

> #51253 0x00000000006fcdf2 in linespec_complete_function (tracker=..., function=0x7fff6159e7f2 "main", func_match_type=symbol_name_match_type::WILD, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2816

> #51254 0x00000000006fcecf in complete_linespec_component (parser=0x7fff6159e500, tracker=..., text=0x7fff6159e7f2 "main", component=linespec_complete_what::FUNCTION, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2848

> #51255 0x00000000006fd63b in linespec_complete (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/linespec.c:3063

> #51256 0x00000000005405b1 in complete_address_and_linespec_locations (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/completer.c:690

> #51257 0x0000000000540eee in location_completer (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/completer.c:1034

> #51258 0x0000000000541024 in location_completer_handle_brkchars (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main", word_ignored=0x0) at /home/pedro/gdb/mygit/src/gdb/completer.c:1071

> #51259 0x00000000005416e4 in complete_line_internal_normal_command (tracker=..., command=0x7fff6159e7f0 "b main", word=0x0, cmd_args=0x7fff6159e7f2 "main", reason=handle_brkchars, c=0x1cb8320) at /home/pedro/gdb/mygit/src/gdb/completer.c:1306

> #51260 0x0000000000541b72 in complete_line_internal_1 (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1531

> #51261 0x0000000000541bb5 in complete_line_internal (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1550

> #51262 0x0000000000542cee in completion_find_completion_word (tracker=..., text=0x7fff615a0905 "b main", quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:2054

> #51263 0x000000000054247a in complete (line=0x7fff615a0905 "b main", word=0x7fff6159eb08, quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:1770

> #51264 0x00000000004f6ed0 in complete_command (arg=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c:364

> #51265 0x00000000004fee70 in do_const_cfunc (c=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:107

> #51266 0x0000000000501e28 in cmd_func (cmd=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:1952

> #51267 0x000000000090d07d in execute_command (p=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/top.c:655

> #51268 0x000000000073b9c2 in catch_command_errors (command=0x90cc11 <execute_command(char const*, int)>, arg=0x7fff615a08fc "complete b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/main.c:401

> #51269 0x000000000073cdaa in captured_main_1 (context=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1163

> #51270 0x000000000073cf9f in captured_main (data=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1188

> #51271 0x000000000073d00a in gdb_main (args=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1213

> #51272 0x000000000041563e in main (argc=6, argv=0x7fff6159f178) at /home/pedro/gdb/mygit/src/gdb/gdb.c:32

> 

> This cycle keeps repeating:

> #1709 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267

> #1710 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475

> #1711 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0xd83be70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> #1712 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389

> #1713 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479

> 

> 

> The symbol on which we call cp_canonicalize_string_no_typedefs is:

> 

> (top-gdb) p *sym

> $4 = {<general_symbol_info> = {m_name = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_

> match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., value = {ivalue = 159449440, block = 0x9810160, bytes = 0x9810160 "\212\271\214", addre

> ss = 0x9810160, common_block = 0x9810160, chain = 0x9810160}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, secti

> on = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x980fc20, owner = {symtab = 0x673bdf0, arch = 0x673bdf0}, domain = VAR_DOMAIN, aclass_index = 19, is_objfile_o

> wned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 157, aux_value = 0x980fe30, hash_next = 0x9821bd0}

> 

> (top-gdb) p sym->m_name 

> $6 = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, in t, cmd_list_element*, char const*), char const*, char const*, char const*)"

> 


I figured out what is special about this symbol.  The issue is that
we have a boolean_option_def typedef in the global namespace in the
program.  Here, in stack.c:

 using boolean_option_def
   = gdb::option::boolean_option_def<frame_print_options>;

actually we have two.  There's another one in valprint.c:

 using boolean_option_def
   = gdb::option::boolean_option_def<value_print_options>;

The recursion happens because we start by breaking the

   gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(....)

symbol name into demanged parse info components.
This happens in cp_canonicalize_string_no_typedefs -> replace_typedefs.

The tree for that symbol looks like this:

(top-gdb) p d_dump (ret_comp, 0)
typed name
  qualified name
    name 'gdb'
    qualified name
      name 'option'
      qualified name
        template
          name 'boolean_option_def'
          template argument list
            name 'value_print_options'
        name 'boolean_option_def'
  function type
    argument list
      pointer
        const
          builtin type char
      argument list
        pointer
          function type
            pointer
              builtin type bool
            argument list
              pointer
                name 'value_print_options'
        argument list
          pointer
            function type
              builtin type void
              argument list
                pointer
                  name 'ui_file'
                argument list
                  builtin type int
                  argument list
                    pointer
                      name 'cmd_list_element'
                    argument list
                      pointer
                        const
                          builtin type char
          argument list
            pointer
              const
                builtin type char
            argument list
              pointer
                const
                  builtin type char
              argument list
                pointer
                  const
                    builtin type char
$5 = void

Let's just focus on the top part, because that's where
the recursion happens:

 typed name
   qualified name
     name 'gdb'
     qualified name
       name 'option'
       qualified name
         template
           name 'boolean_option_def'
           template argument list
             name 'value_print_options'
         name 'boolean_option_def'

As we walk the tree of components, we get to replace_typedefs_qualified_name,
and enter the loop:

  while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)
    {
      if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)
	{

The DEMANGLE_COMPONENT_NAME case is hit twice, once for
"gdb", and another for "option".  After those two,
the next d_left (comp)->type is DEMANGLE_COMPONENT_TEMPLATE.

This now gets us to the else branch within that
DEMANGLE_COMPONENT_QUAL_NAME loop:

      else
	{
	  /* The current node is not a name, so simply replace any
	     typedefs in it.  Then print it to the stream to continue
	     checking for more typedefs in the tree.  */
	  replace_typedefs (info, d_left (comp), finder, data);

which, after some more replace_typedefs recursion, gets us to

	case DEMANGLE_COMPONENT_NAME:
	  inspect_type (info, ret_comp, finder, data);
	  break;

This is for this part of the tree:

         template
           name 'boolean_option_def'

and it's this inspect_type call that's problematic.

[come back here later]

inspect_type does a symbol lookup for "boolean_option_def",
which finds the unrelated typedef at the global namespace.
In this session I'm debugging, it finds the one in valprint.c:

(top-gdb) p *sym
$10 = {<general_symbol_info> = {m_name = 0x3a5dc90 "boolean_option_def", value = {ivalue = 0, block = 0x0, bytes = 0x0, address = 0x0, common_block = 0x0, chain = 0x0}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, section = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x3a613f0, owner = {symtab = 0x34ab930, arch = 0x34ab930}, domain = VAR_DOMAIN, aclass_index = 8, is_objfile_owned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 2987, aux_value = 0x0, hash_next = 0x3a694e0}
(top-gdb) p *sym.owner.symtab 
$11 = {next = 0x34ab750, compunit_symtab = 0x34ab6d0, linetable = 0x3b83e60, filename = 0x2df4b20 "/home/pedro/gdb/mygit/src/gdb/valprint.c", language = language_cplus, fullname = 0x0}

This symbol's type is of course a typedef, so we try to resolve it.
check_typedef returns this type:

 (top-gdb) p *type->main_type
 $14 = 
 {name = 0x2de5a40 "gdb::option::boolean_option_def<value_print_options>",
  code = TYPE_CODE_STRUCT,
 ...

So, since a typedef was resolved, we get here:

	  /* Turn the result into a new tree.  Note that this
	     tree will contain pointers into NAME, so NAME cannot
	     be free'd until all typedef conversion is done and
	     the final result is converted into a string.  */
	  i = cp_demangled_name_to_comp (name, NULL);
	  if (i != NULL)
	    {
	      /* Merge the two trees.  */
	      cp_merge_demangle_parse_infos (info, ret_comp, i.get ());

	      /* Replace any newly introduced typedefs -- but not
		 if the type is anonymous (that would lead to infinite
		 looping).  */
	      if (!is_anon)
		{
		  fprintf_unfiltered (gdb_stdlog, "replace_typedefs: %s\n",
				      name);

		  replace_typedefs (info, ret_comp, finder, data);

And it's that last replace_typedefs call that triggers the recursion,
since we're now going to process a tree

 gdb::option::boolean_option_def<value_print_options>
              ^^^^^^^^^^^^^^^^^^

that will again end up doing a lookup for that global scope
boolean_option_def typedef, rinse repeat.

Locally, I put a recursion limit in inspect_type, and the resulting 
symbol name that cp_canonicalize_string_no_typedefs returns is:

$18 = std::unique_ptr<char> containing 0x32f81a0 "gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::
option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::boolean_option_def<va
lue_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_
print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_prin
t_options><value_print_options><value_print_options><value_print_options><value_print_options>::boolean_option_def(char const*, bool* (*)(value_print_options*), void (*)(ui
_file*, int, cmd_list_element*, char const*), char const*, char const*, char const*)"

Obviously broken due to recursion around boolean_option_def.


Now, let's get back to that inspect_type call that I mentioned
was problematic.  See [come back here later] above.

To recap, that happens when we're processing a DEMANGLE_COMPONENT_QUAL_NAME
subtree.  We've processed "gdb", and then "option", and now we're going to
process the "boolean_option_def" component:

 typed name
   qualified name
     name 'gdb'
     qualified name
       name 'option'
       qualified name
         template
           name 'boolean_option_def'      <<< 
           template argument list
             name 'value_print_options'
         name 'boolean_option_def'

To me, it looks quite obvious that the problem is
that we're looking up that boolean_option_def symbol in the
global name namespace.  I mean, this is a qualified symbol
name, like:

   gdb::option::boolean_option_def

so we should be looking for a "boolean_option_def" typedef in
the gdb::option namespace or struct/class.  Right?

The question to me is then, how to do that.
In replace_typedefs_qualified_name, after we've processed
"gdb" and "option", we have "gdb::option::" stored in
the "buf" string_file, which seems perfect.  However, how do we
get that scope down to inspect_type, since we have layers of
replace_typedefs calls before we get there?

I can think of two ways:

#1 - store the current scope in the demangle_parse_info
context object, and have inspect_type consult that.

#2 - Make replace_typedefs_qualified_name
look ahead in the tree, inlining the
DEMANGLE_COMPONENT_TEMPLATE case here, basically
inlining enough forward peeking in order to do a directly
inspect_type call from within replace_typedefs_qualified_name.
Of course, we would likely have to do the same for other
node types, not just DEMANGLE_COMPONENT_TEMPLATE.

And then, I'm not sure we should pass the scope as extra
parameter to inspect_type, or, whether we should replace
the "boolean_option_def" node in the tree with one that has
a full-scoped name "gdb::option::boolean_option_def" name,
similarly to how replace_typedefs_qualified_name
handles the DEMANGLE_COMPONENT_NAME case.

One difficulty I have with this, is that I couldn't find
any case of this code in replace_typedefs_qualified_name:

	  /* The current node is not a name, so simply replace any
	     typedefs in it.  Then print it to the stream to continue
	     checking for more typedefs in the tree.  */
	  replace_typedefs (info, d_left (comp), finder, data);

actually ever replacing any typedef in the testsuite.
I mean, I put an abort() in place if the returned component
as a string is different from the name before the 
replace_typedefs call, and then ran the gdb.cp/ testcases,
and the abort never triggered...

So, in order to get the discussion going, I wrote a patch
for solution #1.  This fixes the issue for me, and causes no
testsuite regressions.  Find it below.

WDYT?

> 

> I'm seeing this on a GDB built with:

>  gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC) 

> on Fedora 27.

> 

> Anyone else seeing this?


I'm also seeing this when I build GDB with GCC 10 (and debug
that GDB with itself), so it must be others see this too?

From af60bb6c6d7c66307db49e524409bc81f1322907 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Mon, 25 May 2020 13:14:10 +0100
Subject: [PATCH] Fix gdb::option::boolean_option_def recursion

---
 gdb/cp-support.c | 15 +++++++++++----
 gdb/cp-support.h |  5 ++++-
 2 files changed, 15 insertions(+), 5 deletions(-)


base-commit: af2c48d8545cbc24aa6caf9b9f2a49ab6c0aaa7b
-- 
2.14.5

Comments

Andrew Burgess May 26, 2020, 5:02 p.m. | #1
* Pedro Alves <palves@redhat.com> [2020-05-25 15:34:27 +0100]:

> Adding Keith, who I believe is the original author of this

> whole replace-typedefs machinery.  [ Help? :-) ]

> 

> On 5/24/20 12:35 PM, Pedro Alves via Gdb-patches wrote:

> > Hi,

> > 

> >> gdb/ChangeLog:

> >>

> >> 	* completer.c (completion_tracker::remove_completion): Define new

> >> 	function.

> >> 	* completer.h (completion_tracker::remove_completion): Declare new

> >> 	function.

> >> 	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols

> >> 	when adding a C++ function symbol.

> >>

> >> gdb/testsuite/ChangeLog:

> >>

> >> 	* gdb.linespec/cp-completion-aliases.cc: New file.

> >> 	* gdb.linespec/cp-completion-aliases.exp: New file.

> > 

> > This causes GDB to crash for me, when debugging GDB itself, and then

> > doing "b main<TAB>".

> > 

> > $ ./gdb -data-directory=data-directory ./gdb -ex "complete b main"

> > GNU gdb (GDB) 10.0.50.20200319-git

> > ...

> > Reading symbols from ./gdb...

> > Setting up the environment for debugging gdb.

> > During symbol reading: unsupported tag: 'DW_TAG_unspecified_type'

> > During symbol reading: debug info gives source 55 included from file at zero line 0

> > During symbol reading: debug info gives command-line macro definition with non-zero line 19: _STDC_PREDEF_H 1

> > Breakpoint 1 at 0xaf48c3: file /home/pedro/gdb/mygit/src/gdbsupport/errors.cc, line 54.

> > During symbol reading: Member function "~_Sp_counted_base" (offset 0x683070) is virtual but the vtable offset is not specified

> > During symbol reading: const value length mismatch for 'std::ratio<1, 1000000000>::num', got 8, expected 0

> > During symbol reading: cannot get low and high bounds for subprogram DIE at 0x694d73

> > During symbol reading: Child DIE 0x6afc31 and its abstract origin 0x6afbcd have different parents

> > During symbol reading: Multiple children of DIE 0x6b2668 refer to DIE 0x6b2657 as their abstract origin

> > Breakpoint 2 at 0x4f6775: file /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c, line 201.

> > Aborted (core dumped)

> > 

> > GDB seemingly crashes due to infinite recursion.  

> > 

> > Here's the top of the stack, showing the recursion starting:

> > 

> > $ gdb ./gdb -c core.4577

> > ...

> > Program terminated with signal SIGABRT, Aborted.

> > #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51

> > 51      }

> > [Current thread is 1 (Thread 0x7f9d6f25ecc0 (LWP 4577))]

> > (gdb) bt -44

> > #51229 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267

> > #51230 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475

> > #51231 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> > #51232 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389

> > #51233 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479

> > #51234 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267

> > #51235 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475

> > #51236 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e80, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> > #51237 0x0000000000555696 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:357

> > #51238 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479

> > #51239 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c8400, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> > #51240 0x0000000000555bd0 in cp_canonicalize_string_full[abi:cxx11](char const*, char const* (*)(type*, void*), void*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:527

> > #51241 0x0000000000555d28 in cp_canonicalize_string_no_typedefs[abi:cxx11](char const*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"...) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:550

> > #51242 0x00000000008c4959 in completion_list_add_symbol (tracker=..., sym=0x980fde0, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5316

> > #51243 0x00000000008c541b in add_symtab_completions (cust=0x673ba40, tracker=..., mode=complete_symbol_mode::LINESPEC, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5576

> > #51244 0x00000000008c5496 in <lambda(compunit_symtab*)>::operator()(compunit_symtab *) const (__closure=0x7fff6159e230, symtab=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5697

> > #51245 0x00000000008c8f14 in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::operator()(gdb::fv_detail::erased_callable, compunit_symtab *) const (__closure=0x0, ecall=..., args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:263

> > #51246 0x00000000008c8f3c in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::_FUN(gdb::fv_detail::erased_callable, compunit_symtab *) () at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:257

> > #51247 0x0000000000603da8 in gdb::function_view<void (compunit_symtab*)>::operator()(compunit_symtab*) const (this=0x7fff6159df00, args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:247

> > #51248 0x00000000007c8483 in psym_expand_symtabs_matching(objfile *, gdb::function_view<bool(char const*, bool)>, const lookup_name_info &, gdb::function_view<bool(char const*)>, gdb::function_view<void(compunit_symtab*)>, search_domain) (objfile=0x232c6a0, file_matcher=..., lookup_name_in=..., symbol_matcher=..., expansion_notify=..., domain=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/psymtab.c:1354

> > #51249 0x00000000008ab6a7 in expand_symtabs_matching(gdb::function_view<bool (char const*, bool)>, lookup_name_info const&, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (file_matcher=..., lookup_name=..., symbol_matcher=..., expansion_notify=..., kind=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/symfile.c:3788

> > #51250 0x00000000008c5ab6 in default_collect_symbol_completion_matches_break_on (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", break_on=0xc3468e "", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5692

> > #51251 0x00000000008c5f79 in default_collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5795

> > #51252 0x00000000008c5fc2 in collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5810

> > #51253 0x00000000006fcdf2 in linespec_complete_function (tracker=..., function=0x7fff6159e7f2 "main", func_match_type=symbol_name_match_type::WILD, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2816

> > #51254 0x00000000006fcecf in complete_linespec_component (parser=0x7fff6159e500, tracker=..., text=0x7fff6159e7f2 "main", component=linespec_complete_what::FUNCTION, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2848

> > #51255 0x00000000006fd63b in linespec_complete (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/linespec.c:3063

> > #51256 0x00000000005405b1 in complete_address_and_linespec_locations (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/completer.c:690

> > #51257 0x0000000000540eee in location_completer (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/completer.c:1034

> > #51258 0x0000000000541024 in location_completer_handle_brkchars (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main", word_ignored=0x0) at /home/pedro/gdb/mygit/src/gdb/completer.c:1071

> > #51259 0x00000000005416e4 in complete_line_internal_normal_command (tracker=..., command=0x7fff6159e7f0 "b main", word=0x0, cmd_args=0x7fff6159e7f2 "main", reason=handle_brkchars, c=0x1cb8320) at /home/pedro/gdb/mygit/src/gdb/completer.c:1306

> > #51260 0x0000000000541b72 in complete_line_internal_1 (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1531

> > #51261 0x0000000000541bb5 in complete_line_internal (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1550

> > #51262 0x0000000000542cee in completion_find_completion_word (tracker=..., text=0x7fff615a0905 "b main", quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:2054

> > #51263 0x000000000054247a in complete (line=0x7fff615a0905 "b main", word=0x7fff6159eb08, quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:1770

> > #51264 0x00000000004f6ed0 in complete_command (arg=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c:364

> > #51265 0x00000000004fee70 in do_const_cfunc (c=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:107

> > #51266 0x0000000000501e28 in cmd_func (cmd=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:1952

> > #51267 0x000000000090d07d in execute_command (p=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/top.c:655

> > #51268 0x000000000073b9c2 in catch_command_errors (command=0x90cc11 <execute_command(char const*, int)>, arg=0x7fff615a08fc "complete b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/main.c:401

> > #51269 0x000000000073cdaa in captured_main_1 (context=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1163

> > #51270 0x000000000073cf9f in captured_main (data=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1188

> > #51271 0x000000000073d00a in gdb_main (args=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1213

> > #51272 0x000000000041563e in main (argc=6, argv=0x7fff6159f178) at /home/pedro/gdb/mygit/src/gdb/gdb.c:32

> > 

> > This cycle keeps repeating:

> > #1709 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267

> > #1710 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475

> > #1711 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0xd83be70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470

> > #1712 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389

> > #1713 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479

> > 

> > 

> > The symbol on which we call cp_canonicalize_string_no_typedefs is:

> > 

> > (top-gdb) p *sym

> > $4 = {<general_symbol_info> = {m_name = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_

> > match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., value = {ivalue = 159449440, block = 0x9810160, bytes = 0x9810160 "\212\271\214", addre

> > ss = 0x9810160, common_block = 0x9810160, chain = 0x9810160}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, secti

> > on = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x980fc20, owner = {symtab = 0x673bdf0, arch = 0x673bdf0}, domain = VAR_DOMAIN, aclass_index = 19, is_objfile_o

> > wned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 157, aux_value = 0x980fe30, hash_next = 0x9821bd0}

> > 

> > (top-gdb) p sym->m_name 

> > $6 = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, in t, cmd_list_element*, char const*), char const*, char const*, char const*)"

> > 

> 

> I figured out what is special about this symbol.  The issue is that

> we have a boolean_option_def typedef in the global namespace in the

> program.  Here, in stack.c:

> 

>  using boolean_option_def

>    = gdb::option::boolean_option_def<frame_print_options>;

> 

> actually we have two.  There's another one in valprint.c:

> 

>  using boolean_option_def

>    = gdb::option::boolean_option_def<value_print_options>;

> 

> The recursion happens because we start by breaking the

> 

>    gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(....)

> 

> symbol name into demanged parse info components.

> This happens in cp_canonicalize_string_no_typedefs -> replace_typedefs.

> 

> The tree for that symbol looks like this:

> 

> (top-gdb) p d_dump (ret_comp, 0)

> typed name

>   qualified name

>     name 'gdb'

>     qualified name

>       name 'option'

>       qualified name

>         template

>           name 'boolean_option_def'

>           template argument list

>             name 'value_print_options'

>         name 'boolean_option_def'

>   function type

>     argument list

>       pointer

>         const

>           builtin type char

>       argument list

>         pointer

>           function type

>             pointer

>               builtin type bool

>             argument list

>               pointer

>                 name 'value_print_options'

>         argument list

>           pointer

>             function type

>               builtin type void

>               argument list

>                 pointer

>                   name 'ui_file'

>                 argument list

>                   builtin type int

>                   argument list

>                     pointer

>                       name 'cmd_list_element'

>                     argument list

>                       pointer

>                         const

>                           builtin type char

>           argument list

>             pointer

>               const

>                 builtin type char

>             argument list

>               pointer

>                 const

>                   builtin type char

>               argument list

>                 pointer

>                   const

>                     builtin type char

> $5 = void

> 

> Let's just focus on the top part, because that's where

> the recursion happens:

> 

>  typed name

>    qualified name

>      name 'gdb'

>      qualified name

>        name 'option'

>        qualified name

>          template

>            name 'boolean_option_def'

>            template argument list

>              name 'value_print_options'

>          name 'boolean_option_def'

> 

> As we walk the tree of components, we get to replace_typedefs_qualified_name,

> and enter the loop:

> 

>   while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)

>     {

>       if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)

> 	{

> 

> The DEMANGLE_COMPONENT_NAME case is hit twice, once for

> "gdb", and another for "option".  After those two,

> the next d_left (comp)->type is DEMANGLE_COMPONENT_TEMPLATE.

> 

> This now gets us to the else branch within that

> DEMANGLE_COMPONENT_QUAL_NAME loop:

> 

>       else

> 	{

> 	  /* The current node is not a name, so simply replace any

> 	     typedefs in it.  Then print it to the stream to continue

> 	     checking for more typedefs in the tree.  */

> 	  replace_typedefs (info, d_left (comp), finder, data);

> 

> which, after some more replace_typedefs recursion, gets us to

> 

> 	case DEMANGLE_COMPONENT_NAME:

> 	  inspect_type (info, ret_comp, finder, data);

> 	  break;

> 

> This is for this part of the tree:

> 

>          template

>            name 'boolean_option_def'

> 

> and it's this inspect_type call that's problematic.

> 

> [come back here later]

> 

> inspect_type does a symbol lookup for "boolean_option_def",

> which finds the unrelated typedef at the global namespace.

> In this session I'm debugging, it finds the one in valprint.c:

> 

> (top-gdb) p *sym

> $10 = {<general_symbol_info> = {m_name = 0x3a5dc90 "boolean_option_def", value = {ivalue = 0, block = 0x0, bytes = 0x0, address = 0x0, common_block = 0x0, chain = 0x0}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, section = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x3a613f0, owner = {symtab = 0x34ab930, arch = 0x34ab930}, domain = VAR_DOMAIN, aclass_index = 8, is_objfile_owned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 2987, aux_value = 0x0, hash_next = 0x3a694e0}

> (top-gdb) p *sym.owner.symtab 

> $11 = {next = 0x34ab750, compunit_symtab = 0x34ab6d0, linetable = 0x3b83e60, filename = 0x2df4b20 "/home/pedro/gdb/mygit/src/gdb/valprint.c", language = language_cplus, fullname = 0x0}

> 

> This symbol's type is of course a typedef, so we try to resolve it.

> check_typedef returns this type:

> 

>  (top-gdb) p *type->main_type

>  $14 = 

>  {name = 0x2de5a40 "gdb::option::boolean_option_def<value_print_options>",

>   code = TYPE_CODE_STRUCT,

>  ...

> 

> So, since a typedef was resolved, we get here:

> 

> 	  /* Turn the result into a new tree.  Note that this

> 	     tree will contain pointers into NAME, so NAME cannot

> 	     be free'd until all typedef conversion is done and

> 	     the final result is converted into a string.  */

> 	  i = cp_demangled_name_to_comp (name, NULL);

> 	  if (i != NULL)

> 	    {

> 	      /* Merge the two trees.  */

> 	      cp_merge_demangle_parse_infos (info, ret_comp, i.get ());

> 

> 	      /* Replace any newly introduced typedefs -- but not

> 		 if the type is anonymous (that would lead to infinite

> 		 looping).  */

> 	      if (!is_anon)

> 		{

> 		  fprintf_unfiltered (gdb_stdlog, "replace_typedefs: %s\n",

> 				      name);

> 

> 		  replace_typedefs (info, ret_comp, finder, data);

> 

> And it's that last replace_typedefs call that triggers the recursion,

> since we're now going to process a tree

> 

>  gdb::option::boolean_option_def<value_print_options>

>               ^^^^^^^^^^^^^^^^^^

> 

> that will again end up doing a lookup for that global scope

> boolean_option_def typedef, rinse repeat.

> 

> Locally, I put a recursion limit in inspect_type, and the resulting 

> symbol name that cp_canonicalize_string_no_typedefs returns is:

> 

> $18 = std::unique_ptr<char> containing 0x32f81a0 "gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::

> option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::boolean_option_def<va

> lue_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_

> print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_prin

> t_options><value_print_options><value_print_options><value_print_options><value_print_options>::boolean_option_def(char const*, bool* (*)(value_print_options*), void (*)(ui

> _file*, int, cmd_list_element*, char const*), char const*, char const*, char const*)"

> 

> Obviously broken due to recursion around boolean_option_def.

> 

> 

> Now, let's get back to that inspect_type call that I mentioned

> was problematic.  See [come back here later] above.

> 

> To recap, that happens when we're processing a DEMANGLE_COMPONENT_QUAL_NAME

> subtree.  We've processed "gdb", and then "option", and now we're going to

> process the "boolean_option_def" component:

> 

>  typed name

>    qualified name

>      name 'gdb'

>      qualified name

>        name 'option'

>        qualified name

>          template

>            name 'boolean_option_def'      <<< 

>            template argument list

>              name 'value_print_options'

>          name 'boolean_option_def'

> 

> To me, it looks quite obvious that the problem is

> that we're looking up that boolean_option_def symbol in the

> global name namespace.  I mean, this is a qualified symbol

> name, like:

> 

>    gdb::option::boolean_option_def

> 

> so we should be looking for a "boolean_option_def" typedef in

> the gdb::option namespace or struct/class.  Right?

> 

> The question to me is then, how to do that.

> In replace_typedefs_qualified_name, after we've processed

> "gdb" and "option", we have "gdb::option::" stored in

> the "buf" string_file, which seems perfect.  However, how do we

> get that scope down to inspect_type, since we have layers of

> replace_typedefs calls before we get there?

> 

> I can think of two ways:

> 

> #1 - store the current scope in the demangle_parse_info

> context object, and have inspect_type consult that.

> 

> #2 - Make replace_typedefs_qualified_name

> look ahead in the tree, inlining the

> DEMANGLE_COMPONENT_TEMPLATE case here, basically

> inlining enough forward peeking in order to do a directly

> inspect_type call from within replace_typedefs_qualified_name.

> Of course, we would likely have to do the same for other

> node types, not just DEMANGLE_COMPONENT_TEMPLATE.

> 

> And then, I'm not sure we should pass the scope as extra

> parameter to inspect_type, or, whether we should replace

> the "boolean_option_def" node in the tree with one that has

> a full-scoped name "gdb::option::boolean_option_def" name,

> similarly to how replace_typedefs_qualified_name

> handles the DEMANGLE_COMPONENT_NAME case.

> 

> One difficulty I have with this, is that I couldn't find

> any case of this code in replace_typedefs_qualified_name:

> 

> 	  /* The current node is not a name, so simply replace any

> 	     typedefs in it.  Then print it to the stream to continue

> 	     checking for more typedefs in the tree.  */

> 	  replace_typedefs (info, d_left (comp), finder, data);

> 

> actually ever replacing any typedef in the testsuite.

> I mean, I put an abort() in place if the returned component

> as a string is different from the name before the 

> replace_typedefs call, and then ran the gdb.cp/ testcases,

> and the abort never triggered...

> 

> So, in order to get the discussion going, I wrote a patch

> for solution #1.  This fixes the issue for me, and causes no

> testsuite regressions.  Find it below.

> 

> WDYT?

> 

> > 

> > I'm seeing this on a GDB built with:

> >  gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC) 

> > on Fedora 27.

> > 

> > Anyone else seeing this?

> 

> I'm also seeing this when I build GDB with GCC 10 (and debug

> that GDB with itself), so it must be others see this too?


Yes I can reproduce the original issue you are seeing trying to
complete on main.

You also mentioned a couple of times seeing problems with the test
gdb.linespec/cp-completion-aliases.exp, this I don't see - you said
even with the patch you already pushed you were still seeing issues
with this test, and I can't reproduce this.

Still I think your first patch looks like a nice improvement, so
thanks for that.  The patch below looks like a good solution to me, I
only had one tiny bit of feedback...

> 

> From af60bb6c6d7c66307db49e524409bc81f1322907 Mon Sep 17 00:00:00 2001

> From: Pedro Alves <palves@redhat.com>

> Date: Mon, 25 May 2020 13:14:10 +0100

> Subject: [PATCH] Fix gdb::option::boolean_option_def recursion

> 

> ---

>  gdb/cp-support.c | 15 +++++++++++----

>  gdb/cp-support.h |  5 ++++-

>  2 files changed, 15 insertions(+), 5 deletions(-)

> 

> diff --git a/gdb/cp-support.c b/gdb/cp-support.c

> index 1e54aaea3b1..2e4db8a7007 100644

> --- a/gdb/cp-support.c

> +++ b/gdb/cp-support.c

> @@ -137,14 +137,15 @@ inspect_type (struct demangle_parse_info *info,

>  	      canonicalization_ftype *finder,

>  	      void *data)

>  {

> -  char *name;

>    struct symbol *sym;

>  

>    /* Copy the symbol's name from RET_COMP and look it up

>       in the symbol table.  */

> -  name = (char *) alloca (ret_comp->u.s_name.len + 1);

> -  memcpy (name, ret_comp->u.s_name.s, ret_comp->u.s_name.len);

> -  name[ret_comp->u.s_name.len] = '\0';

> +  std::string name_str;

> +  if (info->current_scope != nullptr)

> +    name_str = info->current_scope;

> +  name_str.append (ret_comp->u.s_name.s, ret_comp->u.s_name.len);

> +  const char *name = name_str.c_str ();

>  

>    /* Ignore any typedefs that should not be substituted.  */

>    for (int i = 0; i < ARRAY_SIZE (ignore_typedefs); ++i)

> @@ -355,7 +356,13 @@ replace_typedefs_qualified_name (struct demangle_parse_info *info,

>  	  /* The current node is not a name, so simply replace any

>  	     typedefs in it.  Then print it to the stream to continue

>  	     checking for more typedefs in the tree.  */

> +

> +	  /* The struct/template/function/field/type name should be

> +	     qualified in the current scope though.  */

> +	  info->current_scope = buf.string ().c_str ();

>  	  replace_typedefs (info, d_left (comp), finder, data);

> +	  info->current_scope = nullptr;


I don't know if you should be using make_scoped_restore to protect
info->current_scope?

Thanks,
Andrew

> +

>  	  gdb::unique_xmalloc_ptr<char> name

>  	    = cp_comp_to_string (d_left (comp), 100);

>  	  if (name == NULL)

> diff --git a/gdb/cp-support.h b/gdb/cp-support.h

> index 6ca898315bb..561de5a94df 100644

> --- a/gdb/cp-support.h

> +++ b/gdb/cp-support.h

> @@ -72,8 +72,11 @@ struct demangle_parse_info

>  

>    /* Any temporary memory used during typedef replacement.  */

>    struct obstack obstack;

> -};

>  

> +  /* The current qualified scope context, during typedef

> +     replacement.  */

> +  const char *current_scope = nullptr;

> +};

>  

>  /* Functions from cp-support.c.  */

>  

> 

> base-commit: af2c48d8545cbc24aa6caf9b9f2a49ab6c0aaa7b

> -- 

> 2.14.5

>
Pedro Franco de Carvalho via Gdb-patches May 26, 2020, 6:09 p.m. | #2
On 5/26/20 6:02 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-05-25 15:34:27 +0100]:

> 


>>> Anyone else seeing this?

>>

>> I'm also seeing this when I build GDB with GCC 10 (and debug

>> that GDB with itself), so it must be others see this too?

> 

> Yes I can reproduce the original issue you are seeing trying to

> complete on main.

> 


Ah, cool.  With current master completing on main should no
longer crash, but you should be able to see it with:

  "gdb gdb::option::[TAB]"

> You also mentioned a couple of times seeing problems with the test

> gdb.linespec/cp-completion-aliases.exp, this I don't see - you said

> even with the patch you already pushed you were still seeing issues

> with this test, and I can't reproduce this.


I don't think I ever said that.  ISTR someone else mentioning seeing
problems with completion tests on the list (Tom de Vries?).  Most (all?)
completion tests got broken with the completion styling.  But Tromey
reversed that feature so that testsuite breakage should be fixed now.

> 

> Still I think your first patch looks like a nice improvement, so

> thanks for that.  The patch below looks like a good solution to me, I

> only had one tiny bit of feedback...

> 

>>


> I don't know if you should be using make_scoped_restore to protect

> info->current_scope?


Yeah, probably.

However, I'm still not clear on whether this is the right fix.
I think we should see what happens if a typedef is really
substituted.

If, say, we really found a typedef called "boolean_option_def"
under gdb::option, what will the name of that resolved typedef
be?  I suppose it will be a fully qualified name, and thus
back in replace_typedefs_qualified_name we should be
replacing the whole qualified name with the new resolved name?
And what if inspect_type resolves a typedef, and then recurses
again into replace_typedefs?  Will leaving info->current_scope
as "gdb::option::" break that?

Thanks,
Pedro Alves
Pedro Franco de Carvalho via Gdb-patches May 26, 2020, 9:17 p.m. | #3
On 5/25/20 7:34 AM, Pedro Alves wrote:
> 

> To recap, that happens when we're processing a DEMANGLE_COMPONENT_QUAL_NAME

> subtree.  We've processed "gdb", and then "option", and now we're going to

> process the "boolean_option_def" component:

> 

>  typed name

>    qualified name

>      name 'gdb'

>      qualified name

>        name 'option'

>        qualified name

>          template

>            name 'boolean_option_def'      <<< 

>            template argument list

>              name 'value_print_options'

>          name 'boolean_option_def'

> 

> To me, it looks quite obvious that the problem is

> that we're looking up that boolean_option_def symbol in the

> global name namespace.  I mean, this is a qualified symbol

> name, like:

> 

>    gdb::option::boolean_option_def

> 


Hmm. I'm not sure that's how the code was originally written to work...
replace_typedefs_qualified_name is supposed to "skip over" *_NAME
components until it finds something that looks like it could be a type name.
All these name components should simply be copied to the buffer. There's
no reason to look them up as symbols.

That would imply that the code simply needs to be modified to look
at DEMANGLE_COMPONENT_TEMPLATE properly.

Something like:

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 1e54aaea3b..7761470b2d 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -314,6 +314,9 @@ replace_typedefs_qualified_name (struct demangle_parse_info *info,
      substituted name.  */
   while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)
     {
+      if (d_left (comp)->type == DEMANGLE_COMPONENT_TEMPLATE)
+       comp = d_left (comp);
+
       if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)
        {
          struct demangle_component newobj;

NOTE: I haven't extensively tested this patch. Fedora 31 system GDB
crashes or fails to set a breakpoint on one of the completed values -- it
does complete it okay. On master the above tentative patch seems to work.

I have much more investigation and testing to do for this before submitting
anything official, though.

> One difficulty I have with this, is that I couldn't find

> any case of this code in replace_typedefs_qualified_name:

> 

> 	  /* The current node is not a name, so simply replace any

> 	     typedefs in it.  Then print it to the stream to continue

> 	     checking for more typedefs in the tree.  */

> 	  replace_typedefs (info, d_left (comp), finder, data);

> 

> actually ever replacing any typedef in the testsuite.

> I mean, I put an abort() in place if the returned component

> as a string is different from the name before the 

> replace_typedefs call, and then ran the gdb.cp/ testcases,

> and the abort never triggered...


Huh. I'm not sure what to say. Normally, I'm pretty thorough with
coverage testing. Either I missed something during test writing,
or perhaps this has code has since been rendered unnecessary?

Keith
Pedro Franco de Carvalho via Gdb-patches May 27, 2020, 7:36 p.m. | #4
On 5/26/20 10:17 PM, Keith Seitz wrote:
> On 5/25/20 7:34 AM, Pedro Alves wrote:

>>

>> To recap, that happens when we're processing a DEMANGLE_COMPONENT_QUAL_NAME

>> subtree.  We've processed "gdb", and then "option", and now we're going to

>> process the "boolean_option_def" component:

>>

>>  typed name

>>    qualified name

>>      name 'gdb'

>>      qualified name

>>        name 'option'

>>        qualified name

>>          template

>>            name 'boolean_option_def'      <<< 

>>            template argument list

>>              name 'value_print_options'

>>          name 'boolean_option_def'

>>

>> To me, it looks quite obvious that the problem is

>> that we're looking up that boolean_option_def symbol in the

>> global name namespace.  I mean, this is a qualified symbol

>> name, like:

>>

>>    gdb::option::boolean_option_def

>>

> 

> Hmm. I'm not sure that's how the code was originally written to work...

> replace_typedefs_qualified_name is supposed to "skip over" *_NAME

> components until it finds something that looks like it could be a type name.

> All these name components should simply be copied to the buffer. There's

> no reason to look them up as symbols.

> 

> That would imply that the code simply needs to be modified to look

> at DEMANGLE_COMPONENT_TEMPLATE properly.

> 

> Something like:

> 

> diff --git a/gdb/cp-support.c b/gdb/cp-support.c

> index 1e54aaea3b..7761470b2d 100644

> --- a/gdb/cp-support.c

> +++ b/gdb/cp-support.c

> @@ -314,6 +314,9 @@ replace_typedefs_qualified_name (struct demangle_parse_info *info,

>       substituted name.  */

>    while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)

>      {

> +      if (d_left (comp)->type == DEMANGLE_COMPONENT_TEMPLATE)

> +       comp = d_left (comp);

> +

>        if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)

>         {

>           struct demangle_component newobj;

> 

> NOTE: I haven't extensively tested this patch. Fedora 31 system GDB

> crashes or fails to set a breakpoint on one of the completed values -- it

> does complete it okay. On master the above tentative patch seems to work.

> 

> I have much more investigation and testing to do for this before submitting

> anything official, though.


Today I experimented with different kinds of symbols to get a better sense
of what the demangled tree node looks like in different scenarios.
I understand this better now.

I now agree that this is looking like the direction that we need to
take (#2 in my original email).

However, as you have it I don't think is fully correct.  Because, say,
you have a symbol like:

  A::B<int>::C<int>::function()

It seems like that change will make it so that we won't ever
try to substitute a typedef past the first template?

so with e.g., 

 typedef int MyInt;

setting a breakpoint like this works:

 "(gdb) b A::B<MyInt>::C<int>::function()"

but like this doesn't:

 "(gdb) b A::B<int>::C<MyInt>::function()"

I started writing a testcase to try some of these different scenarios, based
on gdb.linespec/cp-completion-aliases-2.exp.  See patches attached.
One of the patches includes the recursion detection I'm using to avoid
crashing GDB.  That patch also includes some hacks to print some debugging
stuff.

I also ran into another case of GDB recursing forever due to a template.
This is when the tree has a qualified name node with the template node on
the right child node instead of on the left.  Seems like we'll need a similar
treatment for templates at the end of replace_typedefs_qualified_name.

Here's how to trigger it with the testcase attached:

(gdb) b NS1::NS2::grab_it(NS1::NS2::magic<int>*) 
warning: inspect_type: max recursion limit reached for NS1
warning: inspect_type: max recursion limit reached for NS1::NS2
warning: inspect_type: max recursion limit reached for magic
warning: inspect_type: max recursion limit reached for NS1
warning: inspect_type: max recursion limit reached for NS1::NS2
warning: inspect_type: max recursion limit reached for magic
Function "NS1::NS2::grab_it(NS1::NS2::magic<int>*)" not defined.
Make breakpoint pending on future shared library load? (y or [n]) 

The tree looks like:

d_dump tree for NS1::NS2::grab_it(NS1::NS2::magic<int>*):
typed name
  qualified name
    name 'NS1'
    qualified name
      name 'NS2'
      name 'grab_it'
  function type
    argument list
      pointer
        qualified name
          name 'NS1'
          qualified name
            name 'NS2'
            template
              name 'magic'
              template argument list
                builtin type int

Seems like this will be more work that I hoped...

Maybe we should start by putting in the recursion limit, to
avoid the crash, and then we can fix the issues more like
other bugs than fatal crashes that make it impossible to debug
gdb itself...
From 84772ca2bf6acd84c971adffe0b94d3696ad5a1b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Wed, 27 May 2020 19:56:18 +0100
Subject: [PATCH 1/2] recursion limit & debug stuff

---
 gdb/cp-support.c        | 82 ++++++++++++++++++++++++++++++++++++++++++++++---
 libiberty/cp-demangle.c |  8 +++--
 2 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 1e54aaea3b1..9ee8cf74f58 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -126,6 +126,12 @@ cp_already_canonical (const char *string)
     return 0;
 }
 
+/* Recursion counter.  */
+int inspect_type_recursion = 0;
+
+/* Set to true to enable inspect_type debug logs.  */
+static int debug_inspect_type;
+
 /* Inspect the given RET_COMP for its type.  If it is a typedef,
    replace the node with the typedef's tree.
 
@@ -146,11 +152,28 @@ 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';
 
+  scoped_restore restore = make_scoped_restore (&inspect_type_recursion);
+  ++inspect_type_recursion;
+
+  if (debug_inspect_type)
+    fprintf_unfiltered (gdb_stdlog, "inspect_type: %d, %s\n", inspect_type_recursion, name);
+
+  if (inspect_type_recursion > 200)
+    {
+      warning (_("inspect_type: max recursion limit reached for %s"), name);
+      return 0;
+    }
+
   /* Ignore any typedefs that should not be substituted.  */
   for (int i = 0; i < ARRAY_SIZE (ignore_typedefs); ++i)
     {
       if (strcmp (name, ignore_typedefs[i]) == 0)
-	return 0;
+	{
+	  if (debug_inspect_type)
+	    fprintf_unfiltered (gdb_stdlog,
+				"inspect_type, ignored typedef\n");
+	  return 0;
+	}
     }
 
   sym = NULL;
@@ -161,6 +184,9 @@ inspect_type (struct demangle_parse_info *info,
     }
   catch (const gdb_exception &except)
     {
+      if (debug_inspect_type)
+	fprintf_unfiltered (gdb_stdlog,
+			    "inspect_type, lookup_symbol failed\n");
       return 0;
     }
 
@@ -176,9 +202,16 @@ inspect_type (struct demangle_parse_info *info,
 	    {
 	      ret_comp->u.s_name.s = new_name;
 	      ret_comp->u.s_name.len = strlen (new_name);
+
+	      if (debug_inspect_type)
+		fprintf_unfiltered (gdb_stdlog,
+				    "inspect_type, finder, return 1, new_name: %s\n",
+				    new_name);
 	      return 1;
 	    }
 
+	  if (debug_inspect_type)
+	    fprintf_unfiltered (gdb_stdlog, "inspect_type, finder, return 0\n");
 	  return 0;
 	}
 
@@ -209,7 +242,11 @@ inspect_type (struct demangle_parse_info *info,
 	     as the symbol's name, e.g., "typedef struct foo foo;".  */
 	  if (type->name () != nullptr
 	      && strcmp (type->name (), name) == 0)
-	    return 0;
+	    {
+	      if (debug_inspect_type)
+		fprintf_unfiltered (gdb_stdlog, "inspect_type, same name as original name\n");
+	      return 0;
+	    }
 
 	  is_anon = (type->name () == NULL
 		     && (type->code () == TYPE_CODE_ENUM
@@ -244,6 +281,9 @@ inspect_type (struct demangle_parse_info *info,
 	     in continuing, so just bow out gracefully.  */
 	  catch (const gdb_exception_error &except)
 	    {
+	      if (debug_inspect_type)
+		fprintf_unfiltered (gdb_stdlog, "inspect_type, error: %s\n",
+				    except.message->c_str ());
 	      return 0;
 	    }
 
@@ -264,7 +304,13 @@ inspect_type (struct demangle_parse_info *info,
 		 if the type is anonymous (that would lead to infinite
 		 looping).  */
 	      if (!is_anon)
-		replace_typedefs (info, ret_comp, finder, data);
+		{
+		  if (debug_inspect_type)
+		    fprintf_unfiltered (gdb_stdlog, "replace_typedefs: %s\n",
+					name);
+
+		  replace_typedefs (info, ret_comp, finder, data);
+		}
 	    }
 	  else
 	    {
@@ -505,6 +551,14 @@ replace_typedefs (struct demangle_parse_info *info,
     }
 }
 
+/* From libiberty.  */
+extern "C" void d_dump (struct demangle_component *dc, int indent);
+
+/* Recursion counter.  */
+int cp_canonicalize_string_full_count;
+
+static int debug_canonicalize = 0;
+
 /* Parse STRING and convert it to canonical form, resolving any
    typedefs.  If parsing fails, or if STRING is already canonical,
    return nullptr.  Otherwise return the canonical form.  If
@@ -523,6 +577,16 @@ cp_canonicalize_string_full (const char *string,
   info = cp_demangled_name_to_comp (string, NULL);
   if (info != NULL)
     {
+      scoped_restore restore = make_scoped_restore (&cp_canonicalize_string_full_count);
+      if (cp_canonicalize_string_full_count++ == 0)
+	{
+	  if (debug_canonicalize)
+	    {
+	      fprintf_unfiltered (gdb_stdlog, "d_dump tree for %s:\n", string);
+	      d_dump (info->tree, 0);
+	    }
+	}
+
       /* Replace all the typedefs in the tree.  */
       replace_typedefs (info.get (), info->tree, finder, data);
 
@@ -534,7 +598,17 @@ cp_canonicalize_string_full (const char *string,
       /* Finally, compare the original string with the computed
 	 name, returning NULL if they are the same.  */
       if (strcmp (us.get (), string) == 0)
-	return nullptr;
+	{
+	  if (debug_canonicalize)
+	    fprintf_unfiltered (gdb_stdlog, "raw == canonical\n");
+	  return nullptr;
+	}
+
+      if (debug_canonicalize)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "raw:       %s\n", string);
+	  fprintf_unfiltered (gdb_stdlog, "canonical: %s\n", us.get ());
+	}
 
       return us;
     }
diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index cbfb2f937ca..ef8877d01ec 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -595,9 +595,11 @@ is_fnqual_component_type (enum demangle_component_type type)
 }
 
 
-#ifdef CP_DEMANGLE_DEBUG
+// #ifdef CP_DEMANGLE_DEBUG
 
-static void
+void d_dump (struct demangle_component *dc, int indent);
+
+void
 d_dump (struct demangle_component *dc, int indent)
 {
   int i;
@@ -853,7 +855,7 @@ d_dump (struct demangle_component *dc, int indent)
   d_dump (d_right (dc), indent + 2);
 }
 
-#endif /* CP_DEMANGLE_DEBUG */
+// #endif /* CP_DEMANGLE_DEBUG */
 
 /* Fill in a DEMANGLE_COMPONENT_NAME.  */
 

base-commit: 636edd0018b72d67ee224e868fb277a658e9b984
-- 
2.14.5
From 61d3b7c315f2621f913c28c5d3b2819994d07c12 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Wed, 27 May 2020 19:31:48 +0100
Subject: [PATCH 2/2] rough testcase

---
 .../gdb.linespec/cp-completion-aliases-2.cc        | 132 +++++++++++++++++++++
 .../gdb.linespec/cp-completion-aliases-2.exp       |  64 ++++++++++
 2 files changed, 196 insertions(+)
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp

diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc
new file mode 100644
index 00000000000..cc1eb0505b8
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc
@@ -0,0 +1,132 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019-2020 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/>.  */
+
+#include <cstring>
+
+namespace NS1 {
+
+namespace NS2 {
+
+struct object
+{
+  int a;
+
+  object ();
+};
+
+object::object ()
+{
+}
+
+typedef object *object_p;
+
+typedef const char *my_string_t;
+
+template<typename T>
+struct magic
+{
+  T x;
+
+  magic (object_p)
+  {
+  }
+
+  template<typename I>
+  static void
+  foo (object_p)
+  {
+  }
+};
+
+template<typename T, typename U>
+struct tragic
+{
+  T t;
+  U u;
+
+  tragic (object_p)
+  {
+  }
+
+  template<typename I>
+  static void
+  foo (object_p)
+  {
+  }
+};
+
+template<typename T> using partial = tragic<int, T>;
+
+typedef partial<int> int_tragic_t;
+
+typedef magic<int> int_magic_t;
+
+static int
+get_value (object_p obj)
+{
+  return obj->a;
+}
+
+static int
+get_something (object_p obj)
+{
+  return obj->a;
+}
+
+static int
+get_something (my_string_t msg)
+{
+  return strlen (msg);
+}
+
+static int
+grab_it (int_magic_t *var)
+{
+  return var->x;
+}
+
+}
+
+}
+
+/* These typedefs have the same name as some of the components within
+   NS1, on purpose, to try to confuse GDB.  */
+using NS2 = int;
+using object = NS1::NS2::object;
+using magic = NS1::NS2::magic<int>;
+
+NS2 ns2_int = 0;
+object o;
+magic m (0);
+NS1::NS2::int_tragic_t trag (0);
+
+int
+main ()
+{
+  NS1::NS2::magic<int> ns_m (0);
+  NS1::NS2::magic<int>::foo<int> (0);
+  NS1::NS2::partial<int>::foo<int> (0);
+  ns_m.x = 4;
+
+  NS1::NS2::object ns_obj;
+  ns_obj.a = 0;
+
+  int ns_val = (NS1::NS2::get_value (&ns_obj) + NS1::NS2::get_something (&ns_obj)
+		+ NS1::NS2::get_something ("abc") + NS1::NS2::grab_it (&ns_m));
+
+  return ns_val;
+}
diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp
new file mode 100644
index 00000000000..b09cd6490e8
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp
@@ -0,0 +1,64 @@
+# Copyright 2019-2020 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/>.
+
+# This file tests GDB's ability to remove symbol aliases from the
+# completion list in C++.
+
+load_lib completion-support.exp
+
+standard_testfile .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
+    return -1
+}
+
+# Disable the completion limit for the whole testcase.
+gdb_test_no_output "set max-completions unlimited"
+
+# This is what I think should happen, but instead, we lose the
+# object_p typedef somewhere.
+test_gdb_complete_unique \
+    "break -q NS1::NS2::get_v" \
+    "break -q NS1::NS2::get_value(NS1::NS2::object_p)"
+
+# Keith's patch fixes this one:
+test_gdb_complete_unique \
+    "break -q NS1::NS2::magic<int>::mag" \
+    "break -q NS1::NS2::magic<int>::magic(NS1::NS2::object*)"
+
+# This works:
+gdb_test "b NS1::NS2::magic<int>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+
+# This works too.  Note that there's a "NS2" typedef in the global
+# namespace.
+gdb_test "b NS1::NS2::magic<NS2>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+
+# But this does not work with Keith's patch.  We never get to replace
+# the "NS2" typedef in "foo<NS2>".
+gdb_test "b NS1::NS2::magic<NS2>::foo<NS2>(NS1::NS2::object*)" "Breakpoint .* at .*"
+
+# These work.
+gdb_test "b NS1::NS2::tragic<int, int>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+gdb_test "b NS1::NS2::tragic<int, int>::foo<int>(NS1::NS2::object_p)" "Breakpoint .* at .*"
+
+# These don't work because GDB doesn't understand the partial<int>
+# alias template.
+gdb_test "b NS1::NS2::partial<int>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+gdb_test "b NS1::NS2::partial<int>::foo<int>(NS1::NS2::object_p)" "Breakpoint .* at .*"
+
+# These crash GDB with recursion around "magic".  There's a "magic"
+# typedef in the global namespace.
+gdb_test "b NS1::NS2::grab_it(NS1::NS2::int_magic_t*)" "Breakpoint .* at .*"
+gdb_test "b NS1::NS2::grab_it(NS1::NS2::magic<int>*)" "Breakpoint .* at .*"
-- 
2.14.5
Pedro Franco de Carvalho via Gdb-patches May 28, 2020, 1:40 p.m. | #5
On 5/27/20 8:36 PM, Pedro Alves via Gdb-patches wrote:
> On 5/26/20 10:17 PM, Keith Seitz wrote:

>> On 5/25/20 7:34 AM, Pedro Alves wrote:

>>>

>>> To recap, that happens when we're processing a DEMANGLE_COMPONENT_QUAL_NAME

>>> subtree.  We've processed "gdb", and then "option", and now we're going to

>>> process the "boolean_option_def" component:

>>>

>>>  typed name

>>>    qualified name

>>>      name 'gdb'

>>>      qualified name

>>>        name 'option'

>>>        qualified name

>>>          template

>>>            name 'boolean_option_def'      <<< 

>>>            template argument list

>>>              name 'value_print_options'

>>>          name 'boolean_option_def'

>>>

>>> To me, it looks quite obvious that the problem is

>>> that we're looking up that boolean_option_def symbol in the

>>> global name namespace.  I mean, this is a qualified symbol

>>> name, like:

>>>

>>>    gdb::option::boolean_option_def

>>>

>>

>> Hmm. I'm not sure that's how the code was originally written to work...

>> replace_typedefs_qualified_name is supposed to "skip over" *_NAME

>> components until it finds something that looks like it could be a type name.

>> All these name components should simply be copied to the buffer. There's

>> no reason to look them up as symbols.

>>

>> That would imply that the code simply needs to be modified to look

>> at DEMANGLE_COMPONENT_TEMPLATE properly.

>>

>> Something like:

>>

>> diff --git a/gdb/cp-support.c b/gdb/cp-support.c

>> index 1e54aaea3b..7761470b2d 100644

>> --- a/gdb/cp-support.c

>> +++ b/gdb/cp-support.c

>> @@ -314,6 +314,9 @@ replace_typedefs_qualified_name (struct demangle_parse_info *info,

>>       substituted name.  */

>>    while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)

>>      {

>> +      if (d_left (comp)->type == DEMANGLE_COMPONENT_TEMPLATE)

>> +       comp = d_left (comp);

>> +

>>        if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)

>>         {

>>           struct demangle_component newobj;

>>

>> NOTE: I haven't extensively tested this patch. Fedora 31 system GDB

>> crashes or fails to set a breakpoint on one of the completed values -- it

>> does complete it okay. On master the above tentative patch seems to work.

>>

>> I have much more investigation and testing to do for this before submitting

>> anything official, though.

> 

> Today I experimented with different kinds of symbols to get a better sense

> of what the demangled tree node looks like in different scenarios.

> I understand this better now.

> 

> I now agree that this is looking like the direction that we need to

> take (#2 in my original email).

> 

> However, as you have it I don't think is fully correct.  Because, say,

> you have a symbol like:

> 

>   A::B<int>::C<int>::function()

> 

> It seems like that change will make it so that we won't ever

> try to substitute a typedef past the first template?

> 

> so with e.g., 

> 

>  typedef int MyInt;

> 

> setting a breakpoint like this works:

> 

>  "(gdb) b A::B<MyInt>::C<int>::function()"

> 

> but like this doesn't:

> 

>  "(gdb) b A::B<int>::C<MyInt>::function()"

> 

> I started writing a testcase to try some of these different scenarios, based

> on gdb.linespec/cp-completion-aliases-2.exp.  See patches attached.

> One of the patches includes the recursion detection I'm using to avoid

> crashing GDB.  That patch also includes some hacks to print some debugging

> stuff.

> 

> I also ran into another case of GDB recursing forever due to a template.

> This is when the tree has a qualified name node with the template node on

> the right child node instead of on the left.  Seems like we'll need a similar

> treatment for templates at the end of replace_typedefs_qualified_name.

> 

> Here's how to trigger it with the testcase attached:

> 

> (gdb) b NS1::NS2::grab_it(NS1::NS2::magic<int>*) 

> warning: inspect_type: max recursion limit reached for NS1

> warning: inspect_type: max recursion limit reached for NS1::NS2

> warning: inspect_type: max recursion limit reached for magic

> warning: inspect_type: max recursion limit reached for NS1

> warning: inspect_type: max recursion limit reached for NS1::NS2

> warning: inspect_type: max recursion limit reached for magic

> Function "NS1::NS2::grab_it(NS1::NS2::magic<int>*)" not defined.

> Make breakpoint pending on future shared library load? (y or [n]) 

> 

> The tree looks like:

> 

> d_dump tree for NS1::NS2::grab_it(NS1::NS2::magic<int>*):

> typed name

>   qualified name

>     name 'NS1'

>     qualified name

>       name 'NS2'

>       name 'grab_it'

>   function type

>     argument list

>       pointer

>         qualified name

>           name 'NS1'

>           qualified name

>             name 'NS2'

>             template

>               name 'magic'

>               template argument list

>                 builtin type int

> 

> Seems like this will be more work that I hoped...

> 

> Maybe we should start by putting in the recursion limit, to

> avoid the crash, and then we can fix the issues more like

> other bugs than fatal crashes that make it impossible to debug

> gdb itself...


I ended up going ahead and fixing it all.  I've sent a new
patch as a new thread, here:

  https://sourceware.org/pipermail/gdb-patches/2020-May/169097.html

Thanks for the hint Keith, that was really helpful in setting me
on the right path.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 1e54aaea3b1..2e4db8a7007 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -137,14 +137,15 @@  inspect_type (struct demangle_parse_info *info,
 	      canonicalization_ftype *finder,
 	      void *data)
 {
-  char *name;
   struct symbol *sym;
 
   /* Copy the symbol's name from RET_COMP and look it up
      in the symbol table.  */
-  name = (char *) alloca (ret_comp->u.s_name.len + 1);
-  memcpy (name, ret_comp->u.s_name.s, ret_comp->u.s_name.len);
-  name[ret_comp->u.s_name.len] = '\0';
+  std::string name_str;
+  if (info->current_scope != nullptr)
+    name_str = info->current_scope;
+  name_str.append (ret_comp->u.s_name.s, ret_comp->u.s_name.len);
+  const char *name = name_str.c_str ();
 
   /* Ignore any typedefs that should not be substituted.  */
   for (int i = 0; i < ARRAY_SIZE (ignore_typedefs); ++i)
@@ -355,7 +356,13 @@  replace_typedefs_qualified_name (struct demangle_parse_info *info,
 	  /* The current node is not a name, so simply replace any
 	     typedefs in it.  Then print it to the stream to continue
 	     checking for more typedefs in the tree.  */
+
+	  /* The struct/template/function/field/type name should be
+	     qualified in the current scope though.  */
+	  info->current_scope = buf.string ().c_str ();
 	  replace_typedefs (info, d_left (comp), finder, data);
+	  info->current_scope = nullptr;
+
 	  gdb::unique_xmalloc_ptr<char> name
 	    = cp_comp_to_string (d_left (comp), 100);
 	  if (name == NULL)
diff --git a/gdb/cp-support.h b/gdb/cp-support.h
index 6ca898315bb..561de5a94df 100644
--- a/gdb/cp-support.h
+++ b/gdb/cp-support.h
@@ -72,8 +72,11 @@  struct demangle_parse_info
 
   /* Any temporary memory used during typedef replacement.  */
   struct obstack obstack;
-};
 
+  /* The current qualified scope context, during typedef
+     replacement.  */
+  const char *current_scope = nullptr;
+};
 
 /* Functions from cp-support.c.  */