Re: [PATCH 2/3] [RFC][gdb/symtab] Add maint set symbol-read-order

[Bruno Larsen <blarsen@redhat.com>]

Cheers,
Bruno

On 22/09/2022 17:20, Tom de Vries via Gdb-patches wrote:
> With the test-case included in this patch, we run into:
> ...
> (gdb) print var_dwarf^M
> $1 = 1^M
> (gdb) PASS: gdb.ctf/dwarf2-and-ctf.exp: print var_dwarf
> print var_ctf^M
> 'var_ctf' has unknown type; cast it to its declared type^M
> (gdb) FAIL: gdb.ctf/dwarf2-and-ctf.exp: print var_ctf
> ...
>
> The problem is that the executable contains both ctf and dwarf2, so the ctf
> info is ignored.
>
> GDB has support for handling multiple debug formats, but the common use case
> for ctf is to be used when dwarf2 is not present, and gdb reflects that,
> assuming that by reading ctf in addition there won't be any extra information,
> so it's not worth the additional cycles and memory.
>
> Add a new maintenance command "maint set symbol-read-order <string>", where
> the argument string "mdebug+stabs+dwarf2/ctf" represent the current default,
> in other words:
> - first mdebug is read
> - then stabs is read
> - then dwarf2 is read,
> - then ctf is read, but only if dwarf2 info was missing.
>
> A bit of background on the relevance of reading order: the formats that create
> partial symtabs (all but dwarf2) have a priority relationship between them,
> where reading earlier means lower priority.  By reading the format with the
> most detail last, we ensure it has the highest priority, which makes sure that
> in case there is overlapping info, the most detailed info is found.  This
> explains the ordering of mdebug, stabs and dwarf2 (because it used to have
> partial symtabs).
>
> By using "maint set symbol-read-order ctf+dwarf2 or dwarf2+ctf, we can make the
> test-case pass.
>
> This is an RFC at this point, so no documentation yet.
>
> Points I'd like to have feedback on:
> - I chose this to be a maintenance command, because I suspect the reading
>    order implications are a bit hard to understand.
> - was it a good idea to use strtok ?
> - I went for genericity, but could haved used instead a simple
>    "set read-ctf-symtab <no|no-dwarf2|yes>".  If that is better, should
>    it be a maintenance command or not?
> - I initially went for the priority first order in the argument string, so
>    reflecting the current default would be "dwarf2/ctf+stabs+mdebug", thinking
>    it would be easier to understand the priority scheme this way, but abandoned
>    that approach midway.  Which is easier to grasp?

A point against genericity, it isn't clear if we are supposed to read 
mdebug+stabs+(dwarf2)/ctf or (mdebug+stabs+dwarf2)/ctf.  The option to 
change how ctf specifically is read would not create this.

About the ordering, I feel like priority order would be less confusing, 
if you decide to go with the generic option.

>
> Tested on x86_64-linux.
> ---
>   gdb/elfread.c                            | 243 ++++++++++++++++++++---
>   gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c |  24 +++
>   gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c   |  26 +++
>   gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp |  54 +++++
>   4 files changed, 317 insertions(+), 30 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c
>   create mode 100644 gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c
>   create mode 100644 gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index ed11c67dc02..740b0a76513 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -51,6 +51,7 @@
>   #include "gdbsupport/scoped_fd.h"
>   #include "debuginfod-support.h"
>   #include "dwarf2/public.h"
> +#include "cli/cli-cmds.h"
>   
>   /* The struct elfinfo is available only during ELF symbol table and
>      psymtab reading.  It is destroyed at the completion of psymtab-reading.
> @@ -1166,6 +1167,152 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
>     symtab_create_debug_printf ("done reading minimal symbols");
>   }
>   
> +/* Symbol types supported by elfread.c.  */
> +
> +enum symbol_type
> +  {
> +    mdebug,
> +    stabs,
> +    dwarf2,
> +    ctf,
> +    sep,
> +  };
> +
> +/* Strings corresponding to enum symbol_type.  */
> +
> +const char *symbol_type_to_string [] =
> +  {
> +    "mdebug",
> +    "stabs",
> +    "dwarf2",
> +    "ctf",
> +    "sep"
> +  };
> +
> +/* Return symbol_type for string S in ST and return true, otherwise return
> +   false.  If ALLOW_SEP is true, we also allow the sep symbol_type.  */
> +
> +static bool
> +string_to_symbol_type (const char *s, enum symbol_type &st,
> +		       bool allow_sep = false)
> +{
> +  size_t symbol_type_to_string_elems
> +    = sizeof (symbol_type_to_string) / sizeof (*symbol_type_to_string);
> +
> +  for (unsigned int i = 0; i < symbol_type_to_string_elems; i++)
> +    {
> +      if (i == sep && !allow_sep)
> +	continue;
> +
> +      if (strcmp (symbol_type_to_string[i], s) != 0)
> +	continue;
> +
> +      st = (symbol_type)i;
> +      return true;
> +    }
> +
> +  return false;
> +}
> +
> +/* The default "mdebug+stabs+dwarf2/ctf" means:
> +   - read mdebug first
> +   - then read stabs
> +   - then read dwarf2
> +   - then read ctf, but only if dwarf2 is not read.  */
> +
> +static std::string symbol_read_order_default = "mdebug+stabs+dwarf2/ctf";
> +
> +/* Set by the "maint set symbol-read-order <string>" command.  */
> +
> +static std::string symbol_read_order = symbol_read_order_default;
> +
> +/* Parsed representation of symbol_read_order.
> +   For a symbol_read_order of "mdebug+stabs+dwarf2/ctf" we have
> +   { mdebug, sep, stabs, sep, dwarf, ctf, sep }.  */
> +
> +std::vector<enum symbol_type> parsed_symbol_read_order;
> +
> +/* Init parsed_symbol_read_order using symbol_read_order.  Return true if
> +   successful.  */
> +
> +static bool
> +parse_symbol_read_order (void)
> +{
> +  parsed_symbol_read_order.clear ();
> +
> +  if (symbol_read_order.empty ())
> +    return false;
> +
> +  const char *s = symbol_read_order.c_str ();
> +
> +  int prev_delim = -1;
> +  for (int i = 0; i < symbol_read_order.size (); ++i)
> +    {
> +      char c = s[i];
> +      if (c != '/' && c != '+')
> +	continue;
> +
> +      if (i == 0 || i == symbol_read_order.size () - 1)
> +	/* Cannot begin or end with delimiter.  */
> +	return false;
> +
> +      if (prev_delim != -1 && prev_delim == i - 1)
> +	/* Cannot have two subsequent delimiters.  */
> +	return false;
> +
> +      prev_delim = i;
> +    }
> +
> +  char s_dup[symbol_read_order.size ()];
> +  strcpy (&s_dup[0], s);
> +
> +  for (const char *p = s; *p != '\0'; ++p)
> +    {
> +      gdb_assert (*p != '/' && *p != '+');
> +      char *token = strtok (p == s ? s_dup : nullptr, "/+");
> +      gdb_assert (strlen (token) != 0);
> +      enum symbol_type st;
> +      if (!string_to_symbol_type (token, st))
> +	{
> +	  parsed_symbol_read_order.clear ();
> +	  return false;
> +	}
> +      parsed_symbol_read_order.push_back (st);
> +
> +      p += strlen (token);
> +      if (*p == '\0')
> +	{
> +	  parsed_symbol_read_order.push_back (sep);
> +	  break;
> +	}
> +
> +      if (*p == '+')
> +	{
> +	  parsed_symbol_read_order.push_back (sep);
> +	}
> +      else if (*p == '/')
> +	continue;
> +      else
> +	gdb_assert_not_reached ("");
> +    }
> +
> +  return true;
> +}
> +
> +static void
> +update_symbol_read_order (const char *args, int from_tty,
> +			  struct cmd_list_element *c)
> +{
> +  if (!parse_symbol_read_order ())
> +    {
> +      warning ("invalid argument: '%s', reverting to default: '%s'",
> +	       symbol_read_order.c_str (), symbol_read_order_default.c_str ());
> +      symbol_read_order = symbol_read_order_default;
> +      bool res = parse_symbol_read_order ();
> +      gdb_assert (res);
> +    }
> +}
> +
>   /* Dwarf-specific helper for elf_symfile_read.  Return true if we managed to
>      load dwarf debug info.  */
>   
> @@ -1270,6 +1417,12 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>   static void
>   elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>   {
> +  if (parsed_symbol_read_order.empty ())
> +    {
> +      bool res = parse_symbol_read_order ();
> +      gdb_assert (res);
> +    }
> +
>     bfd *abfd = objfile->obfd.get ();
>     struct elfinfo ei;
>   
> @@ -1294,38 +1447,59 @@ elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>        we don't do this then the XCOFF info is found first - for code in
>        an included file XCOFF info is useless.  */
>   
> -  if (ei.mdebugsect)
> -    {
> -      const struct ecoff_debug_swap *swap;
> -
> -      /* .mdebug section, presumably holding ECOFF debugging
> -	 information.  */
> -      swap = get_elf_backend_data (abfd)->elf_backend_ecoff_debug_swap;
> -      if (swap)
> -	elfmdebug_build_psymtabs (objfile, swap, ei.mdebugsect);
> -    }
> -  if (ei.stabsect)
> +  for (int i = 0; i < parsed_symbol_read_order.size (); ++i)
>       {
> -      asection *str_sect;
> -
> -      /* Stab sections have an associated string table that looks like
> -	 a separate section.  */
> -      str_sect = bfd_get_section_by_name (abfd, ".stabstr");
> -
> -      /* FIXME should probably warn about a stab section without a stabstr.  */
> -      if (str_sect)
> -	elfstab_build_psymtabs (objfile,
> -				ei.stabsect,
> -				str_sect->filepos,
> -				bfd_section_size (str_sect));
> -    }
> -
> -  bool has_dwarf2 = elf_symfile_read_dwarf2 (objfile, symfile_flags);
> +      bool loaded_p = false;
> +      switch (parsed_symbol_read_order[i])
> +	{
> +	case mdebug:
> +	  if (ei.mdebugsect)
> +	    {
> +	      const struct ecoff_debug_swap *swap;
> +
> +	      /* .mdebug section, presumably holding ECOFF debugging
> +		 information.  */
> +	      swap = get_elf_backend_data (abfd)->elf_backend_ecoff_debug_swap;
> +	      if (swap)
> +		elfmdebug_build_psymtabs (objfile, swap, ei.mdebugsect);
> +	      loaded_p = true;
> +	    }
> +	  break;
> +	case stabs:
> +	  if (ei.stabsect)
> +	    {
> +	      asection *str_sect;
> +
> +	      /* Stab sections have an associated string table that looks like
> +		 a separate section.  */
> +	      str_sect = bfd_get_section_by_name (abfd, ".stabstr");
> +
> +	      /* FIXME should probably warn about a stab section without a stabstr.  */
> +	      if (str_sect)
> +		elfstab_build_psymtabs (objfile,
> +					ei.stabsect,
> +					str_sect->filepos,
> +					bfd_section_size (str_sect));
> +	      loaded_p = true;
> +	    }
> +	  break;
> +	case ctf:
> +	  if (ei.ctfsect)
> +	    {
> +	      elfctf_build_psymtabs (objfile);
> +	      loaded_p = true;
> +	    }
> +	  break;
> +	case dwarf2:
> +	  loaded_p = elf_symfile_read_dwarf2 (objfile, symfile_flags);
> +	}
>   
> -  /* Read the CTF section only if there is no DWARF info.  */
> -  if (!has_dwarf2 && ei.ctfsect)
> -    {
> -      elfctf_build_psymtabs (objfile);
> +      if (loaded_p)
> +	{
> +	  for (i++; i < parsed_symbol_read_order.size (); ++i)
> +	    if (parsed_symbol_read_order[i] == sep)
> +	      break;
> +	}
>       }
>   }
>   
> @@ -1420,4 +1594,13 @@ _initialize_elfread ()
>     add_symtab_fns (bfd_target_elf_flavour, &elf_sym_fns);
>   
>     gnu_ifunc_fns_p = &elf_gnu_ifunc_fns;
> +
> +  add_setshow_string_cmd ("symbol-read-order", class_maintenance,
> +			  &symbol_read_order, _("\
> +Set the symbol read order."), _("		 \
> +Show the symbol read order."), nullptr,
> +			  update_symbol_read_order, nullptr,
> +			   &maintenance_set_cmdlist,
> +			   &maintenance_show_cmdlist);
> +
>   }
> diff --git a/gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c
> new file mode 100644
> index 00000000000..834c6d281ae
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c
> @@ -0,0 +1,24 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>.  */
> +
> +int var_ctf = 2;
> +
> +int
> +foo (void)
> +{
> +  return var_ctf;
> +}
> diff --git a/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c
> new file mode 100644
> index 00000000000..d9bfc29dd6d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c
> @@ -0,0 +1,26 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>.  */
> +
> +int var_dwarf = 1;
> +
> +extern int foo ();
> +
> +int
> +main (void)
> +{
> +  return var_dwarf + foo ();
> +}
> diff --git a/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp
> new file mode 100644
> index 00000000000..2827babd00d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp
> @@ -0,0 +1,54 @@
> +# 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/>.
> +
> +standard_testfile .c -2.c
> +
> +set objfile [standard_output_file ${testfile}.o]
> +set objfile2 [standard_output_file ${testfile}-2.o]
> +
> +set s1 ${srcdir}/${subdir}/${srcfile}
> +set s2 ${srcdir}/${subdir}/${srcfile2}
> +
> +set opts {}
> +lappend opts debug
> +if { [gdb_compile $s1 ${objfile} object $opts] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +set opts {}
> +lappend opts "additional_flags=-gctf"
> +if { [gdb_compile $s2 ${objfile2} object $opts] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +set opts {}
> +lappend opts "additional_flags=-Wl,-ctf-variables"
> +if { [gdb_compile [list ${objfile} ${objfile2}] $binfile executable $opts] != "" } {
> +    unsupported "failed to link"
> +    return -1
> +}
> +
> +foreach_with_prefix order {dwarf2+ctf ctf+dwarf2} {
> +    clean_restart
> +
> +    gdb_test "maint set symbol-read-order $order"
> +
> +    gdb_load $binfile
> +
> +    gdb_test "print var_dwarf" " = 1"
> +    gdb_test "print var_ctf" " = 2"
> +}