[RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Eli Zaretskii]

> Date: Tue, 12 Apr 2005 09:58:25 -0400
> From: bug-gdb@rich-paul.net
> 
> While using gdb 6.3,  I was having a problem with the directory name and the
> file name being concatenated by the edit command without an intervening slash.
> My solution was just to add the slash to the sprintf format string
> in cli/cli-cmds.c, near line 650.  It worked for me.

Thanks for pointing this out.

In fact, the code there had quite a few problems besides the one you
found: it didn't use symtab->fullname, failed miserably for DOS-style
d:/foo/bar file names, etc.

Does anyone object to the following patch?

2005-04-27  Eli Zaretskii  <eliz@gnu.org>

	* cli/cli-cmds.c (edit_command): Use symtab->fullname if
	possible.  Use IS_ABSOLUTE_PATH instead of checking for a literal
	'/'.  Make sure there's a slash between the directory and the file
	name.  Simplify and clarify the code logic.


--- gdb/cli/cli-cmds.c~0	2005-03-26 16:18:14.000000000 +0300
+++ gdb/cli/cli-cmds.c	2005-04-27 17:14:46.000000000 +0300
@@ -554,7 +554,7 @@ edit_command (char *arg, int from_tty)
   int cmdlen, log10;
   unsigned m;
   char *editor;
-  char *p;
+  char *p, *fn, *dn;
 
   /* Pull in the current default source line if necessary */
   if (arg == 0)
@@ -627,23 +627,33 @@ edit_command (char *arg, int from_tty)
 
   if ((editor = (char *) getenv ("EDITOR")) == NULL)
       editor = "/bin/ex";
-  
+
   /* Approximate base-10 log of line to 1 unit for digit count */
   for(log10=32, m=0x80000000; !(sal.line & m) && log10>0; log10--, m=m>>1);
   log10 = 1 + (int)((log10 + (0 == ((m-1) & sal.line)))/3.32192809);
 
-  cmdlen = strlen(editor) + 1
-         + (NULL == sal.symtab->dirname ? 0 : strlen(sal.symtab->dirname) + 1)
-	 + (NULL == sal.symtab->filename? 0 : strlen(sal.symtab->filename)+ 1)
-	 + log10 + 2;
-  
+  dn = "";
+  if (NULL != sal.symtab->fullname)
+    fn = sal.symtab->fullname;
+  else if (NULL == sal.symtab->filename)
+    {
+      fn = "unknown";
+      if (NULL != sal.symtab->dirname)
+	dn = sal.symtab->dirname;
+    }
+  else if (IS_ABSOLUTE_PATH (sal.symtab->filename))
+    fn = sal.symtab->filename;
+  else
+    {
+      fn = sal.symtab->filename;
+      dn = sal.symtab->dirname;
+      if (NULL == dn)
+	dn = ".";
+    }
+  /*         $EDITOR      blank  +NN  blank    dir       /     file      \0 */
+  cmdlen = strlen(editor) + 1 + log10 + 2 + strlen(dn) + 1 + strlen(fn) + 1;
   p = xmalloc(cmdlen);
-  sprintf(p,"%s +%d %s%s",editor,sal.line,
-     (NULL == sal.symtab->dirname ? "./" :
-        (NULL != sal.symtab->filename && *(sal.symtab->filename) != '/') ?
-	   sal.symtab->dirname : ""),
-     (NULL == sal.symtab->filename ? "unknown" : sal.symtab->filename)
-  );
+  sprintf (p, "%s +%d %s%s%s", editor, sal.line, dn, (*dn ? "/" : ""), fn);
   shell_escape(p, from_tty);
 
   xfree(p);

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Daniel Jacobowitz]

On Wed, Apr 27, 2005 at 05:30:11PM +0300, Eli Zaretskii wrote:
> > Date: Tue, 12 Apr 2005 09:58:25 -0400
> > From: bug-gdb@rich-paul.net
> > 
> > While using gdb 6.3,  I was having a problem with the directory name and the
> > file name being concatenated by the edit command without an intervening slash.
> > My solution was just to add the slash to the sprintf format string
> > in cli/cli-cmds.c, near line 650.  It worked for me.
> 
> Thanks for pointing this out.
> 
> In fact, the code there had quite a few problems besides the one you
> found: it didn't use symtab->fullname, failed miserably for DOS-style
> d:/foo/bar file names, etc.
> 
> Does anyone object to the following patch?
> 
> 2005-04-27  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* cli/cli-cmds.c (edit_command): Use symtab->fullname if
> 	possible.  Use IS_ABSOLUTE_PATH instead of checking for a literal
> 	'/'.  Make sure there's a slash between the directory and the file
> 	name.  Simplify and clarify the code logic.

I can simplify this a whole lot further :-)

You should use symtab_to_fullname.  Then all the fallback logic is
unnecessary; if symtab_to_fullname fails, GDB does not know where the
file is.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Eli Zaretskii]

> Date: Wed, 27 Apr 2005 10:36:20 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: bug-gdb@rich-paul.net, bug-gdb@gnu.org, gdb-patches@sources.redhat.com
> 
> I can simplify this a whole lot further :-)
> 
> You should use symtab_to_fullname.  Then all the fallback logic is
> unnecessary; if symtab_to_fullname fails, GDB does not know where the
> file is.

Thanks for the tip.  Here's the revised patch:


2005-04-27  Eli Zaretskii  <eliz@gnu.org>

	* cli/cli-cmds.c (edit_command): If symtab->fullname is not yet
	set, use symtab_to_fullname , instead of trying to do its job.

--- gdb/cli/cli-cmds.c~0	2005-03-26 16:18:14.000000000 +0300
+++ gdb/cli/cli-cmds.c	2005-04-27 18:37:34.000000000 +0300
@@ -554,7 +554,7 @@ edit_command (char *arg, int from_tty)
   int cmdlen, log10;
   unsigned m;
   char *editor;
-  char *p;
+  char *p, *fn;
 
   /* Pull in the current default source line if necessary */
   if (arg == 0)
@@ -627,23 +627,26 @@ edit_command (char *arg, int from_tty)
 
   if ((editor = (char *) getenv ("EDITOR")) == NULL)
       editor = "/bin/ex";
-  
+
   /* Approximate base-10 log of line to 1 unit for digit count */
   for(log10=32, m=0x80000000; !(sal.line & m) && log10>0; log10--, m=m>>1);
   log10 = 1 + (int)((log10 + (0 == ((m-1) & sal.line)))/3.32192809);
 
-  cmdlen = strlen(editor) + 1
-         + (NULL == sal.symtab->dirname ? 0 : strlen(sal.symtab->dirname) + 1)
-	 + (NULL == sal.symtab->filename? 0 : strlen(sal.symtab->filename)+ 1)
-	 + log10 + 2;
-  
+  /* If we don't already know the full absolute file name of the
+     source file, find it now.  */
+  if (NULL == sal.symtab->fullname)
+    {
+      fn = symtab_to_fullname (sal.symtab);
+      if (NULL == fn)
+	fn = "unknown";
+    }
+  else
+    fn = sal.symtab->fullname;
+
+  /*         $EDITOR      blank  +NN  blank    file      \0 */
+  cmdlen = strlen(editor) + 1 + log10 + 2 +  strlen(fn) + 1;
   p = xmalloc(cmdlen);
-  sprintf(p,"%s +%d %s%s",editor,sal.line,
-     (NULL == sal.symtab->dirname ? "./" :
-        (NULL != sal.symtab->filename && *(sal.symtab->filename) != '/') ?
-	   sal.symtab->dirname : ""),
-     (NULL == sal.symtab->filename ? "unknown" : sal.symtab->filename)
-  );
+  sprintf (p, "%s +%d %s", editor, sal.line, fn);
   shell_escape(p, from_tty);
 
   xfree(p);

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Mark Kettenis]

   Date: Wed, 27 Apr 2005 18:48:46 +0300
   From: "Eli Zaretskii" <eliz@gnu.org>

   Thanks for the tip.  Here's the revised patch:

   2005-04-27  Eli Zaretskii  <eliz@gnu.org>

	   * cli/cli-cmds.c (edit_command): If symtab->fullname is not yet
	   set, use symtab_to_fullname , instead of trying to do its job.

And you get extra bonus points for eliminating sprintf(3) ;-)

Is it just me, or do others also find the NULL == expression bits a
bit odd?  Anyway, they're not your fault, although I'd welcome you ti
fix those ;-).

Mark

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Daniel Jacobowitz]

On Wed, Apr 27, 2005 at 06:48:46PM +0300, Eli Zaretskii wrote:
> Thanks for the tip.  Here's the revised patch:

The rest of this is mostly stylistic; a few comments...

> +  /* If we don't already know the full absolute file name of the
> +     source file, find it now.  */
> +  if (NULL == sal.symtab->fullname)
> +    {
> +      fn = symtab_to_fullname (sal.symtab);
> +      if (NULL == fn)
> +	fn = "unknown";
> +    }
> +  else
> +    fn = sal.symtab->fullname;
> +
> +  /*         $EDITOR      blank  +NN  blank    file      \0 */
> +  cmdlen = strlen(editor) + 1 + log10 + 2 +  strlen(fn) + 1;
>    p = xmalloc(cmdlen);
> -  sprintf(p,"%s +%d %s%s",editor,sal.line,
> -     (NULL == sal.symtab->dirname ? "./" :
> -        (NULL != sal.symtab->filename && *(sal.symtab->filename) != '/') ?
> -	   sal.symtab->dirname : ""),
> -     (NULL == sal.symtab->filename ? "unknown" : sal.symtab->filename)
> -  );
> +  sprintf (p, "%s +%d %s", editor, sal.line, fn);

It strikes me as odd that you can use "edit" if GDB doesn't know where
the source file is.  I realize this is a pre-existing condition, but...
Also, symtab_to_fullname includes the cached fullname check.  So what's
your opinion of boiling the whole thing down to:

  fn = symtab_to_fullname (sal.symtab);
  if (fn == NULL)
    error (_("Could not find file \"%s\""), sal.symtab->filename);
  p = xstrprintf ("%s +%d %s", editor, sal.line, fn);

Mark, can I have those bonus points? :-)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Eli Zaretskii]

> Date: Wed, 27 Apr 2005 14:04:10 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: bug-gdb@rich-paul.net, bug-gdb@gnu.org, gdb-patches@sources.redhat.com
> 
> It strikes me as odd that you can use "edit" if GDB doesn't know where
> the source file is.

I thought about that, and decided to leave that code alone: GDB might
indeed not know where's the source, but the user could know more.
Most editors let you open files from inside the editor, so a user
could still edit the file.

> Also, symtab_to_fullname includes the cached fullname check.  So what's
> your opinion of boiling the whole thing down to:
> 
>   fn = symtab_to_fullname (sal.symtab);
>   if (fn == NULL)
>     error (_("Could not find file \"%s\""), sal.symtab->filename);
>   p = xstrprintf ("%s +%d %s", editor, sal.line, fn);
> 
> Mark, can I have those bonus points? :-)

Well, perhaps you should commit this, then.  There's not a single line
written by me in this snippet, so it would be misleading for me to
claim the credits in the logs.

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Eli Zaretskii]

> Date: Wed, 27 Apr 2005 19:42:51 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: gdb-patches@sources.redhat.com
> 
> Is it just me, or do others also find the NULL == expression bits a
> bit odd?

There's a school of thought that writing comparisons with a constant
like this prevent errors whereby one uses "=" instead of "==": any
reasonable compiler will yell bloody murder if you say "NULL = foo",
while "foo = NULL" might go unnoticed with some compilers.

However, it looks like the fragment I was patching is the only one
which uses this notation in the while cli directory, so perhaps it's
okay to swap the identifiers to the more traditional form.

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Eli Zaretskii]

> Date: Wed, 27 Apr 2005 14:04:10 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: bug-gdb@rich-paul.net, bug-gdb@gnu.org, gdb-patches@sources.redhat.com
> 
> It strikes me as odd that you can use "edit" if GDB doesn't know where
> the source file is.  I realize this is a pre-existing condition, but...
> Also, symtab_to_fullname includes the cached fullname check.  So what's
> your opinion of boiling the whole thing down to:
> 
>   fn = symtab_to_fullname (sal.symtab);
>   if (fn == NULL)
>     error (_("Could not find file \"%s\""), sal.symtab->filename);
>   p = xstrprintf ("%s +%d %s", editor, sal.line, fn);
> 
> Mark, can I have those bonus points? :-)

I ended up committing the attached.  Note that it quotes the file
name, to account for possible special characters that would confise
the shell.  (I use "..." for quoting because this is more portable to
various shells, including on Windows.)

Thanks to Daniel and Mark for valuable input.


2005-04-28  Eli Zaretskii  <eliz@gnu.org>

	* cli/cli-cmds.c (edit_command): If symtab->fullname is not yet
	set, use symtab_to_fullname, instead of trying to do its job.  Use
	xstrprintf instead of malloc and sprintf.


Index: gdb/cli/cli-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-cmds.c,v
retrieving revision 1.58
retrieving revision 1.59
diff -u -r1.58 -r1.59
--- gdb/cli/cli-cmds.c	16 Mar 2005 15:58:41 -0000	1.58
+++ gdb/cli/cli-cmds.c	28 Apr 2005 20:32:41 -0000	1.59
@@ -554,7 +554,7 @@
   int cmdlen, log10;
   unsigned m;
   char *editor;
-  char *p;
+  char *p, *fn;
 
   /* Pull in the current default source line if necessary */
   if (arg == 0)
@@ -627,25 +627,26 @@
 
   if ((editor = (char *) getenv ("EDITOR")) == NULL)
       editor = "/bin/ex";
-  
+
   /* Approximate base-10 log of line to 1 unit for digit count */
   for(log10=32, m=0x80000000; !(sal.line & m) && log10>0; log10--, m=m>>1);
   log10 = 1 + (int)((log10 + (0 == ((m-1) & sal.line)))/3.32192809);
 
-  cmdlen = strlen(editor) + 1
-         + (NULL == sal.symtab->dirname ? 0 : strlen(sal.symtab->dirname) + 1)
-	 + (NULL == sal.symtab->filename? 0 : strlen(sal.symtab->filename)+ 1)
-	 + log10 + 2;
-  
-  p = xmalloc(cmdlen);
-  sprintf(p,"%s +%d %s%s",editor,sal.line,
-     (NULL == sal.symtab->dirname ? "./" :
-        (NULL != sal.symtab->filename && *(sal.symtab->filename) != '/') ?
-	   sal.symtab->dirname : ""),
-     (NULL == sal.symtab->filename ? "unknown" : sal.symtab->filename)
-  );
-  shell_escape(p, from_tty);
+  /* If we don't already know the full absolute file name of the
+     source file, find it now.  */
+  if (!sal.symtab->fullname)
+    {
+      fn = symtab_to_fullname (sal.symtab);
+      if (!fn)
+	fn = "unknown";
+    }
+  else
+    fn = sal.symtab->fullname;
 
+  /* Quote the file name, in case it has whitespace or other special
+     characters.  */
+  p = xstrprintf ("%s +%d \"%s\"", editor, sal.line, fn);
+  shell_escape(p, from_tty);
   xfree(p);
 }
 

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Daniel Jacobowitz]

On Thu, Apr 28, 2005 at 11:36:50PM +0300, Eli Zaretskii wrote:
> I ended up committing the attached.  Note that it quotes the file
> name, to account for possible special characters that would confise
> the shell.  (I use "..." for quoting because this is more portable to
> various shells, including on Windows.)
> 
> Thanks to Daniel and Mark for valuable input.

Fine with me.  Thanks.

The use of "" is not a bulletproof quoting mechanism, but it's better
than previously.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Eli Zaretskii]

> Date: Thu, 28 Apr 2005 16:42:01 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: bug-gdb@rich-paul.net, bug-gdb@gnu.org, gdb-patches@sources.redhat.com
> 
> The use of "" is not a bulletproof quoting mechanism

Yep, known.  But it's IMHO the most portable one can get without going
into the pains of quotesys and its ilk (which doesn't support
non-Posix shells at all).

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Daniel Jacobowitz]

On Fri, Apr 29, 2005 at 12:02:27AM +0300, Eli Zaretskii wrote:
> > Date: Thu, 28 Apr 2005 16:42:01 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: bug-gdb@rich-paul.net, bug-gdb@gnu.org, gdb-patches@sources.redhat.com
> > 
> > The use of "" is not a bulletproof quoting mechanism
> 
> Yep, known.  But it's IMHO the most portable one can get without going
> into the pains of quotesys and its ilk (which doesn't support
> non-Posix shells at all).

Or using a mechanism to start other processes that takes an argument
vector :-)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Fix file name generation in edit_command (was: Ver 6.3 edit command failing)

[Eli Zaretskii]

> Date: Thu, 28 Apr 2005 17:18:03 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: bug-gdb@gnu.org, gdb-patches@sources.redhat.com
> 
> Or using a mechanism to start other processes that takes an argument
> vector :-)

Unfortunately, that's not a magic wand, either.  Specifically, in the
case in point, $EDITOR can be a _shell_command_, not a file name of a
program.  That is, I could say

     export EDITOR="emacs -q"

and expect it to work.  This will fail with vector-argument method of
invoking subprocesses.