Re: Proposed patch for gdb/mi 741

[Elena Zannoni <ezannoni at redhat dot com>]

J. Johnston writes:
 > Elena Zannoni wrote:
 > > 
 > > J. Johnston writes:
 > >  > The following solves a number of problems with the mi -environment commands.
 > >  > For starters, each command now has an mi cmd wrapper so arguments may use the
 > >  > syntax of adding double-quotes to handle extraneous characters such as spaces.
 > >  >
 > >  > The -environment-pwd command now lists the output in mi syntax:
 > >  >
 > >  > e.g.
 > >  > -environment-pwd
 > >  > ^done,cwd="...."
 > >  >
 > >  > The -environment-directory command has been changed to output in mi format.
 > >  > It also has been changed to allow for no arguments being passed.
 > >  > If no arguments are passed, it behaves like the gdb dir command and resets the
 > >  > source search path to the default, however, no confirmation query is performed.  If an empty
 > >  > string "" is passed, it is ignored so this can be used to display the current
 > >  > search path without modifying it.
 > >  >
 > >  > e.g.
 > >  > -environment-directory /usr/bin /usr/local/bin
 > >  > ^done,source-path="/usr/bin:/usr/local/bin:$cdir:$cwd"
 > >  >
 > >  > The -environment-path command has been changed to output in mi format.
 > >  > It also has been changed to allow for no arguments being passed.  When
 > >  > no arguments are passed, the current object search path is displayed.
 > >  >
 > >  > e.g.
 > >  > -environment-path
 > >  > ^done,path="....."
 > >  >
 > >  > For mi1 or below, the previous behavior is preserved by rerouting the commands
 > >  > to their old cli counterparts.
 > > 
 > > Hmmm, I think the testsuite changes are ok.  Also the addition of the
 > > new file is fine. However I don't think we should be duplicating the
 > > mod_path() code. Would it be possible to maybe split mod_path() in 2
 > > or more functions, and have MI call one of them?
 > > I mean
 > > 
 > > mod_path()
 > > {
 > >  do_cli_specific_stuff;
 > >  call add_path(args1);
 > > ....
 > > }
 > > 
 > > mi_env_path()
 > > {
 > >   do_mi_specific_stuff;
 > >   call add_path(args2);
 > > .....
 > > }
 > > 
 > > similarly for other code that may have been duplicated.
 > > 
 > > Thanks
 > > Elena
 > 
 > I have taken your advice and have added a new interface: add_path which takes
 > an additional argument.  The mod_path() routine calls it one way, the mi code
 > calls it another.  Of the other code I copied, there is no value add in trying
 > to make it common as I copied a few lines here and there.
 > 
 > I found I had to modify the tilde_expand() prototype in defs.h to get my
 > build working.  I have a combined source tree and there was a conflict with 
 > tilde.h found in the readline directory.  The defs.h prototype was not
 > declaring the argument as const.
 > 

Ah, right. readline has changed tilde_expand. You must have a newer
version of realine installed?  We cannot make that change yet, because
we need to build with the readline in our source tree, unfortunately.
But the upgrade of readline is coming soon. (which reminds me...)

I don't like one minor thing. The inconsistent meaning of the 'no argument'
for env-path and env-dir. I would think it would be more intuitive if both
would behave the same w/o argument: display the current values.
Maybe have a "--reset" or something like that to restore the defaults?
(suggestions welcome).


Otherwise the patch is ok, except for a couple of little things. The
comments. They need to start with a capital letter and end with a
period, and 2 spaces after periods. (I know, it's a pain) No externs
in .c files: include inferior.h (update Makefile) and add source_path
to defs.h. 

(mention pr number in changelog)

oh, hum, who is responsible for source.c?

thanks
Elena



 > Ok to commit?
 > 
 > -- Jeff J.
 > 
 > gdb/ChangeLog:
 > 
 > 2002-11-07  Jeff Johnston  <jjohnstn@redhat.com>
 > 
 > 	* defs.h (init_last_source_visited): New prototype.
 > 	(add_path): Ditto.
 > 	(tilde_expand): Change prototype to const char * to match readline.h.
 > 	* source.c (add_path): New function that adds to a specified path.
 > 	(mod_path): Change to call add_path.
 > 	(init_last_source_visited): New function to allow interfaces to 
 > 	initialize static variable: last_source_visited.
 > 
 > gdb/mi/ChangeLog:
 > 
 > 2002-11-07  Jeff Johnston  <jjohnstn@redhat.com>
 > 
 >         * mi-cmds.c (-environment-directory) Change to use mi_cmd_env_dir,
 >         (-environment-cd): Change to use mi_cmd_env_cd,.
 >         (-environment-pwd): Change to use mi_cmd_env_pwd.
 >         (-environment-path): Change to use mi_cmd_env_path.
 >         * mi-cmds.h (mi_cmd_env_cd, mi_cmd_env_dir): New prototypes.
 >         (mi_cmd_env_path, mi_cmd_env_pwd): Ditto.
 >         * mi-cmd-env.c: New file.  Part of fix for PR gdb/741.
 >         * gdbmi.texinfo (environment-cd): Update output and example.
 >         (environment-pwd): Ditto.
 >         (environment-dir): Update output, description, and examples.
 >         (environment-path): Ditto.
 > 
 > gdb/testsuite/gdb.mi/ChangeLog:
 > 
 > 2002-11-07  Jeff Johnston  <jjohnstn@redhat.com>
 > 
 >         * mi-basics.exp: Change tests for -environment-directory.  Also add
 >         tests for -environment-cd and -environment-pwd.  Part of fix for
 >         PR gdb/741.Index: defs.h
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/defs.h,v
 > retrieving revision 1.102
 > diff -u -r1.102 defs.h
 > --- defs.h	15 Oct 2002 02:16:51 -0000	1.102
 > +++ defs.h	7 Nov 2002 21:47:03 -0000
 > @@ -572,10 +572,14 @@
 >  
 >  extern void mod_path (char *, char **);
 >  
 > +extern void add_path (char *, char **, int);
 > +
 >  extern void directory_command (char *, int);
 >  
 >  extern void init_source_path (void);
 >  
 > +extern void init_last_source_visited (void);
 > +
 >  extern char *symtab_to_filename (struct symtab *);
 >  
 >  /* From exec.c */
 > @@ -616,7 +620,7 @@
 >  
 >  /* From readline (but not in any readline .h files).  */
 >  
 > -extern char *tilde_expand (char *);
 > +extern char *tilde_expand (const char *);
 >  
 >  /* Control types for commands */
 >  
 > Index: source.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/source.c,v
 > retrieving revision 1.36
 > diff -u -r1.36 source.c
 > --- source.c	24 Oct 2002 21:02:53 -0000	1.36
 > +++ source.c	7 Nov 2002 21:47:03 -0000
 > @@ -357,6 +357,12 @@
 >    forget_cached_source_info ();
 >  }
 >  
 > +void
 > +init_last_source_visited (void)
 > +{
 > +  last_source_visited = NULL;
 > +}
 > +
 >  /* Add zero or more directories to the front of the source path.  */
 >  
 >  void
 > @@ -387,6 +393,18 @@
 >  void
 >  mod_path (char *dirname, char **which_path)
 >  {
 > +  add_path (dirname, which_path, 1);
 > +}
 > +
 > +/* Workhorse of mod_path.  Takes an extra argument to determine
 > +   if dirname should be parsed for separators that indicate multiple
 > +   directories.  This allows for interfaces that pre-parse the dirname
 > +   and allow specification of traditional separator characters such
 > +   as space or tab. */
 > +
 > +void
 > +add_path (char *dirname, char **which_path, int parse_separators)
 > +{
 >    char *old = *which_path;
 >    int prefix = 0;
 >  
 > @@ -403,9 +421,16 @@
 >        struct stat st;
 >  
 >        {
 > -	char *separator = strchr (name, DIRNAME_SEPARATOR);
 > -	char *space = strchr (name, ' ');
 > -	char *tab = strchr (name, '\t');
 > +	char *separator = NULL;
 > +	char *space = NULL;
 > +	char *tab = NULL;
 > +
 > +	if (parse_separators)
 > +	  {
 > +	    separator = strchr (name, DIRNAME_SEPARATOR);
 > +	    space = strchr (name, ' ');
 > +	    tab = strchr (name, '\t');
 > +	  }
 >  
 >  	if (separator == 0 && space == 0 && tab == 0)
 >  	  p = dirname = name + strlen (name);
 > @@ -536,7 +561,8 @@
 >  	    tinybuf[0] = DIRNAME_SEPARATOR;
 >  	    tinybuf[1] = '\0';
 >  
 > -	    /* If we have already tacked on a name(s) in this command,                     be sure they stay on the front as we tack on some more.  */
 > +	    /* If we have already tacked on a name(s) in this command, be sure they stay 
 > +	       on the front as we tack on some more.  */
 >  	    if (prefix)
 >  	      {
 >  		char *temp, c;
 > Index: mi/mi-cmd-env.c
 > ===================================================================
 > RCS file: mi/mi-cmd-env.c
 > diff -N mi/mi-cmd-env.c
 > --- mi/mi-cmd-env.c	1 Jan 1970 00:00:00 -0000
 > +++ mi/mi-cmd-env.c	7 Nov 2002 21:47:04 -0000
 > @@ -0,0 +1,174 @@
 > +/* MI Command Set - environment commands.
 > +   Copyright 2002 Free Software Foundation, Inc.
 > +   Contributed by Red Hat Inc.
 > +
 > +   This file is part of GDB.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 2 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program; if not, write to the Free Software
 > +   Foundation, Inc., 59 Temple Place - Suite 330,
 > +   Boston, MA 02111-1307, USA.  */
 > +
 > +#include <string.h>
 > +#include <sys/stat.h>
 > +
 > +#include "defs.h"
 > +#include "target.h"
 > +#include "frame.h"
 > +#include "value.h"
 > +#include "mi-cmds.h"
 > +#include "mi-out.h"
 > +#include "ui-out.h"
 > +#include "symtab.h"
 > +#include "filenames.h"
 > +#include "environ.h"
 > +#include "command.h"
 > +#include "top.h"
 > +
 > +extern struct environ *inferior_environ;
 > +extern char *source_path;
 > +
 > +static void env_cli_command (const char *cli, char *args);
 > +static void env_mod_path (char *dirname, char **which_path);
 > +
 > +static const char path_var_name[] = "PATH";
 > +
 > +/* the following is copied from mi-main.c so for m1 and below we
 > +   can perform old behavior and use cli commands */
 > +static void
 > +env_execute_cli_command (const char *cli, char *args)
 > +{
 > +  if (cli != 0)
 > +    {
 > +      struct cleanup *old_cleanups;
 > +      char *run;
 > +      xasprintf (&run, cli, args);
 > +      old_cleanups = make_cleanup (xfree, run);
 > +      execute_command ( /*ui */ run, 0 /*from_tty */ );
 > +      do_cleanups (old_cleanups);
 > +      return;
 > +    }
 > +}
 > +
 > +
 > +/* Print working directory. */
 > +enum mi_cmd_result
 > +mi_cmd_env_pwd (char *command, char **argv, int argc)
 > +{
 > +  if (argc > 0)
 > +    error ("mi_cmd_env_pwd: No arguments required");
 > +          
 > +  if (mi_version (uiout) < 2)
 > +    {
 > +      env_execute_cli_command ("pwd", NULL);
 > +      return MI_CMD_DONE;
 > +    }
 > +     
 > +  /* otherwise mi level 2 or higher */
 > +
 > +  getcwd (gdb_dirbuf, sizeof (gdb_dirbuf));
 > +  ui_out_field_string (uiout, "cwd", gdb_dirbuf);
 > +
 > +  return MI_CMD_DONE;
 > +}
 > +
 > +/* Change working directory. */
 > +enum mi_cmd_result
 > +mi_cmd_env_cd (char *command, char **argv, int argc)
 > +{
 > +  if (argc == 0 || argc > 1)
 > +    error ("mi_cmd_env_cd: Usage DIRECTORY");
 > +          
 > +  env_execute_cli_command ("cd %s", argv[0]);
 > +
 > +  return MI_CMD_DONE;
 > +}
 > +
 > +static void
 > +env_mod_path (char *dirname, char **which_path)
 > +{
 > +  if (dirname == 0 || dirname[0] == '\0')
 > +    return;
 > +
 > +  /* call add_path with last arg 0 to indicate not to parse for separator chars */
 > +  add_path (dirname, which_path, 0);
 > +}
 > +
 > +/* Add one or more directories to start of executable search path */
 > +enum mi_cmd_result
 > +mi_cmd_env_path (char *command, char **argv, int argc)
 > +{
 > +  char *exec_path;
 > +  char *env;
 > +  int i;
 > +
 > +  if (mi_version (uiout) < 2)
 > +    {
 > +      for (i = argc - 1; i >= 0; --i)
 > +	env_execute_cli_command ("path %s", argv[i]);
 > +      return MI_CMD_DONE;
 > +    }
 > +
 > +  /* otherwise mi level 2 or higher */
 > +  dont_repeat ();
 > +  env = get_in_environ (inferior_environ, path_var_name);
 > +
 > +  /* Can be null if path is not set */
 > +  if (!env)
 > +    env = "";
 > +  exec_path = xstrdup (env);
 > +
 > +  for (i = argc - 1; i >= 0; --i)
 > +    env_mod_path (argv[i], &exec_path);
 > +
 > +  set_in_environ (inferior_environ, path_var_name, exec_path);
 > +  xfree (exec_path);
 > +  env = get_in_environ (inferior_environ, path_var_name);
 > +  ui_out_field_string (uiout, "path", env);
 > +
 > +  return MI_CMD_DONE;
 > +}
 > +
 > +/* Add zero or more directories to the front of the source path.  */
 > +enum mi_cmd_result
 > +mi_cmd_env_dir (char *command, char **argv, int argc)
 > +{
 > +  int i;
 > +
 > +  dont_repeat ();
 > +
 > +  if (mi_version (uiout) < 2)
 > +    {
 > +      for (i = argc - 1; i >= 0; --i)
 > +	env_execute_cli_command ("dir %s", argv[i]);
 > +      return MI_CMD_DONE;
 > +    }
 > +
 > +  /* otherwise mi 2 or higher */
 > +  if (argc == 0)
 > +    {
 > +      /* no args implies reset to default path */
 > +      xfree (source_path);
 > +      init_source_path ();
 > +    }
 > +  else
 > +    {
 > +      for (i = argc - 1; i >= 0; --i)
 > +	env_mod_path (argv[i], &source_path);
 > +      init_last_source_visited ();
 > +    }
 > +
 > +  ui_out_field_string (uiout, "source-path", source_path);
 > +  forget_cached_source_info ();
 > +}
 > +
 > Index: mi/mi-cmds.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/mi/mi-cmds.c,v
 > retrieving revision 1.8
 > diff -u -r1.8 mi-cmds.c
 > --- mi/mi-cmds.c	6 Mar 2001 08:21:45 -0000	1.8
 > +++ mi/mi-cmds.c	7 Nov 2002 21:47:04 -0000
 > @@ -56,10 +56,10 @@
 >    {"display-enable", 0, 0},
 >    {"display-insert", 0, 0},
 >    {"display-list", 0, 0},
 > -  {"environment-cd", "cd %s", 0},
 > -  {"environment-directory", "dir %s", 0},
 > -  {"environment-path", "path %s", 0},
 > -  {"environment-pwd", "pwd", 0},
 > +  {"environment-cd", 0, 0, mi_cmd_env_cd},
 > +  {"environment-directory", 0, 0, mi_cmd_env_dir},
 > +  {"environment-path", 0, 0, mi_cmd_env_path},
 > +  {"environment-pwd", 0, 0, mi_cmd_env_pwd},
 >    {"exec-abort", 0, 0},
 >    {"exec-arguments", "set args %s", 0},
 >    {"exec-continue", 0, mi_cmd_exec_continue},
 > Index: mi/mi-cmds.h
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/mi/mi-cmds.h,v
 > retrieving revision 1.5
 > diff -u -r1.5 mi-cmds.h
 > --- mi/mi-cmds.h	6 Mar 2001 08:21:45 -0000	1.5
 > +++ mi/mi-cmds.h	7 Nov 2002 21:47:04 -0000
 > @@ -64,6 +64,10 @@
 >  extern mi_cmd_argv_ftype mi_cmd_data_read_memory;
 >  extern mi_cmd_argv_ftype mi_cmd_data_write_memory;
 >  extern mi_cmd_argv_ftype mi_cmd_data_write_register_values;
 > +extern mi_cmd_argv_ftype mi_cmd_env_cd;
 > +extern mi_cmd_argv_ftype mi_cmd_env_dir;
 > +extern mi_cmd_argv_ftype mi_cmd_env_path;
 > +extern mi_cmd_argv_ftype mi_cmd_env_pwd;
 >  extern mi_cmd_args_ftype mi_cmd_exec_continue;
 >  extern mi_cmd_args_ftype mi_cmd_exec_finish;
 >  extern mi_cmd_args_ftype mi_cmd_exec_next;
 > Index: gdbmi.texinfo
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/mi/gdbmi.texinfo,v
 > retrieving revision 1.29
 > diff -u -r1.29 gdbmi.texinfo
 > --- gdbmi.texinfo	3 Oct 2002 22:31:31 -0000	1.29
 > +++ gdbmi.texinfo	7 Nov 2002 21:55:40 -0000
 > @@ -1665,10 +1665,12 @@
 >  @subsubheading Synopsis
 >  
 >  @example
 > - -environment-directory @var{pathdir}
 > + -environment-directory [ @var{pathdir} ]+
 >  @end example
 >  
 > -Add directory @var{pathdir} to beginning of search path for source files.
 > +Add directories @var{pathdir} to beginning of search path for source files.
 > +If no argument is given, reset search path to default.  An empty string for
 > +@var{pathdir} is ignored so it may be used to display the current search path.
 >  
 >  @subsubheading @value{GDBN} Command
 >  
 > @@ -1679,7 +1681,13 @@
 >  @smallexample
 >  (@value{GDBP})
 >  -environment-directory /kwikemart/marge/ezannoni/flathead-dev/devo/gdb
 > -^done
 > +^done,source-path="/kwikemart/marge/ezannoni/flathead-dev/devo/gdb:$cdir:$cwd"
 > +(@value{GDBP})
 > +-environment-directory ""
 > +^done,source-path="/kwikemart/marge/ezannoni/flathead-dev/devo/gdb:$cdir:$cwd"
 > +(@value{GDBP})
 > +-environment-directory
 > +^done,source-path="$cdir:$cwd"
 >  (@value{GDBP})
 >  @end smallexample
 >  
 > @@ -1690,10 +1698,12 @@
 >  @subsubheading Synopsis
 >  
 >  @example
 > - -environment-path ( @var{pathdir} )+
 > + -environment-path [ @var{pathdir} ]+
 >  @end example
 >  
 >  Add directories @var{pathdir} to beginning of search path for object files.
 > +If no paths or an empty path is specified, the current object search path
 > +is displayed with no modification.
 >  
 >  @subsubheading @value{GDBN} Command
 >  
 > @@ -1704,7 +1714,10 @@
 >  @smallexample
 >  (@value{GDBP})
 >  -environment-path /kwikemart/marge/ezannoni/flathead-dev/ppc-eabi/gdb
 > -^done
 > +^done,path="/kwikemart/marge/ezannoni/flathead-dev/ppc-eabi/gdb:/usr/bin"
 > +(@value{GDBP})
 > +-environment-path 
 > +^done,path="/kwikemart/marge/ezannoni/flathead-dev/ppc-eabi/gdb:/usr/bin"
 >  (@value{GDBP})
 >  @end smallexample
 >  
 > @@ -1729,8 +1742,7 @@
 >  @smallexample
 >  (@value{GDBP})
 >  -environment-pwd
 > -~Working directory /kwikemart/marge/ezannoni/flathead-dev/devo/gdb.
 > -^done
 > +^done,cwd="/kwikemart/marge/ezannoni/flathead-dev/devo/gdb"
 >  (@value{GDBP})
 >  @end smallexample
 >  
 > Index: mi-basics.exp
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-basics.exp,v
 > retrieving revision 1.6
 > diff -u -r1.6 mi-basics.exp
 > --- mi-basics.exp	27 Jun 2001 17:27:07 -0000	1.6
 > +++ mi-basics.exp	7 Nov 2002 21:56:00 -0000
 > @@ -150,24 +150,50 @@
 >  
 >      # Clear the search directories, then specify one to be searched
 >      # Tests:
 > -    # -environment-directory
 >      # -environment-directory arg
 > +    # -environment-directory empty-string
 > +    # -environment-directory
 >  
 >  #exp_internal 1
 > -    mi_gdb_test "202-environment-directory" \
 > -             "\\\^done" \
 > +    mi_gdb_test "202-environment-directory ${srcdir}/${subdir}" \
 > +             "\\\^done,source-path=\"${srcdir}/${subdir}:\\\$cdir:\\\$cwd\"" \
 > +             "environment-directory arg operation"
 > +
 > +    mi_gdb_test "203-environment-directory \"\"" \
 > +             "\\\^done,source-path=\"${srcdir}/${subdir}:\\\$cdir:\\\$cwd\"" \
 > +             "environment-directory empty-string operation"
 > +
 > +    mi_gdb_test "204-environment-directory" \
 > +             "\\\^done,source-path=\"\\\$cdir:\\\$cwd\"" \
 >               "environment-directory operation"
 >  
 > -    mi_gdb_test "203-environment-directory ${srcdir}/${subdir}" \
 > -             "\\\^done" \
 > -             "environment-directory arg operation"
 >  #exp_internal 0
 >  }
 >  
 > +proc test_cwd_specification {} {
 > +    global mi_gdb_prompt
 > +    global objdir
 > +    global subdir
 > +
 > +    # Change the working directory, then print the current working directory
 > +    # Tests:
 > +    # -environment-cd ${objdir}
 > +    # -environment-pwd
 > +
 > +    mi_gdb_test "205-environment-cd ${objdir}" \
 > +             "\\\^done" \
 > +             "environment-cd arg operation"
 > +
 > +    mi_gdb_test "206-environment-pwd" \
 > +             "\\\^done,cwd=\"${objdir}\"" \
 > +             "environment-pwd operation"
 > +}
 > +
 >  if [test_mi_interpreter_selection] {
 >    test_exec_and_symbol_mi_operatons
 >    test_breakpoints_deletion
 >    test_dir_specification
 > +  test_cwd_specification
 >  }
 >  
 >  mi_gdb_exit