[Crash-utility] [PATCH v2] crash: dis: introduce count in reverse and forward mode

[Dave Anderson]

Hi Aaron,

I didn't mean to scare you off, but at the same time I think you
understand how strongly I prefer to avoid screwing around with the
gdb code unless it's absolutely necessary.  (Not to mention that 
the dual-patching of printcmd.c by both gdb-7.6.patch and the
gdb-7.6-ppc64le-support.patch, may have made it unworkable.)

So I took my own advice, and implemented the "dis -[rf] address count"
capability with a simple patch to the top-level kernel.c disassembly
code:

  https://github.com/crash-utility/crash/commit/6c891b667f75543db279e1a29bf3d646a83d175d

Also, inspired by your reference to "Add negative repeat count to 'x' command" 
gdb patch, I added a new "rd -R" option last week:

  https://github.com/crash-utility/crash/commit/72654f8c62fb1277939a21daa92bc8b1af70ed71

Thanks,
  Dave




----- Original Message -----
> ----- Original Message -----
> > On Fri 2019-06-21 11:45 -0400, Dave Anderson wrote:
> > > 
> > > 
> > > ----- Original Message -----
> > > > Changes since v1:
> > > > 
> > > >  - Update 'dis' help page
> > > >  - Resolve patch fuzz
> > > > 
> > > > 
> > > > The purpose of this patch is to add support for a count value in reverse
> > > > or forward mode, during disassembly:
> > > > 
> > > > 	'dis [-r|-f] [symbol|address] [count]'
> > > > 
> > > > For example:
> > > > 
> > > >   crash> dis -f list_del+0x16 4
> > > >   0xffffffff812b3346 <list_del+22>:       jne    0xffffffff812b3381
> > > >   <list_del+81>
> > > >   0xffffffff812b3348 <list_del+24>:       mov    (%rbx),%rax
> > > >   0xffffffff812b334b <list_del+27>:       mov    0x8(%rax),%r8
> > > >   0xffffffff812b334f <list_del+31>:       cmp    %r8,%rbx
> > > > 
> > > >   crash> dis -r list_del+0x16 4
> > > >   0xffffffff812b333c <list_del+12>:       mov    0x8(%rdi),%rax
> > > >   0xffffffff812b3340 <list_del+16>:       mov    (%rax),%r8
> > > >   0xffffffff812b3343 <list_del+19>:       cmp    %r8,%rdi
> > > >   0xffffffff812b3346 <list_del+22>:       jne    0xffffffff812b3381
> > > >   <list_del+81>
> > [ ... ]
> > 
> > Hi Dave,
> > 
> > > To be honest, it's not clear how that can be addressed, because any patches
> > > to printcmd.c must be applicable to the non-ppc64le and the ppc64le
> > > versions
> > > of the file.
> > 
> > Unfortunately, I believe this is the underlying issue.
> > I did not compile test under ppc64le.
> > 
> > > So let's take a step back, and consider the risk/reward of this quite intrusive
> > > patchset to such a crucial gdb file.
> > 
> > In my opinion and if I understand correctly, the original GDB commit
> > bb556f1facb ("Add negative repeat count to 'x' command") is rather
> > self-contained and should not cause an issue. I was particularly careful in
> > this regard. Moving forward, I could strip out all but absolutely necessary
> > changes from the original GDB commit, to support this feature.
> > 
> > > First, "dis -f <address> <count>" is pretty much meaningless.  In order
> > > to accomplish that, just don't use the "-f" argument!
> > 
> > I agree - this is superficial; count in forward mode was added since
> > reverse mode also accepts a count.
> > 
> > > Secondly, for "dis -r <address> count", you could simply run "dis -r
> > > <address> | tail -4".
> > > For that matter, is it really all that onerous to see the fully
> > > disassembled function
> > > up to the target address?
> > 
> > Personally, I would prefer to avoid using an external command in this
> > particular case. Especially since the GDB command examine supports a
> > positive or negative (i.e. in GDB release 7.12 and above) count.
> > 
> > > And again, even if you wanted to support it, it also seems like it could be accomplished
> > > within the cmd_dis() function, given that the full output is pre-gathered into a tmpfile
> > > prior to being displayed.
> > 
> > We can indeed but it would be easier to utilise GDB for this as explained
> > above.
> >
> > If you don't mind another iteration, I'll attempt to address the above
> > build issues. Please let me know your thoughts.
> 
> I can't stop you from trying, but I have to say that you're not calming 
> my nerves, because:
>   
>   (1) I don't care whether gdb-7.12 has that capability.
>   (2) If it is at all possible to refrain from tinkering with
>       the temperamental beast gdb, I would always rather do so.
> 
> In the interest of simplicity and maintainability, it just seems that 
> here in cmd_dis():
> 
>    2019                 gdb_pass_through(buf5, NULL, GNU_RETURN_ON_ERROR);
>    2020 
>    2021                 if (req->flags & GNU_COMMAND_FAILED) {
>    2022                         close_tmpfile();
>    2023                         error(FATAL, dis_err, req->addr);
>    2024                 }
>    2025 
>    2026                 rewind(pc->tmpfile);
>    2027                 while (fgets(buf2, BUFSIZE, pc->tmpfile)) {
> 
> at line 2025, you could do something like:
> 
>                         if (count_entered and reverse)
>                                 set_reverse_offset(req->count);
>                         else
>                                 rewind(pc->tmpfile)
> 
> Your choice...
> 
> Dave
> 
> 
> > 
> > 
> > Kind regards,
> > 
> > --
> > Aaron Tomlin
> >