Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

On 09/23/2015 06:51 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> +static const unsigned char *
>> +aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
>
> Please add comment like this to all these $ARCH_breakpoint_from_pc functions.
>
> /* Implementation of linux_target_ops method "breakpoint_from_pc".  */
>

Done

>> diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
>> index e0bfa1a..da5876d 100644
>> --- a/gdb/gdbserver/linux-cris-low.c
>> +++ b/gdb/gdbserver/linux-cris-low.c
>> @@ -62,7 +62,7 @@ cris_cannot_fetch_register (int regno)
>>   extern int debug_threads;
>>
>>   static CORE_ADDR
>> -cris_get_pc (struct regcache *regcache, void)
>> +cris_get_pc (struct regcache *regcache)
>>   {
>>     unsigned long pc;
>>     collect_register_by_name (regcache, "pc", &pc);
>
> This is a unrelated change.  Please move it out this patch, and submit
> it separately.
>

Done

>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index f5b64ab..bb08761 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -3012,7 +3012,11 @@ linux_wait_1 (ptid_t ptid,
>>     if (!ptid_equal (step_over_bkpt, null_ptid)
>>         && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
>>       {
>> -      unsigned int increment_pc = the_low_target.breakpoint_len;
>> +      int increment_pc = 0;
>> +      CORE_ADDR stop_pc = event_child->stop_pc;
>> +
>> +      (*the_low_target.breakpoint_from_pc)
>> +	(&stop_pc, &increment_pc);
>>
>
> There was a problem here and your patch doesn't fix it.  I want to raise
> it here first.  It is incorrect to get increment_pc by
> the_low_target.breakpoint_len or (*the_low_target.breakpoint_from_pc)
> for arm/thumb, because given the stop_pc, we can't tell the breakpoint
> size (2-byte or 4-byte).We need a new target hook, say
> breakpoint_from_current_state, and its default implementation is
> breakpoint_from_pc.  However, its arm implementation checks whether
> the program is in thumb mode by CPSR and return the right breakpoint size.
>
> IIUC, the code here is used for step-over GDBserver breakpoint, so it is
> not used for arm target until we support conditional breakpoint or
> tracepoint, but we should fix it before supporting conditional
> breakpoint and tracepoint for arm target.
>

Indeed good point. It's too bad another target hook needs to be there 
for this but there's no choice I think.

I will fix it in my next patchset introducing conditional breakpoints.

>>         if (debug_threads)
>>   	{
>> @@ -6932,6 +6936,17 @@ current_lwp_ptid (void)
>>     return ptid_of (current_thread);
>>   }
>>
>> +const unsigned char *
>> +linux_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
>> +{
>> +  if (the_low_target.breakpoint_from_pc != NULL)
>> +    {
>> +      return (*the_low_target.breakpoint_from_pc) (pcptr, lenptr);
>> +    }
>
> "{" and "}" is not needed.

Done

>
>> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
>> index f8f6e78..c623150 100644
>> --- a/gdb/gdbserver/linux-low.h
>> +++ b/gdb/gdbserver/linux-low.h
>> @@ -141,8 +141,13 @@ struct linux_target_ops
>>
>>     CORE_ADDR (*get_pc) (struct regcache *regcache);
>>     void (*set_pc) (struct regcache *regcache, CORE_ADDR newpc);
>> -  const unsigned char *breakpoint;
>> -  int breakpoint_len;
>> +
>> +  /* Return the raw breakpoint for this target based on PC.  Note that the PC
>
> s/PC/*PCPTR/
>
>> +     can be NULL, the default breakpoint for the target should be returned in
>
> PC can't be NULL after your patch #2.  You can remove the second
> sentence in this patch or patch #2.
>
I think you mean after patch #3 ?

But it can still be NULL see in #3 :

/* Default if no pc is set to arm breakpoint.  */
+  if (pcptr == NULL)
+    {
+      *lenptr = arm_breakpoint_len;
+      return (unsigned char *) &arm_breakpoint;
+    }

>> +     this case. The PC is ajusted to the real memory location in case a flag was
>> +     present in the PC.  */
>> +  const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
>> +
>

Note also that I removed the win32 changes, they were not needed and 
broken, I can't test win32 properly anyway.