From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70744 invoked by alias); 22 Apr 2019 13:57:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 70735 invoked by uid 89); 22 Apr 2019 13:57:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=filter, pm, Etc, Filter X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 22 Apr 2019 13:57:05 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D1C12308427C; Mon, 22 Apr 2019 13:57:03 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id EE59410027D9; Mon, 22 Apr 2019 13:57:02 +0000 (UTC) Subject: Re: [PATCH] solib-svr4: Pass down svr4_info as much as possible To: Simon Marchi , gdb-patches@sourceware.org References: <20190409131410.10205-1-palves@redhat.com> From: Pedro Alves Message-ID: <09d82f92-8f73-b2b1-827e-28cd7b7dc688@redhat.com> Date: Mon, 22 Apr 2019 13:57:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-04/txt/msg00419.txt.bz2 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 > 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 *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