Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Michael Snyder]

Andrew Cagney wrote:
> 
> > But I have no idea how to detect the situation that you are
> > trying to test for.  So I'd like to toss it back to you.
> > Can you find a better way to test for this?  Maybe with
> > (ugh) another state variable?
> >
> > And if not, can you put those two lines into an ifdef,
> > so they won't affect targets for which they're not intended?
> 
> Well it can't be a (ugh) #ifdef :-)

Not even to temporarily circumvent a known bug?
I understand your objection (multi-arch, if not general
distaste), but there are lots of ifdefs in infrun.c already...

FYI, I was thinking along the lines of
	(currently_stepping (ecs)
#ifdef TM_i386SOL2_H
	&& !(step_range_end
	    && INNER_THAN (read_sp (), (step_sp - 16)))
#endif
	));

which would preserve the behavior on x86 solaris, while
protecting the rest of the world until Peter can come up
with a better implementation.

Michael

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Andrew Cagney]

> Well it can't be a (ugh) #ifdef :-)
> 
> 
> Not even to temporarily circumvent a known bug?


Not even to tempoarily circumvent a known bug.

> I understand your objection (multi-arch, if not general
> distaste), but there are lots of ifdefs in infrun.c already...
> 
> FYI, I was thinking along the lines of
> 	(currently_stepping (ecs)
> #ifdef TM_i386SOL2_H
> 	&& !(step_range_end
> 	    && INNER_THAN (read_sp (), (step_sp - 16)))
> #endif
> 	));
> 
> which would preserve the behavior on x86 solaris, while
> protecting the rest of the world until Peter can come up
> with a better implementation.


I honestly think it would be better to revert PeterS's N year old change 
and file a PR.

	Andrew

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Peter.Schauer]

Hellooooooo.

This ugly hack is coming back to haunt me every other year, and if I'd
know about a cleaner way to solve this, I'd already have done it long ago.

You already asked me about this `gem' in August 1999 and I tried to explain
the issue back then (see appended message below).
As of CVS GDB from May 27 2001 this hack is still needed for
Solaris x86 and Linux x86 (at least for a debian 2.2r2 release, for which
I have just run the signals.exp test with and without the hack).

Kevin Buettner also tried to come up with a better solution to no avail
(see second message below).

I think we have two options here:

- Back out the change unconditionally as Andrew suggested and live with
  the signals.exp regressions this will cause.

- Try to refine the test, so that the `serious bugs in debugging native
  Linux x86' are not triggered.
  Which problems are you encountering due to the hack ?

Or perhaps you are able to find a cleaner solution, which would be most
welcome.

Here are the old messages explaining the issue:

: Subject: Re: GDB: an old change in infrun.c
: To: msnyder@cygnus.com (Michael Snyder)
: Date: Sat, 28 Aug 99 15:00:48 MET DST
: Cc: kevinb@cygnus.com
: In-Reply-To: <37BDF4C2.3BD6@cygnus.com>; from "Michael Snyder" at Aug 20, 99 5:37 pm
: 
: Sorry for the late reply, I have been on vacation.
: 
: We didn't meet, but we had some GDB mail exchange in the past (it's also
: interesting to see kevinb work at Cygnus now).
: 
: Unfortunately I am no longer doing much work on GDB nowadays (hopefully
: I will be able to contribute again next year), below is what I found in
: my logs when handling the problem back then.
: 
: I don't know if the problem is still happening with the current GDB source
: (I don't even know if signals.exp is still the same).
: Back then I also added the following comment to signals.exp, which might
: contain some more information as well:
: 
:               # SVR4 and Linux based i*86 systems exhibit this behaviour
:               # as well (it is uncovered by the `continue from a break
:               # in a signal handler' test below).
:               # As these systems use procfs, where we tell the kernel not
:               # to tell gdb about `pass' signals, and the trace flag is
:               # cleared by the kernel before entering the sigtramp
:               # routine, GDB will not notice the execution of the signal
:               # handler.
:               # Upon return from the signal handler, GDB will receive
:               # a SIGTRAP from the set trace flag in the restored context.
:               # The SIGTRAP marks the end of a (albeit long winded)
:               # single step for GDB, causing this test to pass.
: 
: Executing signals.exp on Linux x86 with and without my change might be
: sufficient to verify if the hack is still needed.
: 
: 
: Here are my original notes:
: 
: The problem (observed under Solaris x86, SVR4 x86 and Linux):
: 
: If the inferior has a pending signal and a single step is issued, the
: kernel saves the process context on the stack and forces execution
: of the users signal handler with a cleared Trace bit.
: 
: 
: The testcase:
: 
: gdb.base/signals.exp when we next over a statement with a pending
: alarm signal and a breakpoint in the alarm signal handler.
: 
: 
: The symptom (before my infrun.c change):
: 
: After the `next' is issued, execution continues without an intervening
: stop in sigtramp (due to the cleared trace bit) until the breakpoint
: in the handler is reached. As DECR_PC_AFTER_BREAK is 1 for the affected
: targets, GDB thinks that it just executed a jump to the handler
: code, and does not recognize that a breakpoint was hit. As the breakpoint
: is not recognized, GDB does not adjust the PC for the breakpoint (as it
: normally would), continues stepping with a wrong PC, and all hell breaks
: loose.
: 
: 
: The (admittedly kludgy) solution:
: 
: Detect that we entered a signal handler handler frame, which usually
: pushes the context on the stack and therefor moves the stack pointer
: to a value which is discernible from a normal function call sequence.
: 
: 
: Please let me know if you have any further questions on the issue,
: 
: > Hi Peter, 
: > 
: > I don't think we've met -- I'm Michael Snyder, and I work 
: > on the GDB team at Cygnus.
: > 
: > I have a question about a change that you made last year.
: > I'm trying to understand the code that calls
: > bpstat_stop_status, and under what circumstances it decides
: > to stop and/or to report a breakpoint.
: > 
: > The change I'm referring to was covered by this ChangeLog:
: > Fri Apr 10 22:36:28 1998  Peter Schauer 
: >         * infrun.c (wait_for_inferior):  Handle breakpoint hit in
: >         signal handler without intervening stop in sigtramp.
: > 
: > and looks about like this:
: >         /* Don't even think about breakpoints
: >            if just proceeded over a breakpoint.
: > 
: >            However, if we are trying to proceed over a breakpoint
: >            and end up in sigtramp, then through_sigtramp_breakpoint
: >            will be set and we should check whether we've hit the
: >            step breakpoint.  */
: >         if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected
: >             && through_sigtramp_breakpoint == NULL)
: >           bpstat_clear (&stop_bpstat);
: >         else
: >           {
: >             /* See if there is a breakpoint at the current PC.  */
: >             stop_bpstat = bpstat_stop_status
: >               (&stop_pc,
: >                (DECR_PC_AFTER_BREAK ?
: >             /* Notice the case of stepping through a jump
: >                that lands just after a breakpoint.
: >                Don't confuse that with hitting the breakpoint.
: >                What we check for is that 1) stepping is going on
: >                and 2) the pc before the last insn does not match
: >                the address of the breakpoint before the current pc
: >                and 3) we didn't hit a breakpoint in a signal handler
: >                without an intervening stop in sigtramp, which is
: >                detected by a new stack pointer value below
: >                any usual function calling stack adjustments.  */
: >                 (currently_stepping (ecs)
: >                  && prev_pc != stop_pc - DECR_PC_AFTER_BREAK
: > +                 && !(step_range_end
: > +                      && INNER_THAN (read_sp (), (step_sp - 16)))) :
: >                 0)
: >               );
: >             /* Following in case break condition called a
: >                function.  */
: >             stop_print_frame = 1;
: >           }
: > 
: > Can you tell me what you were trying to accomplish
: > (eg. make it stop, make it keep going, make it print or
: > not print a breakpoint message), and under what 
: > circumstances?
: > 
: > 				Thanks,
: > 				Michael
:
:
: 
: Date: Fri, 17 Sep 1999 14:26:01 -0700
: From: Kevin Buettner <kevinb@cygnus.com>
: To: "Peter.Schauer" <Peter.Schauer@REGENT.E-TECHNIK.TU-MUENCHEN.DE>,
:         kevinb@cygnus.com (Kevin Buettner)
: Subject: Re: RFA: breakpoint.c and infrun.c changes
: 
: On Sep 17,  9:59pm, Peter.Schauer wrote:
: 
: > I did CC you on a reply I sent to msnyder on this issue a few weeks ago,
: > I append it below, in case the message got lost.
: 
: I did indeed receive your reply.  I am sorry for not acknowledging it.
: 
: > :               # SVR4 and Linux based i*86 systems exhibit this behaviour
: > :               # as well (it is uncovered by the `continue from a break
: > :               # in a signal handler' test below).
: > :               # As these systems use procfs, where we tell the kernel not
: > :               # to tell gdb about `pass' signals, and the trace flag is
: > :               # cleared by the kernel before entering the sigtramp
: > :               # routine, GDB will not notice the execution of the signal
: > :               # handler.
: > :               # Upon return from the signal handler, GDB will receive
: > :               # a SIGTRAP from the set trace flag in the restored context.
: > :               # The SIGTRAP marks the end of a (albeit long winded)
: > :               # single step for GDB, causing this test to pass.
: > : 
: > : Executing signals.exp on Linux x86 with and without my change might be
: > : sufficient to verify if the hack is still needed.
: 
: I have done this and (unfortunately) your hack is still needed.  I still
: think there's got to be a better way to do things, but unfortunately
: I don't (yet) know what it is.
: 
: Thanks for your help.
: 
: Kevin

> Hellooooooo, Peter Schauer!
> 
> Back in April of 1998 a patch from you was checked in (you may have
> submitted it some time earlier) for x86 Solaris.  Among other things,
> it contained a test in infrun.c (wait_for_inferior) that looked like:
> 
> 	[stuff...]
> 	(CURRENTLY_STEPPING ()
> 	&& prev_pc != stop_pc - DECR_PC_AFTER_BREAK
> 	&& !(step_range_end
> 	    && read_sp () INNER_THAN (step_sp - 16)));
> 
> A comment explains that the INNER_THAN expression is meant to detect
> hitting a breakpoint in a signal handler without an intervening stop
> in sigtramp.
> 
> The lines have metamorphosed since then, but the expression with
> (step_sp - 16) is still in there, and I would really like to get
> rid of it.  Especially since I have now found that it can cause
> serious bugs in debugging native Linux x86.
> 
> But I have no idea how to detect the situation that you are 
> trying to test for.  So I'd like to toss it back to you.
> Can you find a better way to test for this?  Maybe with 
> (ugh) another state variable?
> 
> And if not, can you put those two lines into an ifdef, 
> so they won't affect targets for which they're not intended?
> 
> 			Thanks a lot, 
> 			Michael

--
Peter Schauer			pes@regent.e-technik.tu-muenchen.de

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Michael Snyder]

"Peter.Schauer" wrote:
> 
> Hellooooooo.
> 
> This ugly hack is coming back to haunt me every other year, and if I'd
> know about a cleaner way to solve this, I'd already have done it long ago.
> 
> You already asked me about this `gem' in August 1999 and I tried to explain
> the issue back then (see appended message below).

> As of CVS GDB from May 27 2001 this hack is still needed for
> Solaris x86 and Linux x86 (at least for a debian 2.2r2 release, for which
> I have just run the signals.exp test with and without the hack).

Ah -- I see.  Yep, I get that failure too.  Good, now at least I know
how to manifest the problem that the code is trying to fix.  

> Kevin Buettner also tried to come up with a better solution to no avail
> (see second message below).
> 
> I think we have two options here:
> 
> - Back out the change unconditionally as Andrew suggested and live with
>   the signals.exp regressions this will cause.
> 
> - Try to refine the test, so that the `serious bugs in debugging native
>   Linux x86' are not triggered.
>   Which problems are you encountering due to the hack ?

Basically, there are any number of ways in which the current SP
may be off by 16 or more from the SP at the beginning of the step.
The most likely problem is that we hit a breakpoint in the middle
of a line (eg. a step_resume breakpoint), but this test tells us
that we are not at a breakpoint.  If that happens, we may report
a random SIGTRAP, or worse, we may attempt to resume the target
without first decrementhing the PC.

To demonstrate this problem, build the "gdb.base/break.exp" test case
and load it into GDB.  Set a breakpoint at line 79, the call to
printf(factorial(atoi("6))).  Now set a breakpoint on the last 
instruction of this line, and attempt a "next".  On my red hat 7.0
linux system, this causes the inferior to SEGV, because it resumes
after the breakpoint without decrementing the PC (because it failed
to recognize that it was at a breakpoint).

We will have to decide whether this is a worse problem than
the problem you were trying to fix.

Michael

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Michael Snyder]

Michael Snyder wrote:

> To demonstrate this problem, build the "gdb.base/break.exp" test case
> and load it into GDB.  Set a breakpoint at line 79, the call to
> printf(factorial(atoi("6))).  Now set a breakpoint on the last
> instruction of this line, and attempt a "next".  On my red hat 7.0
> linux system, this causes the inferior to SEGV, because it resumes
> after the breakpoint without decrementing the PC (because it failed
> to recognize that it was at a breakpoint).

Aaaaannnd I picked a bad example.  Your step_sp - 16 code is not the
cause of this segv (in fact this reveals _another_ bug).  Let me see
if I can reconstruct my example (it's been a long week and I did not
take notes).

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Michael Snyder]

"Peter.Schauer" wrote:
> 
> Hellooooooo.
> 
> This ugly hack is coming back to haunt me every other year, and if I'd
> know about a cleaner way to solve this, I'd already have done it long ago.

OK, would you review the attached change please?
It's meant to replace the (step_sp - 16) code by
using a state variable.  I set the state variable
when we resume the target with a random signal while
singlestepping, and I clear it if we enter a trampoline.
This passes signals.exp, and does not create any new failures
on i386-linux.

Michael
2001-06-27  Michael Snyder  <msnyder@redhat.com>

	* infrun.c (struct execution_control_state): Add new field, 
	stepping_with_random_signal.
	(init_execution_control_state): Initialize new field.
	(handle_inferior_event): If stepping and passing a random
	signal to the inferior, set 'stepping_with_random_signal'.
	Use this state variable to detect hitting a breakpoint in
	a signal handler without an intervening stop in sigtramp.
	(check_sigtramp2): Clear stepping_with_random_signal if we
	enter a signal trampoline.
	(prepare_to_wait): Clear stepping_with_random_signal.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.39
diff -c -3 -p -r1.39 infrun.c
*** infrun.c	2001/06/26 00:26:42	1.39
--- infrun.c	2001/06/27 21:55:10
*************** struct execution_control_state
*** 1200,1205 ****
--- 1200,1206 ----
      enum infwait_states infwait_state;
      ptid_t waiton_ptid;
      int wait_some_more;
+     int stepping_with_random_signal;
    };
  
  void init_execution_control_state (struct execution_control_state * ecs);
*************** fetch_inferior_event (void *client_data)
*** 1339,1344 ****
--- 1340,1347 ----
  void
  init_execution_control_state (struct execution_control_state *ecs)
  {
+   /* FIXME: memset?  We wouldn't have to keep adding inits for new fields. */
+   ecs->stepping_with_random_signal = 0;
    /* ecs->another_trap? */
    ecs->random_signal = 0;
    ecs->remove_breakpoints_on_following_step = 0;
*************** handle_inferior_event (struct execution_
*** 2097,2114 ****
  	else
  	  {
  	    /* See if there is a breakpoint at the current PC.  */
! 	    stop_bpstat = bpstat_stop_status
  	      (&stop_pc,
! 	    /* Pass TRUE if our reason for stopping is something other
! 	       than hitting a breakpoint.  We do this by checking that
! 	       1) stepping is going on and 2) we didn't hit a breakpoint
! 	       in a signal handler without an intervening stop in
! 	       sigtramp, which is detected by a new stack pointer value
! 	       below any usual function calling stack adjustments.  */
! 		(currently_stepping (ecs)
! 		 && !(step_range_end
! 		      && INNER_THAN (read_sp (), (step_sp - 16))))
! 	      );
  	    /* Following in case break condition called a
  	       function.  */
  	    stop_print_frame = 1;
--- 2100,2114 ----
  	else
  	  {
  	    /* See if there is a breakpoint at the current PC.  */
! 	    stop_bpstat = bpstat_stop_status 
  	      (&stop_pc,
! 	       /* Pass TRUE if our reason for stopping is something other
! 		  than hitting a breakpoint.  We do this by checking that
! 		  1) stepping is going on and 2) we didn't hit a breakpoint
! 		  in a signal handler without an intervening stop in
! 		  sigtramp, which is detected by a state variable.  */
! 	       currently_stepping (ecs)
! 	       && !ecs->stepping_with_random_signal);
  	    /* Following in case break condition called a
  	       function.  */
  	    stop_print_frame = 1;
*************** handle_inferior_event (struct execution_
*** 2257,2262 ****
--- 2257,2266 ----
             platforms.  --JimB, 20 May 1999 */
  	check_sigtramp2 (ecs);
  	keep_going (ecs);
+ 	/* Remember if we are stepping with a random signal. */
+ 	if (currently_stepping (ecs) &&
+ 	    signal_stop[stop_signal] == 0)
+ 	  ecs->stepping_with_random_signal = 1;
  	return;
        }
  
*************** check_sigtramp2 (struct execution_contro
*** 2965,2970 ****
--- 2969,2975 ----
  
        ecs->remove_breakpoints_on_following_step = 1;
        ecs->another_trap = 1;
+       ecs->stepping_with_random_signal = 0;
      }
  }
  
*************** prepare_to_wait (struct execution_contro
*** 3232,3237 ****
--- 3237,3243 ----
        registers_changed ();
        ecs->waiton_ptid = pid_to_ptid (-1);
        ecs->wp = &(ecs->ws);
+       ecs->stepping_with_random_signal = 0;
      }
    /* This is the old end of the while loop.  Let everybody know we
       want to wait for the inferior some more and get called again
From ac131313@cygnus.com Wed Jun 27 15:59:00 2001
From: Andrew Cagney <ac131313@cygnus.com>
To: Jim Blandy <jimb@cygnus.com>
Cc: gdb-patches@sources.redhat.com
Subject: Re: RFA: make D10V gdbarch functions static
Date: Wed, 27 Jun 2001 15:59:00 -0000
Message-id: <3B3A6556.1020300@cygnus.com>
References: <20010626184709.61B435E9CB@zwingli.cygnus.com>
X-SW-Source: 2001-06/msg00437.html
Content-length: 1135

> 2001-06-26  Jim Blandy  <jimb@redhat.com>
> 
> * d10v-tdep.c (10v_frame_chain_valid, d10v_use_struct_convention,
> 	d10v_breakpoint_from_pc, d10v_register_byte,
> 	d10v_register_raw_size, d10v_register_virtual_size,
> 	d10v_register_virtual_type, d10v_register_convertible,
> 	d10v_register_convert_to_virtual, d10v_register_convert_to_raw,
> 	d10v_make_daddr, d10v_make_iaddr, d10v_daddr_p, d10v_iaddr_p,
> 	d10v_convert_iaddr_to_raw, d10v_convert_daddr_to_raw,
> 	d10v_store_struct_return, d10v_store_return_value,
> 	d10v_extract_struct_value_address, d10v_frame_saved_pc,
> 	d10v_saved_pc_after_call, d10v_pop_frame, d10v_skip_prologue,
> 	d10v_frame_chain, d10v_frame_init_saved_regs,
> 	d10v_init_extra_frame_info, d10v_read_pc, d10v_write_pc,
> 	d10v_read_sp, d10v_write_sp, d10v_write_fp, d10v_read_fp,
> 	d10v_push_return_address, d10v_push_arguments,
> 	d10v_extract_return_value): Make these functions static.


I think everyone is pretty happy with the idea that static functions are 
a good thing.  BTW, if you're feeling daring, you could try deleting 
config/d10v/tm-d10v.h - I'd even call that obvious :-)

	Andrew

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Peter.Schauer]

Yes, that fixes it for Linux, which is ptrace based, but the problem persists
on Solaris x86, which is procfs based.

From procfs.c:

static int
register_gdb_signals (procinfo *pi, gdb_sigset_t *signals)
{
  int signo;

  for (signo = 0; signo < NSIG; signo ++)
    if (signal_stop_state  (target_signal_from_host (signo)) == 0 &&
        signal_print_state (target_signal_from_host (signo)) == 0 &&
        signal_pass_state  (target_signal_from_host (signo)) == 1)
      prdelset (signals, signo);
    else
      praddset (signals, signo);

  return proc_set_traced_signals (pi, signals);
}

This is a good thing, because it avoids unnecessary program stops, but as
a consequence there is no intervening stop in the signal trampoline, and
stepping_with_random_signal never gets set for the SIGALRM signal on procfs
based systems.

As I said, I think this can only be solved by looking at the new stack
pointer value, which should be way below the previous stack pointer value,
due to the pushed signal context.

Well, another solution might be to set the traced signal set unconditionally
to all signals before taking a step, but that might slow down things
considerably.

> This is a multi-part message in MIME format.
> --------------28602BB2E928B6CED30B543F
> Content-Type: text/plain; charset=us-ascii
> Content-Transfer-Encoding: 7bit
> 
> "Peter.Schauer" wrote:
> > 
> > Hellooooooo.
> > 
> > This ugly hack is coming back to haunt me every other year, and if I'd
> > know about a cleaner way to solve this, I'd already have done it long ago.
> 
> OK, would you review the attached change please?
> It's meant to replace the (step_sp - 16) code by
> using a state variable.  I set the state variable
> when we resume the target with a random signal while
> singlestepping, and I clear it if we enter a trampoline.
> This passes signals.exp, and does not create any new failures
> on i386-linux.
> 
> Michael
> --------------28602BB2E928B6CED30B543F
> Content-Type: text/plain; charset=us-ascii;
>  name="step_sp.diff"
> Content-Transfer-Encoding: 7bit
> Content-Disposition: inline;
>  filename="step_sp.diff"
> 
> 2001-06-27  Michael Snyder  <msnyder@redhat.com>
> 
> 	* infrun.c (struct execution_control_state): Add new field, 
> 	stepping_with_random_signal.
> 	(init_execution_control_state): Initialize new field.
> 	(handle_inferior_event): If stepping and passing a random
> 	signal to the inferior, set 'stepping_with_random_signal'.
> 	Use this state variable to detect hitting a breakpoint in
> 	a signal handler without an intervening stop in sigtramp.
> 	(check_sigtramp2): Clear stepping_with_random_signal if we
> 	enter a signal trampoline.
> 	(prepare_to_wait): Clear stepping_with_random_signal.
> 
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.39
> diff -c -3 -p -r1.39 infrun.c
> *** infrun.c	2001/06/26 00:26:42	1.39
> --- infrun.c	2001/06/27 21:55:10
> *************** struct execution_control_state
> *** 1200,1205 ****
> --- 1200,1206 ----
>       enum infwait_states infwait_state;
>       ptid_t waiton_ptid;
>       int wait_some_more;
> +     int stepping_with_random_signal;
>     };
>   
>   void init_execution_control_state (struct execution_control_state * ecs);
> *************** fetch_inferior_event (void *client_data)
> *** 1339,1344 ****
> --- 1340,1347 ----
>   void
>   init_execution_control_state (struct execution_control_state *ecs)
>   {
> +   /* FIXME: memset?  We wouldn't have to keep adding inits for new fields. */
> +   ecs->stepping_with_random_signal = 0;
>     /* ecs->another_trap? */
>     ecs->random_signal = 0;
>     ecs->remove_breakpoints_on_following_step = 0;
> *************** handle_inferior_event (struct execution_
> *** 2097,2114 ****
>   	else
>   	  {
>   	    /* See if there is a breakpoint at the current PC.  */
> ! 	    stop_bpstat = bpstat_stop_status
>   	      (&stop_pc,
> ! 	    /* Pass TRUE if our reason for stopping is something other
> ! 	       than hitting a breakpoint.  We do this by checking that
> ! 	       1) stepping is going on and 2) we didn't hit a breakpoint
> ! 	       in a signal handler without an intervening stop in
> ! 	       sigtramp, which is detected by a new stack pointer value
> ! 	       below any usual function calling stack adjustments.  */
> ! 		(currently_stepping (ecs)
> ! 		 && !(step_range_end
> ! 		      && INNER_THAN (read_sp (), (step_sp - 16))))
> ! 	      );
>   	    /* Following in case break condition called a
>   	       function.  */
>   	    stop_print_frame = 1;
> --- 2100,2114 ----
>   	else
>   	  {
>   	    /* See if there is a breakpoint at the current PC.  */
> ! 	    stop_bpstat = bpstat_stop_status 
>   	      (&stop_pc,
> ! 	       /* Pass TRUE if our reason for stopping is something other
> ! 		  than hitting a breakpoint.  We do this by checking that
> ! 		  1) stepping is going on and 2) we didn't hit a breakpoint
> ! 		  in a signal handler without an intervening stop in
> ! 		  sigtramp, which is detected by a state variable.  */
> ! 	       currently_stepping (ecs)
> ! 	       && !ecs->stepping_with_random_signal);
>   	    /* Following in case break condition called a
>   	       function.  */
>   	    stop_print_frame = 1;
> *************** handle_inferior_event (struct execution_
> *** 2257,2262 ****
> --- 2257,2266 ----
>              platforms.  --JimB, 20 May 1999 */
>   	check_sigtramp2 (ecs);
>   	keep_going (ecs);
> + 	/* Remember if we are stepping with a random signal. */
> + 	if (currently_stepping (ecs) &&
> + 	    signal_stop[stop_signal] == 0)
> + 	  ecs->stepping_with_random_signal = 1;
>   	return;
>         }
>   
> *************** check_sigtramp2 (struct execution_contro
> *** 2965,2970 ****
> --- 2969,2975 ----
>   
>         ecs->remove_breakpoints_on_following_step = 1;
>         ecs->another_trap = 1;
> +       ecs->stepping_with_random_signal = 0;
>       }
>   }
>   
> *************** prepare_to_wait (struct execution_contro
> *** 3232,3237 ****
> --- 3237,3243 ----
>         registers_changed ();
>         ecs->waiton_ptid = pid_to_ptid (-1);
>         ecs->wp = &(ecs->ws);
> +       ecs->stepping_with_random_signal = 0;
>       }
>     /* This is the old end of the while loop.  Let everybody know we
>        want to wait for the inferior some more and get called again
> 
> --------------28602BB2E928B6CED30B543F--
> 
> 
> 


-- 
Peter Schauer			pes@regent.e-technik.tu-muenchen.de

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Michael Snyder]

"Peter.Schauer" wrote:
> 
> Yes, that fixes it for Linux, which is ptrace based, but the problem persists
> on Solaris x86, which is procfs based.
> 
> >From procfs.c:
> 
> static int
> register_gdb_signals (procinfo *pi, gdb_sigset_t *signals)
> {
>   int signo;
> 
>   for (signo = 0; signo < NSIG; signo ++)
>     if (signal_stop_state  (target_signal_from_host (signo)) == 0 &&
>         signal_print_state (target_signal_from_host (signo)) == 0 &&
>         signal_pass_state  (target_signal_from_host (signo)) == 1)
>       prdelset (signals, signo);
>     else
>       praddset (signals, signo);
> 
>   return proc_set_traced_signals (pi, signals);
> }
> 
> This is a good thing, because it avoids unnecessary program stops, but as
> a consequence there is no intervening stop in the signal trampoline, and
> stepping_with_random_signal never gets set for the SIGALRM signal on procfs
> based systems.

Hmmmm... I disapprove of this!  I don't think the target back-end
should be messing about with signals like this.  Core gdb is perfectly
well capable of managing the signals, without the back-end doing an
end-run around it in this way.  And having the back-end do it
(while it may slightly improve performance) introduces exactly this
sort of problem because now core gdb has less information about
what is actually going on.

How would you feel about the idea of removing this "optimization"
from procfs?  Would that make my patch effective?  I am not aware
of any complaints about debugging performance on Solaris, so this
may be an optimization that is too costly for the benefit received.


> As I said, I think this can only be solved by looking at the new stack
> pointer value, which should be way below the previous stack pointer value,
> due to the pushed signal context.
> 
> Well, another solution might be to set the traced signal set unconditionally
> to all signals before taking a step, but that might slow down things
> considerably.

I would guess that it would not be percievable.

Michael

> 
> > This is a multi-part message in MIME format.
> > --------------28602BB2E928B6CED30B543F
> > Content-Type: text/plain; charset=us-ascii
> > Content-Transfer-Encoding: 7bit
> >
> > "Peter.Schauer" wrote:
> > >
> > > Hellooooooo.
> > >
> > > This ugly hack is coming back to haunt me every other year, and if I'd
> > > know about a cleaner way to solve this, I'd already have done it long ago.
> >
> > OK, would you review the attached change please?
> > It's meant to replace the (step_sp - 16) code by
> > using a state variable.  I set the state variable
> > when we resume the target with a random signal while
> > singlestepping, and I clear it if we enter a trampoline.
> > This passes signals.exp, and does not create any new failures
> > on i386-linux.
> >
> > Michael
> > --------------28602BB2E928B6CED30B543F
> > Content-Type: text/plain; charset=us-ascii;
> >  name="step_sp.diff"
> > Content-Transfer-Encoding: 7bit
> > Content-Disposition: inline;
> >  filename="step_sp.diff"
> >
> > 2001-06-27  Michael Snyder  <msnyder@redhat.com>
> >
> >       * infrun.c (struct execution_control_state): Add new field,
> >       stepping_with_random_signal.
> >       (init_execution_control_state): Initialize new field.
> >       (handle_inferior_event): If stepping and passing a random
> >       signal to the inferior, set 'stepping_with_random_signal'.
> >       Use this state variable to detect hitting a breakpoint in
> >       a signal handler without an intervening stop in sigtramp.
> >       (check_sigtramp2): Clear stepping_with_random_signal if we
> >       enter a signal trampoline.
> >       (prepare_to_wait): Clear stepping_with_random_signal.
> >
> > Index: infrun.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/infrun.c,v
> > retrieving revision 1.39
> > diff -c -3 -p -r1.39 infrun.c
> > *** infrun.c  2001/06/26 00:26:42     1.39
> > --- infrun.c  2001/06/27 21:55:10
> > *************** struct execution_control_state
> > *** 1200,1205 ****
> > --- 1200,1206 ----
> >       enum infwait_states infwait_state;
> >       ptid_t waiton_ptid;
> >       int wait_some_more;
> > +     int stepping_with_random_signal;
> >     };
> >
> >   void init_execution_control_state (struct execution_control_state * ecs);
> > *************** fetch_inferior_event (void *client_data)
> > *** 1339,1344 ****
> > --- 1340,1347 ----
> >   void
> >   init_execution_control_state (struct execution_control_state *ecs)
> >   {
> > +   /* FIXME: memset?  We wouldn't have to keep adding inits for new fields. */
> > +   ecs->stepping_with_random_signal = 0;
> >     /* ecs->another_trap? */
> >     ecs->random_signal = 0;
> >     ecs->remove_breakpoints_on_following_step = 0;
> > *************** handle_inferior_event (struct execution_
> > *** 2097,2114 ****
> >       else
> >         {
> >           /* See if there is a breakpoint at the current PC.  */
> > !         stop_bpstat = bpstat_stop_status
> >             (&stop_pc,
> > !         /* Pass TRUE if our reason for stopping is something other
> > !            than hitting a breakpoint.  We do this by checking that
> > !            1) stepping is going on and 2) we didn't hit a breakpoint
> > !            in a signal handler without an intervening stop in
> > !            sigtramp, which is detected by a new stack pointer value
> > !            below any usual function calling stack adjustments.  */
> > !             (currently_stepping (ecs)
> > !              && !(step_range_end
> > !                   && INNER_THAN (read_sp (), (step_sp - 16))))
> > !           );
> >           /* Following in case break condition called a
> >              function.  */
> >           stop_print_frame = 1;
> > --- 2100,2114 ----
> >       else
> >         {
> >           /* See if there is a breakpoint at the current PC.  */
> > !         stop_bpstat = bpstat_stop_status
> >             (&stop_pc,
> > !            /* Pass TRUE if our reason for stopping is something other
> > !               than hitting a breakpoint.  We do this by checking that
> > !               1) stepping is going on and 2) we didn't hit a breakpoint
> > !               in a signal handler without an intervening stop in
> > !               sigtramp, which is detected by a state variable.  */
> > !            currently_stepping (ecs)
> > !            && !ecs->stepping_with_random_signal);
> >           /* Following in case break condition called a
> >              function.  */
> >           stop_print_frame = 1;
> > *************** handle_inferior_event (struct execution_
> > *** 2257,2262 ****
> > --- 2257,2266 ----
> >              platforms.  --JimB, 20 May 1999 */
> >       check_sigtramp2 (ecs);
> >       keep_going (ecs);
> > +     /* Remember if we are stepping with a random signal. */
> > +     if (currently_stepping (ecs) &&
> > +         signal_stop[stop_signal] == 0)
> > +       ecs->stepping_with_random_signal = 1;
> >       return;
> >         }
> >
> > *************** check_sigtramp2 (struct execution_contro
> > *** 2965,2970 ****
> > --- 2969,2975 ----
> >
> >         ecs->remove_breakpoints_on_following_step = 1;
> >         ecs->another_trap = 1;
> > +       ecs->stepping_with_random_signal = 0;
> >       }
> >   }
> >
> > *************** prepare_to_wait (struct execution_contro
> > *** 3232,3237 ****
> > --- 3237,3243 ----
> >         registers_changed ();
> >         ecs->waiton_ptid = pid_to_ptid (-1);
> >         ecs->wp = &(ecs->ws);
> > +       ecs->stepping_with_random_signal = 0;
> >       }
> >     /* This is the old end of the while loop.  Let everybody know we
> >        want to wait for the inferior some more and get called again
> >
> > --------------28602BB2E928B6CED30B543F--
> >
> >
> >
> 
> --
> Peter Schauer                   pes@regent.e-technik.tu-muenchen.de

Re: [RFC] if (INNER_THAN (read_sp(), step_sp - 16))

[Peter.Schauer]

> How would you feel about the idea of removing this "optimization"
> from procfs?  Would that make my patch effective?  I am not aware
> of any complaints about debugging performance on Solaris, so this
> may be an optimization that is too costly for the benefit received.

Ah yes, removing the optimization altogether didn't occur to me.
It would indeed fix the signals.exp problem, if your patch is applied as well.

I am a little bit nervous about doing this though, as this optimization is
present in procfs.c from day one.

And as soon as the optimization is removed for SIGALRM, it causes pthreads.exp
to fail with

FAIL: gdb.threads/pthreads.exp: Continue to creation of second thread (timeout)

or with

lwp_to_thread: td_ta_map_lwp2thr: Debugger service failed.
FAIL: gdb.threads/pthreads.exp: Continue to creation of first thread

I currently do not have enough time (and expertise) to look at the cause of
this, but it causes similar problems with pthreads.exp on Sparc Solaris also,
as soon as SIGALRM is not passed through without an intervening stop.

You might be able to fix all this, but the change affects other platforms
as well (and SIGCHLD, SIGLWP, SIGWAITING and SIGCANCEL, which might be needed
to get passed through without an intervening stop for the thread library),
and I for one would not dare to make this radical change.

But I agree that it would be a much cleaner solution.

> "Peter.Schauer" wrote:
> > 
> > Yes, that fixes it for Linux, which is ptrace based, but the problem persists
> > on Solaris x86, which is procfs based.
> > 
> > >From procfs.c:
> > 
> > static int
> > register_gdb_signals (procinfo *pi, gdb_sigset_t *signals)
> > {
> >   int signo;
> > 
> >   for (signo = 0; signo < NSIG; signo ++)
> >     if (signal_stop_state  (target_signal_from_host (signo)) == 0 &&
> >         signal_print_state (target_signal_from_host (signo)) == 0 &&
> >         signal_pass_state  (target_signal_from_host (signo)) == 1)
> >       prdelset (signals, signo);
> >     else
> >       praddset (signals, signo);
> > 
> >   return proc_set_traced_signals (pi, signals);
> > }
> > 
> > This is a good thing, because it avoids unnecessary program stops, but as
> > a consequence there is no intervening stop in the signal trampoline, and
> > stepping_with_random_signal never gets set for the SIGALRM signal on procfs
> > based systems.
> 
> Hmmmm... I disapprove of this!  I don't think the target back-end
> should be messing about with signals like this.  Core gdb is perfectly
> well capable of managing the signals, without the back-end doing an
> end-run around it in this way.  And having the back-end do it
> (while it may slightly improve performance) introduces exactly this
> sort of problem because now core gdb has less information about
> what is actually going on.
> 
> How would you feel about the idea of removing this "optimization"
> from procfs?  Would that make my patch effective?  I am not aware
> of any complaints about debugging performance on Solaris, so this
> may be an optimization that is too costly for the benefit received.
> 
> 
> > As I said, I think this can only be solved by looking at the new stack
> > pointer value, which should be way below the previous stack pointer value,
> > due to the pushed signal context.
> > 
> > Well, another solution might be to set the traced signal set unconditionally
> > to all signals before taking a step, but that might slow down things
> > considerably.
> 
> I would guess that it would not be percievable.
> 
> Michael
> 
> > 
> > > This is a multi-part message in MIME format.
> > > --------------28602BB2E928B6CED30B543F
> > > Content-Type: text/plain; charset=us-ascii
> > > Content-Transfer-Encoding: 7bit
> > >
> > > "Peter.Schauer" wrote:
> > > >
> > > > Hellooooooo.
> > > >
> > > > This ugly hack is coming back to haunt me every other year, and if I'd
> > > > know about a cleaner way to solve this, I'd already have done it long ago.
> > >
> > > OK, would you review the attached change please?
> > > It's meant to replace the (step_sp - 16) code by
> > > using a state variable.  I set the state variable
> > > when we resume the target with a random signal while
> > > singlestepping, and I clear it if we enter a trampoline.
> > > This passes signals.exp, and does not create any new failures
> > > on i386-linux.
> > >
> > > Michael
> > > --------------28602BB2E928B6CED30B543F
> > > Content-Type: text/plain; charset=us-ascii;
> > >  name="step_sp.diff"
> > > Content-Transfer-Encoding: 7bit
> > > Content-Disposition: inline;
> > >  filename="step_sp.diff"
> > >
> > > 2001-06-27  Michael Snyder  <msnyder@redhat.com>
> > >
> > >       * infrun.c (struct execution_control_state): Add new field,
> > >       stepping_with_random_signal.
> > >       (init_execution_control_state): Initialize new field.
> > >       (handle_inferior_event): If stepping and passing a random
> > >       signal to the inferior, set 'stepping_with_random_signal'.
> > >       Use this state variable to detect hitting a breakpoint in
> > >       a signal handler without an intervening stop in sigtramp.
> > >       (check_sigtramp2): Clear stepping_with_random_signal if we
> > >       enter a signal trampoline.
> > >       (prepare_to_wait): Clear stepping_with_random_signal.
> > >
> > > Index: infrun.c
> > > ===================================================================
> > > RCS file: /cvs/src/src/gdb/infrun.c,v
> > > retrieving revision 1.39
> > > diff -c -3 -p -r1.39 infrun.c
> > > *** infrun.c  2001/06/26 00:26:42     1.39
> > > --- infrun.c  2001/06/27 21:55:10
> > > *************** struct execution_control_state
> > > *** 1200,1205 ****
> > > --- 1200,1206 ----
> > >       enum infwait_states infwait_state;
> > >       ptid_t waiton_ptid;
> > >       int wait_some_more;
> > > +     int stepping_with_random_signal;
> > >     };
> > >
> > >   void init_execution_control_state (struct execution_control_state * ecs);
> > > *************** fetch_inferior_event (void *client_data)
> > > *** 1339,1344 ****
> > > --- 1340,1347 ----
> > >   void
> > >   init_execution_control_state (struct execution_control_state *ecs)
> > >   {
> > > +   /* FIXME: memset?  We wouldn't have to keep adding inits for new fields. */
> > > +   ecs->stepping_with_random_signal = 0;
> > >     /* ecs->another_trap? */
> > >     ecs->random_signal = 0;
> > >     ecs->remove_breakpoints_on_following_step = 0;
> > > *************** handle_inferior_event (struct execution_
> > > *** 2097,2114 ****
> > >       else
> > >         {
> > >           /* See if there is a breakpoint at the current PC.  */
> > > !         stop_bpstat = bpstat_stop_status
> > >             (&stop_pc,
> > > !         /* Pass TRUE if our reason for stopping is something other
> > > !            than hitting a breakpoint.  We do this by checking that
> > > !            1) stepping is going on and 2) we didn't hit a breakpoint
> > > !            in a signal handler without an intervening stop in
> > > !            sigtramp, which is detected by a new stack pointer value
> > > !            below any usual function calling stack adjustments.  */
> > > !             (currently_stepping (ecs)
> > > !              && !(step_range_end
> > > !                   && INNER_THAN (read_sp (), (step_sp - 16))))
> > > !           );
> > >           /* Following in case break condition called a
> > >              function.  */
> > >           stop_print_frame = 1;
> > > --- 2100,2114 ----
> > >       else
> > >         {
> > >           /* See if there is a breakpoint at the current PC.  */
> > > !         stop_bpstat = bpstat_stop_status
> > >             (&stop_pc,
> > > !            /* Pass TRUE if our reason for stopping is something other
> > > !               than hitting a breakpoint.  We do this by checking that
> > > !               1) stepping is going on and 2) we didn't hit a breakpoint
> > > !               in a signal handler without an intervening stop in
> > > !               sigtramp, which is detected by a state variable.  */
> > > !            currently_stepping (ecs)
> > > !            && !ecs->stepping_with_random_signal);
> > >           /* Following in case break condition called a
> > >              function.  */
> > >           stop_print_frame = 1;
> > > *************** handle_inferior_event (struct execution_
> > > *** 2257,2262 ****
> > > --- 2257,2266 ----
> > >              platforms.  --JimB, 20 May 1999 */
> > >       check_sigtramp2 (ecs);
> > >       keep_going (ecs);
> > > +     /* Remember if we are stepping with a random signal. */
> > > +     if (currently_stepping (ecs) &&
> > > +         signal_stop[stop_signal] == 0)
> > > +       ecs->stepping_with_random_signal = 1;
> > >       return;
> > >         }
> > >
> > > *************** check_sigtramp2 (struct execution_contro
> > > *** 2965,2970 ****
> > > --- 2969,2975 ----
> > >
> > >         ecs->remove_breakpoints_on_following_step = 1;
> > >         ecs->another_trap = 1;
> > > +       ecs->stepping_with_random_signal = 0;
> > >       }
> > >   }
> > >
> > > *************** prepare_to_wait (struct execution_contro
> > > *** 3232,3237 ****
> > > --- 3237,3243 ----
> > >         registers_changed ();
> > >         ecs->waiton_ptid = pid_to_ptid (-1);
> > >         ecs->wp = &(ecs->ws);
> > > +       ecs->stepping_with_random_signal = 0;
> > >       }
> > >     /* This is the old end of the while loop.  Let everybody know we
> > >        want to wait for the inferior some more and get called again
> > >
> > > --------------28602BB2E928B6CED30B543F--
> > >
> > >
> > >

-- 
Peter Schauer			pes@regent.e-technik.tu-muenchen.de