This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
> 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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |