Re: [PATCH 2/2] Add options to skip unavailable locals

[Pedro Alves <palves@redhat.com>]

On 07/22/2013 02:37 AM, Yao Qi wrote:

> 2013-07-22  Pedro Alves  <pedro@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
> 
> 	* mi/mi-cmd-stack.c (list_args_or_locals): Adjust prototype.
> 	(parse_no_frames_option): Remove.
> 	(mi_cmd_stack_list_locals): Handle --skip-unavailable.
> 	(mi_cmd_stack_list_args): Adjust.
> 	(mi_cmd_stack_list_variables): Handle --skip-unavailable.
> 	(list_args_or_locals): New parameter 'skip_unavailable'.
> 	Handle it.
> 	* valprint.c (scalar_type_p): Rename to ...
> 	(val_print_scalar_type_p): ... this.  Make extern.
> 	(val_print, value_check_printable): Adjust.
> 	* valprint.h (val_print_scalar_type_p): Declare.
> 	* value.c (value_entirely_unavailable): New function.
> 	* value.h (value_entirely_unavailable): Declare.
> 
> 	* NEWS: Mention the new option "--skip-unavailable" to these
> 	MI commands.

Hmm, this "these" reads out of nowhere.  "Which these?"

> 
> gdb/doc:
> 
> 2013-07-22  Pedro Alves  <pedro@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.texinfo (GDB/MI Stack Manipulation)<-stack-list-locals>:

Space before "<".

> 	Document new --skip-unavailable option.
> 	<-stack-list-variables>: Document new --skip-unavailable option.
> 

> +If the @code{--skip-unavailable} option is specified, local variables
> +that are not available are not listed.  Partially available locals
> +variables are still displayed, however.

s/available locals variables/available local variables/


> +If the @code{--skip-unavailable} option is specified, local variables
> +and arguments that are not collected are not listed.

This still says "that are not collected".  I think it should say
"that that are not available", like the other bits (and similarly
to the adjustment done in other patches, IIRC).

> @@ -295,18 +293,39 @@ mi_cmd_stack_list_args (char *command, char **argv, int argc)
>    struct ui_out *uiout = current_uiout;
>    int raw_arg = 0;
>    enum py_bt_status result = PY_BT_ERROR;
> +  int skip_unavailable = 0;
> +  int oind = 0;
>  
> -  if (argc > 0)
> -    raw_arg = parse_no_frames_option (argv[0]);
> +  /* We can't use mi_getopt here, because the number of options is not
> +     determined.  */

Hmm.  Isn't that easy to fix though?  We'd just need an mi_getopt
variant that doesn't error out when it sees an unknown option, but
instead returns that option's position (similarly to getopt).  It's
then the caller's responsibility to parse the rest of the option
string.

> +  for (oind = 0; oind < argc; oind++)
> +    {
> +      int found = 0;
>  
> -  if (argc < 1 || (argc > 3 && ! raw_arg) || (argc == 2 && ! raw_arg))
> +      if (strcmp (argv[oind], "--no-frame-filters") == 0)
> +	{
> +	  raw_arg = oind + 1;
> +	  found = 1;
> +	}
> +      else if (strcmp (argv[oind], "--skip-unavailable") == 0)
> +	{
> +	  skip_unavailable = 1;
> +	  found = 1;
> +	}
> +
> +      if (!found)
> +	break;
> +    }
> +
> +  if (argc - oind != 1 && argc - oind != 3)
>      error (_("-stack-list-arguments: Usage: " \
> -	     "[--no-frame-filters] PRINT_VALUES [FRAME_LOW FRAME_HIGH]"));
> +	     "[--no-frame-filters] [--skip-unavailable] "
> +	     "PRINT_VALUES [FRAME_LOW FRAME_HIGH]"));
>  
> -  if (argc >= 3)
> +  if (argc - oind == 3)
>      {
> -      frame_low = atoi (argv[1 + raw_arg]);
> -      frame_high = atoi (argv[2 + raw_arg]);
> +      frame_low = atoi (argv[1 + oind]);
> +      frame_high = atoi (argv[2 + oind]);
>      }
>    else
>      {
> @@ -316,7 +335,7 @@ mi_cmd_stack_list_args (char *command, char **argv, int argc)
>        frame_high = -1;
>      }
>  


> @@ -597,7 +626,6 @@ list_args_or_locals (enum what_to_list what, enum print_values values,
>  	  if (print_me)
>  	    {
>  	      struct symbol *sym2;
> -	      struct frame_arg arg, entryarg;
>  
>  	      if (SYMBOL_IS_ARGUMENT (sym))
>  		sym2 = lookup_symbol (SYMBOL_LINKAGE_NAME (sym),
> @@ -607,33 +635,56 @@ list_args_or_locals (enum what_to_list what, enum print_values values,
>  		sym2 = sym;
>  	      gdb_assert (sym2 != NULL);
>  
> -	      memset (&arg, 0, sizeof (arg));
> -	      arg.sym = sym2;
> -	      arg.entry_kind = print_entry_values_no;
> -	      memset (&entryarg, 0, sizeof (entryarg));
> -	      entryarg.sym = sym2;
> -	      entryarg.entry_kind = print_entry_values_no;
> -
> -	      switch (values)
> +	      /* Need to read the value before being able to determine
> +		 whether its unavailable.  */
> +	      if (values == PRINT_ALL_VALUES
> +		  || values == PRINT_SIMPLE_VALUES
> +		  || skip_unavailable)
> +		val = read_var_value (sym2, fi);
> +
> +	      if (skip_unavailable
> +		  && (value_entirely_unavailable (val)
> +		      /* A scalar object that does not have all bits
> +			 available is also considered unavailable,
> +			 because all bits contribute to its
> +			 representation.  */
> +		      || (val_print_scalar_type_p (value_type (val))
> +			  && !value_bytes_available (val,
> +						     value_embedded_offset (val),
> +						     TYPE_LENGTH (value_type (val))))))
> +		;
> +	      else

I don't think this has been updated right for entry values.  With
entry values, we now have _two_ values to account for.  I think
we need to do this once for each of the regular arg and the
entry arg?

>  		{
> -		case PRINT_SIMPLE_VALUES:
> -		  type = check_typedef (sym2->type);
> -		  if (TYPE_CODE (type) != TYPE_CODE_ARRAY
> -		      && TYPE_CODE (type) != TYPE_CODE_STRUCT
> -		      && TYPE_CODE (type) != TYPE_CODE_UNION)
> +		  struct frame_arg arg, entryarg;
> +
> +		  memset (&arg, 0, sizeof (arg));
> +		  arg.sym = sym2;
> +		  arg.entry_kind = print_entry_values_no;
> +		  memset (&entryarg, 0, sizeof (entryarg));
> +		  entryarg.sym = sym2;
> +		  entryarg.entry_kind = print_entry_values_no;
> +
> +		  switch (values)
>  		    {
> -		case PRINT_ALL_VALUES:
> -		      read_frame_arg (sym2, fi, &arg, &entryarg);
> +		    case PRINT_SIMPLE_VALUES:
> +		      type = check_typedef (sym2->type);
> +		      if (TYPE_CODE (type) != TYPE_CODE_ARRAY
> +			  && TYPE_CODE (type) != TYPE_CODE_STRUCT
> +			  && TYPE_CODE (type) != TYPE_CODE_UNION)
> +			{
> +			case PRINT_ALL_VALUES:
> +			  read_frame_arg (sym2, fi, &arg, &entryarg);
> +			}
> +		      break;
>  		    }
> -		  break;
> -		}
>  
> -	      if (arg.entry_kind != print_entry_values_only)
> -		list_arg_or_local (&arg, what, values);
> -	      if (entryarg.entry_kind != print_entry_values_no)
> -		list_arg_or_local (&entryarg, what, values);
> -	      xfree (arg.error);
> -	      xfree (entryarg.error);
> +		  if (arg.entry_kind != print_entry_values_only)
> +		    list_arg_or_local (&arg, what, values);
> +		  if (entryarg.entry_kind != print_entry_values_no)
> +		    list_arg_or_local (&entryarg, what, values);
> +		  xfree (arg.error);
> +		  xfree (entryarg.error);
> +		}
>  	    }
>  	}
>  

-- 
Pedro Alves