Re: [RFC] Show (tilde-)expanded filenames to the user? (was: Re: [PATCH] gcore: expand tilde in filename, like in "dump memory" command.)

[Azat Khuzhin <a3at dot mail at gmail dot com>]

On Thu, Aug 8, 2013 at 9:42 PM, Pedro Alves <palves@redhat.com> wrote:
> On 08/07/2013 10:04 PM, Azat Khuzhin wrote:
>> Before this patch next command will fail:
>> (gdb) generate-core-file ~/core
>> Failed to open '~/core' for output.
>>
>> After this patch:
>> (gdb) generate-core-file ~/core
>> Saved corefile ~/core
>
> While reviewing Azat's patch, I wondered whether
> GDB should instead display the expanded filename here,
> like:
>
>  (gdb) gcore ~/core
>  Saved corefile /home/pedro/core
>
> There are many examples of commands that show the
> expanded filename, like e.g.,:
>
>  (gdb) file ~/foo
>  A program is being debugged already.
>  Are you sure you want to change the file? (y or n) y
>  Load new symbol table from "/home/pedro/foo"? (y or n)
>
> or "info files", "info inferiors", and probably a bunch
> of others.
>
> I started adjusting his patch, but then noticed we
> have cases where we have code that shows the non-expanded
> filename to the user, like "save breakpoints", and it
> might have been written that way on purpose:
>
>   pathname = tilde_expand (filename);
>   cleanup = make_cleanup (xfree, pathname);
>   fp = gdb_fopen (pathname, "w");
>   if (!fp)
>     error (_("Unable to open file '%s' for saving (%s)"),
>            filename, safe_strerror (errno));
> ...
>   if (from_tty)
>     printf_filtered (_("Saved to file '%s'.\n"), filename);
>
>
> (gdb) save breakpoints ~/bp
> Saved to file '~/bp'.
>
> So I plan to check his patch in as is first.
>
> But, I think we should have a policy here, and all commands
> should follow it.  Those that don't would be considered
> bugs.  The question then is, which policy is most appropriate?
> grepping around for "tilde_expand", it seems to be the
> showing expanded filenames is more common.  Particularly,
> whenever we stash the filename in some structure (objfiles,
> DSOs, etc.), I don't think ever bother to store the non-expanded
> name, so it looks like the only cases we show non-expanded
> names are in high level command errors and output, (parse
> argument, try to open file).

I agree, the file name must printed in one manner.

>
> The patch below applies on top of Azat's and makes GDB show
> the expanded filename with "gcore" too, in case that's the
> policy to follow.
>
> WDYT?
>
> ------
> gcore: Make tilde-expanded filename visible.
>
> Before:
>
>   (gdb) generate-core-file ~/core
>   Failed to open '~/core' for output.
>
> After:
>
>   (gdb) generate-core-file ~/core
>   Saved corefile ~/core
>
> gdb/
> 2013-08-08  Pedro Alves  <palves@redhat.com>
>
>         * gcore.c (create_gcore_bfd): Don't use tilde_expand here.
>         (gcore_command): Use tilde_expand here, and when showing the
>         filename to the user, show the expanded version.
> ---
>
> diff --git a/gdb/gcore.c b/gdb/gcore.c
> index 9c83ec8..327aa8e 100644
> --- a/gdb/gcore.c
> +++ b/gdb/gcore.c
> @@ -52,12 +52,7 @@ static int gcore_memory_sections (bfd *);
>  bfd *
>  create_gcore_bfd (const char *filename)
>  {
> -  char *fullname;
> -  bfd *obfd;
> -
> -  fullname = tilde_expand (filename);
> -  obfd = gdb_bfd_openw (fullname, default_gcore_target ());
> -  xfree (fullname);
> +  bfd *obfd = gdb_bfd_openw (fullname, default_gcore_target ());

Seems that here must be filename (not fullname) ?

>
>    if (!obfd)
>      error (_("Failed to open '%s' for output."), filename);
> @@ -127,8 +122,9 @@ do_bfd_delete_cleanup (void *arg)
>  static void
>  gcore_command (char *args, int from_tty)
>  {
> -  struct cleanup *old_chain;
> -  char *corefilename, corefilename_buffer[40];
> +  struct cleanup *filename_chain;
> +  struct cleanup *bfd_chain;
> +  char *corefilename;
>    bfd *obfd;
>
>    /* No use generating a corefile without a target process.  */
> @@ -136,15 +132,15 @@ gcore_command (char *args, int from_tty)
>      noprocess ();
>
>    if (args && *args)
> -    corefilename = args;
> +    corefilename = tilde_expand (args);
>    else
>      {
>        /* Default corefile name is "core.PID".  */
> -      xsnprintf (corefilename_buffer, sizeof (corefilename_buffer),
> -                "core.%d", PIDGET (inferior_ptid));
> -      corefilename = corefilename_buffer;
> +      corefilename = xstrprintf ("core.%d", PIDGET (inferior_ptid));
>      }
>
> +  filename_chain = make_cleanup (xfree, corefilename);
> +
>    if (info_verbose)
>      fprintf_filtered (gdb_stdout,
>                       "Opening corefile '%s' for output.\n", corefilename);
> @@ -153,16 +149,17 @@ gcore_command (char *args, int from_tty)
>    obfd = create_gcore_bfd (corefilename);
>
>    /* Need a cleanup that will close and delete the file.  */
> -  old_chain = make_cleanup (do_bfd_delete_cleanup, obfd);
> +  bfd_chain = make_cleanup (do_bfd_delete_cleanup, obfd);
>
>    /* Call worker function.  */
>    write_gcore_file (obfd);
>
>    /* Succeeded.  */
> -  fprintf_filtered (gdb_stdout, "Saved corefile %s\n", corefilename);
> -
> -  discard_cleanups (old_chain);
> +  discard_cleanups (bfd_chain);
>    gdb_bfd_unref (obfd);
> +
> +  fprintf_filtered (gdb_stdout, "Saved corefile %s\n", corefilename);
> +  do_cleanups (filename_chain);
>  }
>
>  static unsigned long
>



-- 
Respectfully
Azat Khuzhin