From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1459 invoked by alias); 6 Aug 2010 21:37:49 -0000 Received: (qmail 1441 invoked by uid 22791); 6 Aug 2010 21:37:47 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.44.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 06 Aug 2010 21:37:40 +0000 Received: from wpaz5.hot.corp.google.com (wpaz5.hot.corp.google.com [172.24.198.69]) by smtp-out.google.com with ESMTP id o76Lbc7F000695 for ; Fri, 6 Aug 2010 14:37:38 -0700 Received: from vws7 (vws7.prod.google.com [10.241.21.135]) by wpaz5.hot.corp.google.com with ESMTP id o76LbbYM016458 for ; Fri, 6 Aug 2010 14:37:37 -0700 Received: by vws7 with SMTP id 7so9305186vws.41 for ; Fri, 06 Aug 2010 14:37:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.161.201 with SMTP id s9mr8629400vcx.137.1281130656830; Fri, 06 Aug 2010 14:37:36 -0700 (PDT) Received: by 10.220.201.197 with HTTP; Fri, 6 Aug 2010 14:37:36 -0700 (PDT) In-Reply-To: <20100806102912.GA20604@host1.dyn.jankratochvil.net> References: <83k4o4w34e.fsf@gnu.org> <20100806102912.GA20604@host1.dyn.jankratochvil.net> Date: Fri, 06 Aug 2010 21:37:00 -0000 Message-ID: Subject: Re: [patch] Fix python gdb.execute to not paginate From: Doug Evans To: Jan Kratochvil Cc: Eli Zaretskii , gdb-patches@sourceware.org, David Malcolm Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-08/txt/msg00089.txt.bz2 Thanks. Comments inline. On Fri, Aug 6, 2010 at 3:29 AM, Jan Kratochvil wrote: > On Fri, 06 Aug 2010 02:49:32 +0200, Doug Evans wrote: >> IWBN to bury the implementation details. =A0I.e. move the setting of >> batch_flag into utils.c. =A0Plus calling init_page_info here feels >> wrong. =A0New function in utils.c that performs all the needed changes, > > OK, done. > > >> and have just one make_cleanup_restore_foo routine to switch back? > > Using the benevolence of your '?' mark - not done in this patch. =A0I fin= d the > code is more simple without introducing new closure structure when the > functionality can be built from the existing cleanup stubs: Sure. I wasn't suggesting introducing a new "closure" for this pass (at least as far has how "closure" is currently used in gdb). Rather, I was imagining all of these fiddly implementation details: > + =A0back_to =3D make_cleanup (do_restore_page_info_cleanup, NULL); > + =A0make_cleanup_restore_uinteger (&lines_per_page); > + =A0make_cleanup_restore_uinteger (&chars_per_line); > + > + =A0make_cleanup_restore_integer (&batch_flag); ... being wrapped in functions that hid them. There's a clear conceptual boundary here. > On Fri, 06 Aug 2010 03:22:41 +0200, Doug Evans wrote: >> Actually, I wonder if it makes sense to bury more implementation >> details. =A0Make an API of it, running a gdb command and collecting the >> result as a string. > > OK, done. =A0I would (also) like a normal canned commands $(...) support. > > > On Fri, 06 Aug 2010 10:34:57 +0200, Eli Zaretskii wrote: >> Okay with those changes. > > Used your text. > > > No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu. > > > Thanks, > Jan > > > gdb/ > 2010-08-06 =A0Jan Kratochvil =A0 > > =A0 =A0 =A0 =A0* defs.h (make_cleanup_restore_uinteger, make_cleanup_rest= ore_ui_file) > =A0 =A0 =A0 =A0(set_batch_flag_and_make_cleanup_restore_page_info): New d= eclarations. > =A0 =A0 =A0 =A0* gdbcmd.h (execute_command_to_string): New declaration. > =A0 =A0 =A0 =A0* python/python.c (struct restore_ui_file_closure, restore= _ui_file) > =A0 =A0 =A0 =A0(make_cleanup_restore_ui_file): Move to utils.c > =A0 =A0 =A0 =A0(execute_gdb_command) : Move ... > =A0 =A0 =A0 =A0* top.c (execute_command_to_string): ... here. =A0Call > =A0 =A0 =A0 =A0set_batch_flag_and_make_cleanup_restore_page_info. > =A0 =A0 =A0 =A0* utils.c (make_cleanup_restore_uinteger): New. > =A0 =A0 =A0 =A0(struct restore_ui_file_closure, do_restore_ui_file) > =A0 =A0 =A0 =A0(make_cleanup_restore_ui_file): Move here from python/pyth= on.c. > =A0 =A0 =A0 =A0(init_page_info) > =A0 =A0 =A0 =A0(do_restore_page_info_cleanup) > =A0 =A0 =A0 =A0(set_batch_flag_and_make_cleanup_restore_page_info): New. > > gdb/testsuite/ > 2010-08-06 =A0Jan Kratochvil =A0 > > =A0 =A0 =A0 =A0* gdb.python/python.exp (show height, set height 10) > =A0 =A0 =A0 =A0(verify pagination beforehand, verify pagination beforehan= d: q) > =A0 =A0 =A0 =A0(gdb.execute does not page, verify pagination afterwards) > =A0 =A0 =A0 =A0(verify pagination afterwards: q): New. > > gdb/doc/ > 2010-08-06 =A0Jan Kratochvil =A0 > =A0 =A0 =A0 =A0 =A0 =A0Eli Zaretskii > > =A0 =A0 =A0 =A0* gdb.texinfo (Mode Options) <-batch> > =A0 =A0 =A0 =A0(Basic Python) : Describe setting width and h= eight. > > --- a/gdb/defs.h > +++ b/gdb/defs.h > @@ -351,10 +351,14 @@ struct obstack; > =A0extern struct cleanup *make_cleanup_obstack_free (struct obstack *obst= ack); > > =A0extern struct cleanup *make_cleanup_restore_integer (int *variable); > +extern struct cleanup *make_cleanup_restore_uinteger (unsigned int *vari= able); > > =A0struct target_ops; > =A0extern struct cleanup *make_cleanup_unpush_target (struct target_ops *= ops); > > +extern > + =A0struct cleanup *make_cleanup_restore_ui_file (struct ui_file **varia= ble); > + Is this (and below) done to avoid the 80 char limit? I haven't seen this formatting style used in gdb (having extern on a line by itself). dwarf2-frame.h does this: extern const struct frame_base * dwarf2_frame_base_sniffer (struct frame_info *this_frame); I like it. I'm not sure what the coding style manual says. > =A0extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void = *); > > =A0extern struct cleanup *make_my_cleanup (struct cleanup **, > @@ -386,6 +390,9 @@ extern int yquery (const char *, ...) ATTRIBUTE_PRINT= F (1, 2); > > =A0extern void init_page_info (void); > > +extern > + =A0struct cleanup *set_batch_flag_and_make_cleanup_restore_page_info (v= oid); > + > =A0extern char *gdb_realpath (const char *); > =A0extern char *xfullpath (const char *); > > --- a/gdb/gdbcmd.h > +++ b/gdb/gdbcmd.h > @@ -129,6 +129,7 @@ extern struct cmd_list_element *showchecklist; > =A0extern struct cmd_list_element *save_cmdlist; > > =A0extern void execute_command (char *, int); > +extern char *execute_command_to_string (char *p, int from_tty); > > =A0enum command_control_type execute_control_command (struct command_line= *); > > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -309,33 +310,6 @@ gdbpy_target_wide_charset (PyObject *self, PyObject = *args) > =A0 return PyUnicode_Decode (cset, strlen (cset), host_charset (), NULL); > =A0} > > -struct restore_ui_file_closure > -{ > - =A0struct ui_file **variable; > - =A0struct ui_file *value; > -}; > - > -static void > -restore_ui_file (void *p) > -{ > - =A0struct restore_ui_file_closure *closure =3D p; > - > - =A0*(closure->variable) =3D closure->value; > -} > - > -/* Remember the current value of *VARIABLE and make it restored when > - =A0 the cleanup is run. =A0*/ > -struct cleanup * > -make_cleanup_restore_ui_file (struct ui_file **variable) > -{ > - =A0struct restore_ui_file_closure *c =3D XNEW (struct restore_ui_file_c= losure); > - > - =A0c->variable =3D variable; > - =A0c->value =3D *variable; > - > - =A0return make_cleanup_dtor (restore_ui_file, (void *) c, xfree); > -} > - > =A0/* A Python function which evaluates a string using the gdb CLI. =A0*/ > > =A0static PyObject * > @@ -376,27 +350,15 @@ execute_gdb_command (PyObject *self, PyObject *args= , PyObject *kw) > =A0 =A0 =A0 /* Copy the argument text in case the command modifies it. = =A0*/ > =A0 =A0 =A0 char *copy =3D xstrdup (arg); > =A0 =A0 =A0 struct cleanup *cleanup =3D make_cleanup (xfree, copy); > - =A0 =A0 =A0struct ui_file *str_file =3D NULL; > > =A0 =A0 =A0 if (to_string) > + =A0 =A0 =A0 result =3D execute_command_to_string (copy, from_tty); > + =A0 =A0 =A0else > =A0 =A0 =A0 =A0{ > - =A0 =A0 =A0 =A0 str_file =3D mem_fileopen (); > - > - =A0 =A0 =A0 =A0 make_cleanup_restore_ui_file (&gdb_stdout); > - =A0 =A0 =A0 =A0 make_cleanup_restore_ui_file (&gdb_stderr); > - =A0 =A0 =A0 =A0 make_cleanup_ui_file_delete (str_file); > - > - =A0 =A0 =A0 =A0 gdb_stdout =3D str_file; > - =A0 =A0 =A0 =A0 gdb_stderr =3D str_file; > + =A0 =A0 =A0 =A0 result =3D NULL; > + =A0 =A0 =A0 =A0 execute_command (copy, from_tty); > =A0 =A0 =A0 =A0} > > - =A0 =A0 =A0execute_command (copy, from_tty); > - > - =A0 =A0 =A0if (str_file) > - =A0 =A0 =A0 result =3D ui_file_xstrdup (str_file, NULL); > - =A0 =A0 =A0else > - =A0 =A0 =A0 result =3D NULL; > - > =A0 =A0 =A0 do_cleanups (cleanup); > =A0 =A0 } > =A0 GDB_PY_HANDLE_EXCEPTION (except); > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -458,6 +458,39 @@ execute_command (char *p, int from_tty) > =A0 =A0 } > =A0} > > +/* Run execute_command for P and FROM_TTY. =A0Capture its output into the > + =A0 returned string, do not display it to the screen. =A0BATCH_FLAG wil= l be > + =A0 temporarily set to true. =A0*/ > + > +char * > +execute_command_to_string (char *p, int from_tty) > +{ > + =A0struct ui_file *str_file; > + =A0struct cleanup *cleanup; > + =A0char *retval; > + > + =A0/* GDB_STDOUT should be better already restored during these > + =A0 =A0 restoration callbacks. =A0*/ > + =A0cleanup =3D set_batch_flag_and_make_cleanup_restore_page_info (); > + > + =A0str_file =3D mem_fileopen (); > + > + =A0make_cleanup_restore_ui_file (&gdb_stdout); > + =A0make_cleanup_restore_ui_file (&gdb_stderr); > + =A0make_cleanup_ui_file_delete (str_file); > + > + =A0gdb_stdout =3D str_file; > + =A0gdb_stderr =3D str_file; > + > + =A0execute_command (p, from_tty); > + > + =A0retval =3D ui_file_xstrdup (str_file, NULL); > + > + =A0do_cleanups (cleanup); > + > + =A0return retval; > +} > + > =A0/* Read commands from `instream' and execute them > =A0 =A0until end of file or error reading instream. =A0*/ > > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -352,6 +352,14 @@ make_cleanup_restore_integer (int *variable) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 xfree); > =A0} > > +/* Remember the current value of *VARIABLE and make it restored when the= cleanup > + =A0 is run. =A0*/ The coding standards have this in gdbint.texinfo: "Put a blank line between the block comments preceding function or variable definitions, and the definition itself." though I see an example earlier in that section that doesn't have a blank line (though that example is for which column to put the function name, not spacing). At any rate, I like it and I'd like to start enforcing it for function definitions. > +struct cleanup * > +make_cleanup_restore_uinteger (unsigned int *variable) > +{ > + =A0return make_cleanup_restore_integer ((int *) variable); > +} > + > =A0/* Helper for make_cleanup_unpush_target. =A0*/ > > =A0static void > @@ -370,6 +378,33 @@ make_cleanup_unpush_target (struct target_ops *ops) > =A0 return make_my_cleanup (&cleanup_chain, do_unpush_target, ops); > =A0} > > +struct restore_ui_file_closure > +{ > + =A0struct ui_file **variable; > + =A0struct ui_file *value; > +}; > + > +static void > +do_restore_ui_file (void *p) > +{ > + =A0struct restore_ui_file_closure *closure =3D p; > + > + =A0*(closure->variable) =3D closure->value; > +} > + > +/* Remember the current value of *VARIABLE and make it restored when > + =A0 the cleanup is run. =A0*/ Ditto. Add blank line. > +struct cleanup * > +make_cleanup_restore_ui_file (struct ui_file **variable) > +{ > + =A0struct restore_ui_file_closure *c =3D XNEW (struct restore_ui_file_c= losure); > + > + =A0c->variable =3D variable; > + =A0c->value =3D *variable; > + > + =A0return make_cleanup_dtor (do_restore_ui_file, (void *) c, xfree); > +} > + > =A0struct cleanup * > =A0make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *func= tion, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0void *arg, =A0void (*free_arg) (void *= )) > @@ -2052,6 +2087,12 @@ static int wrap_column; > =A0void > =A0init_page_info (void) > =A0{ > + =A0if (batch_flag) > + =A0 =A0{ > + =A0 =A0 =A0lines_per_page =3D UINT_MAX; > + =A0 =A0 =A0chars_per_line =3D UINT_MAX; > + =A0 =A0} > + =A0else > =A0#if defined(TUI) > =A0 if (!tui_get_command_dimension (&chars_per_line, &lines_per_page)) > =A0#endif > @@ -2096,6 +2137,32 @@ init_page_info (void) > =A0 set_width (); > =A0} > > +static void > +do_restore_page_info_cleanup (void *arg) > +{ > + =A0set_screen_size (); > + =A0set_width (); > +} > + > +/* Temporarily set BATCH_FLAG and the associated unlimited terminal size. > + =A0 Provide cleanup for restoring the original state. =A0*/ > + > +struct cleanup * > +set_batch_flag_and_make_cleanup_restore_page_info (void) > +{ > + =A0struct cleanup *back_to; > + > + =A0back_to =3D make_cleanup (do_restore_page_info_cleanup, NULL); > + =A0make_cleanup_restore_uinteger (&lines_per_page); > + =A0make_cleanup_restore_uinteger (&chars_per_line); > + > + =A0make_cleanup_restore_integer (&batch_flag); > + =A0batch_flag =3D 1; > + =A0init_page_info (); > + > + =A0return back_to; > +} > + > =A0/* Set the screen size based on LINES_PER_PAGE and CHARS_PER_LINE. =A0= */ > > =A0static void > --- a/gdb/testsuite/gdb.python/python.exp > +++ b/gdb/testsuite/gdb.python/python.exp > @@ -87,3 +87,26 @@ gdb_test "python import itertools; print 'IMPOR'+'TED'= " "IMPORTED" "pythonX.Y/li > =A0gdb_test_no_output \ > =A0 =A0 "python x =3D gdb.execute('printf \"%d\", 23', to_string =3D True= )" > =A0gdb_test "python print x" "23" > + > +# Test (no) pagination of the executed command. > +gdb_test "show height" {Number of lines gdb thinks are in a page is unli= mited\.} > +set lines 10 > +gdb_test_no_output "set height $lines" > + > +set test "verify pagination beforehand" > +gdb_test_multiple "python print \"\\n\" * $lines" $test { > + =A0 =A0-re "---Type to continue, or q to quit---$" { > + =A0 =A0 =A0 pass $test > + =A0 =A0} > +} > +gdb_test "q" "Quit" "verify pagination beforehand: q" > + > +gdb_test "python if gdb.execute('python print \"\\\\n\" * $lines', to_st= ring=3DTrue) =3D=3D \"\\n\" * [expr $lines + 1]: print \"yes\"" "yes" "gdb.= execute does not page" > + > +set test "verify pagination afterwards" > +gdb_test_multiple "python print \"\\n\" * $lines" $test { > + =A0 =A0-re "---Type to continue, or q to quit---$" { > + =A0 =A0 =A0 pass $test > + =A0 =A0} > +} > +gdb_test "q" "Quit" "verify pagination afterwards: q" > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -1031,9 +1031,9 @@ Run in batch mode. =A0Exit with status @code{0} aft= er processing all the > =A0command files specified with @samp{-x} (and all commands from > =A0initialization files, if not inhibited with @samp{-n}). =A0Exit with > =A0nonzero status if an error occurs in executing the @value{GDBN} comman= ds > -in the command files. =A0Batch mode also disables pagination; > -@pxref{Screen Size} and acts as if @kbd{set confirm off} were in > -effect (@pxref{Messages/Warnings}). > +in the command files. =A0Batch mode also disables pagination, sets unlim= ited > +terminal width and height @pxref{Screen Size}, and acts as if @kbd{set c= onfirm > +off} were in effect (@pxref{Messages/Warnings}). > > =A0Batch mode may be useful for running @value{GDBN} as a filter, for > =A0example to download and run a program on another computer; in order to > @@ -20484,7 +20484,9 @@ By default, any output produced by @var{command} = is sent to > =A0@value{GDBN}'s standard output. =A0If the @var{to_string} parameter is > =A0@code{True}, then output will be collected by @code{gdb.execute} and > =A0returned as a string. =A0The default is @code{False}, in which case the > -return value is @code{None}. > +return value is @code{None}. =A0If @var{to_string} is @code{True}, the > +@value{GDBN} virtual terminal will be temporarily set to unlimited width > +and height, and its pagination will be disabled; @pxref{Screen Size}. > =A0@end defun > > =A0@findex gdb.breakpoints > Ok with those changes.