[PATCH] gcore: expand tilde in filename, like in "dump memory" command.

[PATCH] gcore: expand tilde in filename, like in "dump memory" command.

[Azat Khuzhin]

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
---
 ChangeLog   |    4 ++++
 gdb/gcore.c |    6 +++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 8b16b09..7aa8e8e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2013-07-22  Azat Khuzhin  <a3at.mail@gmail.com>
+
+	* gdb/gcore.c: Use tilde_expand() to handle tilde in filename
+
 2013-07-22  Joel Brobecker  <brobecker@adacore.com>
 
 	* src-release (VER): Use $(TOOL)/common/create-version.sh
diff --git a/gdb/gcore.c b/gdb/gcore.c
index 620be54..af3fe18 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -34,6 +34,7 @@
 #include "regcache.h"
 #include "regset.h"
 #include "gdb_bfd.h"
+#include "readline/tilde.h"
 
 /* The largest amount of memory to read from the target at once.  We
    must throttle it to limit the amount of memory used by GDB during
@@ -51,7 +52,10 @@ static int gcore_memory_sections (bfd *);
 bfd *
 create_gcore_bfd (const char *filename)
 {
-  bfd *obfd = gdb_bfd_openw (filename, default_gcore_target ());
+  char *fullname = tilde_expand (filename);
+
+  bfd *obfd = gdb_bfd_openw (fullname, default_gcore_target ());
+  xfree (fullname);
 
   if (!obfd)
     error (_("Failed to open '%s' for output."), filename);
-- 
1.7.10.4

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

[Pedro Alves]

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).

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 ());
 
   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

Re: [PATCH] gcore: expand tilde in filename, like in "dump memory" command.

[Pedro Alves]

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

Thanks!  Our cvs server is unfortunately stuck at the moment, but
I'll apply this as soon as it gets unstuck.

> ---
>  ChangeLog   |    4 ++++
>  gdb/gcore.c |    6 +++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 8b16b09..7aa8e8e 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@

> +2013-07-22  Azat Khuzhin  <a3at.mail@gmail.com>
> +
> +	* gdb/gcore.c: Use tilde_expand() to handle tilde in filename

File paths mentioned in ChangeLog entries are relative to the directory
that holds the ChangeLog.  So gdb/ should be dropped.
No () when mentioning a function by name.  Period at end of
sentence.

I notice you don't seem to have a copyright assignment on file.
The change is small enough that we can put it in without one
(that's what "tiny" means in the ChangeLog).  If you intend to
contribute further, contact me off list, and I'll get you
started on the process.

>  bfd *
>  create_gcore_bfd (const char *filename)
>  {
> -  bfd *obfd = gdb_bfd_openw (filename, default_gcore_target ());
> +  char *fullname = tilde_expand (filename);
> +
> +  bfd *obfd = gdb_bfd_openw (fullname, default_gcore_target ());

Empty line goes after last declaration.

> +  xfree (fullname);
>  
>    if (!obfd)
>      error (_("Failed to open '%s' for output."), filename);

Here's the adjusted patch I plan to check in as soon as I'm able to.

------------
From: Azat Khuzhin <a3at.mail@gmail.com>
Date: 2013-08-08 01:04:35 +0400

gcore: expand tilde in filename, like in "dump memory" command.

Before this patch this fails:

 (gdb) generate-core-file ~/core
 Failed to open '~/core' for output.

After this patch:

 (gdb) generate-core-file ~/core
 Saved corefile ~/core

gdb/
2013-08-08  Azat Khuzhin  <a3at.mail@gmail.com>  (tiny change)

	* gcore.c (create_gcore_bfd): Use tilde_expand.
---

 gdb/gcore.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/gcore.c b/gdb/gcore.c
index 620be54..9c83ec8 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -34,6 +34,7 @@
 #include "regcache.h"
 #include "regset.h"
 #include "gdb_bfd.h"
+#include "readline/tilde.h"
 
 /* The largest amount of memory to read from the target at once.  We
    must throttle it to limit the amount of memory used by GDB during
@@ -51,7 +52,12 @@ static int gcore_memory_sections (bfd *);
 bfd *
 create_gcore_bfd (const char *filename)
 {
-  bfd *obfd = gdb_bfd_openw (filename, default_gcore_target ());
+  char *fullname;
+  bfd *obfd;
+
+  fullname = tilde_expand (filename);
+  obfd = gdb_bfd_openw (fullname, default_gcore_target ());
+  xfree (fullname);
 
   if (!obfd)
     error (_("Failed to open '%s' for output."), filename);

Re: [PATCH] gcore: expand tilde in filename, like in "dump memory" command.

[Azat Khuzhin]

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
>
> Thanks!  Our cvs server is unfortunately stuck at the moment, but
> I'll apply this as soon as it gets unstuck.
>
>> ---
>>  ChangeLog   |    4 ++++
>>  gdb/gcore.c |    6 +++++-
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 8b16b09..7aa8e8e 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,7 @@
>
>> +2013-07-22  Azat Khuzhin  <a3at.mail@gmail.com>
>> +
>> +     * gdb/gcore.c: Use tilde_expand() to handle tilde in filename
>
> File paths mentioned in ChangeLog entries are relative to the directory
> that holds the ChangeLog.  So gdb/ should be dropped.
> No () when mentioning a function by name.  Period at end of
> sentence.
>
> I notice you don't seem to have a copyright assignment on file.
> The change is small enough that we can put it in without one
> (that's what "tiny" means in the ChangeLog).  If you intend to
> contribute further, contact me off list, and I'll get you
> started on the process.

Yes, I also think that it is small-enough to avoid copyright assignment.
Thanks, I'm thinking about this.

>
>>  bfd *
>>  create_gcore_bfd (const char *filename)
>>  {
>> -  bfd *obfd = gdb_bfd_openw (filename, default_gcore_target ());
>> +  char *fullname = tilde_expand (filename);
>> +
>> +  bfd *obfd = gdb_bfd_openw (fullname, default_gcore_target ());
>
> Empty line goes after last declaration.

Sorry, didn't notice it.

>
>> +  xfree (fullname);
>>
>>    if (!obfd)
>>      error (_("Failed to open '%s' for output."), filename);
>
> Here's the adjusted patch I plan to check in as soon as I'm able to.

Thanks for you review Pedro!

>
> ------------
> From: Azat Khuzhin <a3at.mail@gmail.com>
> Date: 2013-08-08 01:04:35 +0400
>
> gcore: expand tilde in filename, like in "dump memory" command.
>
> Before this patch this fails:
>
>  (gdb) generate-core-file ~/core
>  Failed to open '~/core' for output.
>
> After this patch:
>
>  (gdb) generate-core-file ~/core
>  Saved corefile ~/core
>
> gdb/
> 2013-08-08  Azat Khuzhin  <a3at.mail@gmail.com>  (tiny change)
>
>         * gcore.c (create_gcore_bfd): Use tilde_expand.
> ---
>
>  gdb/gcore.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/gcore.c b/gdb/gcore.c
> index 620be54..9c83ec8 100644
> --- a/gdb/gcore.c
> +++ b/gdb/gcore.c
> @@ -34,6 +34,7 @@
>  #include "regcache.h"
>  #include "regset.h"
>  #include "gdb_bfd.h"
> +#include "readline/tilde.h"
>
>  /* The largest amount of memory to read from the target at once.  We
>     must throttle it to limit the amount of memory used by GDB during
> @@ -51,7 +52,12 @@ static int gcore_memory_sections (bfd *);
>  bfd *
>  create_gcore_bfd (const char *filename)
>  {
> -  bfd *obfd = gdb_bfd_openw (filename, default_gcore_target ());
> +  char *fullname;
> +  bfd *obfd;
> +
> +  fullname = tilde_expand (filename);
> +  obfd = gdb_bfd_openw (fullname, default_gcore_target ());
> +  xfree (fullname);
>
>    if (!obfd)
>      error (_("Failed to open '%s' for output."), filename);
>



-- 
Respectfully
Azat Khuzhin

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

[Azat Khuzhin]

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

Re: [RFC] Show (tilde-)expanded filenames to the user?

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> But, I think we should have a policy here, and all commands
Pedro> should follow it.  Those that don't would be considered
Pedro> bugs.  The question then is, which policy is most appropriate?
Pedro> grepping around for "tilde_expand", it seems to be the
Pedro> showing expanded filenames is more common.

I tend to think that showing the expanded name is best.
My reason is that this way the user sees what actually happened.

Tom

Re: [RFC] Show (tilde-)expanded filenames to the user?

[Pedro Alves]

On 08/08/2013 08:49 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> But, I think we should have a policy here, and all commands
> Pedro> should follow it.  Those that don't would be considered
> Pedro> bugs.  The question then is, which policy is most appropriate?
> Pedro> grepping around for "tilde_expand", it seems to be the
> Pedro> showing expanded filenames is more common.
> 
> I tend to think that showing the expanded name is best.
> My reason is that this way the user sees what actually happened.

Alright, I audited tilde_expand uses throughout the tree, and
noticed most indeed show the expanded string to the user.  I'm
pushing a few patches to fix easy cases that didn't do that.

Thanks,
-- 
Pedro Alves