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

[Joel Brobecker <brobecker at gnat dot com>]

> 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: infrun.c.diff
Description: Text document