[PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[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 (just not 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 there is not mechanism to
ensure that the psymtab is laded at this point.

To fix the issue, this patch introduces a call to objfile::require_partial_symbols
in cooked_index_functions::expand_symtabs_matching.  This ensures that
we have a psymtab for the objfile we are inspecting when we need it.

I wondered if require_partial_symbols should be enforced earlier (like
in objfile::lookup_symbol for this example). However since
expand_symtabs_matching can be called from multiple places, it feels
that other code paths could lead to similar problems.

I also wanted to allow the require_partial_symbols function to log (by
setting its verbose argument to true), but this caused a regression in
gdb.python/py-events.exp (unexpected extra output):

    set variable $x = 32
    Reading symbols from .../gdb/testsuite/outputs/gdb.python/py-events/py-events-shlib.so...
    (gdb) FAIL: gdb.python/py-events.exp: do something

As a consequence this patch does not allow require_partial_symbols to
log when it lazily loads psymtab, but this can be adjusted if requested.

Regression tested on x86_64-gnu-linux.

Change-Id: I39a13a937fdbaae613a5cf68864b021000554546
---
 gdb/dwarf2/read.c                             |  3 +
 ...fork-no-detach-follow-child-dlopen-shlib.c | 23 ++++++++
 .../fork-no-detach-follow-child-dlopen.c      | 40 +++++++++++++
 .../fork-no-detach-follow-child-dlopen.exp    | 56 +++++++++++++++++++
 4 files changed, 122 insertions(+)
 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/dwarf2/read.c b/gdb/dwarf2/read.c
index 37e9587e43e..489b857459b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18559,6 +18559,9 @@ cooked_index_functions::expand_symtabs_matching
       domain_enum domain,
       enum search_domain kind)
 {
+  /* Make sure that the psymtab have been read for objfile.  */
+  objfile->require_partial_symbols (false);
+
   dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
   if (per_objfile->per_bfd->index_table == nullptr)
     return true;
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..5d8c808ddd3
--- /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 <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)
+    return -1;
+
+  return WEXITSTATUS (wstatus);
+}
+
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..d7679e5fe06
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.exp
@@ -0,0 +1,56 @@
+# 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_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 .*$::subdir/$::srcfile2:\[0-9\]+.*"
+
+    # We must also be able to display the source for che current function.
+    gdb_test "list" "return a \\+ b;.*"
+}
+
+do_test

base-commit: 455fe767086af57738678e9c02d0af068c2700aa
-- 
2.25.1

Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Tom Tromey]

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

> The new indexer's expand_symtabs_matching callback does not have a call
> to the objfile's require_partial_symbols, so there is not mechanism to
> ensure that the psymtab is laded at this point.

I wonder if this lazy loading is even worthwhile any more.
Perhaps just removing SYMFILE_NO_READ entirely is the way to go.

Tom

Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Lancelot SIX]

>> The new indexer's expand_symtabs_matching callback does not have a call
>> to the objfile's require_partial_symbols, so there is not mechanism to
>> ensure that the psymtab is laded at this point.
> 
> I wonder if this lazy loading is even worthwhile any more.
> Perhaps just removing SYMFILE_NO_READ entirely is the way to go.
> 
> Tom

Hi,

Thanks.

This is not something I initially considered since I guess that some 
benefit is expected from this (at least I guess this was the case at 
some point in time).

I did some quick digging,  and it looks like this was introduced in 
b11896a5276900e7fda0bb6b7cd9d2b31a4945b3.  However the commit message is 
short of context (or maybe this is due to some import process to git at 
some point in time, I do not really know).  Is this lazy loading made 
for cases where a process forks and the child execs just after?  In such 
case, it makes sense not to bother loading the psymtab since it will 
become unnecessary pretty soon.

I'll have a look into removing the lazy loading machinery (mostly 
SYMFILE_NO_READ and OBJF_PSYMTABS_READ related code should be what I am 
looking for) and come up with an alternative patch.

Best,
Lancelot.

Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Lancelot SIX]

Hi

Before changing anything, I wanted to have some metric to estimate the 
impact the change could have to the use case of this feature.  Having 
chatted with Pedro offlist, it looks like that this feature was 
introduced to improve cases where one launches a build under GDB (like 
gdb --args make), and wants to be dropped debugging compiler if it 
breaks down somewhere.

It is possible to achieve this with a command like:
     gdb -ex "set detach-on-fork off" \
         -ex "set schedule-multiple on" \
         -ex "set non-stop on" -ex "r&" --args /usr/bin/make

We did run something similar to this (launch a build of GDB under GDB), 
and ended up with having things slowing down gradually.  Pedro had a 
look at what "perf top" says, and it appears that we end up spending a 
significant amount of time in "update_global_location_list".  On my 
system, perf top gave me up to 55% of GDB's time there.  It looks like 
bp_locations is not cleaned as it should, and it grows way to much. on 
my system, I had 6000+ entries in the vector.

So long story short, looks like this should require some attention 
before we can evaluate if removing the lazy loading is acceptable.

Do you think we can have the fix for the regression we identified merged 
in before evaluating if the we want to keep the lazy loading?

[Thanks Pedro for having a look at this wit me]

Best,
Lancelot.

On 27/04/2022 00:44, Lancelot SIX wrote:
>>> The new indexer's expand_symtabs_matching callback does not have a call
>>> to the objfile's require_partial_symbols, so there is not mechanism to
>>> ensure that the psymtab is laded at this point.
>>
>> I wonder if this lazy loading is even worthwhile any more.
>> Perhaps just removing SYMFILE_NO_READ entirely is the way to go.
>>
>> Tom
> 
> Hi,
> 
> Thanks.
> 
> This is not something I initially considered since I guess that some 
> benefit is expected from this (at least I guess this was the case at 
> some point in time).
> 
> I did some quick digging,  and it looks like this was introduced in 
> b11896a5276900e7fda0bb6b7cd9d2b31a4945b3.  However the commit message is 
> short of context (or maybe this is due to some import process to git at 
> some point in time, I do not really know).  Is this lazy loading made 
> for cases where a process forks and the child execs just after?  In such 
> case, it makes sense not to bother loading the psymtab since it will 
> become unnecessary pretty soon.
> 
> I'll have a look into removing the lazy loading machinery (mostly 
> SYMFILE_NO_READ and OBJF_PSYMTABS_READ related code should be what I am 
> looking for) and come up with an alternative patch.
> 
> Best,
> Lancelot.

RE: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Six, Lancelot]

[AMD Official Use Only - General]

Hi Tom,

I did create a branch where I completely removed the lazy loading of partial symtabs (I can push it somewhere if this is of interest).

I have experimented with a scenario where I run `make` under GDB in a project using a debug build of Clang.  Using the lazy loading, everything runs in ~15 minutes (without parallel building), while if I deactivate the lazy loading the build takes ~50 minutes.  So the lazy loading definitely brings improvement for some scenarios.

Another test I did was to run the testsuite with the patch I propose, plus another one forcing lazy-lading as much as possible.  I do have some regressions, but most of them are linked to information messages appearing when the test does not expect them (or failing to appear when the test expect them like after using the 'file' command).  I did not investigate all the regressions, but the important point is that if I do not have the change in cooked_index_functions::expand_symtabs_matching I see GDB crash in many tests, while with this change it does not.

Best,
Lancelot.

-----Original Message-----
From: Tom Tromey <tom@tromey.com> 
Sent: 26 April 2022 20:29
To: Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>
Cc: Six, Lancelot <Lancelot.Six@amd.com>; lsix@lancelotsix.com
Subject: Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[CAUTION: External Email]

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

> The new indexer's expand_symtabs_matching callback does not have a 
> call to the objfile's require_partial_symbols, so there is not 
> mechanism to ensure that the psymtab is laded at this point.

I wonder if this lazy loading is even worthwhile any more.
Perhaps just removing SYMFILE_NO_READ entirely is the way to go.

Tom

Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Pedro Alves]

Hi Lancelot,

On 2022-05-04 18:11, Six, Lancelot via Gdb-patches wrote:
> I did create a branch where I completely removed the lazy loading of partial symtabs (I can push it somewhere if this is of interest).
> 
> I have experimented with a scenario where I run `make` under GDB in a project using a debug build of Clang.  Using the lazy loading, everything runs in ~15 minutes (without parallel building), while if I deactivate the lazy loading the build takes ~50 minutes.  So the lazy loading definitely brings improvement for some scenarios.
> 
> Another test I did was to run the testsuite with the patch I propose, plus another one forcing lazy-lading as much as possible.  I do have some regressions, but most of them are linked to information messages appearing when the test does not expect them (or failing to appear when the test expect them like after using the 'file' command).  I did not investigate all the regressions, but the important point is that if I do not have the change in cooked_index_functions::expand_symtabs_matching I see GDB crash in many tests, while with this change it does not.

Thanks for the extra testing.  With the current available data, it seems to me like the lazy loading is still
beneficial, so I think we should treat handling fixing this regression orthogonally from re-evaluating lazy
loading further, or fixing the breakpoint module leak we uncovered with the "debug make" scenario.

The fix being a one liner, and including a testcase, seems quite low risk to me.  I think we should
put it in.  Let's just give Tromey a bit more time in case he has concerns, though.

Meanwhile, please find below some nits on the testcase.  The patch LGTM with these fixed.

> +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)
> +    return -1;
> +

Should add an assert like:

  assert (WIFEXITED (status) && WEXITSTATUS (status) == 0);

Sometimes GDB bugs lead to the child crashing or something like that.

> +  return WEXITSTATUS (wstatus);

This can then be "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..d7679e5fe06
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/fork-no-detach-follow-child-dlopen.exp
> @@ -0,0 +1,56 @@
> +# 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
> +}
> +

gdb_load_shlib call missing.

> +proc do_test {} {
> +    clean_restart $::binfile
> +    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 .*$::subdir/$::srcfile2:\[0-9\]+.*"

Don't expect "$::subdir/", as it won't be there when remote-host testing.

See e.g. 160f8a8f32f5566077e4a4b13943bc7c70bc5da2, d4330bde6891c74498cea8866ce9992181798861,
79dc332ba2db30e02e816f6ecbffca548f2f6c30.

> +
> +    # We must also be able to display the source for che current function.

che -> the

> +    gdb_test "list" "return a \\+ b;.*"
> +}
> +
> +do_test
> 
> base-commit: 455fe767086af57738678e9c02d0af068c2700aa

Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Tom Tromey]

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

> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 37e9587e43e..489b857459b 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -18559,6 +18559,9 @@ cooked_index_functions::expand_symtabs_matching
>        domain_enum domain,
>        enum search_domain kind)
>  {
> +  /* Make sure that the psymtab have been read for objfile.  */
> +  objfile->require_partial_symbols (false);

Why 'false' here, when psymtab.c uses 'true'?

Also, can this bug affect the other methods in cooked_index_functions?

thanks,
Tom

Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Lancelot SIX]

Hi,

Thanks for the comments.

On 18/05/2022 21:09, Tom Tromey wrote:
> [CAUTION: External Email]
> 
>>>>>> Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index 37e9587e43e..489b857459b 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -18559,6 +18559,9 @@ cooked_index_functions::expand_symtabs_matching
>>         domain_enum domain,
>>         enum search_domain kind)
>>   {
>> +  /* Make sure that the psymtab have been read for objfile.  */
>> +  objfile->require_partial_symbols (false);
> 
> Why 'false' here, when psymtab.c uses 'true'?

This is because otherwise I see a regression in the testsuite (but this 
is probably not a good enough reason, see below).

> 
> Also, can this bug affect the other methods in cooked_index_functions?

I did not initially find other problems, at least not as clear as this 
one.  However the regression mentioned above is a tell I should have 
picked up.

In the regression, we see:

     set variable $x = 32
     Reading symbols from 
.../gdb/testsuite/outputs/gdb.python/py-events/py-events-shlib.so...
     (gdb) FAIL: gdb.python/py-events.exp: do something

This is because the symbol reading from py-events-shlib.so happens quite 
late.  If I compare with how this was done before the new indexer is 
activated (I used 600f5f702728f66ced24f8497c75c58ff442aeb6 as a 
reference), the psymbols for py-events-shlib.so are read from the 
find_pc_sect_compunit_symtab methad, which happens to be called much 
earlier.

So if I also add a objfile->require_partial_symbols call in 
cooked_index_functions::find_pc_sect_compunit_symtab, everything "looks 
good".

The various methods in psymbol_functions consistently uses 
require_partial_symbols.  This could be what should be done in various 
subclasses of quick_symbol_functions used in the new indexer.

However, I feel that it could be clearer to have this done in objfile 
before it actually calls the quick_symbol_functions. This makes it so 
that only the objfile implementation needs to take care of this (This is 
what was initially done with ALL_OBJFILE_PSYMTABS_REQUIRED when lazy 
loading was introduced).

WDYT?

I'll prepare a patch to do this shortly, including changes to address 
Pedro's comments.

Best,
Lancelot.
> 
> thanks,
> Tom