[PATCH][gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32

[PATCH][gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32

[Tom de Vries]

Hi,

When running test-case gdb.python/py-finish-breakpoint2.exp with target board
unix/-m32, we run into:
...
(gdb) continue^M
Continuing.^M
Exception #10^M
^M
Breakpoint 3, throw_exception_1 (e=10) at py-finish-breakpoint2.cc:23^M
23        throw new int (e);^M
(gdb) FAIL: gdb.python/py-finish-breakpoint2.exp: \
  check FinishBreakpoint in catch()
...
With -m64, the test passes.

Relevant bit of the source:
...
    36    try
    37      {
    38        throw_exception_1 (10);
    39      }
    40    catch (const int *e)
    41      {
    42          std::cerr << "Exception #" << *e << std::endl;
    43      }
    44    i += 1; /* Break after exception 1.  */
...

The -m64 scenario in more detail:
- the test-case runs to throw_exception_1.
- it installs a FinishBreakpoint, which is a temporary breakpoint set at the
  return address of a frame.
- for -m64, that address is:
    400c47:       83 45 e4 01             addl   $0x1,-0x1c(%rbp)
  which corresponds the "i += 1 at line 44"
- the test-case then continues
- an exception is throw in throw_exection_1
- the exception is caught at line 40, and a message is printed
- line 44 is executed, and the FinishBreakpoint triggers.

With -m32, we have instead:
- the address where the finish breakpoint is set is:
    8048a0a:       83 c4 10                add    $0x10,%esp
  which is the lasn insn generated for the call at line 38
- the test-case continues
- an exception is throw in throw_exection_1
- consequently, the FinishBreakpoint is not triggered.

In conclusion, the test worked by accident for -m64, because the first insn
after the call to throw_exception_1 is also the first insn after the try.
And that just happens to be not the case for -m32.

Fix this by removing this part of the test.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32

gdb/testsuite/ChangeLog:

2021-01-18  Tom de Vries  <tdevries@suse.de>

	* gdb.python/py-finish-breakpoint2.exp: Remove part of test that
	works by accident.

---
 gdb/testsuite/gdb.python/py-finish-breakpoint2.exp | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
index 10c4b6e81b8..4193f073996 100644
--- a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
@@ -46,12 +46,6 @@ gdb_test "source $pyfile" ".*Python script imported.*" \
          
 gdb_breakpoint "throw_exception_1"
 gdb_test "continue" "Breakpoint .*throw_exception_1.*" "run to exception 1"
-
-gdb_test "python print (len(gdb.breakpoints()))" "3" "check BP count"
-gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
-gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "check FinishBreakpoint in catch()"
-gdb_test "python print (len(gdb.breakpoints()))" "3" "check finish BP removal"
-
 gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second exception"
 gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
 gdb_test "continue" ".*exception did not finish.*" "FinishBreakpoint with exception thrown not caught"

Re: [PATCH][gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32

[Simon Marchi]

On 2021-01-18 5:31 a.m., Tom de Vries wrote:
> Hi,
> 
> When running test-case gdb.python/py-finish-breakpoint2.exp with target board
> unix/-m32, we run into:
> ...
> (gdb) continue^M
> Continuing.^M
> Exception #10^M
> ^M
> Breakpoint 3, throw_exception_1 (e=10) at py-finish-breakpoint2.cc:23^M
> 23        throw new int (e);^M
> (gdb) FAIL: gdb.python/py-finish-breakpoint2.exp: \
>   check FinishBreakpoint in catch()
> ...
> With -m64, the test passes.
> 
> Relevant bit of the source:
> ...
>     36    try
>     37      {
>     38        throw_exception_1 (10);
>     39      }
>     40    catch (const int *e)
>     41      {
>     42          std::cerr << "Exception #" << *e << std::endl;
>     43      }
>     44    i += 1; /* Break after exception 1.  */
> ...
> 
> The -m64 scenario in more detail:
> - the test-case runs to throw_exception_1.
> - it installs a FinishBreakpoint, which is a temporary breakpoint set at the
>   return address of a frame.
> - for -m64, that address is:
>     400c47:       83 45 e4 01             addl   $0x1,-0x1c(%rbp)
>   which corresponds the "i += 1 at line 44"
> - the test-case then continues
> - an exception is throw in throw_exection_1
> - the exception is caught at line 40, and a message is printed
> - line 44 is executed, and the FinishBreakpoint triggers.
> 
> With -m32, we have instead:
> - the address where the finish breakpoint is set is:
>     8048a0a:       83 c4 10                add    $0x10,%esp
>   which is the lasn insn generated for the call at line 38
> - the test-case continues
> - an exception is throw in throw_exection_1
> - consequently, the FinishBreakpoint is not triggered.
> 
> In conclusion, the test worked by accident for -m64, because the first insn
> after the call to throw_exception_1 is also the first insn after the try.
> And that just happens to be not the case for -m32.
> 
> Fix this by removing this part of the test.
> 
> Tested on x86_64-linux.
> 
> Any comments?
> 
> Thanks,
> - Tom
> 
> [gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32
> 
> gdb/testsuite/ChangeLog:
> 
> 2021-01-18  Tom de Vries  <tdevries@suse.de>
> 
> 	* gdb.python/py-finish-breakpoint2.exp: Remove part of test that
> 	works by accident.
> 
> ---
>  gdb/testsuite/gdb.python/py-finish-breakpoint2.exp | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
> index 10c4b6e81b8..4193f073996 100644
> --- a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
> @@ -46,12 +46,6 @@ gdb_test "source $pyfile" ".*Python script imported.*" \
>           
>  gdb_breakpoint "throw_exception_1"
>  gdb_test "continue" "Breakpoint .*throw_exception_1.*" "run to exception 1"
> -
> -gdb_test "python print (len(gdb.breakpoints()))" "3" "check BP count"
> -gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
> -gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "check FinishBreakpoint in catch()"
> -gdb_test "python print (len(gdb.breakpoints()))" "3" "check finish BP removal"
> -
>  gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second exception"
>  gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>  gdb_test "continue" ".*exception did not finish.*" "FinishBreakpoint with exception thrown not caught"
> 

Hmm I'd like to understand better the original intent of the test.

Clearly, it tries to test two different things, because the first
time it expects to see the print from the "stop" method, and the
second time it expects to see the print from the "out_of_scope"
method.  But I don't really understand what's different in both
cases.  Both times it just runs to throw_exception_1 and sets
a FinishBreakpoint.  The throw_exception_1 function just throws,
so presumably, we should expect the same outcome both times,
don't we?  And that outcome should be that the out_of_scope
method gets called, right?

When I try to replicate the bit you removed by hand, I indeed
get two different things with -m32 and -m64, and both are not
the one I expect.  With -m64, "stop", gets called:

$ ./gdb -q -nx --data-directory=data-directory m64 -ex "b throw_exception_1" -ex r -x ~/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.py -ex "python ExceptionFinishBreakpoint(gdb.newest_frame())"
Reading symbols from m64...
Breakpoint 1 at 0x11f7: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc, line 23.
Starting program: /home/simark/build/binutils-gdb/gdb/m64 

Breakpoint 1, throw_exception_1 (e=10) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc:23
23        throw new int (e);
Python script imported
init ExceptionFinishBreakpoint
(gdb) continue
Continuing.
Exception #10
stopped at ExceptionFinishBreakpoint


And with -m32, nothing gets gets called, the FinishBreakpoint
just never hits (we just hit throw_exception_1 later):

./gdb -q -nx --data-directory=data-directory m32 -ex "b throw_exception_1" -ex r -x ~/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.py -ex "python ExceptionFinishBreakpoint(gdb.newest_frame())"
Reading symbols from m32...
Breakpoint 1 at 0x1261: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc, line 23.
Starting program: /home/simark/build/binutils-gdb/gdb/m32 

Breakpoint 1, throw_exception_1 (e=10) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc:23
23        throw new int (e);
Python script imported
init ExceptionFinishBreakpoint
(gdb) continue
Continuing.
Exception #10

Breakpoint 1, throw_exception_1 (e=10) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc:23
23        throw new int (e);
(gdb) 


Surely, there is some bug in there.  Shouldn't we at least
see the same behavior for -m32 and -m64 for what I shown
above?  Can you help me understand what is happening here?

Simon

Re: [PATCH][gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32

[Tom de Vries]

On 1/21/21 8:04 AM, Simon Marchi wrote:
> On 2021-01-18 5:31 a.m., Tom de Vries wrote:
>> Hi,
>>
>> When running test-case gdb.python/py-finish-breakpoint2.exp with target board
>> unix/-m32, we run into:
>> ...
>> (gdb) continue^M
>> Continuing.^M
>> Exception #10^M
>> ^M
>> Breakpoint 3, throw_exception_1 (e=10) at py-finish-breakpoint2.cc:23^M
>> 23        throw new int (e);^M
>> (gdb) FAIL: gdb.python/py-finish-breakpoint2.exp: \
>>   check FinishBreakpoint in catch()
>> ...
>> With -m64, the test passes.
>>
>> Relevant bit of the source:
>> ...
>>     36    try
>>     37      {
>>     38        throw_exception_1 (10);
>>     39      }
>>     40    catch (const int *e)
>>     41      {
>>     42          std::cerr << "Exception #" << *e << std::endl;
>>     43      }
>>     44    i += 1; /* Break after exception 1.  */
>> ...
>>
>> The -m64 scenario in more detail:
>> - the test-case runs to throw_exception_1.
>> - it installs a FinishBreakpoint, which is a temporary breakpoint set at the
>>   return address of a frame.
>> - for -m64, that address is:
>>     400c47:       83 45 e4 01             addl   $0x1,-0x1c(%rbp)
>>   which corresponds the "i += 1 at line 44"
>> - the test-case then continues
>> - an exception is throw in throw_exection_1
>> - the exception is caught at line 40, and a message is printed
>> - line 44 is executed, and the FinishBreakpoint triggers.
>>
>> With -m32, we have instead:
>> - the address where the finish breakpoint is set is:
>>     8048a0a:       83 c4 10                add    $0x10,%esp
>>   which is the lasn insn generated for the call at line 38
>> - the test-case continues
>> - an exception is throw in throw_exection_1
>> - consequently, the FinishBreakpoint is not triggered.
>>
>> In conclusion, the test worked by accident for -m64, because the first insn
>> after the call to throw_exception_1 is also the first insn after the try.
>> And that just happens to be not the case for -m32.
>>
>> Fix this by removing this part of the test.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
>>
>> Thanks,
>> - Tom
>>
>> [gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32
>>
>> gdb/testsuite/ChangeLog:
>>
>> 2021-01-18  Tom de Vries  <tdevries@suse.de>
>>
>> 	* gdb.python/py-finish-breakpoint2.exp: Remove part of test that
>> 	works by accident.
>>
>> ---
>>  gdb/testsuite/gdb.python/py-finish-breakpoint2.exp | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
>> index 10c4b6e81b8..4193f073996 100644
>> --- a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
>> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
>> @@ -46,12 +46,6 @@ gdb_test "source $pyfile" ".*Python script imported.*" \
>>           
>>  gdb_breakpoint "throw_exception_1"
>>  gdb_test "continue" "Breakpoint .*throw_exception_1.*" "run to exception 1"
>> -
>> -gdb_test "python print (len(gdb.breakpoints()))" "3" "check BP count"
>> -gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>> -gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "check FinishBreakpoint in catch()"
>> -gdb_test "python print (len(gdb.breakpoints()))" "3" "check finish BP removal"
>> -
>>  gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second exception"
>>  gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>>  gdb_test "continue" ".*exception did not finish.*" "FinishBreakpoint with exception thrown not caught"
>>
> 
> Hmm I'd like to understand better the original intent of the test.
> 
> Clearly, it tries to test two different things, because the first
> time it expects to see the print from the "stop" method, and the
> second time it expects to see the print from the "out_of_scope"
> method.

Agreed, the intent of the test is to test both scenarios, in the context
of throwing an exception.

> But I don't really understand what's different in both
> cases.  Both times it just runs to throw_exception_1 and sets
> a FinishBreakpoint.  The throw_exception_1 function just throws,
> so presumably, we should expect the same outcome both times,
> don't we?  And that outcome should be that the out_of_scope
> method gets called, right?
> 

Well, the difference between the two scenarios is in the call stack at
the point that we set the FinishBreakpoint:
- in the first scenario, the call stack is:
  main -> throw_exception_1
- in the second scenario, the call stack is:
  main -> throw_exception -> throw_exception_1

In the second scenario, the FinishBreakpoint is set in the
throw_exception function on the insn after the call to
throw_exception_1.  This guarantees that FinishBreakpoint.stop will not
trigger, so FinishBreakpoint.out_of_scope will get called.

In the first scenario, the FinishBreakpoint is set on the insn after the
call to throw_exception_1 in main. For -m32, that happens to be an insn
in the try clause, and FinishBreakpoint.stop will not trigger.  For
-m64, that happens to be the location after the try/catch, and
FinishBreakpoint.stop will trigger.

The different outcomes for -m32 and -m64 are both valid given the
semantics of FinishBreakpoint, which is "set at the return address of a
frame".  It all depends where that return address is.  There is no
guarantee that the return address is an insn that uniquely represent the
function return control path.

> When I try to replicate the bit you removed by hand, I indeed
> get two different things with -m32 and -m64, and both are not
> the one I expect.  With -m64, "stop", gets called:
> 
> $ ./gdb -q -nx --data-directory=data-directory m64 -ex "b throw_exception_1" -ex r -x ~/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.py -ex "python ExceptionFinishBreakpoint(gdb.newest_frame())"
> Reading symbols from m64...
> Breakpoint 1 at 0x11f7: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc, line 23.
> Starting program: /home/simark/build/binutils-gdb/gdb/m64 
> 
> Breakpoint 1, throw_exception_1 (e=10) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc:23
> 23        throw new int (e);
> Python script imported
> init ExceptionFinishBreakpoint
> (gdb) continue
> Continuing.
> Exception #10
> stopped at ExceptionFinishBreakpoint
> 

Right, the FinishBreakpoint is set at the insn after the try/catch and
the stop triggers.

> And with -m32, nothing gets gets called, the FinishBreakpoint
> just never hits (we just hit throw_exception_1 later):
> 

Right, the FinishBreakpoint is set in the try clause and stop doesn't
trigger.  If we run to exit, we'll hit out_of_scope.

> ./gdb -q -nx --data-directory=data-directory m32 -ex "b throw_exception_1" -ex r -x ~/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.py -ex "python ExceptionFinishBreakpoint(gdb.newest_frame())"
> Reading symbols from m32...
> Breakpoint 1 at 0x1261: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc, line 23.
> Starting program: /home/simark/build/binutils-gdb/gdb/m32 
> 
> Breakpoint 1, throw_exception_1 (e=10) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc:23
> 23        throw new int (e);
> Python script imported
> init ExceptionFinishBreakpoint
> (gdb) continue
> Continuing.
> Exception #10
> 
> Breakpoint 1, throw_exception_1 (e=10) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc:23
> 23        throw new int (e);
> (gdb) 
> 
> 
> Surely, there is some bug in there.  Shouldn't we at least
> see the same behavior for -m32 and -m64 for what I shown
> above?  Can you help me understand what is happening here?

I don't see a bug here.  I think different behaviour for -m32 and -m64
(and for other architectures) is possible.  All I see here is a test
that depends on a coincidence.

Perhaps it helps to think of a Finish Breakpoint as a "Return address
Breakpoint".

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32

[Tom de Vries]

On 1/21/21 9:29 AM, Tom de Vries wrote:
> The different outcomes for -m32 and -m64 are both valid given the
> semantics of FinishBreakpoint, which is "set at the return address of a
> frame".  It all depends where that return address is.  There is no
> guarantee that the return address is an insn that uniquely represent the
> function return control path.

And, reading the documentation:
...
Function: FinishBreakpoint.out_of_scope (self)

    In some circumstances (e.g. longjmp, C++ exceptions, GDB return
    command, …), a function may not properly terminate, and thus never
    hit the finish breakpoint. When GDB notices such a situation, the
    out_of_scope callback will be triggered.
...
this may be somewhat misleading or unclear, given that it's possible (as
the -m64 case demonstrates) that both:
- the function does properly terminate, and
- the finish breakpoint still hits (meaning the stop method is called).

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Fix gdb.python/py-finish-breakpoint2.exp with -m32

[Simon Marchi]

On 2021-01-21 3:45 a.m., Tom de Vries wrote:
> On 1/21/21 9:29 AM, Tom de Vries wrote:
>> The different outcomes for -m32 and -m64 are both valid given the
>> semantics of FinishBreakpoint, which is "set at the return address of a
>> frame".  It all depends where that return address is.  There is no
>> guarantee that the return address is an insn that uniquely represent the
>> function return control path.
> 
> And, reading the documentation:
> ...
> Function: FinishBreakpoint.out_of_scope (self)
> 
>     In some circumstances (e.g. longjmp, C++ exceptions, GDB return
>     command, …), a function may not properly terminate, and thus never
>     hit the finish breakpoint. When GDB notices such a situation, the
>     out_of_scope callback will be triggered.
> ...
> this may be somewhat misleading or unclear, given that it's possible (as
> the -m64 case demonstrates) that both:
> - the function does properly terminate, and
> - the finish breakpoint still hits (meaning the stop method is called).
> 
> Thanks,
> - Tom
> 

I think don't see how FinishBreakpoint can be useful with that kind of
inconsistency.  Given that an exception is thrown in both cases, and the
throw_exception_1 call frame never properly terminates, it seems obvious
to me that out_of_scope should be called both times, and stop should not
be called, both times.

Simon

[RFC][gdb/python] FinishBreakPoint update

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

[ was: Re: [PATCH][gdb/testsuite] Fix
gdb.python/py-finish-breakpoint2.exp with -m32 ]

On 1/21/21 3:22 PM, Simon Marchi wrote:
> On 2021-01-21 3:45 a.m., Tom de Vries wrote:
>> On 1/21/21 9:29 AM, Tom de Vries wrote:
>>> The different outcomes for -m32 and -m64 are both valid given the
>>> semantics of FinishBreakpoint, which is "set at the return address of a
>>> frame".  It all depends where that return address is.  There is no
>>> guarantee that the return address is an insn that uniquely represent the
>>> function return control path.
>>
>> And, reading the documentation:
>> ...
>> Function: FinishBreakpoint.out_of_scope (self)
>>
>>     In some circumstances (e.g. longjmp, C++ exceptions, GDB return
>>     command, …), a function may not properly terminate, and thus never
>>     hit the finish breakpoint. When GDB notices such a situation, the
>>     out_of_scope callback will be triggered.
>> ...
>> this may be somewhat misleading or unclear, given that it's possible (as
>> the -m64 case demonstrates) that both:
>> - the function does properly terminate, and
>> - the finish breakpoint still hits (meaning the stop method is called).
>>
>> Thanks,
>> - Tom
>>
> 
> I think don't see how FinishBreakpoint can be useful with that kind of
> inconsistency.  Given that an exception is thrown in both cases, and the
> throw_exception_1 call frame never properly terminates, it seems obvious
> to me that out_of_scope should be called both times, and stop should not
> be called, both times.

Hi Simon,

thanks for your reaction, apologies for my late reaction.

I ran into this once more and gave it another try, that is, to
consolidate current state in terms of implementation, documentation and
testing.

I propose to commit this patch (perhaps split in two), and then
deprecate the functionality.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-python-FinishBreakPoint-update.patch --]
[-- Type: text/x-patch, Size: 15127 bytes --]

[gdb/python] FinishBreakPoint update

I.

Consider the python gdb.FinishBreakpoint class, an extension of the
gdb.Breakpoint class.

It sets a temporary breakpoint on the return address of a frame.

This type of breakpoints is thread-specific.

II.

If the FinishBreakpoint is hit, it is deleted, and the method return_value
can be used to get the value returned by the function.

Let's demonstrate this:
...
$ cat -n test.c
     1  int foo (int a) { return a + 2; }
     2  int main () { return foo (1); }
$ gcc -g test.c
$ gdb -q -batch a.out -ex "set trace-commands on" -ex "tbreak foo" -ex run \
  -ex "python bp = gdb.FinishBreakpoint()" -ex "info breakpoints" \
  -ex continue -ex "info breakpoints" -ex "python print (bp.return_value)"
+tbreak foo
Temporary breakpoint 1 at 0x40049e: file test.c, line 1.
+run

Temporary breakpoint 1, foo (a=1) at test.c:1
1       int foo (int a) { return a + 2; }
+python bp = gdb.FinishBreakpoint()
Temporary breakpoint 2 at 0x4004b4: file test.c, line 2.
+info breakpoints
Num     Type           Disp Enb Address    What
2       breakpoint     del  y   0x004004b4 in main at test.c:2 thread 1
        stop only in thread 1
+continue

Temporary breakpoint 2, 0x004004b4 in main () at test.c:2
2       int main () { return foo (1); }
+info breakpoints
No breakpoints or watchpoints.
+python print (bp.return_value)
3
...

III.

Another possibility is that the FinishBreakpoint is not hit, because the
function did not terminate, f.i. because of longjmp, C++ exceptions, or GDB
return command.

Let's demonstrate this, using C++ exceptions:
...
$ cat -n test.c
     1  int foo (int a) { throw 1; return a + 2; }
     2  int main () {
     3    try { return foo (1); } catch (...) {};
     4    return 1;
     5  }
$ g++ -g test.c
$ gdb -q -batch a.out -ex "set trace-commands on" -ex "tbreak foo" -ex run \
  -ex "python bp = gdb.FinishBreakpoint()" -ex "info breakpoints" \
  -ex continue -ex "info breakpoints" -ex "python print (bp.return_value)"
+tbreak foo
Temporary breakpoint 1 at 0x400712: file test.c, line 1.
+run

Temporary breakpoint 1, foo (a=1) at test.c:1
1       int foo (int a) { throw 1; return a + 2; }
+python bp = gdb.FinishBreakpoint()
Temporary breakpoint 2 at 0x400742: file test.c, line 3.
+info breakpoints
Num     Type           Disp Enb Address    What
2       breakpoint     del  y   0x00400742 in main() at test.c:3 thread 1
        stop only in thread 1
+continue
[Inferior 1 (process 25269) exited with code 01]
Thread-specific breakpoint 2 deleted - thread 1 no longer in the thread list.
+info breakpoints
No breakpoints or watchpoints.
+python print (bp.return_value)
None
...

Indeed, we do not hit the FinishBreakpoint.  Instead, it's deleted when the
thread disappears, like any other thread-specific breakpoint.

I think this is a bug: the deletion is meant to be handled by FinishBreakpoint
itself.

IV.

Fix aforementioned bug by:
- adding an observer of the thread_exit event in FinishBreakpoint
- making sure that this observer is called before the
  remove_threaded_breakpoints observer of that same event.

This changes the behaviour to:
...
+continue
[Inferior 1 (process 30256) exited with code 01]
+info breakpoints
No breakpoints or watchpoints.
+python print (bp.return_value)
None
...

V.

An out_of_scope callback can be defined to make this more verbose:
...
$ cat fbp.py
class FBP(gdb.FinishBreakpoint):
    def __init__(self):
        gdb.FinishBreakpoint.__init__(self)

    def out_of_scope(self):
        try:
            frame = gdb.selected_frame ()
            sal = frame.find_sal ()
            print ("out_of_scope triggered at %s:%s"
                   % (sal.symtab.fullname(), sal.line))
        except gdb.error as e:
            print ("out_of_scope triggered at thread/inferior exit")
...
and using that gets us:
...
+continue
[Inferior 1 (process 30742) exited with code 01]
out_of_scope triggered at thread/inferior exit
+info breakpoints
No breakpoints or watchpoints.
+python print (bp.return_value)
None
...

VI.

This out_of_scope event can be triggered earlier than inferior/thread exit.

Let's demonstrate this:
...
$ cat -n test.c
     1  int bar (int a) { throw 1; return a + 2; }
     2  int foo (int a) {
     3    int res = a;
     4    try
     5      {
     6        res += bar (1);
     7      }
     8    catch (...)
     9      {
    10      }
    11    return res;
    12  }
    13  int main () {
    14    int res = 0;
    15    res += foo (1);
    16    res += 2;
    17    return res * 2;
    18  }
$ g++ -g test.c
$ gdb -q -batch -ex "source fbp.py" a.out -ex "set trace-commands on" \
  -ex "tbreak bar" -ex run -ex "python bp = FBP()" -ex "info breakpoints" \
  -ex "tbreak 16" -ex continue -ex "info breakpoints" \
  -ex "python print (bp.return_value)"
+tbreak bar
Temporary breakpoint 1 at 0x400712: file test.c, line 1.
+run

Temporary breakpoint 1, bar (a=1) at test.c:1
1       int bar (int a) { throw 1; return a + 2; }
+python bp = FBP()
Temporary breakpoint 2 at 0x40074f: file test.c, line 6.
+info breakpoints
Num     Type           Disp Enb Address    What
2       breakpoint     del  y   0x0040074f in foo(int) at test.c:6 thread 1
        stop only in thread 1
+tbreak 16
Temporary breakpoint 3 at 0x400784: file test.c, line 16.
+continue

Temporary breakpoint 3, main () at test.c:16
16        res += 2;
out_of_scope triggered at /home/vries/gdb_versions/devel/test.c:16
+info breakpoints
No breakpoints or watchpoints.
+python print (bp.return_value)
None
...

Note that the out_of_scope event triggers at the breakpoint we set at
test.c:16.  If we'd set that breakpoint at line 17, the out_of_scope event
would trigger at line 17 instead.

Also note that it can't be triggered earlier, say by setting the breakpoint in
foo at line 11.  We would get instead "out_of_scope triggered at
thread/inferior exit".

VII.

Now consider a reduced version of
src/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc:
...
$ cat -n test.c
     1  #include <iostream>
     2
     3  void
     4  throw_exception_1 (int e)
     5  {
     6    throw new int (e);
     7  }
     8
     9  int
    10  main (void)
    11  {
    12    int i;
    13    try
    14      {
    15        throw_exception_1 (10);
    16      }
    17    catch (const int *e)
    18      {
    19          std::cerr << "Exception #" << *e << std::endl;
    20      }
    21    i += 1;
    22
    23    return i;
    24  }
$ g++ -g test.c
...

Now let's try to see if the FinishBreakPoint triggers:
...
$ gdb -q -batch -ex "source fbp.py" a.out -ex "set trace-commands on" \
  -ex "tbreak throw_exception_1" -ex run -ex "python bp = FBP()" \
  -ex "info breakpoints" -ex continue -ex "info breakpoints" \
  -ex "python print (bp.return_value)"
+tbreak throw_exception_1
Temporary breakpoint 1 at 0x400bd5: file test.c, line 6.
+run

Temporary breakpoint 1, throw_exception_1 (e=10) at test.c:6
6         throw new int (e);
+python bp = FBP()
Temporary breakpoint 2 at 0x400c2f: file test.c, line 21.
+info breakpoints
Num     Type           Disp Enb Address    What
2       breakpoint     del  y   0x00400c2f in main() at test.c:21 thread 1
        stop only in thread 1
+continue
Exception #10

Temporary breakpoint 2, main () at test.c:21
21        i += 1;
+info breakpoints
No breakpoints or watchpoints.
+python print (bp.return_value)
None
...

Surprisingly, it did.  The explanation is that FinishBreakPoint is really a
frame-return-address breakpoint, and that address happens to be at line 21,
which is still executed after the throw in throw_exception_1.

Interestingly, with -m32 the FinishBreakPoint doesn't trigger, because the
frame-return-address happens to be an instruction which is part of line 15.

VIII.

In conclusion, the FinishBreakpoint is a frame-return-address breakpoint.

After being set, either:
- it triggers, or
- an out-of-scope event will be generated.

If an out-of-scope event is generated, it will be due to incomplete function
termination.

OTOH, incomplete function termination does not guarantee an out-of-scope event
instead of hitting the breakpoint.

IX.

The documentation states that 'A finish breakpoint is a temporary breakpoint
set at the return address of a frame, based on the finish command'.

It's indeed somewhat similar to the finish command, at least in the sense that
both may stop at the frame-return-address.

But the finish command can accurately detect function termination.

And the finish command will stop at any other address that is the first
address not in the original function.

The documentation needs updating to accurately describe what it does.

X.

A better implementation of a finish breakpoint would be one that borrows from
the finish command implementation.  That one:
- installs a thread_fsm, and
- continues

A finish breakpoint would do the same minus the continue, but it requires gdb
to handle multiple thread_fsms at a time (because other commands may wish to
install their own thread_fsm), which AFAICT is not supported yet.

XI.

This patch repairs a minor part of the functionality, and updates
documentation and test-cases to match actual behaviour.

The question remains how useful the functionality is, as it is now
( see f.i. discussion at
https://sourceware.org/pipermail/gdb-patches/2021-January/175290.html ).
Perhaps it would be better to deprecate this in a follow-up patch in some form
or another, say by disabling it by default and introducing a maintenance
command that switches it on, with the warning that it is deprecated.

Tested on x86_64-linux with native and target board unix/-m32, by rebuilding
and running the test-cases:
- gdb.python/py-finish-breakpoint.exp
- gdb.python/py-finish-breakpoint2.exp

---
 gdb/breakpoint.c                                   | 10 ++++++++++
 gdb/doc/python.texi                                |  6 ++++--
 gdb/python/py-finishbreakpoint.c                   | 21 +++++++++++++++++++++
 gdb/testsuite/gdb.python/py-finish-breakpoint2.exp | 16 +++++++++++++---
 4 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3b626b8f4aa..7de735561c9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15437,6 +15437,10 @@ initialize_breakpoint_ops (void)
 
 static struct cmd_list_element *enablebreaklist = NULL;
 
+#if HAVE_PYTHON
+extern gdb::observers::token bpfinishpy_handle_thread_exit_observer_token;
+#endif
+
 /* See breakpoint.h.  */
 
 cmd_list_element *commands_cmd_element = nullptr;
@@ -16050,6 +16054,12 @@ This is useful for formatted output in user-defined commands."));
 
   gdb::observers::about_to_proceed.attach (breakpoint_about_to_proceed,
 					   "breakpoint");
+#if HAVE_PYTHON
+  gdb::observers::thread_exit.attach
+    (remove_threaded_breakpoints, "breakpoint",
+     { &bpfinishpy_handle_thread_exit_observer_token });
+#else
   gdb::observers::thread_exit.attach (remove_threaded_breakpoints,
 				      "breakpoint");
+#endif
 }
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index d8f682a091c..235b126abdb 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5712,7 +5712,7 @@ attribute is @code{None}.  This attribute is writable.
 @tindex gdb.FinishBreakpoint
 
 A finish breakpoint is a temporary breakpoint set at the return address of
-a frame, based on the @code{finish} command.  @code{gdb.FinishBreakpoint}
+a frame.  @code{gdb.FinishBreakpoint}
 extends @code{gdb.Breakpoint}.  The underlying breakpoint will be disabled 
 and deleted when the execution will run out of the breakpoint scope (i.e.@: 
 @code{Breakpoint.stop} or @code{FinishBreakpoint.out_of_scope} triggered).
@@ -5731,7 +5731,9 @@ details about this argument.
 In some circumstances (e.g.@: @code{longjmp}, C@t{++} exceptions, @value{GDBN} 
 @code{return} command, @dots{}), a function may not properly terminate, and
 thus never hit the finish breakpoint.  When @value{GDBN} notices such a
-situation, the @code{out_of_scope} callback will be triggered.
+situation, the @code{out_of_scope} callback will be triggered.  Note
+though that improper function termination does not guarantee that the
+finish breakpoint is not hit.
 
 You may want to sub-class @code{gdb.FinishBreakpoint} and override this
 method:
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 1d8373d807e..a881103fdad 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -398,6 +398,24 @@ bpfinishpy_handle_exit (struct inferior *inf)
     bpfinishpy_detect_out_scope_cb (bp, nullptr);
 }
 
+/* Attached to `thread_exit' notifications, triggers all the necessary out of
+   scope notifications.  */
+
+static void
+bpfinishpy_handle_thread_exit (struct thread_info *tp, int ignore)
+{
+  gdbpy_enter enter_py (target_gdbarch (), current_language);
+
+  for (breakpoint *bp : all_breakpoints_safe ())
+    {
+      if (tp->global_num == bp->thread)
+	bpfinishpy_detect_out_scope_cb (bp, nullptr);
+    }
+}
+
+extern gdb::observers::token bpfinishpy_handle_thread_exit_observer_token;
+gdb::observers::token bpfinishpy_handle_thread_exit_observer_token;
+
 /* Initialize the Python finish breakpoint code.  */
 
 int
@@ -414,6 +432,9 @@ gdbpy_initialize_finishbreakpoints (void)
 				      "py-finishbreakpoint");
   gdb::observers::inferior_exit.attach (bpfinishpy_handle_exit,
 					"py-finishbreakpoint");
+  gdb::observers::thread_exit.attach
+    (bpfinishpy_handle_thread_exit,
+     bpfinishpy_handle_thread_exit_observer_token, "py-finishbreakpoint");
 
   return 0;
 }
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
index 58e086ad3b4..46c39d0d108 100644
--- a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
@@ -50,10 +50,20 @@ gdb_test "continue" "Breakpoint .*throw_exception_1.*" "run to exception 1"
 gdb_test "python print (len(gdb.breakpoints()))" "3" "check BP count"
 gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" \
     "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
-gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "check FinishBreakpoint in catch()"
-gdb_test "python print (len(gdb.breakpoints()))" "3" "check finish BP removal"
 
-gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second exception"
+gdb_test_multiple "continue" "continue after setting FinishBreakpoint" {
+    -re -wrap ".*stopped at ExceptionFinishBreakpoint.*" {
+	pass "$gdb_test_name (scenario 1, triggered)"
+	gdb_test "python print (len(gdb.breakpoints()))" "3" \
+	    "check finish BP removal"
+	gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" \
+	    "continue to second exception"
+    }
+    -re -wrap ".*Breakpoint.* throw_exception_1.*" {
+	pass "$gdb_test_name (scenario 2, not triggered)"
+    }
+}
+
 gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" \
     "init ExceptionFinishBreakpoint" "set FinishBP after the exception again"
 gdb_test "continue" ".*exception did not finish.*" "FinishBreakpoint with exception thrown not caught"