From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 462 invoked by alias); 25 May 2013 21:05:48 -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 450 invoked by uid 89); 25 May 2013 21:05:47 -0000 X-Spam-SWARE-Status: No, score=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_CP autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sat, 25 May 2013 21:05:42 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r4PL5fbe025892 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 25 May 2013 17:05:41 -0400 Received: from host2.jankratochvil.net (ovpn-116-16.ams2.redhat.com [10.36.116.16]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r4PL5auF028021 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO) for ; Sat, 25 May 2013 17:05:39 -0400 Date: Sat, 25 May 2013 21:05:00 -0000 From: Jan Kratochvil To: gdb-patches@sourceware.org Subject: Re: [RFA 5/7 take 2] Improved linker-debugger interface Message-ID: <20130525210535.GA22288@host2.jankratochvil.net> References: <20130516144340.GA2105@blade.nx> <20130516144838.GF2105@blade.nx> <20130517185002.GA10447@host2.jankratochvil.net> <20130524083044.GC4602@blade.nx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130524083044.GC4602@blade.nx> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-05/txt/msg00939.txt.bz2 On Fri, 24 May 2013 10:30:44 +0200, Gary Benson wrote: > Jan Kratochvil wrote: > > Meaning of 'direct' seems ambiguous/unclear to me, maybe 'uncached'? > > Just a hint, OK even with 'direct'. [...] > Would "svr4_current_sos_1" be acceptable, or is the "_1" naming > discouraged? *_direct seems already more explanatory to me than *_1. Your new function comment explains it better: /* Read the full list of currently loaded shared objects directly from the inferior, without referring to any libraries read and stored by the probes interface. */ > > > + if (!svr4_read_so_list (lm, prev_lm, &link, 0)) > > > > You should set the last IGNORE_FIRST parameter properly. While > > glibc has "" there AFAIK some OSes like Solaris may have some valid > > pathname there which would confuse GDB listing the executable also > > as a shared library. > > I set IGNORE_FIRST to zero here because for this particular call > svr4_read_so_list never sees the first element in the list. If > solist_update_incremental is called at the top of the list then > the "if (info->solib_list == NULL) return 0;" at the top of > solist_update_incremental causes it to defer to solist_update_full. > That uses svr4_current_sos_direct, which does set IGNORE_FIRST > correctly. OK, that makes sense. But it does not seem obvious to me, add there a comment please, something like: /* The IGNORE_FIRST parameter does not need to be used as this point of code doing incremental fetch is never used for the first element of the list. solist_update_full is called for the first element instead. */ > > > @@ -1460,6 +2032,8 @@ enable_break (struct svr4_info *info, int from_tty) > > > info->interp_text_sect_low = info->interp_text_sect_high = 0; > > > info->interp_plt_sect_low = info->interp_plt_sect_high = 0; > > > > > > + free_probes_table (info); > > > > Why is this one needed and free_solib_list is not needed? > > free_solib_list is called in svr4_solib_create_inferior_hook. > I originally had the two calls together, in enable_break, but > doing it that way caused breakpoint resetting errors when the > inferior was restarted. There is a check for this in > info-shared.exp. When I add free_solib_list call there info-shared.exp still PASSes, even if I also remove the free_solib_list call from svr4_solib_create_inferior_hook. I do not see why free_solib_list should be called at one place and free_probes_table at other place, if it is really needed it would be worth a comment as I do not understand now why. The difference between placing it to svr4_solib_create_inferior_hook vs. placing it to enable_break is only in svr4_relocate_main_executable being called in the meantime. I may miss something but I do not see how svr4_relocate_main_executable could be dependent on the shared library list state. It is pre-approved if you call free_probes_table and free_solib_list together, otherwise I would like another reply mail about it. > +/* Copy library list. */ > + > +static struct so_list * > +svr4_copy_library_list (struct so_list *src) > +{ > + struct so_list *dst = NULL; > + struct so_list **link = &dst; > + > + while (src != NULL) > + { > + struct so_list *new; > + > + new = xmalloc (sizeof (struct so_list)); > + memcpy (new, src, sizeof (struct so_list)); > + > + if (src->lm_info != NULL) svr4 lm_info can never be NULL. If you refer to [patch] Fix crash in svr4_clear_so http://sourceware.org/ml/gdb-patches/2013-05/msg00792.html that could happen only when creating struct so_list in svr4_read_so_list but here struct so_list has to be already finished. Other solib-svr4.c code also assumes so_list->lm_info is not NULL. > + { > + new->lm_info = xmalloc (sizeof (struct lm_info)); > + memcpy (new->lm_info, src->lm_info, sizeof (struct lm_info)); > + } > + > + new->next = NULL; > + *link = new; > + link = &new->next; > + > + src = src->next; > + } > + > + return dst; > +} [...] > static int > -svr4_current_sos_via_xfer_libraries (struct svr4_library_list *list) > +svr4_current_sos_via_xfer_libraries (struct svr4_library_list *list, > + const char *annex) > { > char *svr4_library_document; > int result; > struct cleanup *back_to; > > + gdb_assert (annex == NULL || target_augmented_libraries_svr4_read()); GNU Coding Standards: gdb_assert (annex == NULL || target_augmented_libraries_svr4_read ()); > + > /* Fetch the list of shared libraries. */ > svr4_library_document = target_read_stralloc (¤t_target, > TARGET_OBJECT_LIBRARIES_SVR4, > - NULL); > + annex); > if (svr4_library_document == NULL) > return 0; > [...] > +/* Create and register solib event breakpoints. PROBES is an array > + of NUM_PROBES elements, each of which is vector of probes. A ^^^ Maybe "An" but you know better, nitpick. > + solib event breakpoint will be created and registered for each > + probe. */ > + > +static void > +svr4_create_probe_breakpoints (struct gdbarch *gdbarch, > + VEC (probe_p) **probes) [...] OK for commit with the changes above and depending on the free_probes_table and free_solib_list enable_break issue resolution. Thanks, Jan