[RFA] Handle solaris dynamic linker name change.

[RFA] Handle solaris dynamic linker name change.

[Vladimir Prus]

As reported by Joel in http://www.cygwin.com/ml/gdb/2007-03/msg00160.html,
on Solaris, the name of dynamic linker as seen by GDB changes midway.
As result, GDB concludes that dynamic linker was unloaded. Now, after
my breakpoint changes this actually has the effect of breaking pending
breakpoints -- because when GDB decides that dynamic linker is
unloaded, it disables the bp_shlib_event breakpoint, and it's never 
enabled back.

In past gdb version, when infrun.c handles BPSTAT_WHAT_CHECK_SHLIBS
it does:

        solib_add
                which calls update_solib_list
        re_enable_breakpoints_in_shlibs

The re_enable_breakpoint_in_shlibs looks at all breakpoints, and
enables back those which are in mapped shared libs.

In current gdb, the re_enable_breakpoints_in_shlibs code is
just not present. And breakpoint_re_set_one explicitly does
nothing about bpstat_shlib_event.

The solutions are:
1. Stop gdb from thinking dynamic linker was unloaded
2. Revive re_enable_breakpoints_in_shlibs
3. Make breakpoint_re_set one try to re-enable bp_shlib_event

I think (1) is best, since it will make (2) or (3) unnecessary.
The attached patch does (1) by just special casing the names of
libraries. A better approach would be to also make sure address
ranges of libraries are the same, but it would require rearranging 
the code a bit -- quite a bit. OK?

- Volodya

	Ignore change in name of dynamic linker during
	execution.  This also unbreaks pending breakpoints.
	* solist.h (struct target_so_ops): New field
	same.
	* solib-svr4.c (svr4_same): New.
	(_initialize_svr4_solib): Register svr4_same.
	* solib.c (update_solib_list): Use ops->same,
	if available.
---
 gdb/solib-svr4.c |   21 +++++++++++++++++++++
 gdb/solib.c      |   12 ++++++++++--
 gdb/solist.h     |    4 ++++
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 1d2737a..d227d32 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1569,6 +1569,26 @@ elf_lookup_lib_symbol (const struct objfile *objfile,
 		(objfile, name, linkage_name, domain, symtab);
 }
 
+static int
+svr4_same (struct so_list *gdb, struct so_list *inferior)
+{
+  if (! strcmp (gdb->so_original_name, inferior->so_original_name))
+    return 1;
+
+  /* On Solaris, when starting inferior we think that
+     dynamic linker is /usr/lib/ld.so.1, but later on,
+     the table of loaded shared libraries contains
+     /lib/ld.so.1.
+     Sometimes one file is a link to another, but sometimes
+     they have identical content, but are not linked to each
+     other.  */
+  if (strcmp (gdb->so_original_name, "/usr/lib/ld.so.1") == 0
+      && strcmp (inferior->so_original_name, "/lib/ld.so.1") == 0)
+    return 1;
+
+  return 0;
+}
+
 extern initialize_file_ftype _initialize_svr4_solib; /* -Wmissing-prototypes */
 
 void
@@ -1585,4 +1605,5 @@ _initialize_svr4_solib (void)
   svr4_so_ops.open_symbol_file_object = open_symbol_file_object;
   svr4_so_ops.in_dynsym_resolve_code = svr4_in_dynsym_resolve_code;
   svr4_so_ops.lookup_lib_global_symbol = elf_lookup_lib_symbol;
+  svr4_so_ops.same = svr4_same;
 }
diff --git a/gdb/solib.c b/gdb/solib.c
index 0ec0a51..d52852a 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -537,8 +537,16 @@ update_solib_list (int from_tty, struct target_ops *target)
 	 the inferior's current list.  */
       while (i)
 	{
-	  if (! strcmp (gdb->so_original_name, i->so_original_name))
-	    break;
+	  if (ops->same)
+	    {
+	      if (ops->same (gdb, i))
+		break;
+	    }
+	  else
+	    {
+	      if (! strcmp (gdb->so_original_name, i->so_original_name))
+		break;	      
+	    }
 
 	  i_link = &i->next;
 	  i = *i_link;
diff --git a/gdb/solist.h b/gdb/solist.h
index 279f1ad..18c547a 100644
--- a/gdb/solist.h
+++ b/gdb/solist.h
@@ -115,6 +115,10 @@ struct target_so_ops
 						 const domain_enum domain,
 						 struct symtab **symtab);
 
+    /* Given two so_list, first from GDB thread list and another
+       present in the list returned by current_sos, return 1 if
+       they are equal -- referring to the same library.  */
+    int (*same) (struct so_list *gdb, struct so_list *inferior);
   };
 
 /* Free the memory associated with a (so_list *).  */
-- 
1.5.3.5

Re: [RFA] Handle solaris dynamic linker name change.

[Doug Evans]

On Dec 24, 2007 9:48 AM, Vladimir Prus <vladimir@codesourcery.com> wrote:
> +  /* On Solaris, when starting inferior we think that
> +     dynamic linker is /usr/lib/ld.so.1, but later on,
> +     the table of loaded shared libraries contains
> +     /lib/ld.so.1.
> +     Sometimes one file is a link to another, but sometimes
> +     they have identical content, but are not linked to each
> +     other.  */
> +  if (strcmp (gdb->so_original_name, "/usr/lib/ld.so.1") == 0
> +      && strcmp (inferior->so_original_name, "/lib/ld.so.1") == 0)
> +    return 1;

Do we want to support "ld --dynamic-linker /my/ld.so.1" here (i.e.
user has specified a different ld.so), or if this happens is the bug
avoided?

Re: [RFA] Handle solaris dynamic linker name change.

[Vladimir Prus]

On Thursday 27 December 2007 21:10:31 Doug Evans wrote:
> On Dec 24, 2007 9:48 AM, Vladimir Prus <vladimir@codesourcery.com> wrote:
> > +  /* On Solaris, when starting inferior we think that
> > +     dynamic linker is /usr/lib/ld.so.1, but later on,
> > +     the table of loaded shared libraries contains
> > +     /lib/ld.so.1.
> > +     Sometimes one file is a link to another, but sometimes
> > +     they have identical content, but are not linked to each
> > +     other.  */
> > +  if (strcmp (gdb->so_original_name, "/usr/lib/ld.so.1") == 0
> > +      && strcmp (inferior->so_original_name, "/lib/ld.so.1") == 0)
> > +    return 1;
> 
> Do we want to support "ld --dynamic-linker /my/ld.so.1" here (i.e.
> user has specified a different ld.so), or if this happens is the bug
> avoided?

I would have expected the above to work -- as then gdb will think
dynamic linker is /my/ld.so.1 from the start, and it's not going
to change to anything else.

From previous discussion it's not clear why the name changes on
Solaris, but it appears the initial name is grabbed from ELF directly,
and will be /my/ld.so.1 in your case.

- Volodya

 

Re: [RFA] Handle solaris dynamic linker name change.

[Joel Brobecker]

Hi Volodya,

> The solutions are:
> 1. Stop gdb from thinking dynamic linker was unloaded
> 2. Revive re_enable_breakpoints_in_shlibs
> 3. Make breakpoint_re_set one try to re-enable bp_shlib_event
> 
> I think (1) is best, since it will make (2) or (3) unnecessary.

I agree.

> 	Ignore change in name of dynamic linker during
> 	execution.  This also unbreaks pending breakpoints.
> 	* solist.h (struct target_so_ops): New field
> 	same.
> 	* solib-svr4.c (svr4_same): New.
> 	(_initialize_svr4_solib): Register svr4_same.
> 	* solib.c (update_solib_list): Use ops->same,
> 	if available.

I really liked the idea of the patch, but I think your change
will affect more than just solaris. the solib-svr4 target_so_ops
is used by many targets, not just solaris ones.

Within the same idea, how about making this a gdbarch method/function?
The default value would be a function that does a *filename_cmp*
(first improvement introduced by your patch ;-), and on solaris,
we would provide a function that first does a filename_cmp, and
if that fails, then looks at the specific name.

I'd like the feedback from other maintainers about this.

Thanks,
-- 
Joel

Re: [RFA] Handle solaris dynamic linker name change.

[Daniel Jacobowitz]

On Thu, Jan 03, 2008 at 09:49:51PM -0800, Joel Brobecker wrote:
> Within the same idea, how about making this a gdbarch method/function?
> The default value would be a function that does a *filename_cmp*
> (first improvement introduced by your patch ;-), and on solaris,
> we would provide a function that first does a filename_cmp, and
> if that fails, then looks at the specific name.
> 
> I'd like the feedback from other maintainers about this.

I think it's fine with that improvement or without it; I really doubt
the code will trigger anywhere else.  My two cents.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Handle solaris dynamic linker name change.

[Joel Brobecker]

> > I'd like the feedback from other maintainers about this.
> 
> I think it's fine with that improvement or without it; I really doubt
> the code will trigger anywhere else.  My two cents.

OK, in that case, let's leave it the way Volodya original proposed it.
I'd just like us to add a comment in svr4_same to say that we don't
restrict the check to solaris, but that the chances of facing this
situation in any other OS are very small.

Looking again at the patch:

> 	Ignore change in name of dynamic linker during
> 	execution.  This also unbreaks pending breakpoints.
> 	* solist.h (struct target_so_ops): New field
> 	same.
> 	* solib-svr4.c (svr4_same): New.
> 	(_initialize_svr4_solib): Register svr4_same.
> 	* solib.c (update_solib_list): Use ops->same,
> 	if available.

Just a very very minor comment: Your fill-column looks very small, and
makes it harder to read your sentences. It's supposed to be 74. Don't
go out of your way to fix, though, in the grand scheme of things,
I don't think that you're breaking a GNU Coding standard rule (I just
did a quick check of the section detailing ChangeLogs).

> +  /* On Solaris, when starting inferior we think that
> +     dynamic linker is /usr/lib/ld.so.1, but later on,
> +     the table of loaded shared libraries contains
> +     /lib/ld.so.1.
> +     Sometimes one file is a link to another, but sometimes
> +     they have identical content, but are not linked to each
> +     other.  */

Same small fill-column ;-). Actually, the real comment is to remind
you that an extra comment as explained at the beginning of this email
would be appreciated.

> +  if (strcmp (gdb->so_original_name, "/usr/lib/ld.so.1") == 0
> +      && strcmp (inferior->so_original_name, "/lib/ld.so.1") == 0)
> +    return 1;

Is it always going to be in that order (original in /usr/lib and
gdb in /usr)? Or was this from experience?

> +    /* Given two so_list, first from GDB thread list and another
> +       present in the list returned by current_sos, return 1 if
> +       they are equal -- referring to the same library.  */
> +    int (*same) (struct so_list *gdb, struct so_list *inferior);

I am not a native English speaker, but the above sounds funny.
I suggest a slightly different version:


    /* Given two so_list objects, one from the GDB thread list
       and another from the list returned by current_sos, return 1
       if they represent the same library.  */

This is all I have - your patch is pre-approved with the comment
adjustments.  We can discuss my question about /usr/lib and /lib
as a separate patch (this is already an improvement that we don't
need to delay).

Thanks!
-- 
Joel

Re: [RFA] Handle solaris dynamic linker name change.

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 2928 bytes --]

On Friday 04 January 2008 18:30:20 Joel Brobecker wrote:
> > > I'd like the feedback from other maintainers about this.
> > 
> > I think it's fine with that improvement or without it; I really doubt
> > the code will trigger anywhere else.  My two cents.
> 
> OK, in that case, let's leave it the way Volodya original proposed it.
> I'd just like us to add a comment in svr4_same to say that we don't
> restrict the check to solaris, but that the chances of facing this
> situation in any other OS are very small.
> 
> Looking again at the patch:
> 
> > 	Ignore change in name of dynamic linker during
> > 	execution.  This also unbreaks pending breakpoints.
> > 	* solist.h (struct target_so_ops): New field
> > 	same.
> > 	* solib-svr4.c (svr4_same): New.
> > 	(_initialize_svr4_solib): Register svr4_same.
> > 	* solib.c (update_solib_list): Use ops->same,
> > 	if available.
> 
> Just a very very minor comment: Your fill-column looks very small, and
> makes it harder to read your sentences. It's supposed to be 74. Don't
> go out of your way to fix, though, in the grand scheme of things,
> I don't think that you're breaking a GNU Coding standard rule (I just
> did a quick check of the section detailing ChangeLogs).
> 
> > +  /* On Solaris, when starting inferior we think that
> > +     dynamic linker is /usr/lib/ld.so.1, but later on,
> > +     the table of loaded shared libraries contains
> > +     /lib/ld.so.1.
> > +     Sometimes one file is a link to another, but sometimes
> > +     they have identical content, but are not linked to each
> > +     other.  */
> 
> Same small fill-column ;-). Actually, the real comment is to remind
> you that an extra comment as explained at the beginning of this email
> would be appreciated.
> 
> > +  if (strcmp (gdb->so_original_name, "/usr/lib/ld.so.1") == 0
> > +      && strcmp (inferior->so_original_name, "/lib/ld.so.1") == 0)
> > +    return 1;
> 
> Is it always going to be in that order (original in /usr/lib and
> gdb in /usr)? Or was this from experience?

This is from experience. Since we don't know why Solaris behave this way,
we probably cannot guess if the other way round is possible, and how
to handle it.

> 
> > +    /* Given two so_list, first from GDB thread list and another
> > +       present in the list returned by current_sos, return 1 if
> > +       they are equal -- referring to the same library.  */
> > +    int (*same) (struct so_list *gdb, struct so_list *inferior);
> 
> I am not a native English speaker, but the above sounds funny.
> I suggest a slightly different version:
> 
> 
>     /* Given two so_list objects, one from the GDB thread list
>        and another from the list returned by current_sos, return 1
>        if they represent the same library.  */
> 
> This is all I have - your patch is pre-approved with the comment
> adjustments.  

Thanks. I've adjusted comments as shown in attachment and checked in.

- Volodya

[-- Attachment #2: delta.diff --]
[-- Type: text/x-diff, Size: 1861 bytes --]

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 32912ea..4c9e82d 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1575,13 +1575,12 @@ svr4_same (struct so_list *gdb, struct so_list *inferior)
   if (! strcmp (gdb->so_original_name, inferior->so_original_name))
     return 1;
 
-  /* On Solaris, when starting inferior we think that
-     dynamic linker is /usr/lib/ld.so.1, but later on,
-     the table of loaded shared libraries contains
-     /lib/ld.so.1.
-     Sometimes one file is a link to another, but sometimes
-     they have identical content, but are not linked to each
-     other.  */
+  /* On Solaris, when starting inferior we think that dynamic linker is
+     /usr/lib/ld.so.1, but later on, the table of loaded shared libraries 
+     contains /lib/ld.so.1.  Sometimes one file is a link to another, but 
+     sometimes they have identical content, but are not linked to each
+     other.  We don't restrict this check for Solaris, but the chances
+     of running into this situation elsewhere are very low.  */
   if (strcmp (gdb->so_original_name, "/usr/lib/ld.so.1") == 0
       && strcmp (inferior->so_original_name, "/lib/ld.so.1") == 0)
     return 1;
diff --git a/gdb/solist.h b/gdb/solist.h
index 915b137..01f734f 100644
--- a/gdb/solist.h
+++ b/gdb/solist.h
@@ -115,9 +115,9 @@ struct target_so_ops
 						 const domain_enum domain,
 						 struct symtab **symtab);
 
-    /* Given two so_list, first from GDB thread list and another
-       present in the list returned by current_sos, return 1 if
-       they are equal -- referring to the same library.  */
+    /* Given two so_list objects, one from the GDB thread list
+       and another from the list returned by current_sos, return 1
+       if they represent the same library.  */
     int (*same) (struct so_list *gdb, struct so_list *inferior);
   };