Re: [RFC] mi: add -a option to the "-data-disassemble" command

[Jan Vrany <jan.vrany@fit.cvut.cz>]

On Mon, 2018-07-30 at 09:04 -0600, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
> 
> Jan> A CLI command "disassemble" allows use to specify a single
> Jan> address - in that case function surrounding that address is
> Jan> disassembled.
> 
> Jan>  	  high = parse_and_eval_address (oarg);
> Jan>  	  end_seen = 1;
> Jan>  	  break;
> Jan> +        case ADDR_OPT:
> Jan> +          addr = parse_and_eval_address (oarg);
> Jan> +          addr_seen = 1;
> Jan> +          break;
> 
> The indentation looked slightly off here.
> It could just be how the patch looked here, but could you double-check
> just in case?

My fault. My editor put 8 spaces instead of tab (\t) at the beginning of
my lines. Will fix. 

> 
> Jan> +  if (!((line_seen && file_seen && num_seen && !start_seen && !end_seen && !addr_seen)
> Jan> +	|| (line_seen && file_seen && !num_seen && !start_seen && !end_seen && !addr_seen)
> Jan> +	|| (!line_seen && !file_seen && !num_seen && start_seen && end_seen && !addr_seen)
> Jan> +	|| (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen && addr_seen)))
> 
> I suppose this ought to check for the case where either -s or -e is
> given along with -a.  That seems like an error.
> 

I'm sorry I'm confused. Let me try to explain. There are (now) four forms:

1a) -f filename -l linenum 
1b) -f filename -l linenum -n lines 
 2) -s start-addr -e end-addr
 3) -a addr

Command  must have one of the above four forms, otherwise it's invalid. Each
"line" in the code above checks one of the above form (in order as written here). 
Note, that there's negation at the very beginning, so the condition holds 
if the command has none of the four forms. Seems "good" to me. Makes sense? 

Perhaps it's easier to see with little reformatting (hope email won't screw it):

  if (!   (( line_seen &&  file_seen &&  num_seen && !start_seen && !end_seen && !addr_seen)
	|| ( line_seen &&  file_seen && !num_seen && !start_seen && !end_seen && !addr_seen)
	|| (!line_seen && !file_seen && !num_seen &&  start_seen &&  end_seen && !addr_seen)
	|| (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen &&  addr_seen)))
    error (_("-data-disassemble: Usage: ( [-f filename -l linenum [-n "
	     "howmany]] | [-s startaddr -e endaddr] | [-a addr] ) [--] mode."));

Now, it appears to me that first two lines can be merged as forms 1a and 1b differ only
in presence of lines. Also, I forgot to update the comment above. So, what about:

   /* Allow only filename + linenum (with how_many which is not
-     required) OR start_addr + end_addr.  */
+     required) OR start_addr + end_addr OR addr  */
 
-  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)))
+  if (!(  ( line_seen &&  file_seen &&              !start_seen && !end_seen && !addr_seen)
+       || (!line_seen && !file_seen && !num_seen &&  start_seen &&  end_seen && !addr_seen)
+       || (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen &&  addr_seen)))

this passes mi-disassemble.exp on my machine. 


> This "if" made me laugh.

:-) I cannot say I was exactly laughing. But followed the crowed anyway.

> 
> Jan> +    mi_gdb_test "112-data-disassemble -a \$pc -- 0" \
> Jan> +	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\}.*\]" \
> Jan> +             "data-disassemble function around pc assembly only"
> Jan> +
> Jan> +    mi_gdb_test "112-data-disassemble -a callee4 -- 0" \
> Jan> +	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\}.*\]" 
> \
> Jan> +             "data-disassemble function callee4 assembly only"
> 
> Probably the second "112" should be "113".  I don't know if you can
> reuse tokens or not, but it seems simple and safe not to.

Sure, will fix and send a patch once we agree on the "if". 

Thanks, Jan