Re: [PATCH 5/6] Add a new gdbarch method to print a single AUXV entry.

[Pedro Alves <palves at redhat dot com>]

On 06/16/2016 07:02 AM, John Baldwin wrote:
> Different platforms have different meanings for auxiliary vector
> entries.  The 'print_auxv' gdbarch method allows an architecture
> to output a suitable description for platform-specific entries.
> 
> A fprint_single_auxv function is split out of fprint_target_auxv.
> This function outputs the description of a single auxiliary vector
> entry to the specified file using caller-supplied formatting and
> strings to describe the vector type.
> 
> The existing switch on auxiliary vector types is moved out of
> fprint_target_auxv into a new default_print_auxv function.
> default_print_auxv chooses an appropriate format and description
> and calls fprint_single_auxv to describe a single vector entry.
> 
> fprint_target_auxv now invokes the gdbarch 'print_auxv' function
> on each vector entry.  If the function is not present or returns
> zero, default_printf_auxv is called to output a description for
> the vector.

I like the idea.  Though, I think we can simplify this.  How about:

- make default_print_auxv be the default gdbarch_print_auxv
  implementation, in gdbarch.sh.   

- make fprint_target_auxv calls gdbarch_print_auxv unconditionally.

- remove the support for gdbarch_print_auxv returning 0.  Instead,
  implementations that want to defer to default_print_auxv simply
  call it directly.


Also, I think it'd be a bit less confusing to rename things like this:

  gdbarch_print_auxv -> gdbarch_print_auxv_entry
  default_print_auxv -> default_print_auxv_entry
  fprint_single_auxv -> fprint_auxv_entry

This way methods that print a single entry are consistently named,
and not so easily confused with methods that print the whole table,
like fprint_target_auxv.

>  
>  
> +/* Print a description of a single AUXV entry on the specified file.  */
> +void

Empty line between comment and function.

> +fprint_single_auxv (struct ui_file *file, const char *name,
> +		    const char *description, enum auxv_format flavor,
> +		    CORE_ADDR type, CORE_ADDR val)
> +{
> +  fprintf_filtered (file, "%-4s %-20s %-30s ",
> +		    plongest (type), name, description);

If not translatable, wrap string in () [not _()], in order to
avoid ARI complaints.

> +  switch (flavor)
> +    {
> +    case dec:
> +      fprintf_filtered (file, "%s\n", plongest (val));
> +      break;
> +    case hex:
> +      fprintf_filtered (file, "%s\n", paddress (target_gdbarch (), val));
> +      break;
> +    case str:
> +      {
> +	struct value_print_options opts;
> +
> +	get_user_print_options (&opts);
> +	if (opts.addressprint)
> +	  fprintf_filtered (file, "%s ", paddress (target_gdbarch (), val));
> +	val_print_string (builtin_type (target_gdbarch ())->builtin_char,
> +			  NULL, val, -1, file, &opts);
> +	fprintf_filtered (file, "\n");
> +      }

Likewise the other format strings.


> +
> +static void
> +default_print_auxv (struct ui_file *file, CORE_ADDR type, CORE_ADDR val)
> +{

Intro comment.   Something like "default implementation of ...".


> +		    
>  /* Print the contents of the target's AUXV on the specified file.  */
>  int
>  fprint_target_auxv (struct ui_file *file, struct target_ops *ops)
>  {
> +  struct gdbarch *gdbarch = target_gdbarch();

Space before open parenthesis.

> diff --git a/gdb/auxv.h b/gdb/auxv.h
> index 9efe604..91d94f9 100644
> --- a/gdb/auxv.h
> +++ b/gdb/auxv.h
> @@ -46,6 +46,14 @@ extern int target_auxv_parse (struct target_ops *ops,
>  extern int target_auxv_search (struct target_ops *ops,
>  			       CORE_ADDR match, CORE_ADDR *valp);
>  
> +/* Print a description of a single AUXV entry on the specified file.  */
> +enum auxv_format { dec, hex, str };

These very generic names used to be OK, because they were
defined inside the function.  But now that this is in a header,
the potential for conflict is much higher.  hex could conflict
with std::hex, for instance.  Please rename the values to
for example: AUXV_FORMAT_DEC / AUXV_FORMAT_HEX / AUXV_FORMAT_STR.
We should add a comment for what each value means, too, since
we lost the pass of obvious-because-of-locality.

> +
> +extern void fprint_single_auxv (struct ui_file *file, const char *name,
> +				const char *description,
> +				enum auxv_format flavor, CORE_ADDR type,

I'd find it more consistent to call parameter "format" instead of "flavor",
which then avoids people looking for enum auxv_flavor.

> +# Print the description of a single auxv entry described by TYPE and VAL
> +# to FILE.  Return 0 and output no description if the TYPE is unknown.
> +# Return 0 if the TYPE of the auxv entry is unknown.
> +# Return 1 if a description was output.
> +M:int:print_auxv:struct ui_file *file, CORE_ADDR type, CORE_ADDR val:file, type, val

Thanks,
Pedro Alves