False positive permanent breakpoints

False positive permanent breakpoints

[Ofir Cohen]

Hello,
We have debugging support for code running on the iGPU, and we've encountered
something that looks like a limitation of gdb.

Problem: gdb freaks out on the iGPU when trying to step over/continue when
               stopped on a permanent breakpoint.

Root-cause: gdb *falsely* identifies some instructions as permanent breakpoints.


Details
======
We don't call set_gdbarch_skip_permanent_breakpoint() on our target, so there
is no support for stepping over permanent breakpoints. Which is fine
since software breakpoints is not something that we support at the
moment.

While trying to fix the root-cause,
i.e. let gdb determine that instruction is indeed a breakpoint instruction,
I ran into a wall.

Looks like gdb is being too *coarse* for the check (whether it is a
breakpoint instruction):
if (err || memcmp (buf, bp_opcode (bp), bp_size (bp)) != 0)

in validate_inserted_breakpoint() function.

I'm saying coarse here because on the iGPU the breakpoint thingy is mandated
by a single bit, and does not have a unique *opcode*.


Proposed solution
==============
Could we abstract (delegate away) this predicate to the _target_ ?

Perhaps setting a default handler for other targets.

Thanks,
Ofir

Re: False positive permanent breakpoints

[Pedro Alves]

On 10/26/2016 03:04 PM, Ofir Cohen wrote:
> Hello,
> We have debugging support for code running on the iGPU, and we've encountered
> something that looks like a limitation of gdb.

What's "the iGPU" ?  An integrated GPU?

> Problem: gdb freaks out on the iGPU when trying to step over/continue when
>                stopped on a permanent breakpoint.
> 
> Root-cause: gdb *falsely* identifies some instructions as permanent breakpoints.
> 
> 
> Details
> ======
> We don't call set_gdbarch_skip_permanent_breakpoint() on our target, so there
> is no support for stepping over permanent breakpoints. Which is fine
> since software breakpoints is not something that we support at the
> moment.
> 
> While trying to fix the root-cause,
> i.e. let gdb determine that instruction is indeed a breakpoint instruction,
> I ran into a wall.
> 
> Looks like gdb is being too *coarse* for the check (whether it is a
> breakpoint instruction):
> if (err || memcmp (buf, bp_opcode (bp), bp_size (bp)) != 0)
> 
> in validate_inserted_breakpoint() function.

So this is in gdbserver, not gdb.

> 
> I'm saying coarse here because on the iGPU the breakpoint thingy is mandated
> by a single bit, and does not have a unique *opcode*.

> Proposed solution
> ==============
> Could we abstract (delegate away) this predicate to the _target_ ?

All can be done, it's just software.  :-)  But not if no port
in the tree is using it...  Otherwise it'll just look like
unnecessary complication and will likely be "cleaned up" again
Are you planning on contributing your port upstream?

I'm a bit confused since you said you don't support software
breakpoints...

> Perhaps setting a default handler for other targets.

Thanks,
Pedro Alves

Re: False positive permanent breakpoints

[Ofir Cohen]

Hi Pedro,
Thanks for the reply.
Let me provide some clarifications :-).

1) The product is part of the "Intel SDK for OpenCL" [1] for Windows and Linux.
     You can see how it looks in [2].

2) iGPU - Yes, Integrated GPU

3) Upstream is in our plans and actually we are preparing a clean
    series of patches on top of 7.12 (currently we are rebased on top
of 7.6, yeah I know...).

4) Software breakpoints are officially supported by the HW, but
    haven't been tested and used yet, so we're OK with them being
"disabled" at the moment.

The sources for gdb (as well as other components) are provided with
the installer script,
which can be downloaded _free of charge_ via [1] --> "Download" .

Is that good enough for making an upstream change?

I can try and prepare a small patch.

- Ofir

[1] https://software.intel.com/en-us/intel-opencl
[2] https://software.intel.com/en-us/node/671873

On 26 October 2016 at 17:28, Pedro Alves <palves@redhat.com> wrote:
> On 10/26/2016 03:04 PM, Ofir Cohen wrote:
>> Hello,
>> We have debugging support for code running on the iGPU, and we've encountered
>> something that looks like a limitation of gdb.
>
> What's "the iGPU" ?  An integrated GPU?
>
>> Problem: gdb freaks out on the iGPU when trying to step over/continue when
>>                stopped on a permanent breakpoint.
>>
>> Root-cause: gdb *falsely* identifies some instructions as permanent breakpoints.
>>
>>
>> Details
>> ======
>> We don't call set_gdbarch_skip_permanent_breakpoint() on our target, so there
>> is no support for stepping over permanent breakpoints. Which is fine
>> since software breakpoints is not something that we support at the
>> moment.
>>
>> While trying to fix the root-cause,
>> i.e. let gdb determine that instruction is indeed a breakpoint instruction,
>> I ran into a wall.
>>
>> Looks like gdb is being too *coarse* for the check (whether it is a
>> breakpoint instruction):
>> if (err || memcmp (buf, bp_opcode (bp), bp_size (bp)) != 0)
>>
>> in validate_inserted_breakpoint() function.
>
> So this is in gdbserver, not gdb.
>
>>
>> I'm saying coarse here because on the iGPU the breakpoint thingy is mandated
>> by a single bit, and does not have a unique *opcode*.
>
>> Proposed solution
>> ==============
>> Could we abstract (delegate away) this predicate to the _target_ ?
>
> All can be done, it's just software.  :-)  But not if no port
> in the tree is using it...  Otherwise it'll just look like
> unnecessary complication and will likely be "cleaned up" again
> Are you planning on contributing your port upstream?
>
> I'm a bit confused since you said you don't support software
> breakpoints...
>
>> Perhaps setting a default handler for other targets.
>
> Thanks,
> Pedro Alves
>

Re: False positive permanent breakpoints

[Pedro Alves]

On 10/26/2016 03:42 PM, Ofir Cohen wrote:
> Hi Pedro,
> Thanks for the reply.
> Let me provide some clarifications :-).
> 
> 1) The product is part of the "Intel SDK for OpenCL" [1] for Windows and Linux.
>      You can see how it looks in [2].

Ah, OK.

> 
> 2) iGPU - Yes, Integrated GPU
> 
> 3) Upstream is in our plans and actually we are preparing a clean
>     series of patches on top of 7.12 (currently we are rebased on top
> of 7.6, yeah I know...).

Nice!

(FAOD, for upstream you'll need to rebase on master.)

> 
> 4) Software breakpoints are officially supported by the HW, but
>     haven't been tested and used yet, so we're OK with them being
> "disabled" at the moment.
> 
> The sources for gdb (as well as other components) are provided with
> the installer script,
> which can be downloaded _free of charge_ via [1] --> "Download" .
> 
> Is that good enough for making an upstream change?
> 
> I can try and prepare a small patch.

I'd rather that upstreaming that bit was done when the
rest of the port is submitted as well.

Thanks,
Pedro Alves

Re: False positive permanent breakpoints

[Ofir Cohen]

> I'd rather that upstreaming that bit was done when the
> rest of the port is submitted as well.
Alright, will do.

As a temporary mitigation,
Is it OK for the target to return NULL from gdbarch_breakpoint_from_pc
to state that it doesn't support software breakpoints ?


There are some places that check that return value, like:
/* Software breakpoints unsupported?  */
if (bpoint == NULL)
  return 0;

in bp_loc_is_permanent(), and others that don't, like breakpoint_xfer_memory().
Furthermore, it performs pointer arithmetic based on the returned value.

Currently the tests we have pass, but I'm concerned about other corner
cases are not covered
that might pop up as a result of this change.

Thanks,
Ofir

On 26 October 2016 at 17:50, Pedro Alves <palves@redhat.com> wrote:
> On 10/26/2016 03:42 PM, Ofir Cohen wrote:
>> Hi Pedro,
>> Thanks for the reply.
>> Let me provide some clarifications :-).
>>
>> 1) The product is part of the "Intel SDK for OpenCL" [1] for Windows and Linux.
>>      You can see how it looks in [2].
>
> Ah, OK.
>
>>
>> 2) iGPU - Yes, Integrated GPU
>>
>> 3) Upstream is in our plans and actually we are preparing a clean
>>     series of patches on top of 7.12 (currently we are rebased on top
>> of 7.6, yeah I know...).
>
> Nice!
>
> (FAOD, for upstream you'll need to rebase on master.)
>
>>
>> 4) Software breakpoints are officially supported by the HW, but
>>     haven't been tested and used yet, so we're OK with them being
>> "disabled" at the moment.
>>
>> The sources for gdb (as well as other components) are provided with
>> the installer script,
>> which can be downloaded _free of charge_ via [1] --> "Download" .
>>
>> Is that good enough for making an upstream change?
>>
>> I can try and prepare a small patch.
>
> I'd rather that upstreaming that bit was done when the
> rest of the port is submitted as well.
>
> Thanks,
> Pedro Alves
>

Re: False positive permanent breakpoints

[Pedro Alves]

On 10/26/2016 04:02 PM, Ofir Cohen wrote:
>> I'd rather that upstreaming that bit was done when the
>> rest of the port is submitted as well.
> Alright, will do.
> 
> As a temporary mitigation,
> Is it OK for the target to return NULL from gdbarch_breakpoint_from_pc
> to state that it doesn't support software breakpoints ?
> 

I think so.

> 
> There are some places that check that return value, like:
> /* Software breakpoints unsupported?  */
> if (bpoint == NULL)
>   return 0;
> 
> in bp_loc_is_permanent(), and others that don't, like breakpoint_xfer_memory().
> Furthermore, it performs pointer arithmetic based on the returned value.

If you don't have software breakpoints in the list, then
bp_location_has_shadow will return false for all breakpoints?
Sounds like we could skip most of the work, but that's an
optimization, not a correctness issue?

> 
> Currently the tests we have pass, but I'm concerned about other corner
> cases are not covered
> that might pop up as a result of this change.

I can't think of any offhand.  Mainly because it's not usual for me to
work with targets that don't support sofware breakpoints.  So I'd
say -- try it and see what breaks.  Which you already did.  :-)

Thanks,
Pedro Alves

Re: False positive permanent breakpoints

[Ofir Cohen]

> If you don't have software breakpoints in the list, then
> bp_location_has_shadow will return false for all breakpoints?
> Sounds like we could skip most of the work, but that's an
> optimization, not a correctness issue?
Yeah, I guess we can live with the minor performance impact we have for now
(and fix that in the future).

> I can't think of any offhand.  Mainly because it's not usual for me to
> work with targets that don't support sofware breakpoints.  So I'd
> say -- try it and see what breaks.  Which you already did.  :-)
Alright, thanks!

- Ofir Cohen

On 26 October 2016 at 18:13, Pedro Alves <palves@redhat.com> wrote:
> On 10/26/2016 04:02 PM, Ofir Cohen wrote:
>>> I'd rather that upstreaming that bit was done when the
>>> rest of the port is submitted as well.
>> Alright, will do.
>>
>> As a temporary mitigation,
>> Is it OK for the target to return NULL from gdbarch_breakpoint_from_pc
>> to state that it doesn't support software breakpoints ?
>>
>
> I think so.
>
>>
>> There are some places that check that return value, like:
>> /* Software breakpoints unsupported?  */
>> if (bpoint == NULL)
>>   return 0;
>>
>> in bp_loc_is_permanent(), and others that don't, like breakpoint_xfer_memory().
>> Furthermore, it performs pointer arithmetic based on the returned value.
>
> If you don't have software breakpoints in the list, then
> bp_location_has_shadow will return false for all breakpoints?
> Sounds like we could skip most of the work, but that's an
> optimization, not a correctness issue?
>
>>
>> Currently the tests we have pass, but I'm concerned about other corner
>> cases are not covered
>> that might pop up as a result of this change.
>
> I can't think of any offhand.  Mainly because it's not usual for me to
> work with targets that don't support sofware breakpoints.  So I'd
> say -- try it and see what breaks.  Which you already did.  :-)
>
> Thanks,
> Pedro Alves
>