stepping over permanent breakpoint steps one instruction too much? (Re: diffgdbdaymail 20141029)

On 10/29/2014 04:20 PM, Pedro Alves wrote:
> On 10/29/2014 11:49 AM, diffgdbdaymail by Jan Kratochvil wrote:
>> --- 20141028Build-gdbgit-f20/fedora-20-x86_64/out/gdb-m32.sum	2014-10-28 10:12:27.757881095 +0100
>> --- GIT commit 6fb9c0f83252a79b2f1a3f8e75fa117ca7a4d589
>> +++ 20141029Build-gdbgit-f20/fedora-20-x86_64/out/gdb-m32.sum	2014-10-29 09:52:20.310537452 +0100
>> +++ GIT commit 6ae274b7dc305ae7cebcf55c5018dab05228235a
>>
>> #gdb.arch/i386-bp_permanent.exp
>>  Running gdb/testsuite/gdb.arch/i386-bp_permanent.exp ...
>>  PASS: gdb.arch/i386-bp_permanent.exp: Stop at the 'standard' start breakpoint (fetching esp).
>>  PASS: gdb.arch/i386-bp_permanent.exp: Stop at permanent breakpoint.
>> -PASS: gdb.arch/i386-bp_permanent.exp: Single stepping past permanent breakpoint.
>> -PASS: gdb.arch/i386-bp_permanent.exp: ESP value does not match - step_permanent_breakpoint wrong.
>> +FAIL: gdb.arch/i386-bp_permanent.exp: Single stepping past permanent breakpoint. (GDB internal error)
>> +FAIL: gdb.arch/i386-bp_permanent.exp: ESP value does not match - step_permanent_breakpoint wrong.
> 
> Mine.  The test is skipped on 64-bit, and seems like I missed the case
> of stepping permanent breakpoints.
> 
> (gdb) PASS: gdb.arch/i386-bp_permanent.exp: Stop at permanent breakpoint.
> stepi
> ../../src/gdb/infrun.c:2237: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) FAIL: gdb.arch/i386-bp_permanent.exp: Single stepping past permanent breakpoint. (GDB internal error)
> Resyncing due to internal error.
> n
> 
> This is a bug, please report it.  For instructions, see:
> <http://www.gnu.org/software/gdb/bugs/>.
> 
> ../../src/gdb/infrun.c:2237: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n
> 0x080484ad in standard ()
> Command aborted.
> (gdb) print $esp
> $2 = (void *) 0xffffc334
> (gdb) FAIL: gdb.arch/i386-bp_permanent.exp: ESP value does not match - step_permanent_breakpoint wrong.

Hmm, this is all looking bogus to me.

The implementation of skip_permanent_breakpoint for i386,
added along with this particular test [1], just increments the
PC past the breakpoint.  OK, that makes sense.

But then, consider the case of the user doing "step/stepi" when
stopped at a permanent breakpoint.  GDB's `resume' calls the
gdbarch_skip_permanent_breakpoint hook, and happily continues
stepping:

  /* Normally, by the time we reach `resume', the breakpoints are either
     removed or inserted, as appropriate.  The exception is if we're sitting
     at a permanent breakpoint; we need to step over it, but permanent
     breakpoints can't be removed.  So we have to test for it here.  */
  if (breakpoint_here_p (aspace, pc) == permanent_breakpoint_here)
    {
      if (gdbarch_skip_permanent_breakpoint_p (gdbarch))
	gdbarch_skip_permanent_breakpoint (gdbarch, regcache);
      else
	error (_("\
The program is stopped at a permanent breakpoint, but GDB does not know\n\
how to step past a permanent breakpoint on this architecture.  Try using\n\
a command like `return' or `jump' to continue execution."));
    }

But since gdbarch_skip_permanent_breakpoint already advanced the PC
manually, this ends up executing the instruction that is _after_
the breakpoint instruction.

And, the test is actually ensuring that that's indeed how
things work.  I mean, the test runs to the int3 instruction,
and then does "stepi", and ensures that "leave" was executed
with that "stepi".  Like this:

(gdb) b *0x0804848c
Breakpoint 2 at 0x804848c
(gdb) c
Continuing.

Breakpoint 2, 0x0804848c in standard ()
(gdb) disassemble
Dump of assembler code for function standard:
   0x08048488 <+0>:     push   %ebp
   0x08048489 <+1>:     mov    %esp,%ebp
   0x0804848b <+3>:     push   %edi
=> 0x0804848c <+4>:     int3
   0x0804848d <+5>:     leave
   0x0804848e <+6>:     ret
   0x0804848f <+7>:     nop
(gdb) si
0x0804848e in standard ()
(gdb) disassemble
Dump of assembler code for function standard:
   0x08048488 <+0>:     push   %ebp
   0x08048489 <+1>:     mov    %esp,%ebp
   0x0804848b <+3>:     push   %edi
   0x0804848c <+4>:     int3
   0x0804848d <+5>:     leave
=> 0x0804848e <+6>:     ret
   0x0804848f <+7>:     nop
End of assembler dump.
(gdb)

I would instead expect that a stepi at 0x0804848c stops at 0x0804848d,
_before_ the "leave" is executed.

The comment in the test reads:

 # We want to fetch esp at the start of '$function' function to make sure
 # skip_permanent_breakpoint implementation really skips only the perm.
 # breakpoint. If, for whatever reason, 'leave' instruction doesn't get
 # executed, esp will not have this value.

But that sounds very conflicted --- either the "implementation really
skips _only_ the perm. breakpoint", or the 'leave' instruction is
executed.

I'm working on fixing GDB in the direction of stepi stopping before
the "leave" is executed.  Does anyone have a different opinion
on how GDB should behave?

[1] https://sourceware.org/ml/gdb-patches/2008-08/msg00521.html

Thanks,
Pedro Alves