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

----- 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 >