[Crash-utility] [PATCH] Fix for "dis" command to correctly display the offset of disassembly code

[lijiang]

On Wed, Feb 22, 2023 at 1:17 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
wrote:

> On 2023/02/21 12:03, Lianbo Jiang wrote:
> > For gdb-10.2, the disassembly code may start with "=>", which needs to
> > be stripped when calculating the address. Otherwise, parsing the address
> > will fail because the current code always assumes that it starts with the
> > "0x". For example:
> >
> >    crash> gdb disassemble 0xffffffffa2317add
> >    Dump of assembler code for function native_queued_spin_lock_slowpath:
> >       0xffffffffa2317ab0 <+0>:     nopl   0x0(%rax,%rax,1)
> >       0xffffffffa2317ab5 <+5>:     push   %rbp
> >       0xffffffffa2317ab6 <+6>:     mov    %rsp,%rbp
> >       ...
> >       0xffffffffa2317ad3 <+35>:    mov    %edx,%eax
> >       0xffffffffa2317ad5 <+37>:    lock cmpxchg %ecx,(%rdi)
> >    => 0xffffffffa2317ad9 <+41>:    cmp    %eax,%edx
> >       0xffffffffa2317adb <+43>:    jne    0xffffffffa2317ac0
> <native_queued_spin_lock_slowpath+16>
> >       0xffffffffa2317add <+45>:    pop    %rbp
> >       0xffffffffa2317ade <+46>:    xchg   %ax,%ax
> >       ...
> Probably the following prints the "=> ".
> Out of curiosity, what did you do before the gdb disas?
>
>
I didn't run any crash commands or gdb commands before the gdb disass, the
current command(gdb disassemble 0xffffffffa2317add) is the first one.

The crash-7(gdb-7.6) doesn't have this issue. The "=>" should be the
current PC indicator.

The pc_prefix() was added by following commit:
2b28d209243f ("2009-10-21  Paul Pluzhnikov  <ppluzhnikov at google.com>")

modified by this one:
ce406537179a (" * infcmd.c (post_create_inferior): Ignore
NOT_AVAILABLE_ERROR   errors when reading the `stop_pc'.      * printcmd.c
(pc_prefix): Use get_frame_pc_if_available instead of        get_frame_pc.")

Thanks.
Lianbo

/* Return a prefix for instruction address:
>     "=> " for current instruction, else "   ".  */
>
> const char *
> pc_prefix (CORE_ADDR addr)
> {
>    if (has_stack_frames ())
>      {
>        struct frame_info *frame;
>        CORE_ADDR pc;
>
>        frame = get_selected_frame (NULL);
>        if (get_frame_pc_if_available (frame, &pc) && pc == addr)
>          return "=> ";
>      }
>    return "   ";
> }
>
> Anyway, the patch looks good to me.
>
> Acked-by: Kazuhito Hagio <k-hagio-ab at nec.com>
>
> Thanks,
> Kazu
>
> >
> > Without the patch:
> >    crash> dis 0xffffffffa2317add -r | tail -5
> >    0xffffffffa2317ad3 <native_queued_spin_lock_slowpath+35>:  mov
> %edx,%eax
> >    0xffffffffa2317ad5 <native_queued_spin_lock_slowpath+37>:  lock
> cmpxchg %ecx,(%rdi)
> >    0xffffffffa2317ad5 <native_queued_spin_lock_slowpath+37>:  cmp
> %eax,%edx
> >                                                         ^^^
> >    0xffffffffa2317adb <native_queued_spin_lock_slowpath+43>:  jne
> 0xffffffffa2317ac0 <native_queued_spin_lock_slowpath+16>
> >    0xffffffffa2317add <native_queued_spin_lock_slowpath+45>:  pop    %rbp
> >
> > With the patch:
> >    crash> dis 0xffffffffa2317add -r | tail -5
> >    0xffffffffa2317ad3 <native_queued_spin_lock_slowpath+35>:  mov
> %edx,%eax
> >    0xffffffffa2317ad5 <native_queued_spin_lock_slowpath+37>:  lock
> cmpxchg %ecx,(%rdi)
> >    0xffffffffa2317ad9 <native_queued_spin_lock_slowpath+41>:  cmp
> %eax,%edx
> >    0xffffffffa2317adb <native_queued_spin_lock_slowpath+43>:  jne
> 0xffffffffa2317ac0 <native_queued_spin_lock_slowpath+16>
> >    0xffffffffa2317add <native_queued_spin_lock_slowpath+45>:  pop    %rbp
> >
> > Reported-by: Vernon Lovejoy <vlovejoy at redhat.com>
> > Signed-off-by: Lianbo Jiang <lijiang at redhat.com>
> > ---
> >   kernel.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel.c b/kernel.c
> > index a42e6ad7d78c..6e98f5f6f6b1 100644
> > --- a/kernel.c
> > +++ b/kernel.c
> > @@ -2112,6 +2112,10 @@ cmd_dis(void)
> >                       rewind(pc->tmpfile);
> >
> >               while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
> > +
> > +                     if (STRNEQ(buf2, "=>"))
> > +                             shift_string_left(buf2, 2);
> > +
> >                       strip_beginning_whitespace(buf2);
> >
> >                       if (do_load_module_filter)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20230222/8fda21f5/attachment-0001.htm>