[rfc] Eliminate write_register from solib-sunos.c

[rfc] Eliminate write_register from solib-sunos.c

[Ulrich Weigand]

Hello,

solib-sunos.c has a place where it adjusts the PC by DECR_PC_AFTER_BREAK.
Unlike all other places to do so, it uses write_register (PC_REGNUM ...).

As I'm trying to eliminate write_register, I'd like to get rid of this.
The following patch replaces the write_register call by write_pc.

Is this OK?

Bye,
Ulrich

ChangeLog:

	* solib-sunos.c (sunos_solib_create_inferior_hook): Use write_pc
	instead of write_register (PC_REGNUM, ...).


diff -urNp gdb-orig/gdb/solib-sunos.c gdb-head/gdb/solib-sunos.c
--- gdb-orig/gdb/solib-sunos.c	2007-01-11 20:57:59.000000000 +0100
+++ gdb-head/gdb/solib-sunos.c	2007-05-04 22:17:49.169681035 +0200
@@ -780,7 +780,7 @@ sunos_solib_create_inferior_hook (void)
   if (DECR_PC_AFTER_BREAK)
     {
       stop_pc -= DECR_PC_AFTER_BREAK;
-      write_register (PC_REGNUM, stop_pc);
+      write_pc (stop_pc);
     }
 
   if (!disable_break ())
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Eliminate write_register from solib-sunos.c

[Daniel Jacobowitz]

On Sat, May 12, 2007 at 02:31:13AM +0200, Ulrich Weigand wrote:
> Hello,
> 
> solib-sunos.c has a place where it adjusts the PC by DECR_PC_AFTER_BREAK.
> Unlike all other places to do so, it uses write_register (PC_REGNUM ...).
> 
> As I'm trying to eliminate write_register, I'd like to get rid of this.
> The following patch replaces the write_register call by write_pc.
> 
> Is this OK?

I noticed this call last week.  I'm not sure if any platform using
solib-sunos.c even triggers it... but if they do, I think it's
decrementing the PC twice.  Didn't infrun take care of it?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Eliminate write_register from solib-sunos.c

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Sat, May 12, 2007 at 02:31:13AM +0200, Ulrich Weigand wrote:
> > Hello,
> > 
> > solib-sunos.c has a place where it adjusts the PC by DECR_PC_AFTER_BREAK.
> > Unlike all other places to do so, it uses write_register (PC_REGNUM ...).
> > 
> > As I'm trying to eliminate write_register, I'd like to get rid of this.
> > The following patch replaces the write_register call by write_pc.
> > 
> > Is this OK?
> 
> I noticed this call last week.  I'm not sure if any platform using
> solib-sunos.c even triggers it... but if they do, I think it's
> decrementing the PC twice.  Didn't infrun take care of it?

Hmm, good point.  I think infrun *should* take care of it; solib-sunos.c
calls wait_for_inferior, which calls handle_inferior_event, which calls
adjust_pc_after_break.

Interestingly enough, it would appear that none of the Solaris targets
are actually using solib-sunos.o.  The only references I can find are
in various bsd *native* target's .mh files.

In fact, this is very weird as those same bsd targets have solib-svr4.o
in their .mt files.  Which one is getting used in that case?  I'll try
to find out what's going on here ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Eliminate write_register from solib-sunos.c

[Mark Kettenis]

> Date: Mon, 14 May 2007 17:00:12 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> Daniel Jacobowitz wrote:
> > On Sat, May 12, 2007 at 02:31:13AM +0200, Ulrich Weigand wrote:
> > > Hello,
> > > 
> > > solib-sunos.c has a place where it adjusts the PC by DECR_PC_AFTER_BREAK.
> > > Unlike all other places to do so, it uses write_register (PC_REGNUM ...).
> > > 
> > > As I'm trying to eliminate write_register, I'd like to get rid of this.
> > > The following patch replaces the write_register call by write_pc.
> > > 
> > > Is this OK?
> > 
> > I noticed this call last week.  I'm not sure if any platform using
> > solib-sunos.c even triggers it... but if they do, I think it's
> > decrementing the PC twice.  Didn't infrun take care of it?
> 
> Hmm, good point.  I think infrun *should* take care of it; solib-sunos.c
> calls wait_for_inferior, which calls handle_inferior_event, which calls
> adjust_pc_after_break.
> 
> Interestingly enough, it would appear that none of the Solaris targets
> are actually using solib-sunos.o.  The only references I can find are
> in various bsd *native* target's .mh files.

This is the shared library support for the old SunOS 4.x (aka Solaris
1.x).  Solaris 2.x (aka SunOS 5.x) uses SVR4-style shared libraries.

> In fact, this is very weird as those same bsd targets have solib-svr4.o
> in their .mt files.  Which one is getting used in that case?  I'll try
> to find out what's going on here ...

There are some a.out BSD targets that use solib-sunos.o.  In
particular OpenBSD/m68k still uses it.  Interestingly enough, that
target sets DECR_PC_AFTER_BREAK to 2.  I'll fire up my Quadra 800 and
investigate somewhere later this week.

Mark

Re: [rfc] Eliminate write_register from solib-sunos.c

[Daniel Jacobowitz]

On Mon, May 14, 2007 at 05:00:12PM +0200, Ulrich Weigand wrote:
> Interestingly enough, it would appear that none of the Solaris targets
> are actually using solib-sunos.o.  The only references I can find are
> in various bsd *native* target's .mh files.
> 
> In fact, this is very weird as those same bsd targets have solib-svr4.o
> in their .mt files.  Which one is getting used in that case?  I'll try
> to find out what's going on here ...

Don't confuse SunOS and Solaris :-) I believe that solib-sunos.c is
for a.out shared libraries.  However I don't know which one ends up
used... maybe they're getting lucky with link order?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Eliminate write_register from solib-sunos.c

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Mon, May 14, 2007 at 05:00:12PM +0200, Ulrich Weigand wrote:
> > Interestingly enough, it would appear that none of the Solaris targets
> > are actually using solib-sunos.o.  The only references I can find are
> > in various bsd *native* target's .mh files.
> > 
> > In fact, this is very weird as those same bsd targets have solib-svr4.o
> > in their .mt files.  Which one is getting used in that case?  I'll try
> > to find out what's going on here ...
> 
> Don't confuse SunOS and Solaris :-) I believe that solib-sunos.c is
> for a.out shared libraries.  However I don't know which one ends up
> used... maybe they're getting lucky with link order?

That's it: the TDEP files come *before* NAT files in link order,
thus the _initialize_svr4_solib function is called before
_initialize_sunos_solib in init.c, and as both set the 
current_target_so_ops variable, the second call wins.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Eliminate write_register from solib-sunos.c

[Ulrich Weigand]

Mark Kettenis wrote:

> > In fact, this is very weird as those same bsd targets have solib-svr4.o
> > in their .mt files.  Which one is getting used in that case?  I'll try
> > to find out what's going on here ...
> 
> There are some a.out BSD targets that use solib-sunos.o.  In
> particular OpenBSD/m68k still uses it.  Interestingly enough, that
> target sets DECR_PC_AFTER_BREAK to 2.  I'll fire up my Quadra 800 and
> investigate somewhere later this week.

Thanks!  It looks like the following native targets use solib-sunos.o
(this works due to init-order for native targets only; a cross-target
would attempt to use solib-svr4.o):

arm*-*-netbsd* (except arm*-*-netbsdelf*)
sparc*-*-netbsd* (except sparc*-*-netbsdelf*)
vax*-*-netbsd* (except vax*-*-netbsdelf*)

  None of those define DECR_PC_AFTER_BREAK.

i[34567]86-*-netbsd* (except i[34567]86-*-netbsdelf*)
i[34567]86-*-openbsd[0-2].* | i[34567]86-*-openbsd3.[0-3]

  These define DECR_PC_AFTER_BREAK to 1.

m68*-*-netbsd* (except m68*-*-netbsdelf*)
m68*-*-openbsd*

  These define DECR_PC_AFTER_BREAK to 2, as you point out.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Eliminate write_register from solib-sunos.c

[Ulrich Weigand]

Mark Kettenis wrote:

> There are some a.out BSD targets that use solib-sunos.o.  In
> particular OpenBSD/m68k still uses it.  Interestingly enough, that
> target sets DECR_PC_AFTER_BREAK to 2.  I'll fire up my Quadra 800 and
> investigate somewhere later this week.

Did you get around to investigate this?


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Eliminate write_register from solib-sunos.c

[Mark Kettenis]

> Date: Mon, 4 Jun 2007 20:53:46 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> Mark Kettenis wrote:
> 
> > There are some a.out BSD targets that use solib-sunos.o.  In
> > particular OpenBSD/m68k still uses it.  Interestingly enough, that
> > target sets DECR_PC_AFTER_BREAK to 2.  I'll fire up my Quadra 800 and
> > investigate somewhere later this week.
> 
> Did you get around to investigate this?

I started things but didn't finish.  And now I've forgotten what
exactly it was that needed to be tested :(.  Can you refresh my mind?

Re: [rfc] Eliminate write_register from solib-sunos.c

[Ulrich Weigand]

Mark Kettenis wrote:
> > Date: Mon, 4 Jun 2007 20:53:46 +0200 (CEST)
> > From: "Ulrich Weigand" <uweigand@de.ibm.com>
> > 
> > Mark Kettenis wrote:
> > 
> > > There are some a.out BSD targets that use solib-sunos.o.  In
> > > particular OpenBSD/m68k still uses it.  Interestingly enough, that
> > > target sets DECR_PC_AFTER_BREAK to 2.  I'll fire up my Quadra 800 and
> > > investigate somewhere later this week.
> > 
> > Did you get around to investigate this?
> 
> I started things but didn't finish.  And now I've forgotten what
> exactly it was that needed to be tested :(.  Can you refresh my mind?

The question was whether that "if (DECR_PC_AFTER_BREAK)" block in 
solib-sunos.c was in fact correct, or whether is should be removed.

As Dan suggested, I've in the meantime investigated that issue on
i386-openbsd3.3 under qemu, and found out that this block *is*
correct, because the breakpoint that is hit here was not actually
installed by GDB, but by the dynamic loader itself:

http://sourceware.org/ml/gdb-patches/2007-06/msg00276.html

So, if that's OK with you, I'd like to commit my original patch
that leaves the "if (DECR_PC_AFTER_BREAK)" block in, and simply
replaces the write_register call with write_pc.  I've now tested
that patch on my i386-openbsd3.3 setup with no regressions.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Eliminate write_register from solib-sunos.c

[Mark Kettenis]

> Date: Fri, 15 Jun 2007 18:58:29 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> Mark Kettenis wrote:
> > > Date: Mon, 4 Jun 2007 20:53:46 +0200 (CEST)
> > > From: "Ulrich Weigand" <uweigand@de.ibm.com>
> > > 
> > > Mark Kettenis wrote:
> > > 
> > > > There are some a.out BSD targets that use solib-sunos.o.  In
> > > > particular OpenBSD/m68k still uses it.  Interestingly enough, that
> > > > target sets DECR_PC_AFTER_BREAK to 2.  I'll fire up my Quadra 800 and
> > > > investigate somewhere later this week.
> > > 
> > > Did you get around to investigate this?
> > 
> > I started things but didn't finish.  And now I've forgotten what
> > exactly it was that needed to be tested :(.  Can you refresh my mind?
> 
> The question was whether that "if (DECR_PC_AFTER_BREAK)" block in 
> solib-sunos.c was in fact correct, or whether is should be removed.
> 
> As Dan suggested, I've in the meantime investigated that issue on
> i386-openbsd3.3 under qemu, and found out that this block *is*
> correct, because the breakpoint that is hit here was not actually
> installed by GDB, but by the dynamic loader itself:
> 
> http://sourceware.org/ml/gdb-patches/2007-06/msg00276.html
> 
> So, if that's OK with you, I'd like to commit my original patch
> that leaves the "if (DECR_PC_AFTER_BREAK)" block in, and simply
> replaces the write_register call with write_pc.  I've now tested
> that patch on my i386-openbsd3.3 setup with no regressions.

Fine with me.

Re: [rfc] Eliminate write_register from solib-sunos.c

[Daniel Jacobowitz]

On Fri, Jun 15, 2007 at 06:58:29PM +0200, Ulrich Weigand wrote:
> So, if that's OK with you, I'd like to commit my original patch
> that leaves the "if (DECR_PC_AFTER_BREAK)" block in, and simply
> replaces the write_register call with write_pc.  I've now tested
> that patch on my i386-openbsd3.3 setup with no regressions.

If you have a moment, could you add an explanation of why this is
necessary to the code?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Eliminate write_register from solib-sunos.c

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, Jun 15, 2007 at 06:58:29PM +0200, Ulrich Weigand wrote:
> > So, if that's OK with you, I'd like to commit my original patch
> > that leaves the "if (DECR_PC_AFTER_BREAK)" block in, and simply
> > replaces the write_register call with write_pc.  I've now tested
> > that patch on my i386-openbsd3.3 setup with no regressions.
> 
> If you have a moment, could you add an explanation of why this is
> necessary to the code?

Certainly.  I've committed the following patch.

Bye,
Ulrich


ChangeLog:

	* solib-sunos.c (sunos_solib_create_inferior_hook): Add comment
	explaining why the PC adjustment code is necessary.

diff -urNp gdb-orig/gdb/solib-sunos.c gdb-head/gdb/solib-sunos.c
--- gdb-orig/gdb/solib-sunos.c	2007-06-15 21:14:47.675137191 +0200
+++ gdb-head/gdb/solib-sunos.c	2007-06-16 00:07:15.018979384 +0200
@@ -775,7 +775,13 @@ sunos_solib_create_inferior_hook (void)
   /* We are now either at the "mapping complete" breakpoint (or somewhere
      else, a condition we aren't prepared to deal with anyway), so adjust
      the PC as necessary after a breakpoint, disable the breakpoint, and
-     add any shared libraries that were mapped in. */
+     add any shared libraries that were mapped in.
+
+     Note that adjust_pc_after_break did not perform any PC adjustment,
+     as the breakpoint the inferior just hit was not inserted by GDB,
+     but by the dynamic loader itself, and is therefore not found on
+     the GDB software break point list.  Thus we have to adjust the
+     PC here.  */
 
   if (gdbarch_decr_pc_after_break (current_gdbarch))
     {

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com