[PATCH] Fix "nosharedlibrary + continue + shared lib event" crash

[PATCH] Fix "nosharedlibrary + continue + shared lib event" crash

[Pedro Alves]

GDB misbehaves if you run the "nosharelibrary" command, continue
execution, and then the program hits the shared library event
breakpoint.  On my system it aborts like this:

 (gdb) nosharedlibrary
 (gdb) c
 Continuing.
 pure virtual method called
 terminate called without an active exception
 Aborted (core dumped)

Though it's really undefined behavior territory, caused by deferencing
a dangling solib event probe pointer.

I've observed this by running "nosharedlibrary" when stopped at the
entry point, but it should happen at any other point, if the program
does a dlopen/dlclose after.

The fix is to discard an objfile's probes from the svr4 probes table
when an objfile is about to be released.

New test included, works with both native and gdbserver testing.

Valgrind log:

 (gdb) starti
 (gdb) nosharedlibrary
 (gdb) c
 Continuing.
 ==24895== Invalid read of size 8
 ==24895==    at 0x89E5FB: solib_event_probe_action(probe_and_action*) (solib-svr4.c:1735)
 ==24895==    by 0x89E95A: svr4_handle_solib_event() (solib-svr4.c:1872)
 ==24895==    by 0x8A7198: handle_solib_event() (solib.c:1274)
 ==24895==    by 0x4E3407: bpstat_stop_status(address_space const*, unsigned long, thread_info*, target_waitstatus const*, bpstats*) (breakpoint.c:5407)
 ==24895==    by 0x721F41: handle_signal_stop(execution_control_state*) (infrun.c:5685)
 ==24895==    by 0x720B11: handle_inferior_event(execution_control_state*) (infrun.c:5129)
 ==24895==    by 0x71DD93: fetch_inferior_event(void*) (infrun.c:3748)
 ==24895==    by 0x7059C3: inferior_event_handler(inferior_event_type, void*) (inf-loop.c:43)
 ==24895==    by 0x874DF0: remote_async_serial_handler(serial*, void*) (remote.c:14039)
 ==24895==    by 0x894101: run_async_handler_and_reschedule(serial*) (ser-base.c:137)
 ==24895==    by 0x8941E6: fd_event(int, void*) (ser-base.c:188)
 ==24895==    by 0x67AFEF: handle_file_event(file_handler*, int) (event-loop.c:732)
 ==24895==  Address 0x18b63860 is 0 bytes inside a block of size 136 free'd
 ==24895==    at 0x4C2E616: operator delete(void*, unsigned long) (vg_replace_malloc.c:585)
 ==24895==    by 0x8C6A12: stap_probe::~stap_probe() (stap-probe.c:124)
 ==24895==    by 0x66F7DB: probe_key_free(bfd*, void*) (elfread.c:1382)
 ==24895==    by 0x69B705: bfdregistry_callback_adaptor(void (*)(registry_container*, void*), registry_container*, void*) (gdb_bfd.c:131)
 ==24895==    by 0x855A57: registry_clear_data(registry_data_registry*, void (*)(void (*)(registry_container*, void*), registry_container*, void*), registry_container*, registry_fields*) (registry.c:79)
 ==24895==    by 0x855B01: registry_container_free_data(registry_data_registry*, void (*)(void (*)(registry_container*, void*), registry_container*, void*), registry_container*, registry_fields*) (registry.c:92)
 ==24895==    by 0x69B783: bfd_free_data(bfd*) (gdb_bfd.c:131)
 ==24895==    by 0x69C4BA: gdb_bfd_unref(bfd*) (gdb_bfd.c:609)
 ==24895==    by 0x7CC33F: objfile::~objfile() (objfiles.c:651)
 ==24895==    by 0x7CD559: objfile_purge_solibs() (objfiles.c:1021)
 ==24895==    by 0x8A7132: no_shared_libraries(char const*, int) (solib.c:1252)
 ==24895==    by 0x548E3D: do_const_cfunc(cmd_list_element*, char const*, int) (cli-decode.c:106)
 ==24895==  Block was alloc'd at
 ==24895==    at 0x4C2D42A: operator new(unsigned long) (vg_replace_malloc.c:334)
 ==24895==    by 0x8C527C: handle_stap_probe(objfile*, sdt_note*, std::vector<probe*, std::allocator<probe*> >*, unsigned long) (stap-probe.c:1561)
 ==24895==    by 0x8C5535: stap_static_probe_ops::get_probes(std::vector<probe*, std::allocator<probe*> >*, objfile*) const (stap-probe.c:1656)
 ==24895==    by 0x66F71B: elf_get_probes(objfile*) (elfread.c:1365)
 ==24895==    by 0x7EDD85: find_probes_in_objfile(objfile*, char const*, char const*) (probe.c:227)
 ==24895==    by 0x4DF382: create_longjmp_master_breakpoint() (breakpoint.c:3275)
 ==24895==    by 0x4F6562: breakpoint_re_set() (breakpoint.c:13828)
 ==24895==    by 0x8A66AA: solib_add(char const*, int, int) (solib.c:1010)
 ==24895==    by 0x89F7C6: enable_break(svr4_info*, int) (solib-svr4.c:2360)
 ==24895==    by 0x8A104C: svr4_solib_create_inferior_hook(int) (solib-svr4.c:2992)
 ==24895==    by 0x8A70B9: solib_create_inferior_hook(int) (solib.c:1215)
 ==24895==    by 0x70C073: post_create_inferior(target_ops*, int) (infcmd.c:467)
 ==24895==
 pure virtual method called
 terminate called without an active exception
 ==24895==
 ==24895== Process terminating with default action of signal 6 (SIGABRT): dumping core
 ==24895==    at 0x7CF3750: raise (raise.c:51)
 ==24895==    by 0x7CF4D30: abort (abort.c:79)
 ==24895==    by 0xB008F4: __gnu_cxx::__verbose_terminate_handler() (in build/gdb/gdb)
 ==24895==    by 0xAFF845: __cxxabiv1::__terminate(void (*)()) (in build/gdb/gdb)
 ==24895==    by 0xAFF890: std::terminate() (in build/gdb/gdb)
 ==24895==    by 0xAFF95E: __cxa_pure_virtual (in build/gdb/gdb)
 ==24895==    by 0x89E610: solib_event_probe_action(probe_and_action*) (solib-svr4.c:1735)
 ==24895==    by 0x89E95A: svr4_handle_solib_event() (solib-svr4.c:1872)
 ==24895==    by 0x8A7198: handle_solib_event() (solib.c:1274)
 ==24895==    by 0x4E3407: bpstat_stop_status(address_space const*, unsigned long, thread_info*, target_waitstatus const*, bpstats*) (breakpoint.c:5407)
 ==24895==    by 0x721F41: handle_signal_stop(execution_control_state*) (infrun.c:5685)
 ==24895==    by 0x720B11: handle_inferior_event(execution_control_state*) (infrun.c:5129)
 ==24895==

Note, this little bit in the patch is just a cleanup that I noticed:

 -  lookup.prob = prob;
    lookup.address = address;

That line isn't necessary because hashing/comparison only looks at the
address.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* solib-svr4.c (svr4_free_objfile_observer): New.
	(probe_and_action::objfile): New field.
	(probes_table_htab_remove_objfile_probes)
	(probes_table_remove_objfile_probes): New functions.
	(register_solib_event_probe): Add 'objfile' parameter.  Store it
	in the new probe_and_action.  Don't store the probe in 'lookup'.
	(svr4_create_probe_breakpoints): Pass objfile to
	register_solib_event_probe.
	(_initialize_svr4_solib): Register a free_objfile observer.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/solib-probes-nosharedlibrary.c,
	gdb.base/solib-probes-nosharedlibrary.exp: New files.
---
 gdb/solib-svr4.c                                   | 46 +++++++++++++++++--
 .../gdb.base/solib-probes-nosharedlibrary.c        | 22 ++++++++++
 .../gdb.base/solib-probes-nosharedlibrary.exp      | 51 ++++++++++++++++++++++
 3 files changed, 116 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.c
 create mode 100644 gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.exp

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index cf83196721f..2c79dfec2bb 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -50,6 +50,7 @@ static struct link_map_offsets *svr4_fetch_link_map_offsets (void);
 static int svr4_have_link_map_offsets (void);
 static void svr4_relocate_main_executable (void);
 static void svr4_free_library_list (void *p_list);
+static void probes_table_remove_objfile_probes (struct objfile *objfile);
 
 /* On SVR4 systems, a list of symbols in the dynamic linker where
    GDB can try to place a breakpoint to monitor shared library
@@ -1025,6 +1026,14 @@ struct svr4_library_list
   CORE_ADDR main_lm;
 };
 
+/* This module's 'free_objfile' observer.  */
+
+static void
+svr4_free_objfile_observer (struct objfile *objfile)
+{
+  probes_table_remove_objfile_probes (objfile);
+}
+
 /* Implementation for target_so_ops.free_so.  */
 
 static void
@@ -1636,6 +1645,9 @@ struct probe_and_action
 
   /* The action.  */
   enum probe_action action;
+
+  /* The objfile where this probe was found.  */
+  struct objfile *objfile;
 };
 
 /* Returns a hash code for the probe_and_action referenced by p.  */
@@ -1660,11 +1672,37 @@ equal_probe_and_action (const void *p1, const void *p2)
   return pa1->address == pa2->address;
 }
 
+/* Traversal function for probes_table_remove_objfile_probes.  */
+
+static int
+probes_table_htab_remove_objfile_probes (void **slot, void *info)
+{
+  probe_and_action *pa = (probe_and_action *) *slot;
+  struct objfile *objfile = (struct objfile *) info;
+
+  if (pa->objfile == objfile)
+    htab_clear_slot (get_svr4_info ()->probes_table, slot);
+
+  return 1;
+}
+
+/* Remove all probes that belong to OBJFILE from the probes table.  */
+
+static void
+probes_table_remove_objfile_probes (struct objfile *objfile)
+{
+  svr4_info *info = get_svr4_info ();
+  if (info->probes_table != nullptr)
+    htab_traverse_noresize (info->probes_table,
+			    probes_table_htab_remove_objfile_probes, objfile);
+}
+
 /* Register a solib event probe and its associated action in the
    probes table.  */
 
 static void
-register_solib_event_probe (probe *prob, CORE_ADDR address,
+register_solib_event_probe (struct objfile *objfile,
+			    probe *prob, CORE_ADDR address,
 			    enum probe_action action)
 {
   struct svr4_info *info = get_svr4_info ();
@@ -1677,7 +1715,6 @@ register_solib_event_probe (probe *prob, CORE_ADDR address,
 					    equal_probe_and_action,
 					    xfree, xcalloc, xfree);
 
-  lookup.prob = prob;
   lookup.address = address;
   slot = htab_find_slot (info->probes_table, &lookup, INSERT);
   gdb_assert (*slot == HTAB_EMPTY_ENTRY);
@@ -1686,6 +1723,7 @@ register_solib_event_probe (probe *prob, CORE_ADDR address,
   pa->prob = prob;
   pa->address = address;
   pa->action = action;
+  pa->objfile = objfile;
 
   *slot = pa;
 }
@@ -2030,7 +2068,7 @@ svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
 	  CORE_ADDR address = p->get_relocated_address (objfile);
 
 	  create_solib_event_breakpoint (gdbarch, address);
-	  register_solib_event_probe (p, address, action);
+	  register_solib_event_probe (objfile, p, address, action);
 	}
     }
 
@@ -3224,4 +3262,6 @@ _initialize_svr4_solib (void)
   svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
   svr4_so_ops.update_breakpoints = svr4_update_solib_event_breakpoints;
   svr4_so_ops.handle_event = svr4_handle_solib_event;
+
+  gdb::observers::free_objfile.attach (svr4_free_objfile_observer);
 }
diff --git a/gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.c b/gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.c
new file mode 100644
index 00000000000..d94b8074ec0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 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
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.exp b/gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.exp
new file mode 100644
index 00000000000..0d5dc870ad5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-probes-nosharedlibrary.exp
@@ -0,0 +1,51 @@
+# Copyright 2005-2019 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/>.
+
+# Regression test for a bug where GDB would misbehave (most likely
+# crash) if you ran the "nosharelibrary" command, continued execution,
+# and then the program hit the shared library event breakpoint.  GDB
+# would deference a dangling solib event probe pointer.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if {$use_gdb_stub && [target_info exists gdb,do_reload_on_run]} {
+    # This is the path taken by gdbserver "target remote" boards.
+    if { [gdb_reload] != 0 } {
+	untested "could not run to initial instruction"
+	return
+    }
+    pass "stopped at entry"
+} else {
+    if { [gdb_starti_cmd] < 0 } {
+	untested "could not run to initial instruction"
+	return
+    }
+    gdb_test "" "Program stopped.*" "stopped at entry"
+}
+
+# The program should stop at the first instruction, before the shared
+# library event breakpoint is first hit.  On systems where probes are
+# present in the dynamic linker, such as GNU/Linux, discarding all
+# shared libraries discards such probes too.  The probes-based
+# interface can no longer be used.
+gdb_test_no_output "nosharedlibrary"
+
+# Continue to main(), past the solib event.
+gdb_breakpoint main
+gdb_continue_to_breakpoint "main"
-- 
2.14.5

Re: [PATCH] Fix "nosharedlibrary + continue + shared lib event" crash

[Simon Marchi]

On 2019-04-09 9:14 a.m., Pedro Alves wrote:
> GDB misbehaves if you run the "nosharelibrary" command, continue
> execution, and then the program hits the shared library event
> breakpoint.  On my system it aborts like this:
> 
>  (gdb) nosharedlibrary
>  (gdb) c
>  Continuing.
>  pure virtual method called
>  terminate called without an active exception
>  Aborted (core dumped)
> 
> Though it's really undefined behavior territory, caused by deferencing
> a dangling solib event probe pointer.
> 
> I've observed this by running "nosharedlibrary" when stopped at the
> entry point, but it should happen at any other point, if the program
> does a dlopen/dlclose after.
> 
> The fix is to discard an objfile's probes from the svr4 probes table
> when an objfile is about to be released.
> 
> New test included, works with both native and gdbserver testing.
> 
> Valgrind log:
> 
>  (gdb) starti
>  (gdb) nosharedlibrary
>  (gdb) c
>  Continuing.
>  ==24895== Invalid read of size 8
>  ==24895==    at 0x89E5FB: solib_event_probe_action(probe_and_action*) (solib-svr4.c:1735)
>  ==24895==    by 0x89E95A: svr4_handle_solib_event() (solib-svr4.c:1872)
>  ==24895==    by 0x8A7198: handle_solib_event() (solib.c:1274)
>  ==24895==    by 0x4E3407: bpstat_stop_status(address_space const*, unsigned long, thread_info*, target_waitstatus const*, bpstats*) (breakpoint.c:5407)
>  ==24895==    by 0x721F41: handle_signal_stop(execution_control_state*) (infrun.c:5685)
>  ==24895==    by 0x720B11: handle_inferior_event(execution_control_state*) (infrun.c:5129)
>  ==24895==    by 0x71DD93: fetch_inferior_event(void*) (infrun.c:3748)
>  ==24895==    by 0x7059C3: inferior_event_handler(inferior_event_type, void*) (inf-loop.c:43)
>  ==24895==    by 0x874DF0: remote_async_serial_handler(serial*, void*) (remote.c:14039)
>  ==24895==    by 0x894101: run_async_handler_and_reschedule(serial*) (ser-base.c:137)
>  ==24895==    by 0x8941E6: fd_event(int, void*) (ser-base.c:188)
>  ==24895==    by 0x67AFEF: handle_file_event(file_handler*, int) (event-loop.c:732)
>  ==24895==  Address 0x18b63860 is 0 bytes inside a block of size 136 free'd
>  ==24895==    at 0x4C2E616: operator delete(void*, unsigned long) (vg_replace_malloc.c:585)
>  ==24895==    by 0x8C6A12: stap_probe::~stap_probe() (stap-probe.c:124)
>  ==24895==    by 0x66F7DB: probe_key_free(bfd*, void*) (elfread.c:1382)
>  ==24895==    by 0x69B705: bfdregistry_callback_adaptor(void (*)(registry_container*, void*), registry_container*, void*) (gdb_bfd.c:131)
>  ==24895==    by 0x855A57: registry_clear_data(registry_data_registry*, void (*)(void (*)(registry_container*, void*), registry_container*, void*), registry_container*, registry_fields*) (registry.c:79)
>  ==24895==    by 0x855B01: registry_container_free_data(registry_data_registry*, void (*)(void (*)(registry_container*, void*), registry_container*, void*), registry_container*, registry_fields*) (registry.c:92)
>  ==24895==    by 0x69B783: bfd_free_data(bfd*) (gdb_bfd.c:131)
>  ==24895==    by 0x69C4BA: gdb_bfd_unref(bfd*) (gdb_bfd.c:609)
>  ==24895==    by 0x7CC33F: objfile::~objfile() (objfiles.c:651)
>  ==24895==    by 0x7CD559: objfile_purge_solibs() (objfiles.c:1021)
>  ==24895==    by 0x8A7132: no_shared_libraries(char const*, int) (solib.c:1252)
>  ==24895==    by 0x548E3D: do_const_cfunc(cmd_list_element*, char const*, int) (cli-decode.c:106)
>  ==24895==  Block was alloc'd at
>  ==24895==    at 0x4C2D42A: operator new(unsigned long) (vg_replace_malloc.c:334)
>  ==24895==    by 0x8C527C: handle_stap_probe(objfile*, sdt_note*, std::vector<probe*, std::allocator<probe*> >*, unsigned long) (stap-probe.c:1561)
>  ==24895==    by 0x8C5535: stap_static_probe_ops::get_probes(std::vector<probe*, std::allocator<probe*> >*, objfile*) const (stap-probe.c:1656)
>  ==24895==    by 0x66F71B: elf_get_probes(objfile*) (elfread.c:1365)
>  ==24895==    by 0x7EDD85: find_probes_in_objfile(objfile*, char const*, char const*) (probe.c:227)
>  ==24895==    by 0x4DF382: create_longjmp_master_breakpoint() (breakpoint.c:3275)
>  ==24895==    by 0x4F6562: breakpoint_re_set() (breakpoint.c:13828)
>  ==24895==    by 0x8A66AA: solib_add(char const*, int, int) (solib.c:1010)
>  ==24895==    by 0x89F7C6: enable_break(svr4_info*, int) (solib-svr4.c:2360)
>  ==24895==    by 0x8A104C: svr4_solib_create_inferior_hook(int) (solib-svr4.c:2992)
>  ==24895==    by 0x8A70B9: solib_create_inferior_hook(int) (solib.c:1215)
>  ==24895==    by 0x70C073: post_create_inferior(target_ops*, int) (infcmd.c:467)
>  ==24895==
>  pure virtual method called
>  terminate called without an active exception
>  ==24895==
>  ==24895== Process terminating with default action of signal 6 (SIGABRT): dumping core
>  ==24895==    at 0x7CF3750: raise (raise.c:51)
>  ==24895==    by 0x7CF4D30: abort (abort.c:79)
>  ==24895==    by 0xB008F4: __gnu_cxx::__verbose_terminate_handler() (in build/gdb/gdb)
>  ==24895==    by 0xAFF845: __cxxabiv1::__terminate(void (*)()) (in build/gdb/gdb)
>  ==24895==    by 0xAFF890: std::terminate() (in build/gdb/gdb)
>  ==24895==    by 0xAFF95E: __cxa_pure_virtual (in build/gdb/gdb)
>  ==24895==    by 0x89E610: solib_event_probe_action(probe_and_action*) (solib-svr4.c:1735)
>  ==24895==    by 0x89E95A: svr4_handle_solib_event() (solib-svr4.c:1872)
>  ==24895==    by 0x8A7198: handle_solib_event() (solib.c:1274)
>  ==24895==    by 0x4E3407: bpstat_stop_status(address_space const*, unsigned long, thread_info*, target_waitstatus const*, bpstats*) (breakpoint.c:5407)
>  ==24895==    by 0x721F41: handle_signal_stop(execution_control_state*) (infrun.c:5685)
>  ==24895==    by 0x720B11: handle_inferior_event(execution_control_state*) (infrun.c:5129)
>  ==24895==
> 
> Note, this little bit in the patch is just a cleanup that I noticed:
> 
>  -  lookup.prob = prob;
>     lookup.address = address;
> 
> That line isn't necessary because hashing/comparison only looks at the
> address.

I am not able to reproduce the problem, and the test doesn't fail here, without
the rest of the patch applied.  I think it's because on my system gdb doesn't use
the probe based interface.

Anyhow, the fix makes sense.  Since the probe is deleted on objfile destruction, the
corresponding probe_and_action structures should too.

Simon

[PATCH] solib-svr4: Pass down svr4_info as much as possible (was: [PATCH] Fix "nosharedlibrary + continue + shared lib event" crash)

[Simon Marchi]

On 2019-04-10 10:49 p.m., Simon Marchi wrote:
> I am not able to reproduce the problem, and the test doesn't fail here, without
> the rest of the patch applied.  I think it's because on my system gdb doesn't use
> the probe based interface.
> 
> Anyhow, the fix makes sense.  Since the probe is deleted on objfile destruction, the
> corresponding probe_and_action structures should too.
> 
> Simon

While reviewing this patch, I had written the patch below to experiment, and
while it's not super important, I think it's a good cleanup.


From aedf5f7d846672ba6edc2780853baa43f35dd3c4 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Wed, 10 Apr 2019 22:02:33 -0400
Subject: [PATCH] solib-svr4: Pass down svr4_info as much as possible

While reviewing

  https://sourceware.org/ml/gdb-patches/2019-04/msg00141.html

I noticed that we relied heavily on global state through the
get_svr4_info function, which uses current_program_space.  I thought we
could improve this (make things more explicit and easier to follow) by

- Making get_svr4_info accept a program_space parameter, making it
  return the SVR4 info for that program space.
- Passing down the svr4_info object from callers as much as possible.

This means looking up the svr4_info for the appropriate program space at
the entry points of the solib-svr4.c file and passing it down.  For now,
these entry points (most of them are "methods" of svr4_so_ops) rely on
current_program_space, but we can later try to change the target_so_ops
interface to pass down the program space.

gdb/ChangeLog:

	* solib-svr4.c (get_svr4_info): Add pspace parameter.
	(svr4_keep_data_in_core): Pass current_program_space to get_svr4_info.
	(open_symbol_file_object): Likewise.
	(svr4_default_sos): Add info parameter.
	(svr4_read_so_list): Likewise.
	(svr4_current_sos_direct): Adjust functions calls to pass down
	info.
	(svr4_current_sos_1): Add info parameter.
	(svr4_current_sos): Call get_svr4_info, pass info down to
	svr4_current_sos_1.
	(svr4_fetch_objfile_link_map): Pass objfile->pspace to
	get_svr4_info.
	(svr4_in_dynsym_resolve_code): Pass current_program_space to
	get_svr4_info.
	(probes_table_htab_remove_objfile_probes): Pass objfile->pspace
	to get_svr4_info.
	(probes_table_remove_objfile_probes): Likewise.
	(register_solib_event_probe): Add info parameter.
	(solist_update_incremental): Pass info parameter down to
	svr4_read_so_list.
	(disable_probes_interface): Add info parameter.
	(svr4_handle_solib_event): Pass current_program_space to
	get_svr4_info.  Adjust disable_probes_interface cleanup.
	(svr4_create_probe_breakpoints): Add info parameter, pass it
	down to register_solib_event_probe.
	(svr4_create_solib_event_breakpoints): Add info parameter,
	pass it down to svr4_create_probe_breakpoints.
	(enable_break): Pass info down to
	svr4_create_solib_event_breakpoints.
	(svr4_solib_create_inferior_hook): Pass current_program_space to
	get_svr4_info.
	(svr4_clear_solib): Likewise.
---
 gdb/solib-svr4.c | 84 +++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 43 deletions(-)

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 2c79dfec2bb6..2aa7b95ce6c1 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -391,21 +391,21 @@ svr4_pspace_data_cleanup (struct program_space *pspace, void *arg)
   xfree (info);
 }

-/* Get the current svr4 data.  If none is found yet, add it now.  This
-   function always returns a valid object.  */
+/* Get the svr4 data for program space PSPACE.  If none is found yet, add it now.
+   This function always returns a valid object.  */

 static struct svr4_info *
-get_svr4_info (void)
+get_svr4_info (program_space *pspace)
 {
   struct svr4_info *info;

-  info = (struct svr4_info *) program_space_data (current_program_space,
+  info = (struct svr4_info *) program_space_data (pspace,
 						  solib_svr4_pspace_data);
   if (info != NULL)
     return info;

   info = XCNEW (struct svr4_info);
-  set_program_space_data (current_program_space, solib_svr4_pspace_data, info);
+  set_program_space_data (pspace, solib_svr4_pspace_data, info);
   return info;
 }

@@ -940,7 +940,7 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
   CORE_ADDR ldsomap;
   CORE_ADDR name_lm;

-  info = get_svr4_info ();
+  info = get_svr4_info (current_program_space);

   info->debug_base = 0;
   locate_base (info);
@@ -969,7 +969,7 @@ open_symbol_file_object (int from_tty)
   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
   int l_name_size = TYPE_LENGTH (ptr_type);
   gdb::byte_vector l_name_buf (l_name_size);
-  struct svr4_info *info = get_svr4_info ();
+  struct svr4_info *info = get_svr4_info (current_program_space);
   symfile_add_flags add_flags = 0;

   if (from_tty)
@@ -1264,9 +1264,8 @@ svr4_current_sos_via_xfer_libraries (struct svr4_library_list *list,
    linker, build a fallback list from other sources.  */

 static struct so_list *
-svr4_default_sos (void)
+svr4_default_sos (svr4_info *info)
 {
-  struct svr4_info *info = get_svr4_info ();
   struct so_list *newobj;

   if (!info->debug_loader_offset_p)
@@ -1296,7 +1295,7 @@ svr4_default_sos (void)
    represent only part of the inferior library list.  */

 static int
-svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
+svr4_read_so_list (svr4_info *info, CORE_ADDR lm, CORE_ADDR prev_lm,
 		   struct so_list ***link_ptr_ptr, int ignore_first)
 {
   CORE_ADDR first_l_name = 0;
@@ -1331,8 +1330,6 @@ svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
          decide when to ignore it.  */
       if (ignore_first && li->l_prev == 0)
 	{
-	  struct svr4_info *info = get_svr4_info ();
-
 	  first_l_name = li->l_name;
 	  info->main_lm_addr = li->lm_addr;
 	  continue;
@@ -1400,7 +1397,7 @@ svr4_current_sos_direct (struct svr4_info *info)
       if (library_list.main_lm)
 	info->main_lm_addr = library_list.main_lm;

-      return library_list.head ? library_list.head : svr4_default_sos ();
+      return library_list.head ? library_list.head : svr4_default_sos (info);
     }

   /* Always locate the debug struct, in case it has moved.  */
@@ -1410,7 +1407,7 @@ svr4_current_sos_direct (struct svr4_info *info)
   /* If we can't find the dynamic linker's base structure, this
      must not be a dynamically linked executable.  Hmm.  */
   if (! info->debug_base)
-    return svr4_default_sos ();
+    return svr4_default_sos (info);

   /* Assume that everything is a library if the dynamic loader was loaded
      late by a static executable.  */
@@ -1428,7 +1425,7 @@ svr4_current_sos_direct (struct svr4_info *info)
      `struct so_list' nodes.  */
   lm = solib_svr4_r_map (info);
   if (lm)
-    svr4_read_so_list (lm, 0, &link_ptr, ignore_first);
+    svr4_read_so_list (info, lm, 0, &link_ptr, ignore_first);

   /* On Solaris, the dynamic linker is not in the normal list of
      shared objects, so make sure we pick it up too.  Having
@@ -1436,12 +1433,12 @@ svr4_current_sos_direct (struct svr4_info *info)
      for skipping dynamic linker resolver code.  */
   lm = solib_svr4_r_ldsomap (info);
   if (lm)
-    svr4_read_so_list (lm, 0, &link_ptr, 0);
+    svr4_read_so_list (info, lm, 0, &link_ptr, 0);

   cleanup.release ();

   if (head == NULL)
-    return svr4_default_sos ();
+    return svr4_default_sos (info);

   return head;
 }
@@ -1450,10 +1447,8 @@ svr4_current_sos_direct (struct svr4_info *info)
    method.  */

 static struct so_list *
-svr4_current_sos_1 (void)
+svr4_current_sos_1 (svr4_info *info)
 {
-  struct svr4_info *info = get_svr4_info ();
-
   /* If the solib list has been read and stored by the probes
      interface then we return a copy of the stored list.  */
   if (info->solib_list != NULL)
@@ -1468,7 +1463,8 @@ svr4_current_sos_1 (void)
 static struct so_list *
 svr4_current_sos (void)
 {
-  struct so_list *so_head = svr4_current_sos_1 ();
+  svr4_info *info = get_svr4_info (current_program_space);
+  struct so_list *so_head = svr4_current_sos_1 (info);
   struct mem_range vsyscall_range;

   /* Filter out the vDSO module, if present.  Its symbol file would
@@ -1548,7 +1544,7 @@ CORE_ADDR
 svr4_fetch_objfile_link_map (struct objfile *objfile)
 {
   struct so_list *so;
-  struct svr4_info *info = get_svr4_info ();
+  struct svr4_info *info = get_svr4_info (objfile->pspace);

   /* Cause svr4_current_sos() to be run if it hasn't been already.  */
   if (info->main_lm_addr == 0)
@@ -1601,7 +1597,7 @@ match_main (const char *soname)
 int
 svr4_in_dynsym_resolve_code (CORE_ADDR pc)
 {
-  struct svr4_info *info = get_svr4_info ();
+  struct svr4_info *info = get_svr4_info (current_program_space);

   return ((pc >= info->interp_text_sect_low
 	   && pc < info->interp_text_sect_high)
@@ -1681,7 +1677,7 @@ probes_table_htab_remove_objfile_probes (void **slot, void *info)
   struct objfile *objfile = (struct objfile *) info;

   if (pa->objfile == objfile)
-    htab_clear_slot (get_svr4_info ()->probes_table, slot);
+    htab_clear_slot (get_svr4_info (objfile->pspace)->probes_table, slot);

   return 1;
 }
@@ -1691,7 +1687,7 @@ probes_table_htab_remove_objfile_probes (void **slot, void *info)
 static void
 probes_table_remove_objfile_probes (struct objfile *objfile)
 {
-  svr4_info *info = get_svr4_info ();
+  svr4_info *info = get_svr4_info (objfile->pspace);
   if (info->probes_table != nullptr)
     htab_traverse_noresize (info->probes_table,
 			    probes_table_htab_remove_objfile_probes, objfile);
@@ -1701,11 +1697,10 @@ probes_table_remove_objfile_probes (struct objfile *objfile)
    probes table.  */

 static void
-register_solib_event_probe (struct objfile *objfile,
+register_solib_event_probe (svr4_info *info, struct objfile *objfile,
 			    probe *prob, CORE_ADDR address,
 			    enum probe_action action)
 {
-  struct svr4_info *info = get_svr4_info ();
   struct probe_and_action lookup, *pa;
   void **slot;

@@ -1857,7 +1852,7 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
 	 above check and deferral to solist_update_full ensures
 	 that this call to svr4_read_so_list will never see the
 	 first element.  */
-      if (!svr4_read_so_list (lm, prev_lm, &link, 0))
+      if (!svr4_read_so_list (info, lm, prev_lm, &link, 0))
 	return 0;
     }

@@ -1869,10 +1864,8 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
    ones set up for the probes-based interface are adequate.  */

 static void
-disable_probes_interface ()
+disable_probes_interface (svr4_info *info)
 {
-  struct svr4_info *info = get_svr4_info ();
-
   warning (_("Probes-based dynamic linker interface failed.\n"
 	     "Reverting to original interface.\n"));

@@ -1887,7 +1880,7 @@ disable_probes_interface ()
 static void
 svr4_handle_solib_event (void)
 {
-  struct svr4_info *info = get_svr4_info ();
+  struct svr4_info *info = get_svr4_info (current_program_space);
   struct probe_and_action *pa;
   enum probe_action action;
   struct value *val = NULL;
@@ -1900,7 +1893,10 @@ svr4_handle_solib_event (void)

   /* If anything goes wrong we revert to the original linker
      interface.  */
-  auto cleanup = make_scope_exit (disable_probes_interface);
+  auto cleanup = make_scope_exit ([info] ()
+    {
+      disable_probes_interface (info);
+    });

   pc = regcache_read_pc (get_current_regcache ());
   pa = solib_event_probe_at (info, pc);
@@ -2055,7 +2051,7 @@ svr4_update_solib_event_breakpoints (void)
    probe.  */

 static void
-svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
+svr4_create_probe_breakpoints (svr4_info *info, struct gdbarch *gdbarch,
 			       const std::vector<probe *> *probes,
 			       struct objfile *objfile)
 {
@@ -2068,7 +2064,7 @@ svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
 	  CORE_ADDR address = p->get_relocated_address (objfile);

 	  create_solib_event_breakpoint (gdbarch, address);
-	  register_solib_event_probe (objfile, p, address, action);
+	  register_solib_event_probe (info, objfile, p, address, action);
 	}
     }

@@ -2088,7 +2084,7 @@ svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
    marker function.  */

 static void
-svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch,
+svr4_create_solib_event_breakpoints (svr4_info *info, struct gdbarch *gdbarch,
 				     CORE_ADDR address)
 {
   struct obj_section *os;
@@ -2151,7 +2147,7 @@ svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch,
 	    }

 	  if (all_probes_found)
-	    svr4_create_probe_breakpoints (gdbarch, probes, os->objfile);
+	    svr4_create_probe_breakpoints (info, gdbarch, probes, os->objfile);

 	  if (all_probes_found)
 	    return;
@@ -2282,7 +2278,7 @@ enable_break (struct svr4_info *info, int from_tty)
 		+ bfd_section_size (tmp_bfd, interp_sect);
 	    }

-	  svr4_create_solib_event_breakpoints (target_gdbarch (), sym_addr);
+	  svr4_create_solib_event_breakpoints (info, target_gdbarch (), sym_addr);
 	  return 1;
 	}
     }
@@ -2444,7 +2440,7 @@ enable_break (struct svr4_info *info, int from_tty)

       if (sym_addr != 0)
 	{
-	  svr4_create_solib_event_breakpoints (target_gdbarch (),
+	  svr4_create_solib_event_breakpoints (info, target_gdbarch (),
 					       load_addr + sym_addr);
 	  return 1;
 	}
@@ -2470,7 +2466,8 @@ enable_break (struct svr4_info *info, int from_tty)
 	  sym_addr = gdbarch_convert_from_func_ptr_addr (target_gdbarch (),
 							 sym_addr,
 							 current_top_target ());
-	  svr4_create_solib_event_breakpoints (target_gdbarch (), sym_addr);
+	  svr4_create_solib_event_breakpoints (info, target_gdbarch (),
+					       sym_addr);
 	  return 1;
 	}
     }
@@ -2487,7 +2484,8 @@ enable_break (struct svr4_info *info, int from_tty)
 	      sym_addr = gdbarch_convert_from_func_ptr_addr (target_gdbarch (),
 							     sym_addr,
 							     current_top_target ());
-	      svr4_create_solib_event_breakpoints (target_gdbarch (), sym_addr);
+	      svr4_create_solib_event_breakpoints (info, target_gdbarch (),
+						   sym_addr);
 	      return 1;
 	    }
 	}
@@ -3010,7 +3008,7 @@ svr4_solib_create_inferior_hook (int from_tty)
 {
   struct svr4_info *info;

-  info = get_svr4_info ();
+  info = get_svr4_info (current_program_space);

   /* Clear the probes-based interface's state.  */
   free_probes_table (info);
@@ -3036,7 +3034,7 @@ svr4_clear_solib (void)
 {
   struct svr4_info *info;

-  info = get_svr4_info ();
+  info = get_svr4_info (current_program_space);
   info->debug_base = 0;
   info->debug_loader_offset_p = 0;
   info->debug_loader_offset = 0;
-- 
2.21.0

Re: [PATCH] solib-svr4: Pass down svr4_info as much as possible

[Simon Marchi]

On 2019-04-19 11:03 a.m., Simon Marchi wrote:
> While reviewing this patch, I had written the patch below to experiment, and
> while it's not super important, I think it's a good cleanup.

I forgot to mention, my patch applies on top of Pedro's patch.

Simon

Re: [PATCH] solib-svr4: Pass down svr4_info as much as possible

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> While reviewing this patch, I had written the patch below to experiment, and
Simon> while it's not super important, I think it's a good cleanup.

Looks reasonable to me.  I'm very much in favor of reducing dependencies
on globals.

thanks,
Tom

Re: [PATCH] solib-svr4: Pass down svr4_info as much as possible

[Simon Marchi]

On 2019-04-19 4:03 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> While reviewing this patch, I had written the patch below to experiment, and
> Simon> while it's not super important, I think it's a good cleanup.
> 
> Looks reasonable to me.  I'm very much in favor of reducing dependencies
> on globals.
> 
> thanks,
> Tom

Thanks.

Pedro, feel free to check this in at the same time as your patch if you think it is good as well.

Simon

Re: [PATCH] Fix "nosharedlibrary + continue + shared lib event" crash

[Pedro Alves]

On 4/11/19 3:49 AM, Simon Marchi wrote:
> On 2019-04-09 9:14 a.m., Pedro Alves wrote:
>> GDB misbehaves if you run the "nosharelibrary" command, continue
>> execution, and then the program hits the shared library event
>> breakpoint.  On my system it aborts like this:
>>
>>  (gdb) nosharedlibrary
>>  (gdb) c
>>  Continuing.
>>  pure virtual method called
>>  terminate called without an active exception
>>  Aborted (core dumped)
>>
>> Though it's really undefined behavior territory, caused by deferencing
>> a dangling solib event probe pointer.
>>
>> I've observed this by running "nosharedlibrary" when stopped at the
>> entry point, but it should happen at any other point, if the program
>> does a dlopen/dlclose after.
>>
>> The fix is to discard an objfile's probes from the svr4 probes table
>> when an objfile is about to be released.
>>
>> New test included, works with both native and gdbserver testing.
>>
>> Valgrind log:
>>
>>  (gdb) starti
>>  (gdb) nosharedlibrary
>>  (gdb) c
>>  Continuing.
>>  ==24895== Invalid read of size 8
>>  ==24895==    at 0x89E5FB: solib_event_probe_action(probe_and_action*) (solib-svr4.c:1735)
>>  ==24895==    by 0x89E95A: svr4_handle_solib_event() (solib-svr4.c:1872)
>>  ==24895==    by 0x8A7198: handle_solib_event() (solib.c:1274)
>>  ==24895==    by 0x4E3407: bpstat_stop_status(address_space const*, unsigned long, thread_info*, target_waitstatus const*, bpstats*) (breakpoint.c:5407)
>>  ==24895==    by 0x721F41: handle_signal_stop(execution_control_state*) (infrun.c:5685)
>>  ==24895==    by 0x720B11: handle_inferior_event(execution_control_state*) (infrun.c:5129)
>>  ==24895==    by 0x71DD93: fetch_inferior_event(void*) (infrun.c:3748)
>>  ==24895==    by 0x7059C3: inferior_event_handler(inferior_event_type, void*) (inf-loop.c:43)
>>  ==24895==    by 0x874DF0: remote_async_serial_handler(serial*, void*) (remote.c:14039)
>>  ==24895==    by 0x894101: run_async_handler_and_reschedule(serial*) (ser-base.c:137)
>>  ==24895==    by 0x8941E6: fd_event(int, void*) (ser-base.c:188)
>>  ==24895==    by 0x67AFEF: handle_file_event(file_handler*, int) (event-loop.c:732)
>>  ==24895==  Address 0x18b63860 is 0 bytes inside a block of size 136 free'd
>>  ==24895==    at 0x4C2E616: operator delete(void*, unsigned long) (vg_replace_malloc.c:585)
>>  ==24895==    by 0x8C6A12: stap_probe::~stap_probe() (stap-probe.c:124)
>>  ==24895==    by 0x66F7DB: probe_key_free(bfd*, void*) (elfread.c:1382)
>>  ==24895==    by 0x69B705: bfdregistry_callback_adaptor(void (*)(registry_container*, void*), registry_container*, void*) (gdb_bfd.c:131)
>>  ==24895==    by 0x855A57: registry_clear_data(registry_data_registry*, void (*)(void (*)(registry_container*, void*), registry_container*, void*), registry_container*, registry_fields*) (registry.c:79)
>>  ==24895==    by 0x855B01: registry_container_free_data(registry_data_registry*, void (*)(void (*)(registry_container*, void*), registry_container*, void*), registry_container*, registry_fields*) (registry.c:92)
>>  ==24895==    by 0x69B783: bfd_free_data(bfd*) (gdb_bfd.c:131)
>>  ==24895==    by 0x69C4BA: gdb_bfd_unref(bfd*) (gdb_bfd.c:609)
>>  ==24895==    by 0x7CC33F: objfile::~objfile() (objfiles.c:651)
>>  ==24895==    by 0x7CD559: objfile_purge_solibs() (objfiles.c:1021)
>>  ==24895==    by 0x8A7132: no_shared_libraries(char const*, int) (solib.c:1252)
>>  ==24895==    by 0x548E3D: do_const_cfunc(cmd_list_element*, char const*, int) (cli-decode.c:106)
>>  ==24895==  Block was alloc'd at
>>  ==24895==    at 0x4C2D42A: operator new(unsigned long) (vg_replace_malloc.c:334)
>>  ==24895==    by 0x8C527C: handle_stap_probe(objfile*, sdt_note*, std::vector<probe*, std::allocator<probe*> >*, unsigned long) (stap-probe.c:1561)
>>  ==24895==    by 0x8C5535: stap_static_probe_ops::get_probes(std::vector<probe*, std::allocator<probe*> >*, objfile*) const (stap-probe.c:1656)
>>  ==24895==    by 0x66F71B: elf_get_probes(objfile*) (elfread.c:1365)
>>  ==24895==    by 0x7EDD85: find_probes_in_objfile(objfile*, char const*, char const*) (probe.c:227)
>>  ==24895==    by 0x4DF382: create_longjmp_master_breakpoint() (breakpoint.c:3275)
>>  ==24895==    by 0x4F6562: breakpoint_re_set() (breakpoint.c:13828)
>>  ==24895==    by 0x8A66AA: solib_add(char const*, int, int) (solib.c:1010)
>>  ==24895==    by 0x89F7C6: enable_break(svr4_info*, int) (solib-svr4.c:2360)
>>  ==24895==    by 0x8A104C: svr4_solib_create_inferior_hook(int) (solib-svr4.c:2992)
>>  ==24895==    by 0x8A70B9: solib_create_inferior_hook(int) (solib.c:1215)
>>  ==24895==    by 0x70C073: post_create_inferior(target_ops*, int) (infcmd.c:467)
>>  ==24895==
>>  pure virtual method called
>>  terminate called without an active exception
>>  ==24895==
>>  ==24895== Process terminating with default action of signal 6 (SIGABRT): dumping core
>>  ==24895==    at 0x7CF3750: raise (raise.c:51)
>>  ==24895==    by 0x7CF4D30: abort (abort.c:79)
>>  ==24895==    by 0xB008F4: __gnu_cxx::__verbose_terminate_handler() (in build/gdb/gdb)
>>  ==24895==    by 0xAFF845: __cxxabiv1::__terminate(void (*)()) (in build/gdb/gdb)
>>  ==24895==    by 0xAFF890: std::terminate() (in build/gdb/gdb)
>>  ==24895==    by 0xAFF95E: __cxa_pure_virtual (in build/gdb/gdb)
>>  ==24895==    by 0x89E610: solib_event_probe_action(probe_and_action*) (solib-svr4.c:1735)
>>  ==24895==    by 0x89E95A: svr4_handle_solib_event() (solib-svr4.c:1872)
>>  ==24895==    by 0x8A7198: handle_solib_event() (solib.c:1274)
>>  ==24895==    by 0x4E3407: bpstat_stop_status(address_space const*, unsigned long, thread_info*, target_waitstatus const*, bpstats*) (breakpoint.c:5407)
>>  ==24895==    by 0x721F41: handle_signal_stop(execution_control_state*) (infrun.c:5685)
>>  ==24895==    by 0x720B11: handle_inferior_event(execution_control_state*) (infrun.c:5129)
>>  ==24895==
>>
>> Note, this little bit in the patch is just a cleanup that I noticed:
>>
>>  -  lookup.prob = prob;
>>     lookup.address = address;
>>
>> That line isn't necessary because hashing/comparison only looks at the
>> address.
> 
> I am not able to reproduce the problem, and the test doesn't fail here, without
> the rest of the patch applied.  I think it's because on my system gdb doesn't use
> the probe based interface.
> 
> Anyhow, the fix makes sense.  Since the probe is deleted on objfile destruction, the
> corresponding probe_and_action structures should too.

Thanks for the review.  I've pushed it in now, with an additional 

 "On systems that use the probes-based solib interface, "

at the beginning of the commit log.

Thanks,
Pedro Alves

Re: [PATCH] solib-svr4: Pass down svr4_info as much as possible

[Pedro Alves]

On 4/19/19 4:03 PM, Simon Marchi wrote:
> On 2019-04-10 10:49 p.m., Simon Marchi wrote:
>> I am not able to reproduce the problem, and the test doesn't fail here, without
>> the rest of the patch applied.  I think it's because on my system gdb doesn't use
>> the probe based interface.
>>
>> Anyhow, the fix makes sense.  Since the probe is deleted on objfile destruction, the
>> corresponding probe_and_action structures should too.
>>
>> Simon
> 
> While reviewing this patch, I had written the patch below to experiment, and
> while it's not super important, I think it's a good cleanup.
> 
> 
> From aedf5f7d846672ba6edc2780853baa43f35dd3c4 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Wed, 10 Apr 2019 22:02:33 -0400
> Subject: [PATCH] solib-svr4: Pass down svr4_info as much as possible
> 
> While reviewing
> 
>   https://sourceware.org/ml/gdb-patches/2019-04/msg00141.html
> 
> I noticed that we relied heavily on global state through the
> get_svr4_info function, which uses current_program_space.  I thought we
> could improve this (make things more explicit and easier to follow) by
> 
> - Making get_svr4_info accept a program_space parameter, making it
>   return the SVR4 info for that program space.
> - Passing down the svr4_info object from callers as much as possible.
> 
> This means looking up the svr4_info for the appropriate program space at
> the entry points of the solib-svr4.c file and passing it down.  For now,
> these entry points (most of them are "methods" of svr4_so_ops) rely on
> current_program_space, but we can later try to change the target_so_ops
> interface to pass down the program space.

Seems fine to me.  Please go ahead.

Thinking a bit longer term we could end up passing down an inferior pointer
around in functions in this file instead.  That's because we use target_gdbarch()
in these routines, which is really inferior->gdbarch.  The program space can be
found at inferior->pspace.  Etc.  Then again, the end up going target calls in
a number of these routines, which implicitly refers to the current
inferior/thread/pspace too...  Anyway, I'm happy with the patch as is, TBC.

> 
> gdb/ChangeLog:
> 
> 	* solib-svr4.c (get_svr4_info): Add pspace parameter.
> 	(svr4_keep_data_in_core): Pass current_program_space to get_svr4_info.
> 	(open_symbol_file_object): Likewise.
> 	(svr4_default_sos): Add info parameter.
> 	(svr4_read_so_list): Likewise.
> 	(svr4_current_sos_direct): Adjust functions calls to pass down
> 	info.
> 	(svr4_current_sos_1): Add info parameter.
> 	(svr4_current_sos): Call get_svr4_info, pass info down to
> 	svr4_current_sos_1.
> 	(svr4_fetch_objfile_link_map): Pass objfile->pspace to
> 	get_svr4_info.
> 	(svr4_in_dynsym_resolve_code): Pass current_program_space to
> 	get_svr4_info.
> 	(probes_table_htab_remove_objfile_probes): Pass objfile->pspace
> 	to get_svr4_info.
> 	(probes_table_remove_objfile_probes): Likewise.
> 	(register_solib_event_probe): Add info parameter.
> 	(solist_update_incremental): Pass info parameter down to
> 	svr4_read_so_list.
> 	(disable_probes_interface): Add info parameter.
> 	(svr4_handle_solib_event): Pass current_program_space to
> 	get_svr4_info.  Adjust disable_probes_interface cleanup.
> 	(svr4_create_probe_breakpoints): Add info parameter, pass it
> 	down to register_solib_event_probe.
> 	(svr4_create_solib_event_breakpoints): Add info parameter,
> 	pass it down to svr4_create_probe_breakpoints.
> 	(enable_break): Pass info down to
> 	svr4_create_solib_event_breakpoints.
> 	(svr4_solib_create_inferior_hook): Pass current_program_space to
> 	get_svr4_info.
> 	(svr4_clear_solib): Likewise.
> ---
>  gdb/solib-svr4.c | 84 +++++++++++++++++++++++-------------------------
>  1 file changed, 41 insertions(+), 43 deletions(-)
> 
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 2c79dfec2bb6..2aa7b95ce6c1 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -391,21 +391,21 @@ svr4_pspace_data_cleanup (struct program_space *pspace, void *arg)
>    xfree (info);
>  }
> 
> -/* Get the current svr4 data.  If none is found yet, add it now.  This
> -   function always returns a valid object.  */
> +/* Get the svr4 data for program space PSPACE.  If none is found yet, add it now.
> +   This function always returns a valid object.  */
> 
>  static struct svr4_info *
> -get_svr4_info (void)
> +get_svr4_info (program_space *pspace)
>  {
>    struct svr4_info *info;
> 
> -  info = (struct svr4_info *) program_space_data (current_program_space,
> +  info = (struct svr4_info *) program_space_data (pspace,
>  						  solib_svr4_pspace_data);
>    if (info != NULL)
>      return info;
> 
>    info = XCNEW (struct svr4_info);
> -  set_program_space_data (current_program_space, solib_svr4_pspace_data, info);
> +  set_program_space_data (pspace, solib_svr4_pspace_data, info);
>    return info;
>  }
> 
> @@ -940,7 +940,7 @@ svr4_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>    CORE_ADDR ldsomap;
>    CORE_ADDR name_lm;
> 
> -  info = get_svr4_info ();
> +  info = get_svr4_info (current_program_space);
> 
>    info->debug_base = 0;
>    locate_base (info);
> @@ -969,7 +969,7 @@ open_symbol_file_object (int from_tty)
>    struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
>    int l_name_size = TYPE_LENGTH (ptr_type);
>    gdb::byte_vector l_name_buf (l_name_size);
> -  struct svr4_info *info = get_svr4_info ();
> +  struct svr4_info *info = get_svr4_info (current_program_space);
>    symfile_add_flags add_flags = 0;
> 
>    if (from_tty)
> @@ -1264,9 +1264,8 @@ svr4_current_sos_via_xfer_libraries (struct svr4_library_list *list,
>     linker, build a fallback list from other sources.  */
> 
>  static struct so_list *
> -svr4_default_sos (void)
> +svr4_default_sos (svr4_info *info)
>  {
> -  struct svr4_info *info = get_svr4_info ();
>    struct so_list *newobj;
> 
>    if (!info->debug_loader_offset_p)
> @@ -1296,7 +1295,7 @@ svr4_default_sos (void)
>     represent only part of the inferior library list.  */
> 
>  static int
> -svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
> +svr4_read_so_list (svr4_info *info, CORE_ADDR lm, CORE_ADDR prev_lm,
>  		   struct so_list ***link_ptr_ptr, int ignore_first)
>  {
>    CORE_ADDR first_l_name = 0;
> @@ -1331,8 +1330,6 @@ svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
>           decide when to ignore it.  */
>        if (ignore_first && li->l_prev == 0)
>  	{
> -	  struct svr4_info *info = get_svr4_info ();
> -
>  	  first_l_name = li->l_name;
>  	  info->main_lm_addr = li->lm_addr;
>  	  continue;
> @@ -1400,7 +1397,7 @@ svr4_current_sos_direct (struct svr4_info *info)
>        if (library_list.main_lm)
>  	info->main_lm_addr = library_list.main_lm;
> 
> -      return library_list.head ? library_list.head : svr4_default_sos ();
> +      return library_list.head ? library_list.head : svr4_default_sos (info);
>      }
> 
>    /* Always locate the debug struct, in case it has moved.  */
> @@ -1410,7 +1407,7 @@ svr4_current_sos_direct (struct svr4_info *info)
>    /* If we can't find the dynamic linker's base structure, this
>       must not be a dynamically linked executable.  Hmm.  */
>    if (! info->debug_base)
> -    return svr4_default_sos ();
> +    return svr4_default_sos (info);
> 
>    /* Assume that everything is a library if the dynamic loader was loaded
>       late by a static executable.  */
> @@ -1428,7 +1425,7 @@ svr4_current_sos_direct (struct svr4_info *info)
>       `struct so_list' nodes.  */
>    lm = solib_svr4_r_map (info);
>    if (lm)
> -    svr4_read_so_list (lm, 0, &link_ptr, ignore_first);
> +    svr4_read_so_list (info, lm, 0, &link_ptr, ignore_first);
> 
>    /* On Solaris, the dynamic linker is not in the normal list of
>       shared objects, so make sure we pick it up too.  Having
> @@ -1436,12 +1433,12 @@ svr4_current_sos_direct (struct svr4_info *info)
>       for skipping dynamic linker resolver code.  */
>    lm = solib_svr4_r_ldsomap (info);
>    if (lm)
> -    svr4_read_so_list (lm, 0, &link_ptr, 0);
> +    svr4_read_so_list (info, lm, 0, &link_ptr, 0);
> 
>    cleanup.release ();
> 
>    if (head == NULL)
> -    return svr4_default_sos ();
> +    return svr4_default_sos (info);
> 
>    return head;
>  }
> @@ -1450,10 +1447,8 @@ svr4_current_sos_direct (struct svr4_info *info)
>     method.  */
> 
>  static struct so_list *
> -svr4_current_sos_1 (void)
> +svr4_current_sos_1 (svr4_info *info)
>  {
> -  struct svr4_info *info = get_svr4_info ();
> -
>    /* If the solib list has been read and stored by the probes
>       interface then we return a copy of the stored list.  */
>    if (info->solib_list != NULL)
> @@ -1468,7 +1463,8 @@ svr4_current_sos_1 (void)
>  static struct so_list *
>  svr4_current_sos (void)
>  {
> -  struct so_list *so_head = svr4_current_sos_1 ();
> +  svr4_info *info = get_svr4_info (current_program_space);
> +  struct so_list *so_head = svr4_current_sos_1 (info);
>    struct mem_range vsyscall_range;
> 
>    /* Filter out the vDSO module, if present.  Its symbol file would
> @@ -1548,7 +1544,7 @@ CORE_ADDR
>  svr4_fetch_objfile_link_map (struct objfile *objfile)
>  {
>    struct so_list *so;
> -  struct svr4_info *info = get_svr4_info ();
> +  struct svr4_info *info = get_svr4_info (objfile->pspace);
> 
>    /* Cause svr4_current_sos() to be run if it hasn't been already.  */
>    if (info->main_lm_addr == 0)
> @@ -1601,7 +1597,7 @@ match_main (const char *soname)
>  int
>  svr4_in_dynsym_resolve_code (CORE_ADDR pc)
>  {
> -  struct svr4_info *info = get_svr4_info ();
> +  struct svr4_info *info = get_svr4_info (current_program_space);
> 
>    return ((pc >= info->interp_text_sect_low
>  	   && pc < info->interp_text_sect_high)
> @@ -1681,7 +1677,7 @@ probes_table_htab_remove_objfile_probes (void **slot, void *info)
>    struct objfile *objfile = (struct objfile *) info;
> 
>    if (pa->objfile == objfile)
> -    htab_clear_slot (get_svr4_info ()->probes_table, slot);
> +    htab_clear_slot (get_svr4_info (objfile->pspace)->probes_table, slot);
> 
>    return 1;
>  }
> @@ -1691,7 +1687,7 @@ probes_table_htab_remove_objfile_probes (void **slot, void *info)
>  static void
>  probes_table_remove_objfile_probes (struct objfile *objfile)
>  {
> -  svr4_info *info = get_svr4_info ();
> +  svr4_info *info = get_svr4_info (objfile->pspace);
>    if (info->probes_table != nullptr)
>      htab_traverse_noresize (info->probes_table,
>  			    probes_table_htab_remove_objfile_probes, objfile);
> @@ -1701,11 +1697,10 @@ probes_table_remove_objfile_probes (struct objfile *objfile)
>     probes table.  */
> 
>  static void
> -register_solib_event_probe (struct objfile *objfile,
> +register_solib_event_probe (svr4_info *info, struct objfile *objfile,
>  			    probe *prob, CORE_ADDR address,
>  			    enum probe_action action)
>  {
> -  struct svr4_info *info = get_svr4_info ();
>    struct probe_and_action lookup, *pa;
>    void **slot;
> 
> @@ -1857,7 +1852,7 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
>  	 above check and deferral to solist_update_full ensures
>  	 that this call to svr4_read_so_list will never see the
>  	 first element.  */
> -      if (!svr4_read_so_list (lm, prev_lm, &link, 0))
> +      if (!svr4_read_so_list (info, lm, prev_lm, &link, 0))
>  	return 0;
>      }
> 
> @@ -1869,10 +1864,8 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
>     ones set up for the probes-based interface are adequate.  */
> 
>  static void
> -disable_probes_interface ()
> +disable_probes_interface (svr4_info *info)
>  {
> -  struct svr4_info *info = get_svr4_info ();
> -
>    warning (_("Probes-based dynamic linker interface failed.\n"
>  	     "Reverting to original interface.\n"));
> 
> @@ -1887,7 +1880,7 @@ disable_probes_interface ()
>  static void
>  svr4_handle_solib_event (void)
>  {
> -  struct svr4_info *info = get_svr4_info ();
> +  struct svr4_info *info = get_svr4_info (current_program_space);
>    struct probe_and_action *pa;
>    enum probe_action action;
>    struct value *val = NULL;
> @@ -1900,7 +1893,10 @@ svr4_handle_solib_event (void)
> 
>    /* If anything goes wrong we revert to the original linker
>       interface.  */
> -  auto cleanup = make_scope_exit (disable_probes_interface);
> +  auto cleanup = make_scope_exit ([info] ()
> +    {
> +      disable_probes_interface (info);
> +    });
> 
>    pc = regcache_read_pc (get_current_regcache ());
>    pa = solib_event_probe_at (info, pc);
> @@ -2055,7 +2051,7 @@ svr4_update_solib_event_breakpoints (void)
>     probe.  */
> 
>  static void
> -svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
> +svr4_create_probe_breakpoints (svr4_info *info, struct gdbarch *gdbarch,
>  			       const std::vector<probe *> *probes,
>  			       struct objfile *objfile)
>  {
> @@ -2068,7 +2064,7 @@ svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
>  	  CORE_ADDR address = p->get_relocated_address (objfile);
> 
>  	  create_solib_event_breakpoint (gdbarch, address);
> -	  register_solib_event_probe (objfile, p, address, action);
> +	  register_solib_event_probe (info, objfile, p, address, action);
>  	}
>      }
> 
> @@ -2088,7 +2084,7 @@ svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
>     marker function.  */
> 
>  static void
> -svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch,
> +svr4_create_solib_event_breakpoints (svr4_info *info, struct gdbarch *gdbarch,
>  				     CORE_ADDR address)
>  {
>    struct obj_section *os;
> @@ -2151,7 +2147,7 @@ svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch,
>  	    }
> 
>  	  if (all_probes_found)
> -	    svr4_create_probe_breakpoints (gdbarch, probes, os->objfile);
> +	    svr4_create_probe_breakpoints (info, gdbarch, probes, os->objfile);
> 
>  	  if (all_probes_found)
>  	    return;
> @@ -2282,7 +2278,7 @@ enable_break (struct svr4_info *info, int from_tty)
>  		+ bfd_section_size (tmp_bfd, interp_sect);
>  	    }
> 
> -	  svr4_create_solib_event_breakpoints (target_gdbarch (), sym_addr);
> +	  svr4_create_solib_event_breakpoints (info, target_gdbarch (), sym_addr);
>  	  return 1;
>  	}
>      }
> @@ -2444,7 +2440,7 @@ enable_break (struct svr4_info *info, int from_tty)
> 
>        if (sym_addr != 0)
>  	{
> -	  svr4_create_solib_event_breakpoints (target_gdbarch (),
> +	  svr4_create_solib_event_breakpoints (info, target_gdbarch (),
>  					       load_addr + sym_addr);
>  	  return 1;
>  	}
> @@ -2470,7 +2466,8 @@ enable_break (struct svr4_info *info, int from_tty)
>  	  sym_addr = gdbarch_convert_from_func_ptr_addr (target_gdbarch (),
>  							 sym_addr,
>  							 current_top_target ());
> -	  svr4_create_solib_event_breakpoints (target_gdbarch (), sym_addr);
> +	  svr4_create_solib_event_breakpoints (info, target_gdbarch (),
> +					       sym_addr);
>  	  return 1;
>  	}
>      }
> @@ -2487,7 +2484,8 @@ enable_break (struct svr4_info *info, int from_tty)
>  	      sym_addr = gdbarch_convert_from_func_ptr_addr (target_gdbarch (),
>  							     sym_addr,
>  							     current_top_target ());
> -	      svr4_create_solib_event_breakpoints (target_gdbarch (), sym_addr);
> +	      svr4_create_solib_event_breakpoints (info, target_gdbarch (),
> +						   sym_addr);
>  	      return 1;
>  	    }
>  	}
> @@ -3010,7 +3008,7 @@ svr4_solib_create_inferior_hook (int from_tty)
>  {
>    struct svr4_info *info;
> 
> -  info = get_svr4_info ();
> +  info = get_svr4_info (current_program_space);
> 
>    /* Clear the probes-based interface's state.  */
>    free_probes_table (info);
> @@ -3036,7 +3034,7 @@ svr4_clear_solib (void)
>  {
>    struct svr4_info *info;
> 
> -  info = get_svr4_info ();
> +  info = get_svr4_info (current_program_space);
>    info->debug_base = 0;
>    info->debug_loader_offset_p = 0;
>    info->debug_loader_offset = 0;
> 

Thanks,
Pedro Alves

Re: [PATCH] solib-svr4: Pass down svr4_info as much as possible

[Simon Marchi]

On 2019-04-22 9:57 a.m., Pedro Alves wrote:
> On 4/19/19 4:03 PM, Simon Marchi wrote:
>> On 2019-04-10 10:49 p.m., Simon Marchi wrote:
>>> I am not able to reproduce the problem, and the test doesn't fail here, without
>>> the rest of the patch applied.  I think it's because on my system gdb doesn't use
>>> the probe based interface.
>>>
>>> Anyhow, the fix makes sense.  Since the probe is deleted on objfile destruction, the
>>> corresponding probe_and_action structures should too.
>>>
>>> Simon
>>
>> While reviewing this patch, I had written the patch below to experiment, and
>> while it's not super important, I think it's a good cleanup.
>>
>>
>> From aedf5f7d846672ba6edc2780853baa43f35dd3c4 Mon Sep 17 00:00:00 2001
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Date: Wed, 10 Apr 2019 22:02:33 -0400
>> Subject: [PATCH] solib-svr4: Pass down svr4_info as much as possible
>>
>> While reviewing
>>
>>   https://sourceware.org/ml/gdb-patches/2019-04/msg00141.html
>>
>> I noticed that we relied heavily on global state through the
>> get_svr4_info function, which uses current_program_space.  I thought we
>> could improve this (make things more explicit and easier to follow) by
>>
>> - Making get_svr4_info accept a program_space parameter, making it
>>   return the SVR4 info for that program space.
>> - Passing down the svr4_info object from callers as much as possible.
>>
>> This means looking up the svr4_info for the appropriate program space at
>> the entry points of the solib-svr4.c file and passing it down.  For now,
>> these entry points (most of them are "methods" of svr4_so_ops) rely on
>> current_program_space, but we can later try to change the target_so_ops
>> interface to pass down the program space.
> 
> Seems fine to me.  Please go ahead.

Pushed, thanks.

> Thinking a bit longer term we could end up passing down an inferior pointer
> around in functions in this file instead.  That's because we use target_gdbarch()
> in these routines, which is really inferior->gdbarch.  The program space can be
> found at inferior->pspace.  Etc.  Then again, the end up going target calls in
> a number of these routines, which implicitly refers to the current
> inferior/thread/pspace too...  Anyway, I'm happy with the patch as is, TBC.

Ok, thanks for the tip.

Simon