Re: RFC (gdb/mi): -stack-list-locals + REVISED PATCH

[Andrew Cagney <cagney@gnu.org>]

> This patch:
> 
> 1) Follows Jim Ingham's advice of using check_typedef to guard against the
>    case of TYPE_CODE_TYPEDEF.
> 
> 2) Avoids a segmentation fault if -stack-list-locals is invoked before the
>    inferior has started execution.
> 
> 3) Still introduces "-stack-list-locals 2". I'm not sure how to simplify this
>    as any change must presumably be beackward compatible. I don't think using
>    different numbers to mean different things is a problem here as mi commands
>    are not intended for the user and so don't need to be remembered by
>    him/her.

True, but they also need to be fairly self documenting.  Anyway, see below:

>     Nick                                         http://www.nick.uklinux.net
> 
> 
> 
> *** mi-cmd-stack.c.~1.19.~	2003-06-12 23:29:37.000000000 +0100
> --- mi-cmd-stack.c	2003-12-09 02:12:45.000000000 +0000
> ***************
> *** 140,145 ****
> --- 140,148 ----
>     if (argc != 1)
>       error ("mi_cmd_stack_list_locals: Usage: PRINT_VALUES");
>   
> +   if (!deprecated_selected_frame)
> +     error ("No frame selected.");

   struct frame_info *frame;
   ...
   frame = get_selected_frame ();
is better.  It throws an error if there is no frame, and follow on code 
can use "frame" instead of "deprecated_selected_frame" (and would be a 
really appreciated cleanup!).

>     list_args_or_locals (1, atoi (argv[0]), deprecated_selected_frame);

I was thinking of something as simple as:

enum print_values {
   PRINT_NO_VALUES,
   PRINT_ALL_VALUES,
   PRINT_SIMPLE_VALUES
};

and then the very mechanical:

   enum print_values print_values;

   if (strcmp (argv[0], "0") == 0
       || strcmp (argv[0], "no-values") == 0)
     print_values = PRINT_NO_VALUES;
   else if (strcmp (argv[0], "1") == 0
            || strcmp (argv[0], "all-values") == 0)
     print_values = PRINT_ALL_VALUES;
   else if (strcmp (argv[0], "simple-values") == 0)
     print_values = PRINT_SIMPLE_VALUES;
   else
     error ("...");

   list_args_or_locals (1, print_values, ...);

(the names aren't the best so feel free to improve).

>     return MI_CMD_DONE;
>   }
> ***************
> *** 273,288 ****
>   		  make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
>   	      ui_out_field_string (uiout, "name", DEPRECATED_SYMBOL_NAME (sym));
>   
> ! 	      if (values)

if (values != PRINT_NO_VALUES)

> ! 		{
> ! 		  struct symbol *sym2;
> ! 		  if (!locals)
> ! 		    sym2 = lookup_symbol (DEPRECATED_SYMBOL_NAME (sym),
> ! 					  block, VAR_DOMAIN,
> ! 					  (int *) NULL,
> ! 					  (struct symtab **) NULL);
> ! 		  else
>   		    sym2 = sym;
>   		  print_variable_value (sym2, fi, stb->stream);
>   		  ui_out_field_stream (uiout, "value", stb);
>   		  do_cleanups (cleanup_tuple);
> --- 276,304 ----
>   		  make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
>   	      ui_out_field_string (uiout, "name", DEPRECATED_SYMBOL_NAME (sym));
>   
> ! 	      struct symbol *sym2;
> ! 	      if (!locals)
> ! 		sym2 = lookup_symbol (DEPRECATED_SYMBOL_NAME (sym),

Since you're here, I'd change this to SYMBOL_PRINT_NAME.  The comment 
below, from symtab.h, hopefully explains the difference (I also hope I 
picked the correct winner):

/* Now come lots of name accessor macros.  Short version as to when to
    use which: Use SYMBOL_NATURAL_NAME to refer to the name of the
    symbol in the original source code.  Use SYMBOL_LINKAGE_NAME if you
    want to know what the linker thinks the symbol's name is.  Use
    SYMBOL_PRINT_NAME for output.  Use SYMBOL_DEMANGLED_NAME if you
    specifically need to know whether SYMBOL_NATURAL_NAME and
    SYMBOL_LINKAGE_NAME are different.  Don't use
    DEPRECATED_SYMBOL_NAME at all: instances of that macro should be
    replaced by SYMBOL_NATURAL_NAME, SYMBOL_LINKAGE_NAME, or perhaps
    SYMBOL_PRINT_NAME.  */

> ! 				      block, VAR_DOMAIN,
> ! 				      (int *) NULL,
> ! 				      (struct symtab **) NULL);
> ! 	      else
>   		    sym2 = sym;
> + 	      if (values == 2)

if (values == PRINT_SIMPLE_VALUES)

> + 		{
> + 		  type_print (sym2->type, "", stb->stream, -1);
> + 		  ui_out_field_stream (uiout, "type", stb);
> + 		  if (TYPE_CODE (check_typedef (sym2->type)) != TYPE_CODE_ARRAY
> + 		      &&
> + 		      TYPE_CODE (check_typedef (sym2->type)) != TYPE_CODE_STRUCT)

You may also want to consider TYPE_CODE_UNION?  Your choice.

> + 		    {
> + 		      print_variable_value (sym2, fi, stb->stream);
> + 		      ui_out_field_stream (uiout, "value", stb);
> + 		    }
> + 		  do_cleanups (cleanup_tuple);
> + 		}
> + 	      else if (values)

if (values == PRINT_ALL_VALUES)

> + 		{
>   		  print_variable_value (sym2, fi, stb->stream);
>   		  ui_out_field_stream (uiout, "value", stb);
>   		  do_cleanups (cleanup_tuple);
> 

Anyway, it's basicly there.  Just the doco update (which is eli's call) 
(and for the assignment to pop up), and testcase.  For the testcase, 
just edit mi-stack.exp.  You don't need to edit mi[12]-stack.exp as 
there's no expectation that this new feature will work with old MI versions.

(since I'm traveling, I may be slow in responding, hopefully though, 
between my self and elena someone will be able to give a thumbs up and 
organize your account).

Andrew