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

[Tom de Vries <tdevries@suse.de>]

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