Re: [PATCHv3 1/3] gdb: more debug output for displaced stepping

[Simon Marchi <simark@simark.ca>]

On 3/27/23 08:32, Andrew Burgess wrote:
> While investigating a displaced stepping issue I wanted an easy way to
> see what GDB thought the original instruction was, and what
> instruction GDB replaced that with when performing the displaced step.
> 
> We do print out the address that is being stepped, so I can track down
> the original instruction, I just need to go find the information
> myself.
> 
> And we do print out the bytes of the new instruction, so I can figure
> out what the replacement instruction was, but it's not really easy.
> 
> Also, the code that prints the bytes of the replacement instruction
> only prints 4 bytes, which clearly isn't always going to be correct.
> 
> In this commit I remove the existing code that prints the bytes of the
> replacement instruction, and add two new blocks of code to
> displaced_step_prepare_throw.  This new code prints the original
> instruction, and the replacement instruction.  In each case we print
> both the bytes that make up the instruction and the completely
> disassembled instruction.
> 
> Here's an example of what the output looks like on x86-64 (this is
> with 'set debug displaced on').  The two interesting lines contain the
> strings 'original insn' and 'replacement insn':
> 
>   (gdb) step
>   [displaced] displaced_step_prepare_throw: displaced-stepping 2892655.2892655.0 now
>   [displaced] displaced_step_prepare_throw: original insn 0x401030: ff 25 e2 2f 00 00	jmp    *0x2fe2(%rip)        # 0x404018 <puts@got.plt>
>   [displaced] prepare: selected buffer at 0x401052
>   [displaced] prepare: saved 0x401052: 1e fa 31 ed 49 89 d1 5e 48 89 e2 48 83 e4 f0 50
>   [displaced] fixup_riprel: %rip-relative addressing used.
>   [displaced] fixup_riprel: using temp reg 2, old value 0x7ffff7f8a578, new value 0x401036
>   [displaced] amd64_displaced_step_copy_insn: copy 0x401030->0x401052: ff a1 e2 2f 00 00 68 00 00 00 00 e9 e0 ff ff ff
>   [displaced] displaced_step_prepare_throw: prepared successfully thread=2892655.2892655.0, original_pc=0x401030, displaced_pc=0x401052
>   [displaced] displaced_step_prepare_throw: replacement insn 0x401052: ff a1 e2 2f 00 00	jmp    *0x2fe2(%rcx)
>   [displaced] finish: restored 2892655.2892655.0 0x401052
>   [displaced] amd64_displaced_step_fixup: fixup (0x401030, 0x401052), insn = 0xff 0xa1 ...
>   [displaced] amd64_displaced_step_fixup: restoring reg 2 to 0x7ffff7f8a578
>   0x00007ffff7e402c0 in puts () from /lib64/libc.so.6
>   (gdb)
> 
> One final note.  For many targets that support displaced stepping (in
> fact all targets except ARM) the replacement instruction is always a
> single instruction.  But on ARM the replacement could actually be a
> series of instructions.
> 
> The debug code tries to handle this by disassembling the entire
> displaced stepping buffer.  Obviously this might actually print more
> than is necessary, but there's (currently) no easy way to know how
> many instructions to disassemble; that knowledge is all locked in the
> architecture specific code.  Still I don't think it really hurts, if
> someone is looking at this debug then hopefully they known what to
> expect.
> 
> Obviously we can imagine schemes where the architecture specific
> displaced stepping code could communicate back how many bytes its
> replacement sequence was, and then our debug print code could use this
> to limit the disassembly.  But this seems like a lot of effort just to
> save printing a few additional instructions in some debug output.
> 
> I'm not proposing to do anything about this issue for now.
> ---
>  gdb/infrun.c | 85 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 5c9babb9104..8c56a9a4dfb 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -74,6 +74,7 @@
>  #include "gdbsupport/common-debug.h"
>  #include "gdbsupport/buildargv.h"
>  #include "extension.h"
> +#include "disasm.h"
>  
>  /* Prototypes for local functions */
>  
> @@ -1807,6 +1808,31 @@ displaced_step_prepare_throw (thread_info *tp)
>    CORE_ADDR original_pc = regcache_read_pc (regcache);
>    CORE_ADDR displaced_pc;
>  
> +  /* Display the instruction we are going to displaced step.  */
> +  if (debug_displaced)
> +    {
> +      string_file tmp_stream;
> +      int dislen = gdb_print_insn (gdbarch, original_pc, &tmp_stream,
> +				   nullptr);
> +
> +      if (dislen > 0)
> +	{
> +	  gdb::byte_vector insn_buf (dislen);
> +	  read_memory (original_pc, insn_buf.data (), insn_buf.size ());
> +
> +	  std::string insn_bytes
> +	    = displaced_step_dump_bytes (insn_buf.data (), insn_buf.size ());
> +
> +	  displaced_debug_printf ("original insn %s: %s \t %s",
> +				  paddress (gdbarch, original_pc),
> +				  insn_bytes.c_str (),
> +				  tmp_stream.string ().c_str ());

If the bytes disassemble to more than one instruction, does tmp_stream
contain new lines characters?  Just wondering what the output would look
like (not a big deal in any case).

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon