public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Tom de Vries <tdevries@suse.de>
To: gdb-patches@sourceware.org
Subject: [PATCH 2/3] [RFC][gdb/symtab] Add maint set symbol-read-order
Date: Thu, 22 Sep 2022 17:20:41 +0200	[thread overview]
Message-ID: <20220922152042.21914-2-tdevries@suse.de> (raw)
In-Reply-To: <20220922152042.21914-1-tdevries@suse.de>

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?

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


  reply	other threads:[~2022-09-22 15:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-22 15:20 [PATCH 1/3] [gdb/symtab] Factor out elf_symfile_read_dwarf2 Tom de Vries
2022-09-22 15:20 ` Tom de Vries [this message]
2022-09-26 10:43   ` [PATCH 2/3] [RFC][gdb/symtab] Add maint set symbol-read-order Bruno Larsen
2023-02-24 12:42     ` Tom de Vries
2022-09-22 15:20 ` [PATCH 3/3] [gdb/testsuite] Fix ctf test-cases on openSUSE Tumbleweed Tom de Vries
2022-10-12 15:11   ` Tom de Vries
2022-10-12 15:08 ` [commited][PATCH 1/3] [gdb/symtab] Factor out elf_symfile_read_dwarf2 Tom de Vries

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220922152042.21914-2-tdevries@suse.de \
    --to=tdevries@suse.de \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).