Re: [rfc / remote protocol] Remote shared library events

[Pedro Alves <pedro_alves@portugalmail.pt>]

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 <drow@false.org> 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=<hex(name)>,Seg_0=<load_addr>,Seg_1=<load_addr>,Seg_2=<load_addr>;

unload:Seg_0=<load_addr>;

... 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