This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Make -e optional for -data-disassemble


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]