Re: [try 2nd 5/8] Displaced stepping for Thumb 32-bit insns

["Ulrich Weigand" <uweigand@de.ibm.com>]

Yao Qi wrote:

> Yes, I run gdb testsuite with can_use_displaced_stepping set to 
> can_use_displaced_stepping_on, and it does expose more problems in 
> current patches.

OK, thanks for verifying!

> Three patches attached here to address these problems 
> found so far.  I don't combine them into one patch, because they belongs 
> to different groups (thumb 16bit, thumb 32bit).

The three patches look good to me.

> After applied these three patches, there are still some failures, which 
> are caused by some reasons, so these three patches here can be regarded 
> as WIP patches.
> 
>    1.  Failures in gdb.arch/thumb2-it.exp and gdb.base/gdb1555.exp. 
> These failures are caused by missing IT support in thumb displaced stepping.

Ah, right.  Fortunately, I think IT support should be relatively easy to
add, in fact we should be able to just completely emulate it:

- The first thing we do when we're about to displaced-step a Thumb insn
  is to check the itstate and see whether we're in an IT block.

- If so, we check whether the condition is true, given the current state
  of the flags.

- If the condition is false, we always use a NOP as the displaced
  instruction; otherwise, compute the displaced instruction as usual.

- In either case, set the CSPR register as if we're outside of any
  IT block while actually executing the displaced instruction.  (This
  also makes sure that the breakpoint at the end will always be
  executed.)

- During fixup after execution is done, re-set IT state in the CSPR
  to the proper value (advanced by one instruction).

See also thumb_get_next_pc_raw for how to manipulate IT state ...

Does this look good to you?

>    2.  Failures in gdb.base/break-interp.exp and gdb.base/nostdlib.exp. 
>   They are appeared on i686-pc-linux-gnu as well.
> 
>    3.  Failures (timeout) in gdb.base/sigstep.exp.  IIUC, it is 
> incorrect to displaced step instructions in signal handler, so failures 
> are expected.
> 
>    4.  Failures in gdb.base/watch-vfork.exp.  Displaced stepping is not 
> completed due to a VFORK event.  Current displaced stepping 
> infrastructure or infrun logic doesn't consider the case that executing 
> instruction in scratch can be "interrupted".  When displaced stepping an 
> vfork syscall, VFORK event comes out earlier than TRAP event.  GDB will 
> be confused.
> 
>    5. Timeout failures in gdb.threads/*.exp.  Similarly to #4, when 
> execution instructions in scratch, thread context switch may happen, and 
> GDB will be confused as well.  #4 and #5 are not arm-specific problem.

So these are apparently all common-code problems.  While those ought to
be fixed, of course, IMO they should not prevent the ARM support patches
from going forward at this point ...

>    6. Failures in gdb.base/watchpoint-solib.exp gdb.mi/mi-simplerun.exp. 
>   They are caused by displaced stepping instruction `mov r12, #imm`. 
> This instruction should be unmodified-copied to scratch, and execute, 
> but experiment shows we can't.  I have a local patch that can control 
> displaced stepping on instructions' level.  Once I turn it on for `mov 
> r12, #imm`, these tests will fail.  The reason is still unknown to me.
> 
>    7. Accessing some high addresses.  Some instructions (alu_imm) may 
> set PC to a hight address, such as 0xffffxxxx, and displaced stepping of 
> this kind instruction should be handled differently.

I'm afraid I don't quite understand those last two points.  Could you
elaborate what exactly is going wrong?


> If my analysis above makes sense and is correct, we still have to fix #1 
> at least, to make displaced stepping really works.  On the other hand, 
> if current patches can be approved, I am happy as well, and can carry 
> less local patches to move on. :)

Agreed, I think we should fix IT support before the patches go in.


> 	gdb/
> 	* arm-tdep.c (thumb_copy_b): Extract correct offset.
> 	(thumb_copy_16bit_ldr_literal): Extract correct value for rt and imm8.
> 	Set pc 4-byte aligned.
> 	Set branch dest address correctly.

The last line refers to thumb_copy_cbnz_cbz, I think.

> +    fprintf_unfiltered (gdb_stdlog,
> +			"displaced: copying thumb ldr r%d [pc #%d]\n"
> +			, rt, imm8);

Comma at the end of the previous line, please.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com