This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] infrun.c: use GDB_SIGNAL_0 when hidding signals, not GDB_SIGNAL_TRAP.


On 10/29/2013 04:57 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> IMO, it doesn't make sense to map random syscall, fork, etc. events to
> Pedro> GDB_SIGNAL_TRAP, and possible have the debuggee see that trap.  This
> Pedro> just seems conceptually wrong to me - these aren't real signals a
> Pedro> debuggee would ever see.  In fact, when stopped for those events, on
> Pedro> Linux, the debuggee isn't in a signal-stop -- there's no way to
> Pedro> resume-and-deliver-signal at that point, for example.
> [...]
> Pedro> Comments?
> 
> It definitely seems like a nice cleanup to me, at least as far as I
> understand this code.  I remember not understanding the choice between
> GDB_SIGNAL_TRAP and GDB_SIGNAL_0 here.
> 
> I am mildly nervous about all the other spots in gdb that check for
> GDB_SIGNAL_TRAP.

Yeah, so was I.  I had thought about this a lot, and tried to
come up with scenarios where it could matter.  That's how I found
this bug, for instance:

 [gdb/16062] stepi sometimes doesn't make progress
 https://sourceware.org/ml/gdb-patches/2013-10/msg00562.html

while I was trying to figure out whether the GDB_SIGNAL_TRAP
on the fork syscall mattered.

I gave it some more thought, and couldn't figure out anything
wrong with this.

As software single-step (sss) takes different paths in
infrun, that could be likewise affected, I spent the past
couple day and a half getting my sss rework branch (which
includes a series that makes x86_64 use software single-step)
into a good enough shape for testing.  I then regression
tested this patch (and a few others) on top of that branch,
native and gdbserver.  The results looked good.  I'm reasonably
confident I didn't break anything, but as always with infrun
stuff, you never really know with absolute certainty.  :-)

I realized I didn't explain my rationale for the change
in the real signal handling code.  See the "There's more."
part in the now extended commit log below.

I'm pushing this in.  I'm posting a follow up series
that hopefully makes things a little more obvious.

-------
infrun.c: use GDB_SIGNAL_0 when hidding signals, not GDB_SIGNAL_TRAP.

IMO, it doesn't make sense to map random syscall, fork, etc. events to
GDB_SIGNAL_TRAP, and possible have the debuggee see that trap.  This
just seems conceptually wrong to me - these aren't real signals a
debuggee would ever see.  In fact, when stopped for those events, on
Linux, the debuggee isn't in a signal-stop -- there's no way to
resume-and-deliver-signal at that point, for example.  E.g., when
stopped at a fork event:

 (gdb) catch fork
 Catchpoint 2 (fork)
 (gdb) c
 Continuing.

 Catchpoint 2 (forked process 4570), 0x000000323d4ba7c4 in __libc_fork () at ../nptl/sysdeps/unix/sysv/linux/fork.c:131
 131       pid = ARCH_FORK ();
 (gdb) set debug infrun 1
 (gdb) signal SIGTRAP
 Continuing with signal SIGTRAP.
 infrun: clear_proceed_status_thread (process 4566)
 infrun: proceed (addr=0xffffffffffffffff, signal=5, step=0)
 infrun: resume (step=0, signal=5), trap_expected=0, current thread [process 4566] at 0x323d4ba7c4
 infrun: wait_for_inferior ()
 infrun: target_wait (-1, status) =
 infrun:   4566 [process 4566],
 infrun:   status->kind = exited, status = 0
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_EXITED
 [Inferior 1 (process 4566) exited normally]
 infrun: stop_stepping
 (gdb)

Note the signal went nowhere.  It was swallowed.

Resuming with a SIGTRAP from a syscall event does queue the signal,
but doesn't deliver it immediately, like "signal SIGTRAP" from a real
signal would.  It's still an artificial SIGTRAP:

 (gdb) catch syscall
 Catchpoint 2 (any syscall)
 (gdb) c
 Continuing.

 Catchpoint 2 (call to syscall clone), 0x000000323d4ba7c4 in __libc_fork () at ../nptl/sysdeps/unix/sysv/linux/fork.c:131
 131       pid = ARCH_FORK ();
 (gdb) set debug infrun 1
 (gdb) signal SIGTRAP
 Continuing with signal SIGTRAP.
 infrun: clear_proceed_status_thread (process 4622)
 infrun: proceed (addr=0xffffffffffffffff, signal=5, step=0)
 infrun: resume (step=0, signal=5), trap_expected=0, current thread [process 4622] at 0x323d4ba7c4
 infrun: wait_for_inferior ()
 infrun: target_wait (-1, status) =
 infrun:   4622 [process 4622],
 infrun:   status->kind = exited syscall
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_SYSCALL_RETURN
 infrun: syscall number = '56'
 infrun: BPSTAT_WHAT_STOP_NOISY
 infrun: stop_stepping

 Catchpoint 2 (returned from syscall clone), 0x000000323d4ba7c4 in __libc_fork () at ../nptl/sysdeps/unix/sysv/linux/fork.c:131
 131       pid = ARCH_FORK ();
 (gdb) c
 Continuing.
 infrun: clear_proceed_status_thread (process 4622)
 infrun: proceed (addr=0xffffffffffffffff, signal=144, step=0)
 infrun: resume (step=0, signal=0), trap_expected=0, current thread [process 4622] at 0x323d4ba7c4
 infrun: wait_for_inferior ()
 infrun: target_wait (-1, status) =
 infrun:   4622 [process 4622],
 infrun:   status->kind = stopped, signal = SIGTRAP
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_STOPPED
 infrun: stop_pc = 0x323d4ba7c4
 infrun: random signal 5

 Program received signal SIGTRAP, Trace/breakpoint trap.
 infrun: stop_stepping
 0x000000323d4ba7c4 in __libc_fork () at ../nptl/sysdeps/unix/sysv/linux/fork.c:131
 131       pid = ARCH_FORK ();
 (gdb)

In all the above, I used 'signal SIGTRAP' to emulate 'handle SIGTRAP
pass'.  As described in "keep_going", 'handle SIGTRAP pass' does have
its place:

      /* Do not deliver GDB_SIGNAL_TRAP (except when the user
	 explicitly specifies that such a signal should be delivered
	 to the target program).  Typically, that would occur when a
	 user is debugging a target monitor on a simulator: the target
	 monitor sets a breakpoint; the simulator encounters this
	 breakpoint and halts the simulation handing control to GDB;
	 GDB, noting that the stop address doesn't map to any known
	 breakpoint, returns control back to the simulator; the
	 simulator then delivers the hardware equivalent of a
	 GDB_SIGNAL_TRAP to the program being debugged.	 */

... and I've made use of that myself when implementing/debugging
stubs/monitors.  But in these cases, treating these events as SIGTRAP
possibly injects signals in the debuggee they'd never see otherwise,
because you need to use ptrace to enable these special events, which
aren't real signals.

There's more.  Take this bit of handle_inferior_event, where we
determine whether a real signal (TARGET_WAITKIND_STOPPED) was random
or not:

  if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
    ecs->random_signal
      = !((bpstat_explains_signal (ecs->event_thread->control.stop_bpstat,
				   GDB_SIGNAL_TRAP)
	   != BPSTAT_SIGNAL_NO)
	  || stopped_by_watchpoint
	  || ecs->event_thread->control.trap_expected
	  || (ecs->event_thread->control.step_range_end
	      && (ecs->event_thread->control.step_resume_breakpoint
		  == NULL)));
  else
    {
      enum bpstat_signal_value sval;

      sval = bpstat_explains_signal (ecs->event_thread->control.stop_bpstat,
				     ecs->event_thread->suspend.stop_signal);
      ecs->random_signal = (sval == BPSTAT_SIGNAL_NO);

      if (sval == BPSTAT_SIGNAL_HIDE)
	ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_TRAP;
    }

Note that the

      if (sval == BPSTAT_SIGNAL_HIDE)
	ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_TRAP;

bit is only reacheable for signals != GDB_SIGNAL_TRAP.  AFAICS, sval
can only be BPSTAT_SIGNAL_HIDE if nothing in the bpstat returns
BPSTAT_SIGNAL_PASS.  So that excludes a "catch signal" for the signal
in question in the bpstat.  All other catchpoints that aren't based on
breakpoints behind the scenes call process_event_stop_test directly
(don't pass through here) (well, almost all: TARGET_WAITKIND_LOADED
does have a fall through, but only for STOP_QUIETLY or
STOP_QUIETLY_NO_SIGSTOP, which still return before this code is
reached).  Catchpoints that are implemented as breakpoints behind the
scenes can only appear in the bpstat if the signal was GDB_SIGNAL_TRAP
(bkpt_breakpoint_hit returns false otherwise).  So that leaves a
target reporting a hardware watchpoint hit with a signal other than
GDB_SIGNAL_TRAP.  And even then it looks quite wrong to me to
magically convert the signal into a GDB_SIGNAL_TRAP here too -- if the
user has set SIGTRAP to "handle pass", the program will see a trap
that gdb invented, not one the program would ever see without gdb in
the picture.

Tested on x86_64 Fedora 17.

gdb/
2013-10-31  Pedro Alves  <palves@redhat.com>

	* infrun.c (handle_syscall_event): Don't set or clear stop_signal.
	(handle_inferior_event) <TARGET_WAITKIND_FORKED,
	TARGET_WAITKIND_VFORKED>: Don't set stop_signal to
	GDB_SIGNAL_TRAP, or clear it.  Pass GDB_SIGNAL_0 to
	bpstat_explains signal, instead of GDB_SIGNAL_TRAP.
	<bpstat handling>: If the bpstat chain wants the signal to be
	hidden, then set stop_signal to GDB_SIGNAL_0 instead of
	GDB_SIGNAL_TRAP.
---
 gdb/infrun.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 17612d7..c9e2fe2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3105,19 +3105,17 @@ handle_syscall_event (struct execution_control_state *ecs)
 			      stop_pc, ecs->ptid, &ecs->ws);
 
       sval = bpstat_explains_signal (ecs->event_thread->control.stop_bpstat,
-				     GDB_SIGNAL_TRAP);
+				     GDB_SIGNAL_0);
       ecs->random_signal = sval == BPSTAT_SIGNAL_NO;
 
       if (!ecs->random_signal)
 	{
 	  /* Catchpoint hit.  */
-	  ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_TRAP;
 	  return 0;
 	}
     }
 
   /* If no catchpoint triggered for this, then keep going.  */
-  ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
   keep_going (ecs);
   return 1;
 }
@@ -3344,13 +3342,12 @@ handle_inferior_event (struct execution_control_state *ecs)
 
 	  sval
 	    = bpstat_explains_signal (ecs->event_thread->control.stop_bpstat,
-				      GDB_SIGNAL_TRAP);
+				      GDB_SIGNAL_0);
 	  ecs->random_signal = sval == BPSTAT_SIGNAL_NO;
 
 	  if (!ecs->random_signal)
 	    {
 	      /* A catchpoint triggered.  */
-	      ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_TRAP;
 	      process_event_stop_test (ecs);
 	      return;
 	    }
@@ -3629,7 +3626,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 	    stop_stepping (ecs);
 	  return;
 	}
-      ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_TRAP;
       process_event_stop_test (ecs);
       return;
 
@@ -3676,7 +3672,7 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 			      stop_pc, ecs->ptid, &ecs->ws);
       ecs->random_signal
 	= (bpstat_explains_signal (ecs->event_thread->control.stop_bpstat,
-				   GDB_SIGNAL_TRAP)
+				   GDB_SIGNAL_0)
 	   == BPSTAT_SIGNAL_NO);
 
       /* Note that this may be referenced from inside
@@ -3691,7 +3687,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
 	  keep_going (ecs);
 	  return;
 	}
-      ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_TRAP;
       process_event_stop_test (ecs);
       return;
 
@@ -4288,7 +4283,7 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
       ecs->random_signal = (sval == BPSTAT_SIGNAL_NO);
 
       if (sval == BPSTAT_SIGNAL_HIDE)
-	ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_TRAP;
+	ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
     }
 
   /* For the program's own signals, act according to
-- 
1.7.11.7


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]