Re: [PATCH] Negative repeat count for 'x' command

Toshihito Kikuchi
2016-04-15T00:30:22Z

Hi Pedro,

We know we all are busy with our main work :).

I walked through all of your comments. Thank you for the thorough review.
Especially your suggestion for find_pc_sect_line gdb_insn_length is really
helpful. These functions are what I was looking for.

Well, I was not a patient person, so I've started a V2 thread two weeks ago.
(https://sourceware.org/ml/gdb-patches/2016-04/msg00023.html)
The difference between V1 and V2 was only NEWS file. Please ignore it. Once I
address all of the comments, I'll start a V3 thread.

Let me answer a couple of non-trivial questions. Otherwise fixes will be
included in V3.

>> +If a negative repeat count is specified for the formats @samp{s} or @samp{i},
>> +the absolute value of the given number of null-terminated strings or
>> +instructions before the address are displayed. For the @samp{i} format,
>> +we use line number information in the debug info to resolve a correct frame
>> +while dissasembling backward.
>
> What does "a correct frame" mean?

It seems that I was using a wrong term. Instead of 'frame', does 'procedure
boundary' make sense?

> Typo "previoius". Double-space after periods. This recursion sounds
> nasty -- sounds like if you request a high number of addresses, crossing
> many lines, we'll hit stack limits. Can we do without recursion?

Yeah, actually this was a painful part. Let me rethink a better algorithm
without using recursive calls (and VEC if possible).

>> + }
>> +
>> /* Print as many objects as specified in COUNT, at most maxelts per line,
>> with the address of the next one at the start of each line. */
>>
>> @@ -913,6 +1164,9 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>> printf_filtered ("\n");
>> gdb_flush (gdb_stdout);
>> }
>> +
>> + if (addr_rewound)
>
> addr_rewound != 0
>
> But actually, please make sure that "address == 0" is treated as a
> valid address (not just here, but in the whole patch), as it is valid
> on non-MMU machines. That may suggest using a separate boolean variable
> to represent "have address".

Thank you for the insightful comment! I never thought non-MMU architectures..

>> +
>> + const char TestStrings[] = {
>> + "ABCD"
>> + "EFGHIJKLMNOPQRSTUVWXYZ\0"
>> + "\0"
>> + "\0"
>> + "\u307B\u3052\u307B\u3052\0"
>> + "012345678901234567890123456789\0"
>> + "!!!!!!\0"
>> + };
>> +*/
>> +
>> +const char TestStrings[] = {
>> + 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48,
>
> I wonder whether 'A', 'B', etc. would work?

These test strings contain non-alphabetical characters shown as
"\u307B\u3052\u307B\u3052\0" above, and that syntax is not allowed without C99.
Also,
I want to keep the same style for 1-byte, 2-bytes, and 4-bytes test strings.
That's why I didn't use a character literals here. Do you have any thoughts?

Thanks,
Toshihito