On Tue, May 25, 2010 at 11:04, Hui Zhu wrote: > Thanks Pedro. > > On Sat, Jan 9, 2010 at 00:24, Pedro Alves wrote: >> On Monday 04 January 2010 14:23:21, Hui Zhu wrote: >>> Sorry guys, the prev patch is so ugly. >> >> :-) >> >>> Thanks for teach me clear about the gdbarch_software_single_step, Pedro. >>> I did some extend with your idea.  Because record_wait need >>> record_resume_step point out this resume is signal step or continue. >>> >>>       if (!step) >>>         { >>>           /* This is not hard single step.  */ >>>           if (!gdbarch_software_single_step_p (gdbarch)) >>>             { >>>               /* This is a normal continue.  */ >>>               step = 1; >>>             } >>>           else >>>             { >>>               /* This arch support soft sigle step.  */ >>>               if (single_step_breakpoints_inserted ()) >>>                 { >>>                   /* This is a soft single step.  */ >>>                   record_resume_step = 1; >>>                 } >>>               else >>>                 { >>>                   /* This is a continue. >>>                      Try to insert a soft single step breakpoint.  */ >>>                   if (!gdbarch_software_single_step (gdbarch, >>>                                                      get_current_frame ())) >>>                     { >>>                       /* This system don't want use soft single step. >>>                          Use hard sigle step.  */ >>>                       step = 1; >>>                     } >>>                 } >>>             } >>>         } >> >> Cool, this looks pretty clear to me now.  Thanks. >> >> >> >>> @@ -1077,6 +1111,7 @@ record_wait (struct target_ops *ops, >>>           /* This is not a single step.  */ >>>           ptid_t ret; >>>           CORE_ADDR tmp_pc; >>> +          struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid); >>> >>>           while (1) >>>             { >>> @@ -1099,6 +1134,9 @@ record_wait (struct target_ops *ops, >>>                   tmp_pc = regcache_read_pc (regcache); >>>                   aspace = get_regcache_aspace (regcache); >>> >>> +                  if (gdbarch_software_single_step_p (gdbarch)) >>> +                    remove_single_step_breakpoints (); >> >> This will gdb_assert inside remove_single_step_breakpoints >> if SSS bkpts are not inserted, but gdbarch_software_single_step_p >> returns true.  This instead is safer: >> >>                  if (single_step_breakpoints_inserted ()) >>                    remove_single_step_breakpoints (); > > OK.  I will fix it. > >> >> But, what if it was infrun that had inserted the single-step >> breakpoints, for a "next" or "step", etc.?  Shouldn't you check >> for record_resume_step too? >> >>               if (!record_resume_step && single_step_breakpoints_inserted ()) >>                 remove_single_step_breakpoints (); >> >> Otherwise, the check below for >> >>                  else if (breakpoint_inserted_here_p (aspace, tmp_pc)) >>                    { >>                      /* There is a breakpoint here.  Let the core >>                         handle it.  */ >>                      if (software_breakpoint_inserted_here_p (aspace, tmp_pc)) >>                        { >> >> would fail, and the finished single-step wouldn't be reported to the >> core, right? > > I think this single step will be handle by line: >      if (record_resume_step) >        { >          /* This is a single step.  */ >          return record_beneath_to_wait (record_beneath_to_wait_ops, >                                         ptid, status, options); >        } > >> >> >> Lastly, you may also want to confirm that the SSS bkpt managed by record.d itself explains the SIGTRAP before removing before issueing another >> single-step.  If any unexplainable SIGTRAP happens for any reason while >> single-stepping, you should report it to infrun instead.  In other words: >> >> With software single-stepping, we can distinguish most random >> SIGTRAPs from SSS SIGTRAPs, so: >> >>                      /* This must be a single-step trap.  Record the >>                         insn and issue another step.  */ >> >> ... the "must" here ends up being a bit too strong.  I'd certainly >> understand ignoring this for simplicity or performance reasons though. > > Ah.  Looks we didn't have good way to handle it.  I change this comment to: >                      /* This is a single-step trap.  Record the >                         insn and issue another step. >                         FIXME: this part can be a random SIGTRAP too. >                         But GDB cannot handle it.  */ > > > Shuchang,  could you try your code just use command si and > reverse-xxx.  If that part OK.  Please help me try this patch. > > Ping, please help me test this patch.  And about hellogcc, you can find us in: > https://groups.google.com/group/hellogcc > https://webchat.freenode.net/ #hellogcc > > Thanks, > Hui > > 2010-05-25  Hui Zhu   > >        * breakpoint.c (single_step_breakpoints_inserted): New >        function. >        * breakpoint.h (single_step_breakpoints_inserted): Extern. >        * record.c (record_resume): Add code for software single step. >        (record_wait): Ditto. > Hello, After do some test with Ping, I found some trouble and fixed them. 1. Add following: @@ -1134,8 +1176,20 @@ record_wait (struct target_ops *ops, break; } + if (gdbarch_software_single_step_p (gdbarch)) + { + /* Try to insert the software single step breakpoint. + If insert success, set step to 0. */ + set_executing (inferior_ptid, 0); + reinit_frame_cache (); + if (gdbarch_software_single_step (gdbarch, + get_current_frame ())) + step = 0; + set_executing (inferior_ptid, 1); + } This is because in record_wait, we cannot call get_current_frame () directly. And the frame message need refresh each exec cycle. 2. Ping found that reverse-exec cannot single step in RISC board. That is because "gdbarch_software_single_step" just can insert the breakpoint to the next addr. So I add following: @@ -1436,7 +1436,8 @@ maybe_software_singlestep (struct gdbarc { int hw_step = 1; - if (gdbarch_software_single_step_p (gdbarch) + if (execution_direction == EXEC_FORWARD + && gdbarch_software_single_step_p (gdbarch) && gdbarch_software_single_step (gdbarch, get_current_frame ())) If reverse, gdb will not user sss breakpoint to single step. 3. Ping got some gdb_assert in sometime. And I am not close to his board. So I didn't know what happen. So I add following: @@ -1534,7 +1535,8 @@ a command like `return' or `jump' to con /* If STEP is set, it's a request to use hardware stepping facilities. But in that case, we should never use singlestep breakpoint. */ - gdb_assert (!(singlestep_breakpoints_inserted_p && step)); + gdb_assert (!(execution_direction == EXEC_FORWARD + && singlestep_breakpoints_inserted_p && step)); The lost one still need be test. Thanks, Hui