Re: [RFA] Show some tips when file cmd get bfd_error_file_ambiguously_recognized

[Doug Evans <dje@google.com>]

On Wed, Jan 27, 2010 at 1:00 AM, Hui Zhu <teawater@gmail.com> wrote:
> Agree with you.  I make a new patch to add a wrapper to bfd_errmsg.
> When the bfd_check_format_matches get
> bfd_error_file_ambiguously_recognized, it will change the error
> message to:
>
> "/xxx/vmlinux": not in executable format: File format is ambiguous, it
> matches formats: elf64-bigmips elf64-tradbigmips, "set gnutarget
> format-name" handle it

I like the patch, but several nits.
First the wording of the error message needs to be improved.
IWBN to spread this over several lines.  There is precedent for it.
How about

"/xxx/vmlinux": not in executable format: File format is ambiguous.
Matching formats: elf64-bigmips, elf64-tradbigmips.
Use "set gnutarget format-name" to specify the format.

More comments inline below.

> Please help me review it.
>
> Thanks,
> Hui
>
> 2010-01-27  Hui Zhu  <teawater@gmail.com>
>
>        * defs.h (gdb_bfd_errmsg): New extern.
>        * exec.c (exec_file_attach): Change bfd_errmsg to
>        gdb_bfd_errmsg.
>        * utils.c (AMBIGUOUS_MESS1): New macro.
>        (AMBIGUOUS_MESS2): New macro.
>        (gdb_bfd_errmsg): New function.
>
> ---
>  defs.h  |    8 ++++++++
>  exec.c  |    6 ++++--
>  utils.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 2 deletions(-)
>
> --- a/defs.h
> +++ b/defs.h
> @@ -1226,4 +1226,12 @@ void dummy_obstack_deallocate (void *obj
>  extern void initialize_progspace (void);
>  extern void initialize_inferiors (void);
>
> +/* The wrapper for the bfd_errmsg.
> +   When error_tag is bfd_error_file_ambiguously_recognized and matching
> +   is not NULL, this function will return more help message to handle
> +   this issue.  */

I'd add a note saying that matching is set by calls to
bfd_check_format_matches and that this function will free it.
Plus I'd rephrase the rest of the comment.

/* A wrapper for bfd_errmsg to produce a more helpful error message
   in the case of bfd_error_file_ambiguously recognized.
   MATCHING, if non-NULL, is the corresponding argument to
bfd_check_format_matches,
   and will be freed.  */

> +
> +extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching);
> +
> +
>  #endif /* #ifndef DEFS_H */

grep for utils.c in defs.h.  There's a section in defs.h for
prototypes for functions from utils.c

> --- a/exec.c
> +++ b/exec.c
> @@ -219,6 +219,7 @@ exec_file_attach (char *filename, int fr
>       char *scratch_pathname;
>       int scratch_chan;
>       struct target_section *sections = NULL, *sections_end = NULL;
> +      char **matching;
>
>       scratch_chan = openp (getenv ("PATH"), OPF_TRY_CWD_FIRST, filename,
>                   write_files ? O_RDWR | O_BINARY : O_RDONLY | O_BINARY,
> @@ -253,13 +254,14 @@ exec_file_attach (char *filename, int fr
>       scratch_pathname = xstrdup (scratch_pathname);
>       cleanups = make_cleanup (xfree, scratch_pathname);
>
> -      if (!bfd_check_format (exec_bfd, bfd_object))
> +      if (!bfd_check_format_matches (exec_bfd, bfd_object, &matching))
>        {
>          /* Make sure to close exec_bfd, or else "run" might try to use
>             it.  */
>          exec_close ();
>          error (_("\"%s\": not in executable format: %s"),
> -                scratch_pathname, bfd_errmsg (bfd_get_error ()));
> +                scratch_pathname,
> +                gdb_bfd_errmsg (bfd_get_error (), matching));
>        }
>
>       /* FIXME - This should only be run for RS6000, but the ifdef is a poor
> --- a/utils.c
> +++ b/utils.c
> @@ -3608,6 +3608,58 @@ compare_positive_ints (const void *ap, c
>   return * (int *) ap - * (int *) bp;
>  }
>
> +#define AMBIGUOUS_MESS1        ", it matches formats:"
> +#define AMBIGUOUS_MESS2        ", \"set gnutarget format-name\" handle it"
> +
> +const char *
> +gdb_bfd_errmsg (bfd_error_type error_tag, char **matching)
> +{
> +  char *ret;
> +  int ret_len, current_ret_len = 0;
> +  char **p;
> +
> +  /* Check if errmsg just need simple return.  */
> +  if (error_tag != bfd_error_file_ambiguously_recognized || !matching)
> +    return bfd_errmsg (error_tag);

I believe the convention is to write matching == NULL instead of !matching.

> +
> +  ret_len = strlen (bfd_errmsg (error_tag)) + strlen (AMBIGUOUS_MESS1)
> +           + 50 + strlen (AMBIGUOUS_MESS2);
> +  ret = xmalloc (ret_len + 1);

I would compute the complete length needed, rather than watching for
when to call xrealloc.
That means two iterations over matching, but that's ok.

> +
> +  /* bfd_errmsg (error_tag) */

There's no real need for this comment, it doesn't contribute enough
(IMO anyway).
Ditto for similar comments below.

> +  strcpy (ret + current_ret_len, bfd_errmsg (error_tag));
> +  current_ret_len += strlen (bfd_errmsg (error_tag));

I'd write

  strcpy (p, ...);
  p += strlen (p);

instead, but that's just a suggestion (but please make the other
changes requested in this email though).
[Note: There is stpcpy, but we can't use it.]

> +
> +  /* AMBIGUOUS_MESS1 */
> +  strcpy (ret + current_ret_len, AMBIGUOUS_MESS1);
> +  current_ret_len += strlen (AMBIGUOUS_MESS1);
> +
> +  /* matching */
> +  for (p = matching; *p; p++)
> +    {
> +      if (current_ret_len + strlen (*p) + 1 > ret_len)
> +        {
> +          ret_len += 50;
> +          ret = xrealloc (ret, ret_len + 1);
> +        }
> +      sprintf (ret + current_ret_len, " %s", *p);
> +      current_ret_len += strlen (*p) + 1;
> +    }
> +  xfree (matching);
> +
> +  /* AMBIGUOUS_MESS2 */
> +  if (current_ret_len + strlen (AMBIGUOUS_MESS2) > ret_len)
> +    {
> +      ret_len += strlen (AMBIGUOUS_MESS2);
> +      ret = xrealloc (ret, ret_len + 1);
> +    }
> +  strcpy (ret + current_ret_len, AMBIGUOUS_MESS2);
> +
> +  make_cleanup (xfree, ret);
> +
> +  return ret;
> +}
> +
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
>  extern initialize_file_ftype _initialize_utils;
>