[binutils-gdb] gdb: Require psymtab before calling quick_functions in objfile

[binutils-gdb] gdb: Require psymtab before calling quick_functions in objfile

[Lancelot SIX]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=fcf8e814206f9f5a0a90b2f509fe12d841fa5163

commit fcf8e814206f9f5a0a90b2f509fe12d841fa5163
Author: Lancelot SIX <lancelot.six@amd.com>
Date:   Tue May 24 13:32:18 2022 +0100

    gdb: Require psymtab before calling quick_functions in objfile
    
    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

Diff:
---
 gdb/objfiles.h                                     | 13 +++++
 gdb/symfile-debug.c                                | 30 ++++++------
 .../fork-no-detach-follow-child-dlopen-shlib.c     | 23 +++++++++
 .../gdb.base/fork-no-detach-follow-child-dlopen.c  | 40 +++++++++++++++
 .../fork-no-detach-follow-child-dlopen.exp         | 57 ++++++++++++++++++++++
 5 files changed, 148 insertions(+), 15 deletions(-)

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 @@ public:
     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).
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