[PATCH] Make -e optional for -data-disassemble

[Brett Lee <brett dot lee dot nps at gmail dot com>]

Sorry, I forgot to add the [PATCH] prefix in the subject.

Sincerely,
Brett Lee

---------- Forwarded message ----------
From: Brett Lee <brett.lee.nps@gmail.com>
Date: Sat, Mar 17, 2012 at 11:10 AM
Subject: Re: Make -e optional for -data-disassemble
To: gdb-patches@sourceware.org


Here are the updated patches against the git repo and the updated ChangeLog.

Thanks,
Brett Lee

On Mon, Mar 12, 2012 at 8:51 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Hello,
>
>> The attached files make the end address (-e option)?optional for the
>> MI command -data-disassemble. ?This is backward compatible because all
>> existing code that?calls -data-disassemble passes both -s and -e.
>
> Normally, changes to the GDB/MI protocol are reviewed by our GDB/MI
> Maintainer, but I thought I'd give a little feedback...
>
>> 2012-03-09 Brett Lee <brett.lee.nps@gmail.com>
>>
>> ? ? ? * mi-cmd-disas.c: Allow -s option without -e option. ?Automatically
>> ? ? ? stops at the end of the function containing the start address or
>> ? ? ? displays an error if the address is not inside any function.
>
> One little nit: You forgot the name of the function in the ChangeLog
> entry. And would you mind formatting it to 70 characters as well?
>
> I see that you have tested your change against the testsuite, which
> is nice. And you even added a test for the new feature, which is
> even nicer. ?But you made the change in a file that tests version 2
> of the protocol, and should not be changed. Please update
> mi-disassemble.exp instead.
>
> You also forgot to provide ChangeLog entries for the manual and
> testsuite changes.
>
> Something important: It looks like the patch was written against
> GDB 7.3, which is a bit old, at this stage. The patch may still
> apply, but it is important that you send patches made against the
> HEAD of our tree. Here is a link to get access to our tree (you can
> use CVS, or our git mirror).
>
> Overall, I have no problem with the new interface. The code does look
> good to me, but I'd like to make a suggestion (please, wait for feedback
> from the other maintainers before implementing anything, you neve know):
>
>> + ?if (start_seen && !end_seen && !file_seen && !line_seen && !num_seen)
>> + ? ?{
>> + ? ? ?if (!find_pc_partial_function (low, NULL, NULL, &high))
>> + ? ? error (_("-data-disassemble: "
>> + ? ? ? ? ? ? ?"No function contains specified address"));
>
> Perhaps you could search the minimal symbols too? The above searches
> the functions for which debugging information is available. But if
> you want to disassemble a glibc function, for instance, debug info
> is likely not going to be available.
>
>> ? ?if (!((line_seen && file_seen && num_seen && !start_seen && !end_seen)
>> ? ? ? || (line_seen && file_seen && !num_seen && !start_seen && !end_seen)
>> - ? ? || (!line_seen && !file_seen && !num_seen && start_seen && end_seen)))
>> - ? ?error (_("-data-disassemble: Usage: ( [-f filename -l linenum [-n "
>> - ? ? ? ? ?"howmany]] | [-s startaddr -e endaddr]) [--] mode."));
>> -
>> - ?if (argc != 1)
>> + ? ? || (!line_seen && !file_seen && !num_seen && start_seen && end_seen))
>> + ? ? ?|| argc != 1)
>> ? ? ?error (_("-data-disassemble: Usage: [-f filename -l linenum "
>> - ? ? ? ? ?"[-n howmany]] [-s startaddr -e endaddr] [--] mode."));
>> + ? ? ? ? ?"[-n howmany]] [-s startaddr [-e endaddr]] [--] mode."));
>
> I like the fact that you merged the two if blocks together to avoid
> the duplication of the usage. But, if it was my choice, I would go
> the opposite way, and provide a more explicit message for each
> branch of the condition, so that users know immediately why their
> command was rejected, instead of having to compare their command to
> the usage. So the general usage would disappear.
>
> --
> Joel

Attachment: code.txt
Description: Text document

Attachment: doc.txt
Description: Text document

Attachment: test.txt
Description: Text document

Attachment: ChangeLog.txt
Description: Text document