Re: Use external editor in 'commands' command

Alfredo Ortega
2009-02-19T08:29:10Z

[-- Attachment #1: Type: text/plain, Size: 4300 bytes --]

2009/2/19 Alfredo Ortega <ortegaalfredo@gmail.com>:
> 2009/2/2 Tom Tromey <tromey@redhat.com>:
>>>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:
>>
>> Alfredo> 1) Following the suggestion of Tom Tromey, i made "commands"
>> Alfredo> a prefix command, and now it is "commands edit n".
>>
>> Your patch does this using strncmp -- but gdb already has built-in
>> machinery for prefix commands. See add_prefix_cmd. So, the idea here
>> would be to change _initialize_breakpoint to use add_prefix_cmd,
>> instead of add_com, when creating the "commands" command. Then, you'd
>> have a separate function to implement "commands edit". Maybe this
>> means introducing a helper function to do some of the work, I don't
>> know.
>>
>> Alfredo> This is a much better patch, but also is a much bigger one (I already
>> Alfredo> sent the FSF form that Tom suggested), so surely there are plenty of
>> Alfredo> errors. Corrections are welcomed.
>>
>> A few nits inline.
>>
>> Alfredo> +#define COMMANDS_EDCOMMAND "edit"
>>
>> With the prefix change, you won't need this.
>>
>> Alfredo> + /* Edit commands with external editor */
>>
>> In the GNU style, comments are full sentences that start with a
>> capital letter and end with a period and two spaces. This one is
>> missing the period, but others are incorrect in other ways.
>>
>> Alfredo> + /* discard the "edit" command */
>>
>> E.g., this one...
>>
>> Alfredo> + get_number (&p);
>> Alfredo> + bnum = get_number (&p);
>>
>> Two calls to get_number seems suspect.
>>
>> Alfredo> + /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
>>
>> There's no need for this comment, IMO.
>>
>> Alfredo> + if (!(vitmp = make_temp_file (NULL)))
>>
>> GNU style prohibits assignments in conditionals.
>>
>> Alfredo> + {
>> Alfredo> + error (_("Can't create temporary file for editing."));
>> Alfredo> + return;
>> Alfredo> + }
>>
>> The "error" function never returns. It calls longjmp. So, this
>> return is not needed. This occurs in a few spots.
>>
>> Alfredo> + l = b->commands;
>> Alfredo> + while (l)
>> Alfredo> + {
>> Alfredo> + fsize = 0;
>> Alfredo> + fsize += fwrite (l->line, 1, strlen (l->line), tmpstream);
>>
>> I think you should probably use "print_command_lines" to print the
>> breakpoint commands to a file.
>>
>> Alfredo> + sysret = system (cmdline);
>> Alfredo> + xfree (cmdline);
>> Alfredo> + if (sysret < 0)
>>
>> I think this should also check "sysret" when it is >= 0, and fail if
>> the editor does not exit with status 0.
>>
>> Thanks for working on this,
>> Tom
>>
>
> Sorry for the late answer.
> I refactored the whole patch, is getting better...add_prefix_cmd()
> made me sweat! :) and print_command_lines() is a little tricky to use,
> so maybe i didn't get it right this time.
> Also I send the FSF paperwork and tried to do all your corrections in
> the following patch.
> Thanks to you for teaching and the patience!
>
> Regards,
>
> Alfred
>

Sorry, I forgot the changelog in the last e-mail. I'm resending the patch.

2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
* breakpoint.c (commands_command,_initialize_breakpoint,
edit_command,check_executing_commands, commands_edit_command,
commands_dump_to_file):
Add the 'edit' keyword to the 'commands' command to allow the
use of an external editor to add or modify commands.
Added an utility function to dump breakpoint commands to a file.
Added an utility function to launch an external editor on breakpoint commands.
Joined common checks in commands_command and edit_command.
* utils.c,defs.h (external_editor,initialize_utils):
Added an utility function to return the external text editor of the system.
Added "set external-editor" and "show external-editor" commands to
set/show the external editor variable

2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
* gdb.texinfo, refcard.tex (breakpoint commands, set
external-editor, show external-editor): Added
documentation of the edit option, for editing commands with an
external editor. Also, added a brief description of the "set external-editor"
and "show external-editor" commands.

[-- Attachment #2: commands2.diff --]
[-- Type: text/x-diff, Size: 9932 bytes --]

diff -upr OLD/src/gdb/breakpoint.c NEW/src/gdb/breakpoint.c
--- OLD/src/gdb/breakpoint.c 2009-01-14 03:10:29.000000000 -0200
+++ NEW/src/gdb/breakpoint.c 2009-02-19 06:12:26.000000000 -0200
@@ -45,6 +45,7 @@
#include "completer.h"
#include "gdb.h"
#include "ui-out.h"
+#include "cli-out.h"
#include "cli/cli-script.h"
#include "gdb_assert.h"
#include "block.h"
@@ -585,20 +586,116 @@ condition_command (char *arg, int from_t
error (_("No breakpoint number %d."), bnum);
}

+/* Dumps breakpoint commands to a file. */
static void
-commands_command (char *arg, int from_tty)
+commands_dump_to_file (char *filename, struct breakpoint *b)
+{
+ struct cleanup *cleanups;
+ struct ui_file *old, *outfile = gdb_fopen (filename, "w");
+ cleanups = make_cleanup_ui_file_delete (outfile);
+ old = cli_out_set_stream (uiout, outfile);
+ print_command_lines (uiout, b->commands, 4);
+ cli_out_set_stream (uiout, old);
+ do_cleanups (cleanups);
+
+}
+
+/* Launches the editor on the breakpoint command. */
+char *
+commands_edit_command (int bnum)
{
+ char *vitmp = NULL;
+ char *editor;
struct breakpoint *b;
- char *p;
- int bnum;
- struct command_line *l;
+ char *cmdline = NULL;
+ int sysret;
+ /* Generates the temporary file name. */
+ vitmp = make_temp_file (NULL);
+ if (!vitmp)
+ error (_("Can't create temporary file for editing."));
+ editor = external_editor ();
+ if (!editor)
+ error (_("External editor not found."));
+ ALL_BREAKPOINTS (b) if (b->number == bnum)
+ {
+ if (&b->commands)
+ commands_dump_to_file (vitmp, b);
+ /* Edit the file. */
+ cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);
+ sprintf (cmdline, "%s \"%s\"", editor, vitmp);
+ sysret = system (cmdline);
+ xfree (cmdline);
+ if (sysret < 0)
+ error (_("Can't execute external editor."));
+ if (sysret > 0)
+ error (_("External editor returned non-zero status."));
+ }
+ return vitmp;
+}
+

/* If we allowed this, we would have problems with when to
free the storage, if we change the commands currently
being read from. */

+static void
+check_executing_commands ()
+{
if (executing_breakpoint_commands)
- error (_("Can't use the \"commands\" command among a breakpoint's commands."));
+ error (_
+ ("Can't use the \"commands\" command among a breakpoint's commands."));
+}
+
+/* Like commands_command but using an external editor. */
+static void
+edit_command (char *args, int from_tty)
+{
+ int bnum;
+ char *p;
+ char *vitmp;
+ struct breakpoint *b;
+ struct command_line *l;
+ FILE *tmpstream = NULL;
+
+ check_executing_commands ();
+ p = args;
+ if (!p)
+ error (_("No breakpoint number."));
+ bnum = get_number (&p);
+ if (p && *p)
+ error (_("Unexpected extra arguments following breakpoint number."));
+ /* Launches the editor. */
+ vitmp = commands_edit_command (bnum);
+ if (!vitmp)
+ return;
+
+ ALL_BREAKPOINTS (b) if (b->number == bnum)
+ {
+ /* Redirect instream to the commands temporal file. */
+ tmpstream = instream;
+ instream = fopen (vitmp, "r");
+ l = read_command_lines (NULL, from_tty, 1);
+ free_command_lines (&b->commands);
+ b->commands = l;
+ breakpoints_changed ();
+ observer_notify_breakpoint_modified (b->number);
+ /* Restore instream and erase temporal file. */
+ instream = tmpstream;
+ unlink (vitmp);
+ return;
+ }
+ error (_("No breakpoint number %d."), bnum);
+}
+
+static void
+commands_command (char *arg, int from_tty)
+{
+ struct breakpoint *b;
+ char *p;
+ int bnum;
+ struct command_line *l;
+
+ check_executing_commands();

p = arg;
bnum = get_number (&p);
@@ -8052,6 +8149,12 @@ Multiple breakpoints at one place are pe
\n\
Do \"help breakpoints\" for info on other commands dealing with breakpoints."

+/* List of subcommands for "commands". */
+static struct cmd_list_element *commands_command_list;
+
+/* List of subcommands for "edit". */
+static struct cmd_list_element *edit_command_list;
+
/* List of subcommands for "catch". */
static struct cmd_list_element *catch_cmdlist;

@@ -8100,14 +8203,20 @@ Usage is `ignore N COUNT'."));
if (xdb_commands)
add_com_alias ("bc", "ignore", class_breakpoint, 1);

- add_com ("commands", class_breakpoint, commands_command, _("\
+ add_prefix_cmd ("commands", class_breakpoint, commands_command, _("\
Set commands to be executed when a breakpoint is hit.\n\
Give breakpoint number as argument after \"commands\".\n\
With no argument, the targeted breakpoint is the last one set.\n\
The commands themselves follow starting on the next line.\n\
Type a line containing \"end\" to indicate the end of them.\n\
Give \"silent\" as the first line to make the breakpoint silent;\n\
-then no output is printed when it is hit, except what the commands print."));
+then no output is printed when it is hit, except what the commands print."),
+ &commands_command_list, "commands ", 1, &cmdlist);
+
+ add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\
+Edit the command using the external-editor variable, EDITOR environment\n\
+variable or /bin/ex, in that precedence."),
+ &edit_command_list, "command edit ", 1, &commands_command_list);

add_com ("condition", class_breakpoint, condition_command, _("\
Specify breakpoint number N to break only if COND is true.\n\
diff -upr OLD/src/gdb/defs.h NEW/src/gdb/defs.h
--- OLD/src/gdb/defs.h 2009-01-14 03:10:28.000000000 -0200
+++ NEW/src/gdb/defs.h 2009-01-19 08:57:04.000000000 -0200
@@ -330,6 +330,8 @@ extern int subset_compare (char *, char

extern char *safe_strerror (int);

+extern char *external_editor( void );
+
#define ALL_CLEANUPS ((struct cleanup *)0)

extern void do_cleanups (struct cleanup *);
diff -upr OLD/src/gdb/doc/gdb.texinfo NEW/src/gdb/doc/gdb.texinfo
--- OLD/src/gdb/doc/gdb.texinfo 2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/gdb.texinfo 2009-01-19 09:26:37.000000000 -0200
@@ -3976,6 +3976,8 @@ follow it immediately with @code{end}; t
With no @var{bnum} argument, @code{commands} refers to the last
breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
recently encountered).
+@item commands edit @r{[}@var{bnum}@r{]}
+This spawns an external editor for adding or editing commands. The final @code{end} is not necessary in this case. @xref{Choosing your Editor}.
@end table

Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5384,6 +5386,7 @@ prefer to use Emacs facilities to view s
* List:: Printing source lines
* Specify Location:: How to specify code locations
* Edit:: Editing source files
+* Choosing your Editor:: Specifying your text editor
* Search:: Searching source files
* Source Path:: Specifying source directories
* Machine Code:: Source and machine code
@@ -5587,6 +5590,7 @@ Edit the file containing @var{function}

@end table

+@node Choosing your Editor
@subsection Choosing your Editor
You can customize @value{GDBN} to use any editor you want
@footnote{
@@ -5611,6 +5615,19 @@ or in the @code{csh} shell,
setenv EDITOR /usr/bin/vi
gdb @dots{}
@end smallexample
+Another option is to use the @code{set external-editor} command:
+
+@table @code
+@item set external-editor
+@kindex set external-editor
+This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.
+@end table
+
+@table @code
+@item show external-editor
+@kindex show external-editor
+This command shows the external text editor that internal gdb commands use.
+@end table

@node Search
@section Searching Source Files
diff -upr OLD/src/gdb/doc/refcard.tex NEW/src/gdb/doc/refcard.tex
--- OLD/src/gdb/doc/refcard.tex 2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/refcard.tex 2009-01-19 08:57:19.000000000 -0200
@@ -355,10 +355,9 @@ delete when reached
ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
times\cr
\cr
-commands {\it n}\par
+commands \opt{{\it edit}} {\it n}\par
\qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached \opt{{\tt edit} using external editor}. \opt{{\tt silent} suppresses default display} \cr
end&end of {\it command-list}\cr
\endsec

diff -upr OLD/src/gdb/utils.c NEW/src/gdb/utils.c
--- OLD/src/gdb/utils.c 2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/utils.c 2009-01-19 09:25:19.000000000 -0200
@@ -96,6 +96,10 @@ static void prompt_for_continue (void);
static void set_screen_size (void);
static void set_width (void);

+/* External text editor */
+
+static char *external_editor_command = NULL;
+
/* A flag indicating whether to timestamp debugging messages. */

static int debug_timestamp = 0;
@@ -2694,6 +2698,12 @@ When set, debugging messages will be mar
NULL,
show_debug_timestamp,
&setdebuglist, &showdebuglist);
+ add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
+Set the external text editor that gdb uses."),
+ _("\
+Show the external text editor."), NULL,
+ NULL, NULL,
+ &setlist, &showlist);
}

/* Machine specific function to handle SIGWINCH signal. */
@@ -3443,3 +3453,16 @@ gdb_buildargv (const char *s)
nomem (0);
return argv;
}
+
+/* Returns the external editor */
+char *
+external_editor (void)
+{
+ char *editor;
+ if (external_editor_command)
+ return external_editor_command;
+ if ((editor = (char *) getenv ("EDITOR")) == NULL)
+ editor = "/bin/ex";
+ return editor;
+
+}