Re: [patch] Fix disp-step-syscall.exp on some i386 targets

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Tue, 28 Feb 2012 08:50:38 +0100, Yao Qi wrote:
> On 02/28/2012 03:22 AM, Jan Kratochvil wrote:
> > +   'int $0x80; ret' and i386_displaced_step_fixup would keep PC at the displaced
> > +   location expecting it just executed 'ret' despite it finished the syscall.
> > +   Hide the 'ret' instruction by 'nop'.  */
> > +
> 
> We write 'ret' into scratchpad, but return 'nop' in closure.  I am
> afraid it is a little risky.

Yes, it is a hack.  I find the most correct way to have a flag in i386-linux
struct displaced_step_closure to mark it.  But we would have then to
complicate i386-linux-tdep by wrapping all the three displaced vector methods
in i386-linux-tdep which I did not find worth it.


> > +struct displaced_step_closure *
> > +i386_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
> > +				     CORE_ADDR from, CORE_ADDR to,
> > +				     struct regcache *regs)
> > +{
> > +  struct displaced_step_closure *closure;
> > +  
> > +  closure = i386_displaced_step_copy_insn (gdbarch, from, to, regs);
> > +
> > +  if (i386_linux_get_syscall_number_from_regcache (regs) != -1)
> 
> I don't understand this.  If we are doing displaced stepping on
> arbitrary instruction, the condition is always true?, I guess.

Not in the first case.

0:int 0x80
2:ret

A:
PC == 0, orig_eax == -1, GDB in i386_displaced_step_copy_insn
PTRACE_SINGLESTEP, waitpid == SIGTRAP | PTRACE_EVENT_FORK
PC == 2, orig_eax == __NR_fork, GDB in i386_displaced_step_fixup

B:
PC == 2, orig_eax == __NR_fork, GDB in i386_displaced_step_copy_insn
PTRACE_SINGLESTEP, waitpid == SIGTRAP
PC == 2, orig_eax == -1, GDB in i386_displaced_step_fixup

C:
PC == 2, orig_eax == -1, initiate standard step over 'ret'
-> PC == return address

The extra flag in struct displaced_step_closure would remember in the B case
for i386_displaced_step_fixup that this step is special due to %orig_rax.
Without special i386_displaced_step_copy_insn the function
i386_displaced_step_fixup in the B case can no longer find anything unusual,
it can no longer see 'int $0x80' anywhere and %orig_eax is also normal.
Only PC did not change but that may happen for various regular instructions.


> > +    {
> > +      /* Since we use simple_displaced_step_copy_insn, our closure is a
> > +	 copy of the instruction.  */
> > +      gdb_byte *insn = (gdb_byte *) closure;
> > +
> > +      /* Fake nop.  */
> > +      insn[0] = 0x90;
> > +    }
> > +
> > +  return closure;
> > +}
> 
> I am afraid this fix is not correct.

It works, it is a hack, I find your patch another kind of hack.


> So, the proper fix would be defining i386_linux_displaced_step_fixup in
> i386-linux-tdep.c to check whether pc is changed (equal to the address
> of scratchpad).  If pc is not changed, this means pc has been updated in
> previous syscall event, and restore pc to its original place.
> Otherwise, call i386_displaced_step_fixup.   The attached patch is to do
> what I said here.  WDYT?

I do not mind much but it makes some assumption if PC did not change it was by
a syscall without checking it really was a syscall at all.  There could be for
example some "jmp *%ebx" with %ebx == _start and it would be falsely relocated
by your patch back to its code location, ignoring its intended jump.  The
patch of mine would not relocate it as %orig_eax remained 0.

But any code messing with the entry point address may confuse this
autodetection anyway so these countercases are more hypothetical.

What do you think about the %orig_eax verification?

I do not mind in general, displaced stepping is not used by default yet.
I just had some kernel ptrace bug suspection and the FAIL was unstable while
regression testing across archs.


Regards,
Jan