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

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