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

[Simon Marchi <simark@simark.ca>]

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 ()
>  
>  \f
>  
> +/* 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.

> +}
> +
> +\f
> +
>  /* 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