Re: [RFA] enable software single step on alpha-osf

[Joel Brobecker <brobecker@gnat.com>]

[-- Attachment #1: Type: text/plain, Size: 3370 bytes --]

> Did the re-ordering of that if statement get committed?
> This is wfi code so the smaller each change is the better.

[muttering something about too many things to do at the same time]

I tested (on Linux) and committed the attached patch. This is almost
the entire infrun.c change, except that I put the following change
on hold:

> >>The second part of the change is more tricky:
> >> +           stop_pc -= DECR_PC_AFTER_BREAK;

Concerning this particular change:

> >+           /* Readjust the stop_pc as it is off by DECR_PC_AFTER_BREAK
> >+              compared to the value it would have if the system stepping
> >+              capability was used. This allows the rest of the code in
> >+              this function to use this address without having to worry
> >+              whether software single step is in use or not.  */
> >+           stop_pc -= DECR_PC_AFTER_BREAK;
> >+           ecs->random_signal = 0;
> 
> Ok, I'm convinced that this is going in the right direction (and was 
> most likely the original intent of the code :-).  The more we can 
> localize DECR_PC_AFTER_BREAK (ie. extract it from bpstat_stop_status()) 
> the better! :-)
> 
> I this will be ok with some tweaks and a litte bit more experimentation.

Cool.

> I _think_ the above should be also writing the decremented stop_pc back 
> to the target.  This is what appears to happen else where.  It also 
> ensures that code that calls read_pc() instead of using stop_pc, gets a 
> consistent value.  However, there could be some sort of dragon lurking 
> here.  Can you please check this out?   The other possability is that 
> something is detecting that the PC wasn't written (status register) and 
> hence, assuming it should be decremented?  It might even be written by 
> resume().

It makes sense. I should be able to verify this change sometime soon.

> I think, the above code should also set a flag to indicate a ``software 
> single step trap'' occured.  That indication can then be passed down to 
> bpstat_stop_status() to so that it knows that this software single-step 
> trap is not a breakpoint and hence that the decr_pc_after_break doesn't 
> apply.  Off hand, a ``software_singlestep_p'' variable (||ed into 
> not_a_sw_breakpoint) or (better?) an extra enum trap_type (unknown_trap, 
> software_singlestep_trap) parameter explictly specifying the type of 
> trap that was identifed.

This needs to be investigated. So far, the not_a_sw_breakpoint seems
to be sufficient, but adding this flag would definitely make things
clearer, especially on the caller side.

May I suggest we treat this as a separate patch that would be applied
after all the issues in this RFA are dealt with?

> For the below, note that I've changed the variable name to 
> ``not_a_sw_breakpoint'' so you'll get a clash when merging.

Thanks.

So, to summarize: 

  1 - The change "+           stop_pc -= DECR_PC_AFTER_BREAK;" seems
      to be going in the right direction.

  2 - However I should hold this change for now because you think I should
      write the adjusted PC value back to the target, by adding something
      like "write_pc_pid (stop_pc, ecs->ptid)"

      I will verify the impact of such a change, and report.
    
  3 - Assuming we get all issues in this RFA resolved, then I will start
      looking at the addition of the software_singlestep flag.

-- 
Joel

[-- Attachment #2: infrun.c.diff --]
[-- Type: text/plain, Size: 1400 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.64
diff -c -r1.64 infrun.c
*** infrun.c	24 Jul 2002 14:38:55 -0000	1.64
--- infrun.c	16 Aug 2002 17:55:05 -0000
***************
*** 1826,1835 ****
  
    if (stop_signal == TARGET_SIGNAL_TRAP)
      {
!       if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
! 	ecs->random_signal = 0;
!       else if (breakpoints_inserted
! 	       && breakpoint_here_p (stop_pc - DECR_PC_AFTER_BREAK))
  	{
  	  ecs->random_signal = 0;
  	  if (!breakpoint_thread_match (stop_pc - DECR_PC_AFTER_BREAK,
--- 1826,1836 ----
  
    if (stop_signal == TARGET_SIGNAL_TRAP)
      {
!       /* Check if a regular breakpoint has been hit before checking
!          for a potential single step breakpoint. Otherwise, GDB will
!          not see this breakpoint hit when stepping onto breakpoints.  */
!       if (breakpoints_inserted
!           && breakpoint_here_p (stop_pc - DECR_PC_AFTER_BREAK))
  	{
  	  ecs->random_signal = 0;
  	  if (!breakpoint_thread_match (stop_pc - DECR_PC_AFTER_BREAK,
***************
*** 1885,1890 ****
--- 1886,1895 ----
  		}
  	    }
  	}
+       else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+         {
+           ecs->random_signal = 0;
+         }
      }
    else
      ecs->random_signal = 1;