Regression

Regression

[Mark Kettenis]

Hi Daniel,

The following commit breaks the signal command on OpenBSD.  Can you
please revert it for now?

2009-01-20  Daniel Jacobowitz  <dan@codesourcery.com>

        PR gdb/9346
        * infcmd.c (signal_command): Do not specify a resume PC.

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.230
retrieving revision 1.229
diff -u -p -r1.230 -r1.229
--- infcmd.c	20 Jan 2009 15:33:13 -0000	1.230
+++ infcmd.c	18 Jan 2009 17:42:16 -0000	1.229
@@ -1145,7 +1145,11 @@ signal_command (char *signum_exp, int fr
     }
 
   clear_proceed_status ();
-  proceed ((CORE_ADDR) -1, oursig, 0);
+  /* "signal 0" should not get stuck if we are stopped at a breakpoint.
+     FIXME: Neither should "signal foo" but when I tried passing
+     (CORE_ADDR)-1 unconditionally I got a testsuite failure which I haven't
+     tried to track down yet.  */
+  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0);
 }
 
 /* Proceed until we reach a different source line with pc greater than

Regression

[Mark Kettenis]

Hi Daniel,

The following commit breaks the signal command on OpenBSD.  Can you
please revert it for now?

2009-01-20  Daniel Jacobowitz  <dan@codesourcery.com>

        PR gdb/9346
        * infcmd.c (signal_command): Do not specify a resume PC.

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.230
retrieving revision 1.229
diff -u -p -r1.230 -r1.229
--- infcmd.c	20 Jan 2009 15:33:13 -0000	1.230
+++ infcmd.c	18 Jan 2009 17:42:16 -0000	1.229
@@ -1145,7 +1145,11 @@ signal_command (char *signum_exp, int fr
     }
 
   clear_proceed_status ();
-  proceed ((CORE_ADDR) -1, oursig, 0);
+  /* "signal 0" should not get stuck if we are stopped at a breakpoint.
+     FIXME: Neither should "signal foo" but when I tried passing
+     (CORE_ADDR)-1 unconditionally I got a testsuite failure which I haven't
+     tried to track down yet.  */
+  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0);
 }
 
 /* Proceed until we reach a different source line with pc greater than

Re: Regression

[Daniel Jacobowitz]

On Tue, Feb 10, 2009 at 04:17:08PM +0100, Mark Kettenis wrote:
> Hi Daniel,
> 
> The following commit breaks the signal command on OpenBSD.  Can you
> please revert it for now?

I'd rather fix OpenBSD than rebreak Linux.  Could you expand on the
problem?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Regression

[Mark Kettenis]

> Date: Tue, 10 Feb 2009 10:26:57 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Tue, Feb 10, 2009 at 04:17:08PM +0100, Mark Kettenis wrote:
> > Hi Daniel,
> > 
> > The following commit breaks the signal command on OpenBSD.  Can you
> > please revert it for now?
> 
> I'd rather fix OpenBSD than rebreak Linux.  Could you expand on the
> problem?

I don't quite understand what's happening here, but I get the
following new failure:

FAIL: gdb.base/annota1.exp: send SIGUSR1

and some more fallout from that (and similar failures in annota3.exp).
It seems that if we've just hit a breakpoint and then do "signal
SIGUSR1" we never enter the signal handler or ignore the breakpoint in
the signal handler.

I'm somewhat surprised to hear that the same test doesn't fail on x86
Linux for you.

Re: Regression

[Pedro Alves]

On Tuesday 10 February 2009 15:47:23, Mark Kettenis wrote:
> I don't quite understand what's happening here, but I get the
> following new failure:
> 
> FAIL: gdb.base/annota1.exp: send SIGUSR1
> 

I see that too.

> and some more fallout from that (and similar failures in annota3.exp).
> It seems that if we've just hit a breakpoint and then do "signal
> SIGUSR1" we never enter the signal handler or ignore the breakpoint in
> the signal handler.

Here's the infrun log on i386-openbsd4.3:

 (gdb) b printf
 Breakpoint 1 at 0x1c000408
 (gdb) r
 Starting program: /net/home/pedro/gdb/baseline/build-openbsd43/gdb/testsuite/gdb.base/annota1

 Breakpoint 1, printf (fmt=0x3c000001 "value is %d\n") at /usr/src/lib/libc/stdio/printf.c:43
 43      /usr/src/lib/libc/stdio/printf.c: No such file or directory.
         in /usr/src/lib/libc/stdio/printf.c
 (gdb) p $pc
 $1 = (void (*)()) 0xa43a633 <printf+19>
 (gdb) b handle_USR1
 Breakpoint 2 at 0x1c000613: file ../../../src/gdb/testsuite/gdb.base/annota1.c, line 18.
 (gdb) set debug infrun 1
 (gdb) signal SIGUSR1
 Continuing with signal SIGUSR1.
 infrun: clear_proceed_status_thread (process 27276)
 infrun: proceed (addr=0xffffffff, signal=30, step=0)
 infrun: resume (step=1, signal=30), trap_expected=1
 infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
 infrun: target_wait (-1, status) = 27276, status->kind = stopped, signal = SIGTRAP
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: stop_pc = 0xeff4636

Here, it seems like telling BSD to single-step, and, to deliver
a signal at the same time, just single-steps, resulting in a SIGTRAP.

 infrun: no stepping, continue
 infrun: resume (step=0, signal=0), trap_expected=0
 infrun: prepare_to_wait
 value is 7
 infrun: target_wait (-1, status) = 27276, status->kind = stopped, signal = SIGTRAP
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: stop_pc = 0xeff4633
 infrun: BPSTAT_WHAT_STOP_NOISY
 infrun: stop_stepping
 
 Breakpoint 1, printf (fmt=0x3c00000e "my_array[2] is %d\n") at /usr/src/lib/libc/stdio/printf.c:43
 43      in /usr/src/lib/libc/stdio/printf.c
 (gdb)







> 
> I'm somewhat surprised to hear that the same test doesn't fail on x86
> Linux for you.

It doesn't for me.

-- 
Pedro Alves

Re: Regression

[Tristan Gingold]

On Feb 10, 2009, at 5:06 PM, Pedro Alves wrote:
> (gdb) set debug infrun 1
> (gdb) signal SIGUSR1
> Continuing with signal SIGUSR1.
> infrun: clear_proceed_status_thread (process 27276)
> infrun: proceed (addr=0xffffffff, signal=30, step=0)
> infrun: resume (step=1, signal=30), trap_expected=1
> infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
> infrun: target_wait (-1, status) = 27276, status->kind = stopped,  
> signal = SIGTRAP
> infrun: infwait_normal_state
> infrun: TARGET_WAITKIND_STOPPED
> infrun: stop_pc = 0xeff4636
>
> Here, it seems like telling BSD to single-step, and, to deliver
> a signal at the same time, just single-steps, resulting in a SIGTRAP.

Fun: at the same time I was trying to fix the regression for darwin/x86.

I don't really understand the semantic of resume(step=1, signal=30).   
Does it mean first step and then
send a signal ?  If so wouldn't be simpler for targets if infrun ran  
two separate operations ?

Tristan.

Re: Regression

[Pedro Alves]

Opps, my logs didn't show something as clearly as I indended:

>  $1 = (void (*)()) 0xa43a633 <printf+19>

 vs 

>  infrun: stop_pc = 0xeff4636

> Here, it seems like telling BSD to single-step, and, to deliver
> a signal at the same time, just single-steps, resulting in a SIGTRAP.

Errr, here it tells that I re-ran the inferior to get first PC, after
having pased the stop_pc bit already, from a previous run.  Different runs,
different PCs, notice the randomization affecting the first bytes of the
addresses.  Sorry for any confusion it may have caused.

Here's a run, to show how OpenBSD forgets to single-step into
the handler:

 (gdb) p $pc
 $1 = (void (*)()) 0x51b0633 <printf+19>
 (gdb) signal SIGUSR1
 Continuing with signal SIGUSR1.
 infrun: clear_proceed_status_thread (process 18606)
 infrun: proceed (addr=0xffffffff, signal=30, step=0)
 infrun: resume (step=1, signal=30), trap_expected=1
 infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
 infrun: target_wait (-1, status) = 18606, status->kind = stopped, signal = SIGTRAP
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: stop_pc = 0x51b0636

Then, I've added a printf in the signal handler itself, to see
if it runs at all:

 (gdb) signal SIGUSR1
 Continuing with signal SIGUSR1.
 infrun: clear_proceed_status_thread (process 1505)
 infrun: proceed (addr=0xffffffff, signal=30, step=0)
 infrun: resume (step=1, signal=30), trap_expected=1
 infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
 handle_USR1
 ^^^^^^^^^^^ (1)
 infrun: target_wait (-1, status) = 1505, status->kind = stopped, signal = SIGTRAP
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: stop_pc = 0x6919636
 infrun: no stepping, continue
 infrun: resume (step=0, signal=0), trap_expected=0
 ^^^^^^^^^^^^^^ (2)
 infrun: prepare_to_wait
 my_array[2] is 3
 infrun: target_wait (-1, status) = 1505, status->kind = exited, status = 0
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_EXITED

So, it does run, according to (1) above, but, since we were stepping
over a breakpoint notice (trap_expected=1), GDB had removed breakpoints
from the inferior.  They're only inserted on the following resume, at (2).
That's why you miss the breakpoint in the signal handler.

Without Daniel's patch, "signal FOO" *would not remove breakpoints*, since
it was calling `proceed' like if it was a "jump".

Here's the same on i386-linux:

 Breakpoint 1, 0x00007ffff784ae40 in printf () from /lib/libc.so.6
 (gdb) set debug infrun 1
 (gdb) signal SIGUSR1
 Continuing with signal SIGUSR1.
 infrun: clear_proceed_status_thread (process 17569)
 infrun: proceed (addr=0xffffffffffffffff, signal=30, step=0)
 infrun: resume (step=1, signal=30), trap_expected=1
 infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
 infrun: target_wait (-1, status) = 17569, status->kind = stopped, signal = SIGTRAP
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: stop_pc = 0x400528
         ^^^^^^^^^^^^^^^^^^
 infrun: no stepping, continue
 infrun: resume (step=0, signal=0), trap_expected=0
 infrun: prepare_to_wait
 infrun: target_wait (-1, status) = 17569, status->kind = stopped, signal = SIGTRAP
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: stop_pc = 0x40052f
 infrun: BPSTAT_WHAT_STOP_NOISY
 infrun: stop_stepping

 Breakpoint 2, handle_USR1 (sig=10) at ../../../src/gdb/testsuite/gdb.base/annota1.c:19
 19      }

That stop was a finished single-step to the start of the signal handler:

 (gdb) disassemble 0x400528 (0x400528 + 100)
 Dump of assembler code from 0x400528 to 0x40058c:
 0x0000000000400528 <handle_USR1+0>:     push   %rbp
 0x0000000000400529 <handle_USR1+1>:     mov    %rsp,%rbp
 0x000000000040052c <handle_USR1+4>:     mov    %edi,-0x4(%rbp)
 0x000000000040052f <handle_USR1+7>:     leaveq
 0x0000000000400530 <handle_USR1+8>:     retq
 (...)

So, linux can single-step into a signal handler, OpenBSD doesn't.

-- 
Pedro Alves

Re: Regression

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Tue, 10 Feb 2009 18:00:35 +0000

Thanks Pedro, for looking into this!

> Opps, my logs didn't show something as clearly as I indended:
> 
> >  $1 = (void (*)()) 0xa43a633 <printf+19>
> 
>  vs 
> 
> >  infrun: stop_pc = 0xeff4636
> 
> > Here, it seems like telling BSD to single-step, and, to deliver
> > a signal at the same time, just single-steps, resulting in a SIGTRAP.
> 
> Errr, here it tells that I re-ran the inferior to get first PC, after
> having pased the stop_pc bit already, from a previous run.  Different runs,
> different PCs, notice the randomization affecting the first bytes of the
> addresses.  Sorry for any confusion it may have caused.
> 
> Here's a run, to show how OpenBSD forgets to single-step into
> the handler:
> 
>  (gdb) p $pc
>  $1 = (void (*)()) 0x51b0633 <printf+19>
>  (gdb) signal SIGUSR1
>  Continuing with signal SIGUSR1.
>  infrun: clear_proceed_status_thread (process 18606)
>  infrun: proceed (addr=0xffffffff, signal=30, step=0)
>  infrun: resume (step=1, signal=30), trap_expected=1
>  infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
>  infrun: target_wait (-1, status) = 18606, status->kind = stopped, signal = SIGTRAP
>  infrun: infwait_normal_state
>  infrun: TARGET_WAITKIND_STOPPED
>  infrun: stop_pc = 0x51b0636
> 
> Then, I've added a printf in the signal handler itself, to see
> if it runs at all:
> 
>  (gdb) signal SIGUSR1
>  Continuing with signal SIGUSR1.
>  infrun: clear_proceed_status_thread (process 1505)
>  infrun: proceed (addr=0xffffffff, signal=30, step=0)
>  infrun: resume (step=1, signal=30), trap_expected=1
>  infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
>  handle_USR1
>  ^^^^^^^^^^^ (1)
>  infrun: target_wait (-1, status) = 1505, status->kind = stopped, signal = SIGTRAP
>  infrun: infwait_normal_state
>  infrun: TARGET_WAITKIND_STOPPED
>  infrun: stop_pc = 0x6919636
>  infrun: no stepping, continue
>  infrun: resume (step=0, signal=0), trap_expected=0
>  ^^^^^^^^^^^^^^ (2)
>  infrun: prepare_to_wait
>  my_array[2] is 3
>  infrun: target_wait (-1, status) = 1505, status->kind = exited, status = 0
>  infrun: infwait_normal_state
>  infrun: TARGET_WAITKIND_EXITED
> 
> So, it does run, according to (1) above, but, since we were stepping
> over a breakpoint notice (trap_expected=1), GDB had removed breakpoints
> from the inferior.  They're only inserted on the following resume, at (2).
> That's why you miss the breakpoint in the signal handler.
> 
> Without Daniel's patch, "signal FOO" *would not remove breakpoints*, since
> it was calling `proceed' like if it was a "jump".
> 
> Here's the same on i386-linux:
> 
>  Breakpoint 1, 0x00007ffff784ae40 in printf () from /lib/libc.so.6
>  (gdb) set debug infrun 1
>  (gdb) signal SIGUSR1
>  Continuing with signal SIGUSR1.
>  infrun: clear_proceed_status_thread (process 17569)
>  infrun: proceed (addr=0xffffffffffffffff, signal=30, step=0)
>  infrun: resume (step=1, signal=30), trap_expected=1
>  infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
>  infrun: target_wait (-1, status) = 17569, status->kind = stopped, signal = SIGTRAP
>  infrun: infwait_normal_state
>  infrun: TARGET_WAITKIND_STOPPED
>  infrun: stop_pc = 0x400528
>          ^^^^^^^^^^^^^^^^^^
>  infrun: no stepping, continue
>  infrun: resume (step=0, signal=0), trap_expected=0
>  infrun: prepare_to_wait
>  infrun: target_wait (-1, status) = 17569, status->kind = stopped, signal = SIGTRAP
>  infrun: infwait_normal_state
>  infrun: TARGET_WAITKIND_STOPPED
>  infrun: stop_pc = 0x40052f
>  infrun: BPSTAT_WHAT_STOP_NOISY
>  infrun: stop_stepping
> 
>  Breakpoint 2, handle_USR1 (sig=10) at ../../../src/gdb/testsuite/gdb.base/annota1.c:19
>  19      }
> 
> That stop was a finished single-step to the start of the signal handler:
> 
>  (gdb) disassemble 0x400528 (0x400528 + 100)
>  Dump of assembler code from 0x400528 to 0x40058c:
>  0x0000000000400528 <handle_USR1+0>:     push   %rbp
>  0x0000000000400529 <handle_USR1+1>:     mov    %rsp,%rbp
>  0x000000000040052c <handle_USR1+4>:     mov    %edi,-0x4(%rbp)
>  0x000000000040052f <handle_USR1+7>:     leaveq
>  0x0000000000400530 <handle_USR1+8>:     retq
>  (...)
> 
> So, linux can single-step into a signal handler, OpenBSD doesn't.

Indeed.  OpenBSD/i386 (as well as NetBSD, FreeBSD and probably Darwin)
clears the trace bit before running the signal handler.

I've looked at the PR mentioned in the commi message, and I don't
quite understand how the change to the signal command has anything to
do with that.

Re: Regression

[Pedro Alves]

On Tuesday 10 February 2009 18:39:04, Mark Kettenis wrote:
> I've looked at the PR mentioned in the commi message, and I don't
> quite understand how the change to the signal command has anything to
> do with that.

Before Daniel's Daniel's patch, `signal_command' called `proceed'
like so:

+  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0);

For a signal != TARGET_SIGNAL_0, say signal FOO, that's effectivelly this:

 proceed (stop_pc, oursig, 0);



void
proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
{
(...)
  if (addr == (CORE_ADDR) -1)
    {
      if (pc == stop_pc && breakpoint_here_p (pc) 
	  && execution_direction != EXEC_REVERSE)
	/* There is a breakpoint at the address we will resume at,
	   step one instruction before inserting breakpoints so that
	   we do not stop right away (and report a second hit at this
	   breakpoint).

	   Note, we don't do this in reverse, because we won't
	   actually be executing the breakpoint insn anyway.
	   We'll be (un-)executing the previous instruction.  */

	oneproc = 1;
      else if (gdbarch_single_step_through_delay_p (gdbarch)
	       && gdbarch_single_step_through_delay (gdbarch,
						     get_current_frame ()))
	/* We stepped onto an instruction that needs to be stepped
	   again before re-inserting the breakpoint, do so.  */
	oneproc = 1;
    }
  else
    {
      regcache_write_pc (regcache, addr);
    }
(...)

This messed with syscall restarting on linux, since it was
writing the PC.  

Notice that the (addr != (CORE_ADDR) -1) code path doesn't
set `oneproc', hence, ends up *not* removing
breakpoints, and *not* single-stepping, even if we were stopped
at a breakpoint.  That is what I call the "jump" behaviour --- a jump
to $PC hits a breakpoint at $PC.

After Daniel's change, signal_command does this unconditionaly:

 proceed ((CORE_ADDR) -1, oursig, 0);

Which means we now go through the "(addr == (CORE_ADDR) -1)"
branch above.  This avoided the regcache_write_pc call.   But, it also
sets `oneproc' because in this case, there's a breakpoint at
stop_pc, and PC is still at stop_pc.  That will make us remove
breakpoints from the inferior, and call `resume' with step=1.
The part that's breakpoint the BSDs is the fact that we now
remove breakpoints from the inferior.

-- 
Pedro Alves

Re: Regression

[Pedro Alves]

Extended this a bit more:

On Tuesday 10 February 2009 19:05:37, Pedro Alves wrote:
> The part that's breaking the BSDs is the fact that we now
> remove breakpoints from the inferior.

On Tuesday 10 February 2009 18:39:04, Mark Kettenis wrote:
> Indeed.  OpenBSD/i386 (as well as NetBSD, FreeBSD and probably Darwin)
> clears the trace bit before running the signal handler.

We don't see the problem on linux, since there's a stop and
resume *right at the begining* of the signal handler, due to the
single-step entering the signal handler.  GDB happens
to re-install breakpoints in the inferior in that resume.  This
happens *before* the inferior's flow passing by the instruction
where there's a breakpoint at ("break handle_USR1" is installed
after the prologue).

-- 
Pedro Alves

Re: Regression

[Pedro Alves]

On Tuesday 10 February 2009 19:18:12, Pedro Alves wrote:
> We don't see the problem on linux, since there's a stop and
> resume *right at the begining* of the signal handler, due to the
> single-step entering the signal handler.  GDB happens
> to re-install breakpoints in the inferior in that resume.  This
> happens *before* the inferior's flow passing by the instruction
> where there's a breakpoint at ("break handle_USR1" is installed
> after the prologue).

I'm thinking that this regression is also happening on linux
archs that rely on software single-stepping, but I haven't
checked it.

-- 
Pedro Alves

Re: Regression

[Daniel Jacobowitz]

On Tue, Feb 10, 2009 at 07:27:44PM +0000, Pedro Alves wrote:
> On Tuesday 10 February 2009 19:18:12, Pedro Alves wrote:
> > We don't see the problem on linux, since there's a stop and
> > resume *right at the begining* of the signal handler, due to the
> > single-step entering the signal handler.  GDB happens
> > to re-install breakpoints in the inferior in that resume.  This
> > happens *before* the inferior's flow passing by the instruction
> > where there's a breakpoint at ("break handle_USR1" is installed
> > after the prologue).
> 
> I'm thinking that this regression is also happening on linux
> archs that rely on software single-stepping, but I haven't
> checked it.

We run automatic periodic testing and arm-linux is one of the covered
platforms - apologies for not having the GDB test results posted
anywhere, we do send out GCC logs.

Unfortunately annota1.exp does not run for cross testing, probably
because it relies on mixed GDB and inferior output.  And a binutils
bug caused the results to be garbage for a couple of months.  Which
means that I have before results, but not after; I'll keep an eye out
for the next batch.

-- 
Daniel Jacobowitz
CodeSourcery