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

[Toshihito Kikuchi <k dot toshihito at yahoo dot de>]

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