[2/3] Add -P command line switch for executing Python scripts

  This commit introduces a new command line switch to GDB, a -P /
--python switch which is used for executing a python script.
Encountering -P curtails normal argument processing by GDB; any
remaining arguments (after the script name) are passed to the
script.

This is work that was originally written as part of the Archer
project.  The original/primary author is Tom Tromey, but it has
been maintained for the Fedora project by Jan Kratochvil, Sergio
Durigan Junior, and perhaps others too.

In its original form, and even in the form found within the Fedora
sources, the code implementing the -P switch had several properties
which I found to be surprising:

1) After script execution, exit() was called which (obviously)
   caused GDB to exit.
2) The printing of GDB's banner (copyright info, bug reporting
   instructions, and help instructions) was suppressed.
3) Due to the exit() noted above, GDB's CLI was not (automatically)
   invoked.  If the CLI was desired, it could be run from the Python
   script via use of the gdb.cli method, which was added as part of
   that work.

I've changed things so that exit() is no longer called.  GDB's CLI
will be invoked after script execution.  Also, GDB's banner will be
printed (or not) as normal.  I.e, the banner will be printed unless
the -q switch is specified.

If the script doesn't want the CLI for some reason, it can explicitly
call exit().  It may be the case that the script would be better off
calling a (yet to be written) gdb.exit() method for doing this
instead.  Such a method could make sure that GDB shuts down properly.

gdb/ChangeLog:

	* main.c (python/python.h): Include.
	(captured_main_1): Add option processing and other support for -P
	switch.
	(captured_main): Add help messages for -P.
	* python/python.h (run_python_script): Declare.
	* python/python.c (run_python_script): New function.
---
 gdb/main.c          | 48 ++++++++++++++++++++++++++++++++++++++------
 gdb/python/python.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/python/python.h |  2 ++
 3 files changed, 93 insertions(+), 6 deletions(-)
  

Hi Kevin,

That sounds like a very useful feature.  See below for comments.

On 2019-07-21 7:54 p.m., Kevin Buettner wrote:
> This commit introduces a new command line switch to GDB, a -P /
> --python switch which is used for executing a python script.
> Encountering -P curtails normal argument processing by GDB; any
> remaining arguments (after the script name) are passed to the
> script.
> 
> This is work that was originally written as part of the Archer
> project.  The original/primary author is Tom Tromey, but it has
> been maintained for the Fedora project by Jan Kratochvil, Sergio
> Durigan Junior, and perhaps others too.
> 
> In its original form, and even in the form found within the Fedora
> sources, the code implementing the -P switch had several properties
> which I found to be surprising:
> 
> 1) After script execution, exit() was called which (obviously)
>    caused GDB to exit.
> 2) The printing of GDB's banner (copyright info, bug reporting
>    instructions, and help instructions) was suppressed.
> 3) Due to the exit() noted above, GDB's CLI was not (automatically)
>    invoked.  If the CLI was desired, it could be run from the Python
>    script via use of the gdb.cli method, which was added as part of
>    that work.
> 
> I've changed things so that exit() is no longer called.  GDB's CLI
> will be invoked after script execution.  Also, GDB's banner will be
> printed (or not) as normal.  I.e, the banner will be printed unless
> the -q switch is specified.
> 
> If the script doesn't want the CLI for some reason, it can explicitly
> call exit().  It may be the case that the script would be better off
> calling a (yet to be written) gdb.exit() method for doing this
> instead.  Such a method could make sure that GDB shuts down properly.

1. Since it's closely related to "-x", it would be nice if it was possible
to use -P with --batch, just like we can with -x.  For example, with this
Python script:

import sys
print('args:', sys.argv)

$ ./gdb --data-directory=data-directory --batch -x test.py
args: ['']
$ ./gdb --data-directory=data-directory --batch -P test.py
$

If you want your execution to be completely unattended, it sounds more fragile
to rely on calling exit at the end of your script.  If there's an error and it
ends with an exception, your exit won't be called.  So I'd prefer if --batch
worked for this case.

2. When using "--batch -x" with a gdb script, gdb's exit code will reflect if we
sourced the script successfully.  Unfortunately, it doesn't work the same way with
"--batch -x" and a Python script.  I think it would be useful if it did (a Python
script would be considered as failing if it ends by raising an exception).  If we
make that work, then it would be nice for "--batch -P" to work the same way.  To
be clear, since it's not an existing feature even for -x, I am not asking you to
implement that as part of this patch.

3. When you run a program under the standalone Python interpreter, sys.argv[0]
is the script name.  Perhaps people will expect it to be the same here?  It
would be confusing if for GDB Python scripts, it's different.

Also, the setting of sys.argv remains for the rest of the Python interpreter's
lifetime:

$ ./gdb --data-directory=data-directory -q -P test.py Hello
args: ['Hello']
(gdb) pi
>>> import sys
>>> sys.argv
['Hello']

I think it's not very likely that people's script would rely on the fact that
sys.argv was initially empty, but if we could reset sys.argv to [''] when we
are done executing that Python script, it would reduce the chances that we break
something.

> gdb/ChangeLog:
> 
> 	* main.c (python/python.h): Include.
> 	(captured_main_1): Add option processing and other support for -P
> 	switch.
> 	(captured_main): Add help messages for -P.
> 	* python/python.h (run_python_script): Declare.
> 	* python/python.c (run_python_script): New function.
> ---
>  gdb/main.c          | 48 ++++++++++++++++++++++++++++++++++++++------
>  gdb/python/python.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python.h |  2 ++
>  3 files changed, 93 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/main.c b/gdb/main.c
> index 678c413021..bc8238e3ce 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -33,6 +33,7 @@
>  
>  #include "interps.h"
>  #include "main.h"
> +#include "python/python.h"
>  #include "source.h"
>  #include "cli/cli-cmds.h"
>  #include "objfiles.h"
> @@ -440,7 +441,7 @@ struct cmdarg
>  };
>  
>  static void
> -captured_main_1 (struct captured_main_args *context)
> +captured_main_1 (struct captured_main_args *context, bool &python_script)
>  {
>    int argc = context->argc;
>    char **argv = context->argv;
> @@ -658,10 +659,14 @@ captured_main_1 (struct captured_main_args *context)
>        {"args", no_argument, &set_args, 1},
>        {"l", required_argument, 0, 'l'},
>        {"return-child-result", no_argument, &return_child_result, 1},
> +#if HAVE_PYTHON
> +      {"python", no_argument, 0, 'P'},
> +      {"P", no_argument, 0, 'P'},
> +#endif
>        {0, no_argument, 0, 0}
>      };
>  
> -    while (1)
> +    while (!python_script)
>        {
>  	int option_index;
> > @@ -679,6 +684,9 @@ captured_main_1 (struct captured_main_args *context)
>  	  case 0:
>  	    /* Long option that just sets a flag.  */
>  	    break;
> +	  case 'P':
> +	    python_script = true;
> +	    break;
>  	  case OPT_SE:
>  	    symarg = optarg;
>  	    execarg = optarg;
> @@ -858,7 +866,20 @@ captured_main_1 (struct captured_main_args *context)
>  
>    /* Now that gdb_init has created the initial inferior, we're in
>       position to set args for that inferior.  */
> -  if (set_args)
> +  if (python_script)
> +    {
> +      /* The first argument is a python script to evaluate, and
> +	 subsequent arguments are passed to the script for
> +	 processing there.  */
> +      if (optind >= argc)
> +	{
> +	  fprintf_unfiltered (gdb_stderr,
> +			      _("%s: Python script file name required\n"),
> +			      argv[0]);
> +	  exit (1);
> +	}
> +    }
> +  else if (set_args)
>      {
>        /* The remaining options are the command-line options for the
>  	 inferior.  The first one is the sym/exec file, and the rest
> @@ -1157,9 +1178,14 @@ static void
>  captured_main (void *data)
>  {
>    struct captured_main_args *context = (struct captured_main_args *) data;
> +  bool python_script = false;
>  
> -  captured_main_1 (context);
> +  captured_main_1 (context, python_script);

I know it's a heated debate (well, not really but still), but I would prefer the
reference to the variable was passed as a pointer, &python_script.  I really
find it confusing to have it this way, since it looks like you are just constantly
passing false to the function (so the variable looks unnecessary).

>  
> +#if HAVE_PYTHON
> +  if (python_script)
> +    run_python_script (context->argc - optind, &context->argv[optind]);
> +#endif
>    /* NOTE: cagney/1999-11-07: There is probably no reason for not
>       moving this loop and the code found in captured_command_loop()
>       into the command_loop() proper.  The main thing holding back that
> @@ -1215,9 +1241,12 @@ print_gdb_help (struct ui_file *stream)
>    fputs_unfiltered (_("\
>  This is the GNU debugger.  Usage:\n\n\
>      gdb [options] [executable-file [core-file or process-id]]\n\
> -    gdb [options] --args executable-file [inferior-arguments ...]\n\n\
> -"), stream);
> +    gdb [options] --args executable-file [inferior-arguments ...]\n"), stream);
> +#if HAVE_PYTHON
>    fputs_unfiltered (_("\
> +    gdb [options] [--python|-P] script-file [script-arguments ...]\n"), stream);
> +#endif
> +  fputs_unfiltered (_("\n\
>  Selection of debuggee and its files:\n\n\
>    --args             Arguments after executable-file are passed to inferior\n\
>    --core=COREFILE    Analyze the core dump COREFILE.\n\
> @@ -1260,6 +1289,13 @@ Output and user interface control:\n\n\
>  #endif
>    fputs_unfiltered (_("\
>    --dbx              DBX compatibility mode.\n\
> +"), stream);
> +#if HAVE_PYTHON
> +  fputs_unfiltered (_("\
> +  --python, -P       Following argument is Python script file; remaining\n\
> +                     arguments are passed to script.\n"), stream);
> +#endif
> +  fputs_unfiltered (_("\
>    -q, --quiet, --silent\n\
>                       Do not print version number on startup.\n\n\
>  "), stream);
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 96bee7c3b0..7bd4d1684f 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1276,6 +1276,55 @@ gdbpy_print_stack_or_quit ()
>  
>  
>  
> +/* Set up the Python argument vector and evaluate a script.  This is
> +   used to implement 'gdb -P'.  */
> +
> +void
> +run_python_script (int argc, char **argv)

Even though the surrounding code is not like that, I would suggest following
our current conventions, putting /* See python.h.  */ here and put the doc
in the .h.

> +{
> +  if (!gdb_python_initialized)
> +    return;
> +
> +  gdbpy_enter enter_py (get_current_arch (), current_language);
> +
> +#if PYTHON_ABI_VERSION < 3

We is the IS_PY3K macro throughout.

> +  PySys_SetArgv (argc - 1, argv + 1);
> +#else
> +  {
> +    wchar_t **wargv = (wchar_t **) alloca (sizeof (*wargv) * (argc + 1));

I'd suggest using XALLOCAVEC:

  XALLOCAVEC (wchar_t *, argc + 1)

> +    int i;

You can inline this declaration in the for loop.

> +
> +    for (i = 1; i < argc; i++)
> +      {
> +	size_t len = mbstowcs (NULL, argv[i], 0);
> +
> +	if (len == (size_t) -1)
> +	  {
> +	    fprintf (stderr, "Invalid multibyte argument #%d \"%s\"\n",
> +		     i, argv[i]);
> +	    exit (1);
> +	  }

I think it would be more gdb-ish to call error () instead of plain exiting.

> +	wargv[i] = (wchar_t *) alloca (sizeof (**wargv) * (len + 1));

Suggest using XALLOCAVEC.  Or even dynamic allocation to be safer, given that
args can be actually quite long for alloca.

> +	size_t len2 = mbstowcs (wargv[i], argv[i], len + 1);
> +	assert (len2 == len);
> +      }
> +    wargv[argc] = NULL;
> +    PySys_SetArgv (argc - 1, wargv + 1);
> +  }
> +#endif
> +
> +  FILE *input = fopen (argv[0], "r");

Do we want to use gdb_fopen_cloexec?

> +  if (! input)

if (input == nullptr)

> +    {
> +      fprintf (stderr, "could not open %s: %s\n", argv[0], strerror (errno));
> +      exit (1);

error () instead of exiting?

> +    }> +  PyRun_SimpleFile (input, argv[0]);
> +  fclose (input);

If using gdb_fopen_cloexec, the file will get closed automatically as we leave the
scope, reducing chances that we leak an open file.

> +}
> +
> +
> +
>  /* Return a sequence holding all the Progspaces.  */
>  
>  static PyObject *
> diff --git a/gdb/python/python.h b/gdb/python/python.h
> index 10cd90d00e..2af0b2934d 100644
> --- a/gdb/python/python.h
> +++ b/gdb/python/python.h
> @@ -28,4 +28,6 @@ extern const struct extension_language_defn extension_language_python;
>  /* Command element for the 'python' command.  */
>  extern cmd_list_element *python_cmd_element;
>  
> +extern void run_python_script (int argc, char **argv);
> +
>  #endif /* PYTHON_PYTHON_H */
> 

Simon
  

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> This commit introduces a new command line switch to GDB, a -P /
Kevin> --python switch which is used for executing a python script.
Kevin> Encountering -P curtails normal argument processing by GDB; any
Kevin> remaining arguments (after the script name) are passed to the
Kevin> script.

Kevin> 1) After script execution, exit() was called which (obviously)
Kevin>    caused GDB to exit.
Kevin> 2) The printing of GDB's banner (copyright info, bug reporting
Kevin>    instructions, and help instructions) was suppressed.
Kevin> 3) Due to the exit() noted above, GDB's CLI was not (automatically)
Kevin>    invoked.  If the CLI was desired, it could be run from the Python
Kevin>    script via use of the gdb.cli method, which was added as part of
Kevin>    that work.

Kevin> I've changed things so that exit() is no longer called.  GDB's CLI
Kevin> will be invoked after script execution.  Also, GDB's banner will be
Kevin> printed (or not) as normal.  I.e, the banner will be printed unless
Kevin> the -q switch is specified.

Kevin> If the script doesn't want the CLI for some reason, it can explicitly
Kevin> call exit().  It may be the case that the script would be better off
Kevin> calling a (yet to be written) gdb.exit() method for doing this
Kevin> instead.  Such a method could make sure that GDB shuts down properly.

The intent of -P was to provide a way to run gdb in a "Python script"
mode.  I think this changes defeat this goal.

For (2), printing the banner is just not great for an interpreter.  The
default for interpreters should be silence, with the script being
interpreted choosing what to emit.  "-q" isn't sufficient because it is
common for #! handling to only allow a single argument.

I think if -P is going to just mean "start gdb, but interpret some
Python script at startup", then it might as well not exist -- an
invocation like '-ex "source blah.py"' is just as good, and can be used
on older gdbs as well.

(1) and (3) seem linked.  Here the idea was to have a way to script gdb
where the command line was completely optional and under control of the
script.

Maybe -P isn't the right vehicle for this.  Later on, I was looking at
having a "Python" gdb interpreter -- not to actually interpret gdb
input, but to at least control gdb output.  And, I looked into compiling
gdb PIE so that one could just "import gdb" from an ordinary Python
interpreter.

thanks,
Tom
  

On Tue, 23 Jul 2019 14:25:06 -0600
Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:  
> 
> Kevin> This commit introduces a new command line switch to GDB, a -P /
> Kevin> --python switch which is used for executing a python script.
> Kevin> Encountering -P curtails normal argument processing by GDB; any
> Kevin> remaining arguments (after the script name) are passed to the
> Kevin> script.  
> 
> Kevin> 1) After script execution, exit() was called which (obviously)
> Kevin>    caused GDB to exit.
> Kevin> 2) The printing of GDB's banner (copyright info, bug reporting
> Kevin>    instructions, and help instructions) was suppressed.
> Kevin> 3) Due to the exit() noted above, GDB's CLI was not (automatically)
> Kevin>    invoked.  If the CLI was desired, it could be run from the Python
> Kevin>    script via use of the gdb.cli method, which was added as part of
> Kevin>    that work.  
> 
> Kevin> I've changed things so that exit() is no longer called.  GDB's CLI
> Kevin> will be invoked after script execution.  Also, GDB's banner will be
> Kevin> printed (or not) as normal.  I.e, the banner will be printed unless
> Kevin> the -q switch is specified.  
> 
> Kevin> If the script doesn't want the CLI for some reason, it can explicitly
> Kevin> call exit().  It may be the case that the script would be better off
> Kevin> calling a (yet to be written) gdb.exit() method for doing this
> Kevin> instead.  Such a method could make sure that GDB shuts down properly.  
> 
> The intent of -P was to provide a way to run gdb in a "Python script"
> mode.  I think this changes defeat this goal.
> 
> For (2), printing the banner is just not great for an interpreter.  The
> default for interpreters should be silence, with the script being
> interpreted choosing what to emit.  "-q" isn't sufficient because it is
> common for #! handling to only allow a single argument.
> 
> I think if -P is going to just mean "start gdb, but interpret some
> Python script at startup", then it might as well not exist -- an
> invocation like '-ex "source blah.py"' is just as good, and can be used
> on older gdbs as well.
> 
> (1) and (3) seem linked.  Here the idea was to have a way to script gdb
> where the command line was completely optional and under control of the
> script.
> 
> Maybe -P isn't the right vehicle for this.  Later on, I was looking at
> having a "Python" gdb interpreter -- not to actually interpret gdb
> input, but to at least control gdb output.  And, I looked into compiling
> gdb PIE so that one could just "import gdb" from an ordinary Python
> interpreter.

Thanks for that critique / explanation.

I'm going to mull this over some more...

Kevin
  

On 7/23/19 9:25 PM, Tom Tromey wrote:
>>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:
> 
> Kevin> This commit introduces a new command line switch to GDB, a -P /
> Kevin> --python switch which is used for executing a python script.
> Kevin> Encountering -P curtails normal argument processing by GDB; any
> Kevin> remaining arguments (after the script name) are passed to the
> Kevin> script.
> 
> Kevin> 1) After script execution, exit() was called which (obviously)
> Kevin>    caused GDB to exit.
> Kevin> 2) The printing of GDB's banner (copyright info, bug reporting
> Kevin>    instructions, and help instructions) was suppressed.
> Kevin> 3) Due to the exit() noted above, GDB's CLI was not (automatically)
> Kevin>    invoked.  If the CLI was desired, it could be run from the Python
> Kevin>    script via use of the gdb.cli method, which was added as part of
> Kevin>    that work.
> 
> Kevin> I've changed things so that exit() is no longer called.  GDB's CLI
> Kevin> will be invoked after script execution.  Also, GDB's banner will be
> Kevin> printed (or not) as normal.  I.e, the banner will be printed unless
> Kevin> the -q switch is specified.
> 
> Kevin> If the script doesn't want the CLI for some reason, it can explicitly
> Kevin> call exit().  It may be the case that the script would be better off
> Kevin> calling a (yet to be written) gdb.exit() method for doing this
> Kevin> instead.  Such a method could make sure that GDB shuts down properly.
> 
> The intent of -P was to provide a way to run gdb in a "Python script"
> mode.  I think this changes defeat this goal.
> 
> For (2), printing the banner is just not great for an interpreter.  The
> default for interpreters should be silence, with the script being
> interpreted choosing what to emit.  "-q" isn't sufficient because it is
> common for #! handling to only allow a single argument.

I'm probably being dense, but I don't really understand this remark.
Can you expand on what does
 "#! handling to only allow a single argument."
mean?

> 
> I think if -P is going to just mean "start gdb, but interpret some
> Python script at startup", then it might as well not exist -- an
> invocation like '-ex "source blah.py"' is just as good, and can be used
> on older gdbs as well.

A selling point that I recalled about the patch when we started
discussing it internally was that you can pass arguments directly to
the script from the command line, like:

 $ gdb -P blah.py py-arg1 py-arg2

I wonder if people find that feature itself useful, regardless of
the original motivation for the patch?

> (1) and (3) seem linked.  Here the idea was to have a way to script gdb
> where the command line was completely optional and under control of the
> script.
> 
> Maybe -P isn't the right vehicle for this.  Later on, I was looking at
> having a "Python" gdb interpreter -- not to actually interpret gdb
> input, but to at least control gdb output.  And, I looked into compiling
> gdb PIE so that one could just "import gdb" from an ordinary Python
> interpreter.

FYI, and FWIW, we ended up dropping this patch from Fedora GDB.

Thanks,
Pedro Alves