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

[Dave Anderson]

----- 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
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>