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

[Andrew Cagney <ac131313@ges.redhat.com>]

Joel,

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

>> The second part of the change is more tricky:
>>  +           stop_pc -= DECR_PC_AFTER_BREAK;
>> Is it fixing any failures?  Software singlestep can be handled in two 
>> different ways:
>> - as a breakpoint
>> - as a hardware single step
>> and which is prefered decides if/when there should be a decrement.
> 
> 
> Yes, this one is actually fixing most of the failures.
> 
> I made several attempts at fixing the regressions before coming with
> this solution. This part of the code is quite tricky, and it seems to me
> that treating single-step breakpoints as hardware single step is the
> simplest way to handle them. I like this because the differences in
> processing between software and hardware single-step become smaller.
> See for instance the change in breakpoint.c which made the use of this
> macro disappear from this file.
> 
> 
>> Anyway, the thing I'm having trouble convincing myself that there can't 
>> be a double decrement -- eg for a hardware watchpoint or similar.
> 
> 
> I've tried as much as I can to make sure this can not happen, but I am
> not familiar enough to have a good level of confidence in my analysis.
> All I can say is: this patch fixes all the regressions observed in the
> testsuite after switching to software single step. I know this is no
> absolute proof, but that gives me a certain level of confidence.


> +           /* 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.

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().

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.

> +         }

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

> !   bp_addr = *pc - (not_a_breakpoint && !SOFTWARE_SINGLE_STEP_P () ? 
> !                    0 : DECR_PC_AFTER_BREAK);
>   
>     ALL_BREAKPOINTS_SAFE (b, temp)
>     {
> --- 2429,2435 ----
>        trap event.  For a trace/singlestep trap event, we would
>        not want to subtract DECR_PC_AFTER_BREAK from the PC. */
>   
> !   bp_addr = *pc - (not_a_breakpoint ? 0 : DECR_PC_AFTER_BREAK);
>   
>     ALL_BREAKPOINTS_SAFE (b, temp)
>     {

Andrew