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

[Michael Snyder <msnyder@vmware.com>]

teawater wrote:
> The old patch make my_waitpid_record set pc even if this is not a breakpoint.
> So I make a new patch that my_waitpid_record just set pc when this is
> a breakpoint.

Well, before I can evaluate the patch, I need a test case
to see what behavior it is fixing.  Doesn't have to be a
formal DEJAGNU script, just something like the printf example
that you posted for the other bug.

Right now, I am unable to get the reverse-20080930-branch
to exhibit any bad behavior that I could attribute to this
issue.  It seems to work just fine...


> 2008-10-24  Hui Zhu  <teawater@gmail.com>
> 
>         * record.c (record_wait): Check breakpint before forward
>         execute in replay mode.
>         Check breakpoint use function "breakpoint_inserted_here_p"
>         in replay mode.
>         Set pc if forward execute, gdbarch_decr_pc_after_break is not
>         0 and this is not single step in replay mode.
> 
>         * linux-nat.c (my_waitpid_record): Add
>         gdbarch_decr_pc_after_break to pc if need.
> 
> 
> 
> On Fri, Oct 24, 2008 at 17:57, teawater <teawater@gmail.com> wrote:
>> Hi buddies,
>>
>> This is the new patch that fix the break bug.
>>
>> But I think I still need to add some code to deal with signal.
>>
>> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_wait): Check breakpint before forward
>>        execute in replay mode.
>>        Check breakpoint use function "breakpoint_inserted_here_p"
>>        in replay mode.
>>        Set pc if forward execute, gdbarch_decr_pc_after_break is not
>>        0 and this is not single step in replay mode.
>>
>>        * linux-nat.c (my_waitpid_record): Add
>>        gdbarch_decr_pc_after_break to pc if need.
>>
>> Thanks,
>> Hui
>>
>> On Fri, Oct 24, 2008 at 16:10, teawater <teawater@gmail.com> wrote:
>>> Thanks Pedro and Michael,
>>>
>>> I think the reason is P record let inferior step recycle in the
>>> linux-nat target.
>>> So when it break by breakpint, it will not let
>>> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
>>> adjust_pc_after_break, The PC is error.
>>>
>>> I will try to deal with it.
>>>
>>> Hui
>>>
>>> On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com> wrote:
>>>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>>>>>> In sum, it appears that decr_pc_after_break doesn't matter when you have
>>>>>> continguous breakpoints, as long as you get from from B1's address to B2's
>>>>>> address by single-stepping.  All is good then, it appears!
>>>>> I agree, at least that is the conclusion I am leaning toward.
>>>>>
>>>> Not so fast!  I knew I had to spend a little extra thinking about
>>>> it, 'cause I knew something was broken, just couldn't find what.  :-)
>>>> *as long as you get from from B1's address to B2's address
>>>> by single-stepping* was a restriction that doesn't always apply.
>>>>
>>>> Here's a test that will fail in forward record/replay mode, but not
>>>> in normal "play" mode.
>>>>
>>>> volatile int global_foo = 0;
>>>>
>>>> int
>>>> main (int argc, char **argv)
>>>> {
>>>>  asm ("nop"); /* 1st insn */
>>>>  asm ("nop"); /* 2nd insn */
>>>>  asm ("nop"); /* 3rd insn */
>>>>  asm ("nop"); /* 4th insn */
>>>>  if (!global_foo)
>>>>    goto ahead;
>>>>  asm ("nop"); /* 5th insn */
>>>>  asm ("nop"); /* 6th insn */
>>>>  asm ("nop"); /* 7th insn */
>>>>  asm ("nop"); /* 8th insn */  <<< break 1 here
>>>>  ahead:
>>>>  asm ("nop"); /* 9th insn */  <<< break 2 here
>>>>  end:
>>>>  return 0;
>>>> }
>>>>
>>>> If you let the program reply until break 2 is hit, and assuming insn
>>>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
>>>> see that adjust_pc_after_break will indeed make it appear that breakpoint
>>>> 1 was hit.  Now, nops are nops, but real code could have something
>>>> else there...
>>>>
>>>> /me goes back to bed.
>>>>
>>>> --
>>>> Pedro Alves
>>>>
>>
>> ------------------------------------------------------------------------
>>
>> --- a/linux-nat.c
>> +++ b/linux-nat.c
>> @@ -514,7 +514,9 @@ my_waitpid_record (int pid, int *status,
>>    struct bp_location *bl;
>>    struct breakpoint *b;
>>    CORE_ADDR pc;
>> +  CORE_ADDR decr_pc_after_break;
>>    struct lwp_info *lp;
>> +  int is_breakpoint = 1;
>>  
>>  wait_begin:
>>    ret = my_waitpid (pid, status, flags);
>> @@ -530,7 +532,7 @@ wait_begin:
>>  
>>    if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP)
>>      {
>> -      /* Check if there is a breakpoint */
>> +      /* Check if there is a breakpoint.  */
>>        pc = 0;
>>        registers_changed ();
>>        for (bl = bp_location_chain; bl; bl = bl->global_next)
>> @@ -602,7 +604,26 @@ wait_begin:
>>        goto wait_begin;
>>      }
>>  
>> +  is_breakpoint = 0;
>> +
>>  out:
>> +  /* Add gdbarch_decr_pc_after_break to pc because pc will be break at address
>> +     add gdbarch_decr_pc_after_break when inferior non-step execute.  */
>> +  if (is_breakpoint)
>> +    {
>> +      decr_pc_after_break = gdbarch_decr_pc_after_break
>> +	(get_regcache_arch (get_thread_regcache (pid_to_ptid (ret))));
>> +      if (decr_pc_after_break)
>> +	{
>> +	  if (!pc)
>> +	    {
>> +	      pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret)));
>> +	    }
>> +	  regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)),
>> +			     pc + decr_pc_after_break);
>> +	}
>> +    }
>> +
>>    return ret;
>>  }
>>  
>> --- a/record.c
>> +++ b/record.c
>> @@ -497,6 +497,33 @@ 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;
>> +
>> +      status->kind = TARGET_WAITKIND_STOPPED;
>> +
>> +      /* 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))
>> +		  && !record_resume_step)
>> +		{
>> +		  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;
>> @@ -521,7 +548,6 @@ record_wait (ptid_t ptid, struct target_
>>  
>>        /* Loop over the record_list, looking for the next place to
>>  	 stop.  */
>> -      status->kind = TARGET_WAITKIND_STOPPED;
>>        do
>>  	{
>>  	  /* Check for beginning and end of log.  */
>> @@ -588,10 +614,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 +654,25 @@ 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
>> +			  && !record_resume_step)
>>  			{
>> -			  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 +703,7 @@ next:
>>  	  perror_with_name (_("Process record: sigaction"));
>>  	}
>>  
>> +replay_out:
>>        if (record_get_sig)
>>  	{
>>  	  status->value.sig = TARGET_SIGNAL_INT;