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

["Peter.Schauer" <Peter dot Schauer at regent dot e-technik dot tu-muenchen dot de>]

> 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