From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21316 invoked by alias); 11 May 2007 23:31:41 -0000 Received: (qmail 21307 invoked by uid 22791); 11 May 2007 23:31:41 -0000 X-Spam-Check-By: sourceware.org Received: from ug-out-1314.google.com (HELO ug-out-1314.google.com) (66.249.92.175) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 11 May 2007 23:31:36 +0000 Received: by ug-out-1314.google.com with SMTP id 75so782966ugb for ; Fri, 11 May 2007 16:31:33 -0700 (PDT) Received: by 10.67.6.1 with SMTP id j1mr3158636ugi.1178926293779; Fri, 11 May 2007 16:31:33 -0700 (PDT) Received: from ?88.210.72.160? ( [88.210.72.160]) by mx.google.com with ESMTP id 29sm6917622uga.2007.05.11.16.31.31; Fri, 11 May 2007 16:31:33 -0700 (PDT) Message-ID: <4644FCC1.2080107@portugalmail.pt> Date: Fri, 11 May 2007 23:31:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; pt-BR; rv:1.8.0.10) Gecko/20070221 Thunderbird/1.5.0.10 Mnenhy/0.7.4.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: Re: [rfc / remote protocol] Remote shared library events References: <20070509201627.GA23422@caradoc.them.org> In-Reply-To: <20070509201627.GA23422@caradoc.them.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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 X-SW-Source: 2007-05/txt/msg00215.txt.bz2 Hi Daniel, all: First, let me say that I really like this patch. I find the segment generalization elegant. Makes a lot of sense. On 5/9/07, Daniel Jacobowitz wrote: > +static void > +solib_target_remove_one_solib (char *soname, int num_bases, > + CORE_ADDR *segment_bases) > +{ > + struct so_list **slot, *removed; > + > + /* We should already have queried the target for shared libraries > + before this point. If we haven't, we may have just connected; > + we'll be querying shortly. */ > + if (!solibs_fetched) > + return; > + > + for (slot = &solib_start; *slot != NULL; slot = &(*slot)->next) > + { > + int i; > + > + if (soname != NULL && strcmp (soname, (*slot)->so_name) != 0) > + continue; > + > + if (num_bases != (*slot)->lm_info->num_bases) > + continue; > + > + for (i = 0; i < num_bases; i++) > + if (segment_bases[i] != (*slot)->lm_info->segment_bases[i]) > + break; > + if (i < num_bases) > + continue; > + > + /* Got a match. */ > + break; > + } > + > + if (*slot == NULL) > + return; This doesn't match what was documented: "The name, the segment offsets, or both may be used to specify which DLL to unload" Specifically, if just the dll name comes along (numbases == 0), it will fail to find the so. Also, I see that you've changed the unloading related to the old version. In that version only one segment was enough on unload. Is it ever possible to have two sos with segments loaded at the same address? While we're on to it, why not drop the "TextSeg", and "DataSeg" segment names? You are hardcoding text to segment 0 and data to segment 1 on the gdb side (remote.c:parse_load_response), and as you've already noted, if some more segments need to be reported, then we'd have to add name pairs for them. Did you consider having packets like these instead?: load:Name=,Seg_0=,Seg_1=,Seg_2=; unload:Seg_0=; ... and have parse_load_response handle Seg_nnn generically? hummm, isn't there the possibility that 'TextSeg == seg[0] && DataSeg == seg[1]' isn't true in some cases, like when data is loaded before text in memory? > @@ -103,7 +107,11 @@ struct target_so_ops > Convenience function for remote debuggers finding host libs. */ > int (*find_and_open_solib) (char *soname, > unsigned o_flags, char **temp_pathname); > - > + > + void (*add_one_solib) (char *soname, int num_bases, > + CORE_ADDR *segment_bases); > + void (*remove_one_solib) (char *soname, int num_bases, > + CORE_ADDR *segment_bases); I see this in some places in gdb, and I've been meaning to ask. I see you're following status quo in that file, but why aren't these SONAME params in target_so_ops 'const char *' ? Shouldn't we strive for const correctness? Not really important for this patch, just curious. Cheers, Pedro Alves