Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder <msnyder@vmware.com>]

Hui, can you hold off on this change?

What I'm finding is that I can reproduce the bad behavior
that Pedro demonstrated on the 20080930 branch, but ONLY
if I omit Pedro's change in adjust_pc_after_break.  If I
add Pedro's change, I can no longer reproduce any bad
behavior.

I've added Pedro's change to the branch now -- why don't
you temporarily take out all these changes, and then see
if you can make it manifest a problem.


teawater wrote:
> Sorry for understand your mean so later Pedro. I made a new patch that
> Set pc if forward execute and gdbarch_decr_pc_after_break is not 0 in
> replay mode. How do you think about it?
> 
> 
> And I think 20080930 branch is need your "adjust_pc_reverse.diff". Do
> you mind I check it in?
> 
> On Tue, Oct 21, 2008 at 08:21, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Tuesday 21 October 2008 00:36:12, teawater wrote:
>>> I think your mean is check breakpoint in address
>>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?
>> Taking x86 as an example, when you're doing normal debugging and you
>> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
>> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
>> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
>> idea is for you to do the same as the kernel/hardware would --- still
>> check for breakpoints at read_pc, but increment PC by 1 before reporting the
>> breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
>> the patch I posted previously, something like:
>>
>> record.c:record_wait ()
>> {
>> ...
>> +         /* Check for breakpoint hits in forward execution.  */
>> +         pc = read_pc ();
>> +         if (execution_direction == EXEC_FORWARD
>> +             && regular_breakpoint_inserted_here_p (pc)
>> +             /* && !single-stepping */)
>> +           {
>> +             status->kind = TARGET_WAITKIND_STOPPED;
>> +             status->value.sig = TARGET_SIGNAL_TRAP;
>> +             if (software_breakpoint_inserted_here_p (pc))
>> +               {
>> +                 pc += gdbarch_decr_pc_after_break (gdbarch);
>> +                 write_pc (pc);
>> +               }
>> +
>>
>> --
>> Pedro Alves
>>
>>
>> ------------------------------------------------------------------------
>>
>> --- a/record.c
>> +++ b/record.c
>> @@ -497,6 +497,30 @@ record_wait (ptid_t ptid, struct target_
>>        int continue_flag = 1;
>>        int first_record_end = 1;
>>        struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
>> +      CORE_ADDR tmp_pc;
>> +
>> +      /* Check breakpoint when forward execute.  */
>> +      if (execution_direction == EXEC_FORWARD)
>> +	{
>> +	  tmp_pc = regcache_read_pc (regcache);
>> +	  if (breakpoint_inserted_here_p (tmp_pc))
>> +	    {
>> +	      if (record_debug)
>> +		{
>> +		  fprintf_unfiltered (gdb_stdlog,
>> +				      "Process record: break at 0x%s.\n",
>> +				      paddr_nz (tmp_pc));
>> +		}
>> +	      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache)))
>> +		{
>> +		  regcache_write_pc (regcache,
>> +				     tmp_pc +
>> +				     gdbarch_decr_pc_after_break
>> +				     (get_regcache_arch (regcache)));
>> +		}
>> +	      goto replay_out;
>> +	    }
>> +	}
>>  
>>        record_get_sig = 0;
>>        act.sa_handler = record_sig_handler;
>> @@ -588,10 +612,6 @@ record_wait (ptid_t ptid, struct target_
>>  	    }
>>  	  else
>>  	    {
>> -	      CORE_ADDR tmp_pc;
>> -	      struct bp_location *bl;
>> -	      struct breakpoint *b;
>> -
>>  	      if (record_debug > 1)
>>  		{
>>  		  fprintf_unfiltered (gdb_stdlog,
>> @@ -632,35 +652,24 @@ record_wait (ptid_t ptid, struct target_
>>  		    }
>>  
>>  		  /* check breakpoint */
>> -		  tmp_pc = read_pc ();
>> -		  for (bl = bp_location_chain; bl; bl = bl->global_next)
>> +		  tmp_pc = regcache_read_pc (regcache);
>> +		  if (breakpoint_inserted_here_p (tmp_pc))
>>  		    {
>> -		      b = bl->owner;
>> -		      gdb_assert (b);
>> -		      if (b->enable_state != bp_enabled
>> -			  && b->enable_state != bp_permanent)
>> -			continue;
>> -
>> -		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
>> -			  || b->type == bp_catch_vfork
>> -			  || b->type == bp_catch_exec
>> -			  || b->type == bp_hardware_watchpoint
>> -			  || b->type == bp_read_watchpoint
>> -			  || b->type == bp_access_watchpoint)
>> +		      if (record_debug)
>>  			{
>> -			  continue;
>> +			  fprintf_unfiltered (gdb_stdlog,
>> +					      "Process record: break at 0x%s.\n",
>> +					      paddr_nz (tmp_pc));
>>  			}
>> -		      if (bl->address == tmp_pc)
>> +		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
>> +			  && execution_direction == EXEC_FORWARD)
>>  			{
>> -			  if (record_debug)
>> -			    {
>> -			      fprintf_unfiltered (gdb_stdlog,
>> -						  "Process record: break at 0x%s.\n",
>> -						  paddr_nz (tmp_pc));
>> -			    }
>> -			  continue_flag = 0;
>> -			  break;
>> +			  regcache_write_pc (regcache,
>> +					     tmp_pc +
>> +					     gdbarch_decr_pc_after_break
>> +					     (get_regcache_arch (regcache)));
>>  			}
>> +		      continue_flag = 0;
>>  		    }
>>  		}
>>  	      if (execution_direction == EXEC_REVERSE)
>> @@ -691,6 +700,7 @@ next:
>>  	  perror_with_name (_("Process record: sigaction"));
>>  	}
>>  
>> +replay_out:
>>        if (record_get_sig)
>>  	{
>>  	  status->value.sig = TARGET_SIGNAL_INT;