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

Azat Khuzhin
2013-08-08T18:16:56Z

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