diff mbox

[v2] Fix inconsistent breakpoint kinds between breakpoints and tracepoints in GDBServer.

Message ID 1445529414-11581-1-git-send-email-antoine.tremblay@ericsson.com
State New, archived
Headers

Commit Message

Antoine Tremblay Oct. 22, 2015, 3:56 p.m. UTC 
  This patch fixes a regression introduced by :
https://sourceware.org/ml/gdb-patches/2015-10/msg00369.html

Tests : gdb.trace/trace-break.exp and gdb.trace/trace-mt.exp would fail on x86
with gdbserver-{native,extended}.

Before this patch, the breakpoint kind set by GDB with a Z packet and the one
set in the case of a tracepoint would be inconsistent on targets that did not
implement breakpoint_kind_from_pc. On x86 for example a breakpoint set by GDB
would have a kind of 1 but a breakpoint set by a tracepoint would have a kind of
0.

This created a missmatch when trying to insert a tracepoint and a breakpoint at
the same location. One of the two breakpoints would be removed with debug
message : "Inconsistent breakpoint kind".

This patch fixes the issue by changing the default 0 breakpoint kind to be
the size of the breakpoint according to sw_breakpoint_from_kind.

The default breakpoint kind must be the breakpoint length to keep consistency
between breakpoints set via GDB and the ones set internally by GDBServer.

No regression on Ubuntu 14.04 x86-64 with gdbserver-{native-extended}

gdb/gdbserver/ChangeLog:

	* linux-low.c (default_breakpoint_kind_from_pc): New function.
	(linux_breakpoint_kind_from_pc): Use default_breakpoint_kind_from_pc for
	the default breakpoint kind.
---
 gdb/gdbserver/linux-low.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Pedro Alves Oct. 22, 2015, 3:58 p.m. UTC | #1 
On 10/22/2015 04:56 PM, Antoine Tremblay wrote:
> This patch fixes a regression introduced by :
> https://sourceware.org/ml/gdb-patches/2015-10/msg00369.html
> 
> Tests : gdb.trace/trace-break.exp and gdb.trace/trace-mt.exp would fail on x86
> with gdbserver-{native,extended}.
> 
> Before this patch, the breakpoint kind set by GDB with a Z packet and the one
> set in the case of a tracepoint would be inconsistent on targets that did not
> implement breakpoint_kind_from_pc. On x86 for example a breakpoint set by GDB
> would have a kind of 1 but a breakpoint set by a tracepoint would have a kind of
> 0.
> 
> This created a missmatch when trying to insert a tracepoint and a breakpoint at
> the same location. One of the two breakpoints would be removed with debug
> message : "Inconsistent breakpoint kind".
> 
> This patch fixes the issue by changing the default 0 breakpoint kind to be
> the size of the breakpoint according to sw_breakpoint_from_kind.
> 
> The default breakpoint kind must be the breakpoint length to keep consistency
> between breakpoints set via GDB and the ones set internally by GDBServer.
> 
> No regression on Ubuntu 14.04 x86-64 with gdbserver-{native-extended}
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* linux-low.c (default_breakpoint_kind_from_pc): New function.
> 	(linux_breakpoint_kind_from_pc): Use default_breakpoint_kind_from_pc for
> 	the default breakpoint kind.

OK.

Thanks,
Pedro Alves
Antoine Tremblay Oct. 22, 2015, 4:03 p.m. UTC | #2 
On 10/22/2015 11:58 AM, Pedro Alves wrote:
> On 10/22/2015 04:56 PM, Antoine Tremblay wrote:
>> This patch fixes a regression introduced by :
>> https://sourceware.org/ml/gdb-patches/2015-10/msg00369.html
>>
>> Tests : gdb.trace/trace-break.exp and gdb.trace/trace-mt.exp would fail on x86
>> with gdbserver-{native,extended}.
>>
>> Before this patch, the breakpoint kind set by GDB with a Z packet and the one
>> set in the case of a tracepoint would be inconsistent on targets that did not
>> implement breakpoint_kind_from_pc. On x86 for example a breakpoint set by GDB
>> would have a kind of 1 but a breakpoint set by a tracepoint would have a kind of
>> 0.
>>
>> This created a missmatch when trying to insert a tracepoint and a breakpoint at
>> the same location. One of the two breakpoints would be removed with debug
>> message : "Inconsistent breakpoint kind".
>>
>> This patch fixes the issue by changing the default 0 breakpoint kind to be
>> the size of the breakpoint according to sw_breakpoint_from_kind.
>>
>> The default breakpoint kind must be the breakpoint length to keep consistency
>> between breakpoints set via GDB and the ones set internally by GDBServer.
>>
>> No regression on Ubuntu 14.04 x86-64 with gdbserver-{native-extended}
>>
>> gdb/gdbserver/ChangeLog:
>>
>> 	* linux-low.c (default_breakpoint_kind_from_pc): New function.
>> 	(linux_breakpoint_kind_from_pc): Use default_breakpoint_kind_from_pc for
>> 	the default breakpoint kind.
>
> OK.
>

I forgot to set the function default_breakpoint_kind_from_pc static, ok 
with that change ?
Pedro Alves Oct. 22, 2015, 4:04 p.m. UTC | #3 
On 10/22/2015 05:03 PM, Antoine Tremblay wrote:
> 
> 
> On 10/22/2015 11:58 AM, Pedro Alves wrote:
>> On 10/22/2015 04:56 PM, Antoine Tremblay wrote:
>>> This patch fixes a regression introduced by :
>>> https://sourceware.org/ml/gdb-patches/2015-10/msg00369.html
>>>
>>> Tests : gdb.trace/trace-break.exp and gdb.trace/trace-mt.exp would fail on x86
>>> with gdbserver-{native,extended}.
>>>
>>> Before this patch, the breakpoint kind set by GDB with a Z packet and the one
>>> set in the case of a tracepoint would be inconsistent on targets that did not
>>> implement breakpoint_kind_from_pc. On x86 for example a breakpoint set by GDB
>>> would have a kind of 1 but a breakpoint set by a tracepoint would have a kind of
>>> 0.
>>>
>>> This created a missmatch when trying to insert a tracepoint and a breakpoint at
>>> the same location. One of the two breakpoints would be removed with debug
>>> message : "Inconsistent breakpoint kind".
>>>
>>> This patch fixes the issue by changing the default 0 breakpoint kind to be
>>> the size of the breakpoint according to sw_breakpoint_from_kind.
>>>
>>> The default breakpoint kind must be the breakpoint length to keep consistency
>>> between breakpoints set via GDB and the ones set internally by GDBServer.
>>>
>>> No regression on Ubuntu 14.04 x86-64 with gdbserver-{native-extended}
>>>
>>> gdb/gdbserver/ChangeLog:
>>>
>>> 	* linux-low.c (default_breakpoint_kind_from_pc): New function.
>>> 	(linux_breakpoint_kind_from_pc): Use default_breakpoint_kind_from_pc for
>>> 	the default breakpoint kind.
>>
>> OK.
>>
> 
> I forgot to set the function default_breakpoint_kind_from_pc static, ok 
> with that change ?

Sure.

(I think you'll need to move the function to target.c to
fix !Linux ports, but I'm OK with doing that as a separate step.)

Thanks,
Pedro Alves
Antoine Tremblay Oct. 22, 2015, 4:06 p.m. UTC | #4 
On 10/22/2015 12:04 PM, Pedro Alves wrote:
> On 10/22/2015 05:03 PM, Antoine Tremblay wrote:
>>
>>
>> On 10/22/2015 11:58 AM, Pedro Alves wrote:
>>> On 10/22/2015 04:56 PM, Antoine Tremblay wrote:
>>>> This patch fixes a regression introduced by :
>>>> https://sourceware.org/ml/gdb-patches/2015-10/msg00369.html
>>>>
>>>> Tests : gdb.trace/trace-break.exp and gdb.trace/trace-mt.exp would fail on x86
>>>> with gdbserver-{native,extended}.
>>>>
>>>> Before this patch, the breakpoint kind set by GDB with a Z packet and the one
>>>> set in the case of a tracepoint would be inconsistent on targets that did not
>>>> implement breakpoint_kind_from_pc. On x86 for example a breakpoint set by GDB
>>>> would have a kind of 1 but a breakpoint set by a tracepoint would have a kind of
>>>> 0.
>>>>
>>>> This created a missmatch when trying to insert a tracepoint and a breakpoint at
>>>> the same location. One of the two breakpoints would be removed with debug
>>>> message : "Inconsistent breakpoint kind".
>>>>
>>>> This patch fixes the issue by changing the default 0 breakpoint kind to be
>>>> the size of the breakpoint according to sw_breakpoint_from_kind.
>>>>
>>>> The default breakpoint kind must be the breakpoint length to keep consistency
>>>> between breakpoints set via GDB and the ones set internally by GDBServer.
>>>>
>>>> No regression on Ubuntu 14.04 x86-64 with gdbserver-{native-extended}
>>>>
>>>> gdb/gdbserver/ChangeLog:
>>>>
>>>> 	* linux-low.c (default_breakpoint_kind_from_pc): New function.
>>>> 	(linux_breakpoint_kind_from_pc): Use default_breakpoint_kind_from_pc for
>>>> 	the default breakpoint kind.
>>>
>>> OK.
>>>
>>
>> I forgot to set the function default_breakpoint_kind_from_pc static, ok
>> with that change ?
>
> Sure.
>
> (I think you'll need to move the function to target.c to
> fix !Linux ports, but I'm OK with doing that as a separate step.)
>

I don't think so since I fix !linux ports like so :

/* Implementation of the target_ops method "breakpoint_kind_from_pc".  */

static int
win32_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
{
   return the_low_target.breakpoint_len;
}

/* Implementation of the target_ops method "sw_breakpoint_from_kind".  */

static const gdb_byte *
win32_sw_breakpoint_from_kind (int kind, int *size)
{
   *size = the_low_target.breakpoint_len;
   return the_low_target.breakpoint;
}

?
Pedro Alves Oct. 22, 2015, 4:11 p.m. UTC | #5 
On 10/22/2015 05:06 PM, Antoine Tremblay wrote:
> On 10/22/2015 12:04 PM, Pedro Alves wrote:

>> (I think you'll need to move the function to target.c to
>> fix !Linux ports, but I'm OK with doing that as a separate step.)
>>
> 
> I don't think so since I fix !linux ports like so :
> 
> /* Implementation of the target_ops method "breakpoint_kind_from_pc".  */
> 
> static int
> win32_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
> {
>    return the_low_target.breakpoint_len;
> }
> 

You don't need this one nor the equivalent in other ports if you
add this to target.h:

#define target_breakpoint_kind_from_pc(PCPTR) \
  (the_target->breakpoint_kind_from_pc \
    ? (*the_target->breakpoint_kind_from_pc) (PCPTR) \
    : default_breakpoint_kind_from_pc ())

(see the other similar macros there)

You'll need to adjust callers to call target_breakpoint_kind_from_pc
instead, of course.

Thanks,
Pedro Alves

> /* Implementation of the target_ops method "sw_breakpoint_from_kind".  */
> 
> static const gdb_byte *
> win32_sw_breakpoint_from_kind (int kind, int *size)
> {
>    *size = the_low_target.breakpoint_len;
>    return the_low_target.breakpoint;
> }
> 
> ?
>
Antoine Tremblay Oct. 22, 2015, 4:16 p.m. UTC | #6 
On 10/22/2015 12:11 PM, Pedro Alves wrote:
> On 10/22/2015 05:06 PM, Antoine Tremblay wrote:
>> On 10/22/2015 12:04 PM, Pedro Alves wrote:
>
>>> (I think you'll need to move the function to target.c to
>>> fix !Linux ports, but I'm OK with doing that as a separate step.)
>>>
>>
>> I don't think so since I fix !linux ports like so :
>>
>> /* Implementation of the target_ops method "breakpoint_kind_from_pc".  */
>>
>> static int
>> win32_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
>> {
>>     return the_low_target.breakpoint_len;
>> }
>>
>
> You don't need this one nor the equivalent in other ports if you
> add this to target.h:
>
> #define target_breakpoint_kind_from_pc(PCPTR) \
>    (the_target->breakpoint_kind_from_pc \
>      ? (*the_target->breakpoint_kind_from_pc) (PCPTR) \
>      : default_breakpoint_kind_from_pc ())
>
> (see the other similar macros there)
>
> You'll need to adjust callers to call target_breakpoint_kind_from_pc
> instead, of course.
>

Yes but then I would need a os_arch_sw_breakpoint_from_kind operation in 
all the arch that this os supports (and that I can't test), you think 
it's still better that way ?
Antoine Tremblay Oct. 22, 2015, 4:18 p.m. UTC | #7 
On 10/22/2015 12:16 PM, Antoine Tremblay wrote:
>
>
> On 10/22/2015 12:11 PM, Pedro Alves wrote:
>> On 10/22/2015 05:06 PM, Antoine Tremblay wrote:
>>> On 10/22/2015 12:04 PM, Pedro Alves wrote:
>>
>>>> (I think you'll need to move the function to target.c to
>>>> fix !Linux ports, but I'm OK with doing that as a separate step.)
>>>>
>>>
>>> I don't think so since I fix !linux ports like so :
>>>
>>> /* Implementation of the target_ops method
>>> "breakpoint_kind_from_pc".  */
>>>
>>> static int
>>> win32_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
>>> {
>>>     return the_low_target.breakpoint_len;
>>> }
>>>
>>
>> You don't need this one nor the equivalent in other ports if you
>> add this to target.h:
>>
>> #define target_breakpoint_kind_from_pc(PCPTR) \
>>    (the_target->breakpoint_kind_from_pc \
>>      ? (*the_target->breakpoint_kind_from_pc) (PCPTR) \
>>      : default_breakpoint_kind_from_pc ())
>>
>> (see the other similar macros there)
>>
>> You'll need to adjust callers to call target_breakpoint_kind_from_pc
>> instead, of course.
>>
>
> Yes but then I would need a os_arch_sw_breakpoint_from_kind operation in
> all the arch that this os supports (and that I can't test), you think
> it's still better that way ?
>

Unless I have another macro that checks for the sw_breakpoint_from_kind 
and if abscent returns breakpoint_len... I could do that I guess..
Pedro Alves Oct. 22, 2015, 4:23 p.m. UTC | #8 
On 10/22/2015 05:18 PM, Antoine Tremblay wrote:
> 
> 
> On 10/22/2015 12:16 PM, Antoine Tremblay wrote:
>>
>>
>> On 10/22/2015 12:11 PM, Pedro Alves wrote:

>>> You don't need this one nor the equivalent in other ports if you
>>> add this to target.h:
>>>
>>> #define target_breakpoint_kind_from_pc(PCPTR) \
>>>    (the_target->breakpoint_kind_from_pc \
>>>      ? (*the_target->breakpoint_kind_from_pc) (PCPTR) \
>>>      : default_breakpoint_kind_from_pc ())
>>>
>>> (see the other similar macros there)
>>>
>>> You'll need to adjust callers to call target_breakpoint_kind_from_pc
>>> instead, of course.
>>>
>>

>> Yes but then I would need a os_arch_sw_breakpoint_from_kind operation in
>> all the arch that this os supports (and that I can't test), you think
>> it's still better that way ?
>>
> 
> Unless I have another macro that checks for the sw_breakpoint_from_kind 
> and if abscent returns breakpoint_len... I could do that I guess..

I guess I'm confused.  Why doesn't what you already had, like
below, work as is?

static const gdb_byte *
win32_sw_breakpoint_from_kind (int kind, int *size)
{
   *size = the_low_target.breakpoint_len;
   return the_low_target.breakpoint;
}

Thanks,
Pedro Alves
Antoine Tremblay Oct. 22, 2015, 4:26 p.m. UTC | #9 
On 10/22/2015 12:23 PM, Pedro Alves wrote:
> On 10/22/2015 05:18 PM, Antoine Tremblay wrote:
>>
>>
>> On 10/22/2015 12:16 PM, Antoine Tremblay wrote:
>>>
>>>
>>> On 10/22/2015 12:11 PM, Pedro Alves wrote:
>
>>>> You don't need this one nor the equivalent in other ports if you
>>>> add this to target.h:
>>>>
>>>> #define target_breakpoint_kind_from_pc(PCPTR) \
>>>>     (the_target->breakpoint_kind_from_pc \
>>>>       ? (*the_target->breakpoint_kind_from_pc) (PCPTR) \
>>>>       : default_breakpoint_kind_from_pc ())
>>>>
>>>> (see the other similar macros there)
>>>>
>>>> You'll need to adjust callers to call target_breakpoint_kind_from_pc
>>>> instead, of course.
>>>>
>>>
>
>>> Yes but then I would need a os_arch_sw_breakpoint_from_kind operation in
>>> all the arch that this os supports (and that I can't test), you think
>>> it's still better that way ?
>>>
>>
>> Unless I have another macro that checks for the sw_breakpoint_from_kind
>> and if abscent returns breakpoint_len... I could do that I guess..
>
> I guess I'm confused.  Why doesn't what you already had, like
> below, work as is?
>
> static const gdb_byte *
> win32_sw_breakpoint_from_kind (int kind, int *size)
> {
>     *size = the_low_target.breakpoint_len;
>     return the_low_target.breakpoint;
> }
>

Ho yes it would, sorry I was confused I though you meant remove both 
win32_breakpoint_kind_from_pc and win32_sw_breakpoint_from_kind before.

Removing only win32_breakpoint_kind_from_pc and adding the default macro 
works.

Thanks,
Antoine
Antoine Tremblay Oct. 22, 2015, 5:42 p.m. UTC | #10 
On 10/22/2015 12:06 PM, Antoine Tremblay wrote:
>
>
> On 10/22/2015 12:04 PM, Pedro Alves wrote:
>> On 10/22/2015 05:03 PM, Antoine Tremblay wrote:
>>>
>>>
>>> On 10/22/2015 11:58 AM, Pedro Alves wrote:
>>>> On 10/22/2015 04:56 PM, Antoine Tremblay wrote:
>>>>> This patch fixes a regression introduced by :
>>>>> https://sourceware.org/ml/gdb-patches/2015-10/msg00369.html
>>>>>
>>>>> Tests : gdb.trace/trace-break.exp and gdb.trace/trace-mt.exp would
>>>>> fail on x86
>>>>> with gdbserver-{native,extended}.
>>>>>
>>>>> Before this patch, the breakpoint kind set by GDB with a Z packet
>>>>> and the one
>>>>> set in the case of a tracepoint would be inconsistent on targets
>>>>> that did not
>>>>> implement breakpoint_kind_from_pc. On x86 for example a breakpoint
>>>>> set by GDB
>>>>> would have a kind of 1 but a breakpoint set by a tracepoint would
>>>>> have a kind of
>>>>> 0.
>>>>>
>>>>> This created a missmatch when trying to insert a tracepoint and a
>>>>> breakpoint at
>>>>> the same location. One of the two breakpoints would be removed with
>>>>> debug
>>>>> message : "Inconsistent breakpoint kind".
>>>>>
>>>>> This patch fixes the issue by changing the default 0 breakpoint
>>>>> kind to be
>>>>> the size of the breakpoint according to sw_breakpoint_from_kind.
>>>>>
>>>>> The default breakpoint kind must be the breakpoint length to keep
>>>>> consistency
>>>>> between breakpoints set via GDB and the ones set internally by
>>>>> GDBServer.
>>>>>
>>>>> No regression on Ubuntu 14.04 x86-64 with gdbserver-{native-extended}
>>>>>
>>>>> gdb/gdbserver/ChangeLog:
>>>>>
>>>>>     * linux-low.c (default_breakpoint_kind_from_pc): New function.
>>>>>     (linux_breakpoint_kind_from_pc): Use
>>>>> default_breakpoint_kind_from_pc for
>>>>>     the default breakpoint kind.
>>>>
>>>> OK.
>>>>
>>>
>>> I forgot to set the function default_breakpoint_kind_from_pc static, ok
>>> with that change ?
>>
>> Sure.
>>

Pushed.
diff mbox

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index c20e257..8d1730b 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -6937,6 +6937,19 @@  current_lwp_ptid (void)
   return ptid_of (current_thread);
 }
 
+/* Return the default breakpoint kind as the size of the breakpoint.  */
+
+int
+default_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
+{
+  int size = 0;
+
+  gdb_assert (the_low_target.sw_breakpoint_from_kind != NULL);
+
+  (*the_low_target.sw_breakpoint_from_kind) (0, &size);
+  return size;
+}
+
 /* Implementation of the target_ops method "breakpoint_kind_from_pc".  */
 
 static int
@@ -6945,8 +6958,7 @@  linux_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
   if (the_low_target.breakpoint_kind_from_pc != NULL)
     return (*the_low_target.breakpoint_kind_from_pc) (pcptr);
   else
-    /* Default breakpoint kind value.  */
-    return 0;
+    return default_breakpoint_kind_from_pc (pcptr);
 }
 
 /* Implementation of the target_ops method "sw_breakpoint_from_kind".  */