[gdb/python] Handle bp creation and deletion in Breakpoint.stop

  In the documentation of gdb.Breakpoint.stop [1] we read:
...
You should not alter the execution state of the inferior (i.e., step, next,
etc.), alter the current frame context (i.e., change the current active
frame), or alter, add or delete any breakpoint.
...

PR python/32423 reports that the failure mode of attempting to delete a
breakpoint in gdb.Breakpoint.stop is:
- actually deleting the breakpoint, followed by
- a segfault.

Improve the failure mode, by instead:
- not deleting the breakpoint, and
- throwing an error.

Likewise for attempting to create a breakpoint.  Doing so reveals that
gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
in Breakpoint.stop.  Drop these incorrect tests.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32423

[1] https://sourceware.org/gdb/current/onlinedocs/gdb.html/Breakpoints-In-Python.html#index-Breakpoint_002estop
---
 gdb/extension.c                               |  7 ++
 gdb/extension.h                               |  2 +
 gdb/python/py-breakpoint.c                    | 14 +++
 gdb/python/py-finishbreakpoint.c              |  7 ++
 gdb/testsuite/gdb.python/py-breakpoint.exp    | 94 +++++++++++++++++++
 .../gdb.python/py-finish-breakpoint.exp       | 91 ------------------
 .../gdb.python/py-finish-breakpoint.py        | 25 -----
 7 files changed, 124 insertions(+), 116 deletions(-)


base-commit: d9df3857da0cef29ed9c0ef75f90700c5f392986
  

Am Montag, 9. Dezember 2024 um 13:54:11 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:

> In the documentation of gdb.Breakpoint.stop [1] we read:
> ...
> You should not alter the execution state of the inferior (i.e., step, next,
> etc.), alter the current frame context (i.e., change the current active
> frame), or alter, add or delete any breakpoint.
> ...
>
> PR python/32423 reports that the failure mode of attempting to delete a
> breakpoint in gdb.Breakpoint.stop is:
> - actually deleting the breakpoint, followed by
> - a segfault.
>
> Improve the failure mode, by instead:
> - not deleting the breakpoint, and
> - throwing an error.
>
> Likewise for attempting to create a breakpoint.  Doing so reveals that
> gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
> in Breakpoint.stop.  Drop these incorrect tests.

I would prefer it if we keep allowing the creation of breakpoints in
Breakpoint.stop, since this makes it possible to have a chain of breakpoints.

Especially useful for FinishBreakpoints, as I wouldn't know how else I
could automate getting the return value from a specific location.


Hannes
  

On 12/9/24 16:05, Hannes Domani wrote:
>   Am Montag, 9. Dezember 2024 um 13:54:11 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
> 
>> In the documentation of gdb.Breakpoint.stop [1] we read:
>> ...
>> You should not alter the execution state of the inferior (i.e., step, next,
>> etc.), alter the current frame context (i.e., change the current active
>> frame), or alter, add or delete any breakpoint.
>> ...
>>
>> PR python/32423 reports that the failure mode of attempting to delete a
>> breakpoint in gdb.Breakpoint.stop is:
>> - actually deleting the breakpoint, followed by
>> - a segfault.
>>
>> Improve the failure mode, by instead:
>> - not deleting the breakpoint, and
>> - throwing an error.
>>
>> Likewise for attempting to create a breakpoint.  Doing so reveals that
>> gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
>> in Breakpoint.stop.  Drop these incorrect tests.
> 
> I would prefer it if we keep allowing the creation of breakpoints in
> Breakpoint.stop, since this makes it possible to have a chain of breakpoints.
> 
> Especially useful for FinishBreakpoints, as I wouldn't know how else I
> could automate getting the return value from a specific location.

Hi Hannes,

could you give an example?  I'm not sure I understand the scenario.

Thanks,
- Tom
  

Am Montag, 9. Dezember 2024 um 16:09:27 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:

> On 12/9/24 16:05, Hannes Domani wrote:
> >  Am Montag, 9. Dezember 2024 um 13:54:11 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
> >
> >> In the documentation of gdb.Breakpoint.stop [1] we read:
> >> ...
> >> You should not alter the execution state of the inferior (i.e., step, next,
> >> etc.), alter the current frame context (i.e., change the current active
> >> frame), or alter, add or delete any breakpoint.
> >> ...
> >>
> >> PR python/32423 reports that the failure mode of attempting to delete a
> >> breakpoint in gdb.Breakpoint.stop is:
> >> - actually deleting the breakpoint, followed by
> >> - a segfault.
> >>
> >> Improve the failure mode, by instead:
> >> - not deleting the breakpoint, and
> >> - throwing an error.
> >>
> >> Likewise for attempting to create a breakpoint.  Doing so reveals that
> >> gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
> >> in Breakpoint.stop.  Drop these incorrect tests.
> >
> > I would prefer it if we keep allowing the creation of breakpoints in
> > Breakpoint.stop, since this makes it possible to have a chain of breakpoints.
> >
> > Especially useful for FinishBreakpoints, as I wouldn't know how else I
> > could automate getting the return value from a specific location.
>
> Hi Hannes,
>
> could you give an example?  I'm not sure I understand the scenario.

For example, what is done with TestFinishBreakpoint:

class TestBreakpoint(gdb.Breakpoint):
    def __init__(self):
        gdb.Breakpoint.__init__(self, spec="test_1", internal=1)
        self.silent = True
        self.count = 0
        print("TestBreakpoint init")

    def stop(self):
        self.count += 1
        try:
            TestFinishBreakpoint(gdb.newest_frame(), self.count)
        except ValueError as e:
            print(e)
        return False


class TestFinishBreakpoint(gdb.FinishBreakpoint):
    def __init__(self, frame, count):
        self.count = count
        gdb.FinishBreakpoint.__init__(self, frame, internal=1)

    def stop(self):
        print("-->", self.number)
        if self.count == 3:
            print("test stop: %d" % self.count)
            return True
        else:
            print("test don't stop: %d" % self.count)
            return False

    def out_of_scope(self):
        print("test didn't finish: %d" % self.count)

It stops after function test_1 returned for the 3rd time.

But I think it would be more common to check for a special return value
instead of a hitcount.


Hannes
  

Tom de Vries <tdevries@suse.de> writes:

> In the documentation of gdb.Breakpoint.stop [1] we read:
> ...
> You should not alter the execution state of the inferior (i.e., step, next,
> etc.), alter the current frame context (i.e., change the current active
> frame), or alter, add or delete any breakpoint.
> ...
>
> PR python/32423 reports that the failure mode of attempting to delete a
> breakpoint in gdb.Breakpoint.stop is:
> - actually deleting the breakpoint, followed by
> - a segfault.
>
> Improve the failure mode, by instead:
> - not deleting the breakpoint, and
> - throwing an error.
>
> Likewise for attempting to create a breakpoint.  Doing so reveals that
> gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
> in Breakpoint.stop.  Drop these incorrect tests.

Do these tests actually cause issues other than violating the docs?
It's not clear if your "Likewise" here means, likewise creating
breakpoints causes problems like use after free, or just likewise doing
this violates the docs, so lets remove that functionality.

If possible I'd much rather we update the docs to cover the allowable
functionality, than restrict GDB to meet the docs.

Thanks,
Andrew
  

On 12/9/24 16:56, Hannes Domani wrote:
>   Am Montag, 9. Dezember 2024 um 16:09:27 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
> 
>> On 12/9/24 16:05, Hannes Domani wrote:
>>>    Am Montag, 9. Dezember 2024 um 13:54:11 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
>>>
>>>> In the documentation of gdb.Breakpoint.stop [1] we read:
>>>> ...
>>>> You should not alter the execution state of the inferior (i.e., step, next,
>>>> etc.), alter the current frame context (i.e., change the current active
>>>> frame), or alter, add or delete any breakpoint.
>>>> ...
>>>>
>>>> PR python/32423 reports that the failure mode of attempting to delete a
>>>> breakpoint in gdb.Breakpoint.stop is:
>>>> - actually deleting the breakpoint, followed by
>>>> - a segfault.
>>>>
>>>> Improve the failure mode, by instead:
>>>> - not deleting the breakpoint, and
>>>> - throwing an error.
>>>>
>>>> Likewise for attempting to create a breakpoint.  Doing so reveals that
>>>> gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
>>>> in Breakpoint.stop.  Drop these incorrect tests.
>>>
>>> I would prefer it if we keep allowing the creation of breakpoints in
>>> Breakpoint.stop, since this makes it possible to have a chain of breakpoints.
>>>
>>> Especially useful for FinishBreakpoints, as I wouldn't know how else I
>>> could automate getting the return value from a specific location.
>>
>> Hi Hannes,
>>
>> could you give an example?  I'm not sure I understand the scenario.
> 
> For example, what is done with TestFinishBreakpoint:
> 
> class TestBreakpoint(gdb.Breakpoint):
>      def __init__(self):
>          gdb.Breakpoint.__init__(self, spec="test_1", internal=1)
>          self.silent = True
>          self.count = 0
>          print("TestBreakpoint init")
> 
>      def stop(self):
>          self.count += 1
>          try:
>              TestFinishBreakpoint(gdb.newest_frame(), self.count)
>          except ValueError as e:
>              print(e)
>          return False
> 
> 
> class TestFinishBreakpoint(gdb.FinishBreakpoint):
>      def __init__(self, frame, count):
>          self.count = count
>          gdb.FinishBreakpoint.__init__(self, frame, internal=1)
> 
>      def stop(self):
>          print("-->", self.number)
>          if self.count == 3:
>              print("test stop: %d" % self.count)
>              return True
>          else:
>              print("test don't stop: %d" % self.count)
>              return False
> 
>      def out_of_scope(self):
>          print("test didn't finish: %d" % self.count)
> 
> It stops after function test_1 returned for the 3rd time.
> 
> But I think it would be more common to check for a special return value
> instead of a hitcount.
> 

So, I went with:
...
$ cat test.c
static int
test_1 (int i)
{
   return i;
}

int
main ()
{
   test_1 (1);
   test_1 (2);
   test_1 (3);
   test_1 (4);

   return 0;
}
...
and:
...
$ gcc -g test.c
...
and copied the python code you indicated into test.py, getting me:
...
$ gdb -q a.out \
     -ex "source test.py" \
     -ex "python TestBreakpoint()" \
     -ex run
Reading symbols from a.out...
TestBreakpoint init
Starting program: /data/vries/gdb/a.out
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
--> -11
test don't stop: 1
--> -12
test don't stop: 2
--> -13
test stop: 3

Breakpoint -13, main () at test.c:13
13	  test_1 (4);
(gdb)
...

Then I took the python code implementing PyCommandsBreakpoint ( 
https://sourceware.org/pipermail/gdb-patches/2024-December/213995.html ) 
and used it to move the breakpoint creation from the stop to the 
py_commands method.  That gets me the same output.  Updated python 
script and output pasted below.

Does this address your concern?

Thanks,
- Tom

--------------------------------------------------------------------------------------

$ cat test.py
class PyCommandsBreakpoint(gdb.Breakpoint):
     bp_dict = dict()

     def __init__(self, *args, **kwargs):
         gdb.Breakpoint.__init__(self, *args, **kwargs)
         PyCommandsBreakpoint.bp_dict[self.number] = self
         self.commands = ""

     @staticmethod
     def run_py_commands(num):
         bp = PyCommandsBreakpoint.bp_dict[num]
         if hasattr(bp, "py_commands"):
             bp.py_commands()

     def __setattr__(self, name, value):
         if name == "commands":
             l = ["python PyCommandsBreakpoint.run_py_commands(%d)" % 
self.number]
             if value != "":
                 l.append(value)
             value = "\n".join(l)

         super().__setattr__(name, value)

class TestBreakpoint(PyCommandsBreakpoint):
     def __init__(self):
         PyCommandsBreakpoint.__init__(self, spec="test_1", internal=1)
         self.silent = True
         self.count = 0
         self.commands = "continue"
         print("TestBreakpoint init")

     def stop(self):
         print("TestBreakpoint stop")
         self.count += 1
         return True

     def py_commands(self):
         try:
             TestFinishBreakpoint(gdb.newest_frame(), self.count)
         except ValueError as e:
             print(e)

class TestFinishBreakpoint(gdb.FinishBreakpoint):
     def __init__(self, frame, count):
         self.count = count
         gdb.FinishBreakpoint.__init__(self, frame, internal=1)

     def stop(self):
         print("-->", self.number)
         if self.count == 3:
             print("test stop: %d" % self.count)
             return True
         else:
             print("test don't stop: %d" % self.count)
             return False

     def out_of_scope(self):
         print("test didn't finish: %d" % self.count)

$ gdb -q a.out \
     -ex "source test.py" \
     -ex "python TestBreakpoint()" \
     -ex run
Reading symbols from a.out...
TestBreakpoint init
Starting program: /data/vries/gdb/a.out
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
TestBreakpoint stop
--> -11
test don't stop: 1
TestBreakpoint stop
--> -12
test don't stop: 2
TestBreakpoint stop
--> -13
test stop: 3

Breakpoint -13, main () at test.c:13
13	  test_1 (4);
(gdb)
  

On 12/10/24 09:33, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> In the documentation of gdb.Breakpoint.stop [1] we read:
>> ...
>> You should not alter the execution state of the inferior (i.e., step, next,
>> etc.), alter the current frame context (i.e., change the current active
>> frame), or alter, add or delete any breakpoint.
>> ...
>>
>> PR python/32423 reports that the failure mode of attempting to delete a
>> breakpoint in gdb.Breakpoint.stop is:
>> - actually deleting the breakpoint, followed by
>> - a segfault.
>>
>> Improve the failure mode, by instead:
>> - not deleting the breakpoint, and
>> - throwing an error.
>>
>> Likewise for attempting to create a breakpoint.  Doing so reveals that
>> gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
>> in Breakpoint.stop.  Drop these incorrect tests.
> 
> Do these tests actually cause issues other than violating the docs?

Hi Andrew,

thanks for the review.

To answer your question, I don't known.

The commit introducing the text is commit 99f5279d98e, discussed here ( 
https://inbox.sourceware.org/gdb-patches/m3pqia8jys.fsf@redhat.com/ ), 
fixing two PRs.

The discussion seems to focus more on the (self) deletion of 
breakpoints, which is considered, paraphrasing, currently not supported, 
but desirable.

> It's not clear if your "Likewise" here means, likewise creating
> breakpoints causes problems like use after free, or just likewise doing
> this violates the docs, so lets remove that functionality.

Sorry for being not clear, I meant the latter.

> If possible I'd much rather we update the docs to cover the allowable
> functionality, than restrict GDB to meet the docs.

I'm not against that.  I just don't known whether the functionality is 
actually supported.

If you tell me that creating breakpoints is indeed supported, I'll be 
happy to update the docs and update this patch.

Before I read the docs and discovered it was not allowed, I tried to fix 
the problem reported in the PR, so I have an enabling fix for that ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=32423#c2 ).  So, perhaps 
we should go that way then?  If so, I should check whether it fixes PR12802.

Another way to look at this is if we really need this, given that we 
have breakpoint commands, where we are allowed to do this sort of thing.

WDYT?

Thanks,
- Tom
  

Am Freitag, 13. Dezember 2024 um 13:31:48 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:

> On 12/9/24 16:56, Hannes Domani wrote:
> >  Am Montag, 9. Dezember 2024 um 16:09:27 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
> >
> >> On 12/9/24 16:05, Hannes Domani wrote:
> >>>    Am Montag, 9. Dezember 2024 um 13:54:11 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
> >>>
> >>>> In the documentation of gdb.Breakpoint.stop [1] we read:
> >>>> ...
> >>>> You should not alter the execution state of the inferior (i.e., step, next,
> >>>> etc.), alter the current frame context (i.e., change the current active
> >>>> frame), or alter, add or delete any breakpoint.
> >>>> ...
> >>>>
> >>>> PR python/32423 reports that the failure mode of attempting to delete a
> >>>> breakpoint in gdb.Breakpoint.stop is:
> >>>> - actually deleting the breakpoint, followed by
> >>>> - a segfault.
> >>>>
> >>>> Improve the failure mode, by instead:
> >>>> - not deleting the breakpoint, and
> >>>> - throwing an error.
> >>>>
> >>>> Likewise for attempting to create a breakpoint.  Doing so reveals that
> >>>> gdb.python/py-finish-breakpoint.exp contains test-cases that add a breakpoint
> >>>> in Breakpoint.stop.  Drop these incorrect tests.
> >>>
> >>> I would prefer it if we keep allowing the creation of breakpoints in
> >>> Breakpoint.stop, since this makes it possible to have a chain of breakpoints.
> >>>
> >>> Especially useful for FinishBreakpoints, as I wouldn't know how else I
> >>> could automate getting the return value from a specific location.
> >>
> >> Hi Hannes,
> >>
> >> could you give an example?  I'm not sure I understand the scenario.
> >
> > For example, what is done with TestFinishBreakpoint:
> >
> > class TestBreakpoint(gdb.Breakpoint):
> >      def __init__(self):
> >          gdb.Breakpoint.__init__(self, spec="test_1", internal=1)
> >          self.silent = True
> >          self.count = 0
> >          print("TestBreakpoint init")
> >
> >      def stop(self):
> >          self.count += 1
> >          try:
> >              TestFinishBreakpoint(gdb.newest_frame(), self.count)
> >          except ValueError as e:
> >              print(e)
> >          return False
> >
> >
> > class TestFinishBreakpoint(gdb.FinishBreakpoint):
> >      def __init__(self, frame, count):
> >          self.count = count
> >          gdb.FinishBreakpoint.__init__(self, frame, internal=1)
> >
> >      def stop(self):
> >          print("-->", self.number)
> >          if self.count == 3:
> >              print("test stop: %d" % self.count)
> >              return True
> >          else:
> >              print("test don't stop: %d" % self.count)
> >              return False
> >
> >      def out_of_scope(self):
> >          print("test didn't finish: %d" % self.count)
> >
> > It stops after function test_1 returned for the 3rd time.
> >
> > But I think it would be more common to check for a special return value
> > instead of a hitcount.
> >
>
> So, I went with:
> ...
> $ cat test.c
> static int
> test_1 (int i)
> {
>   return i;
> }
>
> int
> main ()
> {
>   test_1 (1);
>   test_1 (2);
>   test_1 (3);
>   test_1 (4);
>
>   return 0;
> }
> ...
> and:
> ...
> $ gcc -g test.c
> ...
> and copied the python code you indicated into test.py, getting me:
> ...
> $ gdb -q a.out \
>     -ex "source test.py" \
>     -ex "python TestBreakpoint()" \
>     -ex run
> Reading symbols from a.out...
> TestBreakpoint init
> Starting program: /data/vries/gdb/a.out
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> --> -11
> test don't stop: 1
> --> -12
> test don't stop: 2
> --> -13
> test stop: 3
>
> Breakpoint -13, main () at test.c:13
> 13      test_1 (4);
> (gdb)
> ...
>
> Then I took the python code implementing PyCommandsBreakpoint (
> https://sourceware.org/pipermail/gdb-patches/2024-December/213995.html )
> and used it to move the breakpoint creation from the stop to the
> py_commands method.  That gets me the same output.  Updated python
> script and output pasted below.
>
> Does this address your concern?

Hmmm, so it's possible to workaround the breakpoint creation with 'commands'.
It took me a minute to figure out how this works, and is a bit roundabout,
so I can't say that I like this as much as the original version.

And it's also not really clear to me why we want to forbid the breakpoint
creation in stop() at all (aside from the docu saying so).


Hannes
  

On 12/13/24 14:08, Tom de Vries wrote:
> On 12/10/24 09:33, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> In the documentation of gdb.Breakpoint.stop [1] we read:
>>> ...
>>> You should not alter the execution state of the inferior (i.e., step, 
>>> next,
>>> etc.), alter the current frame context (i.e., change the current active
>>> frame), or alter, add or delete any breakpoint.
>>> ...
>>>
>>> PR python/32423 reports that the failure mode of attempting to delete a
>>> breakpoint in gdb.Breakpoint.stop is:
>>> - actually deleting the breakpoint, followed by
>>> - a segfault.
>>>
>>> Improve the failure mode, by instead:
>>> - not deleting the breakpoint, and
>>> - throwing an error.
>>>
>>> Likewise for attempting to create a breakpoint.  Doing so reveals that
>>> gdb.python/py-finish-breakpoint.exp contains test-cases that add a 
>>> breakpoint
>>> in Breakpoint.stop.  Drop these incorrect tests.
>>
>> Do these tests actually cause issues other than violating the docs?
> 
> Hi Andrew,
> 
> thanks for the review.
> 
> To answer your question, I don't known.
> 
> The commit introducing the text is commit 99f5279d98e, discussed here 
> ( https://inbox.sourceware.org/gdb-patches/m3pqia8jys.fsf@redhat.com/ ), 
> fixing two PRs.
> 
> The discussion seems to focus more on the (self) deletion of 
> breakpoints, which is considered, paraphrasing, currently not supported, 
> but desirable.
> 
>> It's not clear if your "Likewise" here means, likewise creating
>> breakpoints causes problems like use after free, or just likewise doing
>> this violates the docs, so lets remove that functionality.
> 
> Sorry for being not clear, I meant the latter.
> 
>> If possible I'd much rather we update the docs to cover the allowable
>> functionality, than restrict GDB to meet the docs.
> 
> I'm not against that.  I just don't known whether the functionality is 
> actually supported.
> 
> If you tell me that creating breakpoints is indeed supported, I'll be 
> happy to update the docs and update this patch.
> 
> Before I read the docs and discovered it was not allowed, I tried to fix 
> the problem reported in the PR, so I have an enabling fix for that 
> ( https://sourceware.org/bugzilla/show_bug.cgi?id=32423#c2 ).  So, 
> perhaps we should go that way then?  If so, I should check whether it 
> fixes PR12802.
> 
> Another way to look at this is if we really need this, given that we 
> have breakpoint commands, where we are allowed to do this sort of thing.
> 
> WDYT?
> 

I've attempted to move this forward by submitting a patch ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/214483.html ) 
that:
- only prevents bp deletion in Breakpoint.stop, and
- removes the bit in the documentation that says bp creation in
   Breakpoint.stop is not allowed.

Thanks,
- Tom