[PATCH v2 0/2] Fix regression with lazy-loading of partial symbols

[PATCH v2 0/2] Fix regression with lazy-loading of partial symbols

[Lancelot SIX]

This series is a V2 for https://sourceware.org/pipermail/gdb-patches/2022-April/188385.html.

Since V1:

- Instead of only changing cooked_index_functions::expand_symtabs_matching,
  change objfile so each call to a method of a quick_function is done after
  ensuring the partial symbols have been read (the exception being the methods
  used to actually read partial symbols).
- Added patch #2 which does some cleanup in
  psymbol_functions::require_partial_symbols.
- Addressed Pedro's comments regarding the testcase.

All feedback are welcome.

Best,
Lancelot.

---

The recent DWARF indexer rewrite introduced a regression when debugging
a forking program.

Here is a way to reproduce the issue (there might be other ways, but one
is enough and this one mimics the situation we encountered).  Consider a
program which forks, and the child loads a shared library and calls a
function in this shared library:

    if (fork () == 0)
      {
        void *solib = dlopen (some_solib, RTLD_NOW);
        void (*foo) () = dlsym (some_solib, "foo");
        foo ();
      }

Suppose that this program is compiled without debug info, but the shared
library it loads has debug info enabled.

When debugging such program with the following options:

  - set detach-on-fork off
  - set follow-fork-mode child

we see something like:

    (gdb) b foo
    Function "foo" not defined.
    Make breakpoint pending on future shared library load? (y or [n]) y
    Breakpoint 1 (foo) pending.
    (gdb) run
    Starting program: a.out
    [Attaching after process 19720 fork to child process 19723]
    [New inferior 2 (process 19723)]
    [Switching to process 19723]

    Thread 2.1 "a.out" hit Breakpoint 1, 0x00007ffff7fc3101 in foo () from .../libfoo.so
    (gdb) list

    Fatal signal: Segmentation fault
    ----- Backtrace -----
    [...]
    ---------------------

Lancelot SIX (2):
  gdb: Require psymtab before calling quick_functions in objfile
  gdb: Simplify psymbol_functions::require_partial_symbols

 gdb/objfiles.h                                | 10 ++++
 gdb/psymtab.c                                 |  2 +-
 gdb/symfile-debug.c                           | 30 +++++-----
 ...fork-no-detach-follow-child-dlopen-shlib.c | 23 ++++++++
 .../fork-no-detach-follow-child-dlopen.c      | 40 +++++++++++++
 .../fork-no-detach-follow-child-dlopen.exp    | 57 +++++++++++++++++++
 6 files changed, 146 insertions(+), 16 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen-shlib.c
 create mode 100644 gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.c
 create mode 100644 gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.exp


base-commit: c4dd8eb523fae5c9d312f4b7b21377eec66e70c3
-- 
2.25.1

[PATCH v2 1/2] gdb: Require psymtab before calling quick_functions in objfile

[Lancelot SIX]

The recent DWARF indexer rewrite introduced a regression when debugging
a forking program.

Here is a way to reproduce the issue (there might be other ways, but one
is enough and this one mimics the situation we encountered).  Consider a
program which forks, and the child loads a shared library and calls a
function in this shared library:

    if (fork () == 0)
      {
        void *solib = dlopen (some_solib, RTLD_NOW);
        void (*foo) () = dlsym (some_solib, "foo");
        foo ();
      }

Suppose that this program is compiled without debug info, but the shared
library it loads has debug info enabled.

When debugging such program with the following options:

  - set detach-on-fork off
  - set follow-fork-mode child

we see something like:

    (gdb) b foo
    Function "foo" not defined.
    Make breakpoint pending on future shared library load? (y or [n]) y
    Breakpoint 1 (foo) pending.
    (gdb) run
    Starting program: a.out
    [Attaching after process 19720 fork to child process 19723]
    [New inferior 2 (process 19723)]
    [Switching to process 19723]

    Thread 2.1 "a.out" hit Breakpoint 1, 0x00007ffff7fc3101 in foo () from .../libfoo.so
    (gdb) list

    Fatal signal: Segmentation fault
    ----- Backtrace -----
    0x55a278f77d76 gdb_internal_backtrace_1
            ../../gdb/bt-utils.c:122
    0x55a278f77f83 _Z22gdb_internal_backtracev
            ../../gdb/bt-utils.c:168
    0x55a27940b83b handle_fatal_signal
            ../../gdb/event-top.c:914
    0x55a27940bbb1 handle_sigsegv
            ../../gdb/event-top.c:987
    0x7effec0343bf ???
            /build/glibc-sMfBJT/glibc-2.31/nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
    0x55a27924c9d3 _ZNKSt15__uniq_ptr_implI18dwarf2_per_cu_data26dwarf2_per_cu_data_deleterE6_M_ptrEv
            /usr/include/c++/9/bits/unique_ptr.h:154
    0x55a279248bc9 _ZNKSt10unique_ptrI18dwarf2_per_cu_data26dwarf2_per_cu_data_deleterE3getEv
            /usr/include/c++/9/bits/unique_ptr.h:361
    0x55a2792ae718 _ZN27dwarf2_base_index_functions23find_last_source_symtabEP7objfile
            ../../gdb/dwarf2/read.c:3164
    0x55a279afb93e _ZN7objfile23find_last_source_symtabEv
            ../../gdb/symfile-debug.c:139
    0x55a279aa3040 _Z20select_source_symtabP6symtab
            ../../gdb/source.c:365
    0x55a279aa22a1 _Z34set_default_source_symtab_and_linev
            ../../gdb/source.c:268
    0x55a27903c44c list_command
            ../../gdb/cli/cli-cmds.c:1185
    0x55a279051233 do_simple_func
            ../../gdb/cli/cli-decode.c:95
    0x55a27905f221 _Z8cmd_funcP16cmd_list_elementPKci
            ../../gdb/cli/cli-decode.c:2514
    0x55a279c3b0ba _Z15execute_commandPKci
            ../../gdb/top.c:660
    0x55a27940a6c3 _Z15command_handlerPKc
            ../../gdb/event-top.c:598
    0x55a27940b032 _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
            ../../gdb/event-top.c:797
    0x55a279caf401 tui_command_line_handler
            ../../gdb/tui/tui-interp.c:278
    0x55a279409098 gdb_rl_callback_handler
            ../../gdb/event-top.c:230
    0x55a279ed5df2 rl_callback_read_char
            ../../../readline/readline/callback.c:281
    0x55a279408bd8 gdb_rl_callback_read_char_wrapper_noexcept
            ../../gdb/event-top.c:188
    0x55a279408de7 gdb_rl_callback_read_char_wrapper
            ../../gdb/event-top.c:205
    0x55a27940a061 _Z19stdin_event_handleriPv
            ../../gdb/event-top.c:525
    0x55a27a23771e handle_file_event
            ../../gdbsupport/event-loop.cc:574
    0x55a27a237f5f gdb_wait_for_event
            ../../gdbsupport/event-loop.cc:700
    0x55a27a235d81 _Z16gdb_do_one_eventv
            ../../gdbsupport/event-loop.cc:237
    0x55a2796c2ef0 start_event_loop
            ../../gdb/main.c:418
    0x55a2796c3217 captured_command_loop
            ../../gdb/main.c:478
    0x55a2796c717b captured_main
            ../../gdb/main.c:1340
    0x55a2796c7217 _Z8gdb_mainP18captured_main_args
            ../../gdb/main.c:1355
    0x55a278d0b381 main
            ../../gdb/gdb.c:32
    ---------------------
    A fatal error internal to GDB has been detected, further
    debugging is not possible.  GDB will now terminate.

    This is a bug, please report it.  For instructions, see:
    <https://www.gnu.org/software/gdb/bugs/>.

The first issue observed is in the message printed when hitting the
breakpoint.  It says that there was a break in the .so file as if there
was no debug info associated with it, but there is.  Later, if we try to
display the source where the execution stopped, we have a segfault.

Note that not having the debug info on the main binary is not strictly
required to encounter some issues, it only is to encounter the segfault.
If the main binary has debug information, GDB shows some source form the
main binary, unrelated to where we stopped.

The core of the issue is that GDB never loads the psymtab for the
library.  It is not loaded when we first see the .so because in case of
detach-on-fork off, follow-fork-mode child, infrun.c sets
child_inf->symfile_flags = SYMFILE_NO_READ to delay the psymtab loading
as much as possible.  If we compare to what was done to handle this
before the new indexer was activated, the psymatb construction for the
shared library was done under
psymbol_functions::expand_symtabs_matching:

    bool
    psymbol_functions::expand_symtabs_matching (...)
    {
        for (partial_symtab *ps : require_partial_symbols (objfile))
        ...
    }

The new indexer's expand_symtabs_matching callback does not have a call
to the objfile's require_partial_symbols, so if the partial symbol table
is not loaded at this point, there is no mechanism to fix this.

Instead of requiring each implementation of the quick_functions to check
that partial symbols have been read, I think it is safer to enforce this
when calling the quick functions.  The general pattern for calling the
quick functions is:

    for (auto *iter : qf)
      iter->the_actual_method_call (...)

This patch proposes to wrap the access of the `qf` field with an accessor
which ensures that partial symbols have been read before iterating:
qf_require_partial_symbols.  All calls to quick functions are updated
except:

- quick_functions::dump
- quick_functions::read_partial_symbols (from
  objfile::require_partial_symbols)
- quick_functions::can_lazily_read_symbols and quick_functions::has_symbols
  (from objfile::has_partial_symbols)

Regression tested on x86_64-gnu-linux.

Change-Id: I39a13a937fdbaae613a5cf68864b021000554546
---
 gdb/objfiles.h                                | 10 ++++
 gdb/symfile-debug.c                           | 30 +++++-----
 ...fork-no-detach-follow-child-dlopen-shlib.c | 23 ++++++++
 .../fork-no-detach-follow-child-dlopen.c      | 40 +++++++++++++
 .../fork-no-detach-follow-child-dlopen.exp    | 57 +++++++++++++++++++
 5 files changed, 145 insertions(+), 15 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen-shlib.c
 create mode 100644 gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.c
 create mode 100644 gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.exp

diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index cb7a1357cfe..9b205c02831 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -487,6 +487,16 @@ struct objfile
     return per_bfd->gdbarch;
   }
 
+  /* Ensure that partial symbols have been read and return the "quick" (aka
+     partial) symbol functions for this symbol reader.  */
+
+  const std::forward_list<quick_symbol_functions_up> &
+    qf_require_partial_symbols ()
+      {
+	this->require_partial_symbols (true);
+	return qf;
+      }
+
   /* Return true if OBJFILE has partial symbols.  */
 
   bool has_partial_symbols ();
diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
index bbbcbcabfde..2af04433444 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -109,7 +109,7 @@ objfile::has_unexpanded_symtabs ()
 		objfile_debug_name (this));
 
   bool result = false;
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     {
       if (iter->has_unexpanded_symtabs (this))
 	{
@@ -134,7 +134,7 @@ objfile::find_last_source_symtab ()
     gdb_printf (gdb_stdlog, "qf->find_last_source_symtab (%s)\n",
 		objfile_debug_name (this));
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     {
       retval = iter->find_last_source_symtab (this);
       if (retval != nullptr)
@@ -155,7 +155,7 @@ objfile::forget_cached_source_info ()
     gdb_printf (gdb_stdlog, "qf->forget_cached_source_info (%s)\n",
 		objfile_debug_name (this));
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     iter->forget_cached_source_info (this);
 }
 
@@ -202,7 +202,7 @@ objfile::map_symtabs_matching_filename
     return result;
   };
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     {
       if (!iter->expand_symtabs_matching (this,
 					  match_one_filename,
@@ -271,7 +271,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
     return true;
   };
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     {
       if (!iter->expand_symtabs_matching (this,
 					  nullptr,
@@ -302,7 +302,7 @@ objfile::print_stats (bool print_bcache)
     gdb_printf (gdb_stdlog, "qf->print_stats (%s, %d)\n",
 		objfile_debug_name (this), print_bcache);
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     iter->print_stats (this, print_bcache);
 }
 
@@ -328,7 +328,7 @@ objfile::expand_symtabs_for_function (const char *func_name)
   lookup_name_info base_lookup (func_name, symbol_name_match_type::FULL);
   lookup_name_info lookup_name = base_lookup.make_ignore_params ();
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     iter->expand_symtabs_matching (this,
 				   nullptr,
 				   &lookup_name,
@@ -347,7 +347,7 @@ objfile::expand_all_symtabs ()
     gdb_printf (gdb_stdlog, "qf->expand_all_symtabs (%s)\n",
 		objfile_debug_name (this));
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     iter->expand_all_symtabs (this);
 }
 
@@ -365,7 +365,7 @@ objfile::expand_symtabs_with_fullname (const char *fullname)
     return filename_cmp (basenames ? basename : fullname, filename) == 0;
   };
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     iter->expand_symtabs_matching (this,
 				   file_matcher,
 				   nullptr,
@@ -390,7 +390,7 @@ objfile::expand_matching_symbols
 		domain_name (domain), global,
 		host_address_to_string (ordered_compare));
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     iter->expand_matching_symbols (this, name, domain, global,
 				   ordered_compare);
 }
@@ -417,7 +417,7 @@ objfile::expand_symtabs_matching
 		host_address_to_string (&expansion_notify),
 		search_domain_name (kind));
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     if (!iter->expand_symtabs_matching (this, file_matcher, lookup_name,
 					symbol_matcher, expansion_notify,
 					search_flags, domain, kind))
@@ -442,7 +442,7 @@ objfile::find_pc_sect_compunit_symtab (struct bound_minimal_symbol msymbol,
 		host_address_to_string (section),
 		warn_if_readin);
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     {
       retval = iter->find_pc_sect_compunit_symtab (this, msymbol, pc, section,
 						   warn_if_readin);
@@ -470,7 +470,7 @@ objfile::map_symbol_filenames (gdb::function_view<symbol_filename_ftype> fun,
 		objfile_debug_name (this),
 		need_fullname);
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     iter->map_symbol_filenames (this, fun, need_fullname);
 }
 
@@ -484,7 +484,7 @@ objfile::find_compunit_symtab_by_address (CORE_ADDR address)
 		hex_string (address));
 
   struct compunit_symtab *result = NULL;
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     {
       result = iter->find_compunit_symtab_by_address (this, address);
       if (result != nullptr)
@@ -509,7 +509,7 @@ objfile::lookup_global_symbol_language (const char *name,
   enum language result = language_unknown;
   *symbol_found_p = false;
 
-  for (const auto &iter : qf)
+  for (const auto &iter : qf_require_partial_symbols ())
     {
       result = iter->lookup_global_symbol_language (this, name, domain,
 						    symbol_found_p);
diff --git a/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen-shlib.c b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen-shlib.c
new file mode 100644
index 00000000000..c276fef24f3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen-shlib.c
@@ -0,0 +1,23 @@
+/* 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 add (int a, int b);
+int
+add (int a, int b)
+{
+  return a + b;
+}
diff --git a/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.c b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.c
new file mode 100644
index 00000000000..b366956df03
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.c
@@ -0,0 +1,40 @@
+/* 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/>.  */
+
+#include <assert.h>
+#include <dlfcn.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+int
+main (void)
+{
+  pid_t pid = fork ();
+  if (pid == 0)
+    {
+      void *shlib = dlopen (SHLIB_PATH, RTLD_NOW);
+      int (*add) (int, int) = dlsym (shlib, "add");
+
+      return add (-2, 2);
+    }
+
+  int wstatus;
+  if (waitpid (pid, &wstatus, 0) == -1)
+    assert (WIFEXITED (wstatus) && WEXITSTATUS (wstatus) == 0);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.exp b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.exp
new file mode 100644
index 00000000000..e8b61450b47
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.exp
@@ -0,0 +1,57 @@
+# 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/>.
+
+# This test is a regression test for when GDB debugs a program with:
+# - set follow-fork-mode child
+# - set detach-on-fork off
+#
+# If the program forks, and the child loads a shared library (via
+# dlopen for example), GDB should still load the symtab for this objfile.
+# When a breakpoint is hit in this file, GDB should display the location
+# in the source of the shlib, and "list" should display the source where
+# the program stopped.
+
+if { [skip_shlib_tests] } {
+    return 0
+}
+
+standard_testfile .c -shlib.c
+set shlib_path [standard_output_file ${testfile}-lib.so]
+
+if { [gdb_compile_shlib $srcdir/$subdir/$srcfile2 $shlib_path {debug}] != "" } {
+    return
+}
+
+set opts [list shlib_load additional_flags=-DSHLIB_PATH="${shlib_path}"]
+if { [build_executable "failed to prepare" ${testfile} ${srcfile} $opts] } {
+    return
+}
+
+proc do_test {} {
+    clean_restart $::binfile
+    gdb_load_shlib $::shlib_path
+    gdb_test_no_output "set follow-fork-mode child"
+    gdb_test_no_output "set detach-on-fork off"
+
+    runto "add" allow-pending
+
+    # Since we have debug info in the shlib, we should have the file name available.
+    gdb_test "frame" "add \(.*\) at .*$::srcfile2:\[0-9\]+.*"
+
+    # We must also be able to display the source for the current function.
+    gdb_test "list" "return a \\+ b;.*"
+}
+
+do_test
-- 
2.25.1

Re: [PATCH v2 1/2] gdb: Require psymtab before calling quick_functions in objfile

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

Lancelot> we see something like:
Lancelot> This patch proposes to wrap the access of the `qf` field with an accessor
Lancelot> which ensures that partial symbols have been read before iterating:
Lancelot> qf_require_partial_symbols.  All calls to quick functions are updated
Lancelot> except:

Thank you for the patch.
I have one small nit.

Lancelot> +  /* Ensure that partial symbols have been read and return the "quick" (aka
Lancelot> +     partial) symbol functions for this symbol reader.  */
Lancelot> +
Lancelot> +  const std::forward_list<quick_symbol_functions_up> &
Lancelot> +    qf_require_partial_symbols ()
Lancelot> +      {
Lancelot> +	this->require_partial_symbols (true);
Lancelot> +	return qf;
Lancelot> +      }
Lancelot> +
 
It seems like this could be private instead of public.

It's ok with that change, you don't need to re-send it.

Tom

Re: [PATCH v2 1/2] gdb: Require psymtab before calling quick_functions in objfile

[Lancelot SIX]

On 20/05/2022 16:51, Tom Tromey wrote:
> [CAUTION: External Email]
> 
>>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Lancelot> we see something like:
> Lancelot> This patch proposes to wrap the access of the `qf` field with an accessor
> Lancelot> which ensures that partial symbols have been read before iterating:
> Lancelot> qf_require_partial_symbols.  All calls to quick functions are updated
> Lancelot> except:
> 
> Thank you for the patch.
> I have one small nit.
> 
> Lancelot> +  /* Ensure that partial symbols have been read and return the "quick" (aka
> Lancelot> +     partial) symbol functions for this symbol reader.  */
> Lancelot> +
> Lancelot> +  const std::forward_list<quick_symbol_functions_up> &
> Lancelot> +    qf_require_partial_symbols ()
> Lancelot> +      {
> Lancelot> +     this->require_partial_symbols (true);
> Lancelot> +     return qf;
> Lancelot> +      }
> Lancelot> +
> 
> It seems like this could be private instead of public.
> 
> It's ok with that change, you don't need to re-send it.
> 
> Tom

Hi,

Thanks for the review.

I have changed the declaration locally as follows (I did some back and 
forth on whether it should stay grouped with the other methods of 
ojbfile bracketed between a pritave:/public: pair or if it should go in 
a private section at the end of the struct — I finally opted for former).

I'll keep this and push it with the revised version of patch #2 once 
approved.

Best,
Lancelot.

---

diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index cb7a1357cfe..9da12ff12e0 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -612,6 +612,19 @@ struct objfile
      this->section_offsets[idx] = offset;
    }

+private:
+
+  /* Ensure that partial symbols have been read and return the "quick" (aka
+     partial) symbol functions for this symbol reader.  */
+  const std::forward_list<quick_symbol_functions_up> &
+  qf_require_partial_symbols ()
+  {
+    this->require_partial_symbols (true);
+    return qf;
+  }
+
+public:
+
    /* The object file's original name as specified by the user,
       made absolute, and tilde-expanded.  However, it is not canonicalized
       (i.e., it has not been passed through gdb_realpath).

Re: [PATCH v2 1/2] gdb: Require psymtab before calling quick_functions in objfile

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <Lancelot.Six@amd.com> writes:

Lancelot> I have changed the declaration locally as follows (I did some back and
Lancelot> forth on whether it should stay grouped with the other methods of 
Lancelot> ojbfile bracketed between a pritave:/public: pair or if it should go
Lancelot> in a private section at the end of the struct — I finally opted for
Lancelot> former).

Yeah, objfile is a bit messy, someday we can try to clean it up.  I
think what you did is fine.

Tom

[PATCH v2 2/2] gdb: Simplify psymbol_functions::require_partial_symbols

[Lancelot SIX]

The previous patch ensured that partial symbols are read before calling
most of the quick_function's methods.

The psymbol_functions class has the require_partial_symbols method which
serves this exact purpose.  This method does not need to try to read partial
symbols anymore, but it can instead assert that any partial symbol have
been read at this point.

Regression tested on x86_64-linux.
---
 gdb/psymtab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 402d6085fe6..2bf6cbc5848 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -84,7 +84,7 @@ psymtab_storage::install_psymtab (partial_symtab *pst)
 psymtab_storage::partial_symtab_range
 psymbol_functions::require_partial_symbols (struct objfile *objfile)
 {
-  objfile->require_partial_symbols (true);
+  gdb_assert ((objfile->flags & OBJF_PSYMTABS_READ) != 0);
   return m_partial_symtabs->range ();
 }
 
-- 
2.25.1

Re: [PATCH v2 2/2] gdb: Simplify psymbol_functions::require_partial_symbols

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

Lancelot> The psymbol_functions class has the require_partial_symbols method which
Lancelot> serves this exact purpose.  This method does not need to try to read partial
Lancelot> symbols anymore, but it can instead assert that any partial symbol have
Lancelot> been read at this point.

Looks good, thank you.

Tom

Re: [PATCH v2 2/2] gdb: Simplify psymbol_functions::require_partial_symbols

[Simon Marchi]

On 2022-05-19 14:45, Lancelot SIX via Gdb-patches wrote:
> The previous patch ensured that partial symbols are read before calling
> most of the quick_function's methods.
>
> The psymbol_functions class has the require_partial_symbols method which
> serves this exact purpose.  This method does not need to try to read partial
> symbols anymore, but it can instead assert that any partial symbol have
> been read at this point.
>
> Regression tested on x86_64-linux.
> ---
>  gdb/psymtab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/psymtab.c b/gdb/psymtab.c
> index 402d6085fe6..2bf6cbc5848 100644
> --- a/gdb/psymtab.c
> +++ b/gdb/psymtab.c
> @@ -84,7 +84,7 @@ psymtab_storage::install_psymtab (partial_symtab *pst)
>  psymtab_storage::partial_symtab_range
>  psymbol_functions::require_partial_symbols (struct objfile *objfile)
>  {
> -  objfile->require_partial_symbols (true);
> +  gdb_assert ((objfile->flags & OBJF_PSYMTABS_READ) != 0);
>    return m_partial_symtabs->range ();
>  }

It sounds like the method should be renamed then.  The "require" meant
that it would read the psymbols if needed.  Now it's really just a
getter, so it could probably be named just "partial_symbols".

Simon

[PATCH v3 2/2] gdb: Change psymbol_functions::require_partial_symbols to partial_symbols

[Lancelot SIX]

Hi,

Here is an updated (V3) version of the last patch of the series which
renames require_partial_symbols into partial_symbols and updates the
relevant comments.

Best,
Lancelot.

---

The previous patch ensured that partial symbols are read before calling
most of the quick_function's methods.

The psymbol_functions class has the require_partial_symbols method which
serves this exact purpose, and does not need to do it anymore.

This patch renames this method to partial_symbols and makes it an accessor
which asserts that partial symbols have been read at this point.

Regression tested on x86_64-linux.
---
 gdb/psympriv.h |  5 ++---
 gdb/psymtab.c  | 37 +++++++++++++++++--------------------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index 677a57edee8..a7ba82a7604 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -553,9 +553,8 @@ struct psymbol_functions : public quick_symbol_functions
     m_psymbol_map.clear ();
   }
 
-  /* Ensure the partial symbols for OBJFILE have been loaded.  Return
-     a range adapter for the psymtabs.  */
-  psymtab_storage::partial_symtab_range require_partial_symbols
+  /* Return a range adapter for the psymtabs.  */
+  psymtab_storage::partial_symtab_range partial_symbols
        (struct objfile *objfile);
 
   /* Return the partial symbol storage associated with this
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 402d6085fe6..e6c9526cc4f 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -76,15 +76,12 @@ psymtab_storage::install_psymtab (partial_symtab *pst)
 
 \f
 
-/* Ensure that the partial symbols for OBJFILE have been loaded.  This
-   will print a message when symbols are loaded.  This function
-   returns a range adapter suitable for iterating over the psymtabs of
-   OBJFILE.  */
+/* See psympriv.h.  */
 
 psymtab_storage::partial_symtab_range
-psymbol_functions::require_partial_symbols (struct objfile *objfile)
+psymbol_functions::partial_symbols (struct objfile *objfile)
 {
-  objfile->require_partial_symbols (true);
+  gdb_assert ((objfile->flags & OBJF_PSYMTABS_READ) != 0);
   return m_partial_symtabs->range ();
 }
 
@@ -236,7 +233,7 @@ psymbol_functions::find_pc_sect_psymtab (struct objfile *objfile,
      its CUs may be missing in PSYMTABS_ADDRMAP as they may be varying
      debug info type in single OBJFILE.  */
 
-  for (partial_symtab *pst : require_partial_symbols (objfile))
+  for (partial_symtab *pst : partial_symbols (objfile))
     if (!pst->psymtabs_addrmap_supported
 	&& pc >= pst->text_low (objfile) && pc < pst->text_high (objfile))
       {
@@ -358,7 +355,7 @@ psymbol_functions::lookup_global_symbol_language (struct objfile *objfile,
 
   lookup_name_info lookup_name (name, symbol_name_match_type::FULL);
 
-  for (partial_symtab *ps : require_partial_symbols (objfile))
+  for (partial_symtab *ps : partial_symbols (objfile))
     {
       struct partial_symbol *psym;
       if (ps->readin_p (objfile))
@@ -607,7 +604,7 @@ psymbol_functions::find_last_source_symtab (struct objfile *ofp)
 {
   struct partial_symtab *cs_pst = NULL;
 
-  for (partial_symtab *ps : require_partial_symbols (ofp))
+  for (partial_symtab *ps : partial_symbols (ofp))
     {
       const char *name = ps->filename;
       int len = strlen (name);
@@ -643,7 +640,7 @@ psymbol_functions::find_last_source_symtab (struct objfile *ofp)
 void
 psymbol_functions::forget_cached_source_info (struct objfile *objfile)
 {
-  for (partial_symtab *pst : require_partial_symbols (objfile))
+  for (partial_symtab *pst : partial_symbols (objfile))
     {
       if (pst->fullname != NULL)
 	{
@@ -839,7 +836,7 @@ psymbol_functions::print_stats (struct objfile *objfile, bool print_bcache)
 		    n_psyms);
 
       i = 0;
-      for (partial_symtab *ps : require_partial_symbols (objfile))
+      for (partial_symtab *ps : partial_symbols (objfile))
 	{
 	  if (!ps->readin_p (objfile))
 	    i++;
@@ -884,7 +881,7 @@ psymbol_functions::dump (struct objfile *objfile)
 void
 psymbol_functions::expand_all_symtabs (struct objfile *objfile)
 {
-  for (partial_symtab *psymtab : require_partial_symbols (objfile))
+  for (partial_symtab *psymtab : partial_symbols (objfile))
     psymtab_to_symtab (objfile, psymtab);
 }
 
@@ -897,7 +894,7 @@ psymbol_functions::map_symbol_filenames
       gdb::function_view<symbol_filename_ftype> fun,
       bool need_fullname)
 {
-  for (partial_symtab *ps : require_partial_symbols (objfile))
+  for (partial_symtab *ps : partial_symbols (objfile))
     {
       const char *fullname;
 
@@ -958,7 +955,7 @@ psymbol_functions::expand_matching_symbols
    int global,
    symbol_compare_ftype *ordered_compare)
 {
-  for (partial_symtab *ps : require_partial_symbols (objfile))
+  for (partial_symtab *ps : partial_symbols (objfile))
     {
       QUIT;
       if (!ps->readin_p (objfile)
@@ -1093,7 +1090,7 @@ psymbol_functions::expand_symtabs_matching
    enum search_domain search)
 {
   /* Clear the search flags.  */
-  for (partial_symtab *ps : require_partial_symbols (objfile))
+  for (partial_symtab *ps : partial_symbols (objfile))
     ps->searched_flag = PST_NOT_SEARCHED;
 
   gdb::optional<lookup_name_info> psym_lookup_name;
@@ -1161,7 +1158,7 @@ psymbol_functions::has_symbols (struct objfile *objfile)
 bool
 psymbol_functions::has_unexpanded_symtabs (struct objfile *objfile)
 {
-  for (partial_symtab *psymtab : require_partial_symbols (objfile))
+  for (partial_symtab *psymtab : partial_symbols (objfile))
     {
       /* Is this already expanded?  */
       if (psymtab->readin_p (objfile))
@@ -1209,7 +1206,7 @@ psymbol_functions::find_compunit_symtab_by_address (struct objfile *objfile,
     {
       std::set<CORE_ADDR> seen_addrs;
 
-      for (partial_symtab *pst : require_partial_symbols (objfile))
+      for (partial_symtab *pst : partial_symbols (objfile))
 	{
 	  fill_psymbol_map (objfile, pst,
 			    &seen_addrs,
@@ -1594,7 +1591,7 @@ maintenance_print_psymbols (const char *args, int from_tty)
 	    }
 	  else
 	    {
-	      for (partial_symtab *ps : psf->require_partial_symbols (objfile))
+	      for (partial_symtab *ps : psf->partial_symbols (objfile))
 		{
 		  int print_for_source = 0;
 
@@ -1665,7 +1662,7 @@ maintenance_info_psymtabs (const char *regexp, int from_tty)
 	      = dynamic_cast<psymbol_functions *> (iter.get ());
 	    if (psf == nullptr)
 	      continue;
-	    for (partial_symtab *psymtab : psf->require_partial_symbols (objfile))
+	    for (partial_symtab *psymtab : psf->partial_symbols (objfile))
 	      {
 		QUIT;
 
@@ -1772,7 +1769,7 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
 	  if (psf == nullptr)
 	    continue;
 
-	  for (partial_symtab *ps : psf->require_partial_symbols (objfile))
+	  for (partial_symtab *ps : psf->partial_symbols (objfile))
 	    {
 	      struct gdbarch *gdbarch = objfile->arch ();
 
-- 
2.25.1

Re: [PATCH v3 2/2] gdb: Change psymbol_functions::require_partial_symbols to partial_symbols

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

Lancelot> Here is an updated (V3) version of the last patch of the series which
Lancelot> renames require_partial_symbols into partial_symbols and updates the
Lancelot> relevant comments.

FWIW this looks good to me.

Tom

Re: [PATCH v3 2/2] gdb: Change psymbol_functions::require_partial_symbols to partial_symbols

[Lancelot SIX]

Hi,

Thanks.

I just pushed the 2 patches.

Best,
Lancelot.

On 26/05/2022 18:57, Tom Tromey wrote:
> [CAUTION: External Email]
> 
>>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Lancelot> Here is an updated (V3) version of the last patch of the series which
> Lancelot> renames require_partial_symbols into partial_symbols and updates the
> Lancelot> relevant comments.
> 
> FWIW this looks good to me.
> 
> Tom