Re: [RFC][patch 1/9] initial Python support

Daniel Jacobowitz
2008-05-28T20:59:48Z

Looks generally good.

For the general syntax: Should we allow $() to always mean Python, or
are there other useful potential scripting interfaces or environment
variable access or anything, in which case we want $(python ) or $(P )
or $python( )? There are only so many nice grouping specifiers. $<>
is not so good but we still have $[] and ${}, plus $env(). I don't
remember if we discussed this before, but we may have.

On Tue, Apr 29, 2008 at 12:52:13PM -0300, Thiago Jung Bauermann wrote:
> There are two caveats with the configure.ac changes: one is that they
> trigger a bug in autoconf 2.59 when library names contain a dot
> (e.g., libpython2.5). I regenerated the configure script using
> autoconf 2.61, but the proper way to fix this at the moment (since we
> must use version 2.59) is to backport the fix using the
> config/override.m4 mechanism. I didn't look into that yet.

We've discussed already the configure options - I'd prefer to avoid
this issue and only have --with-python and maybe
--with-python-lib/--with-python-include. Parallel to the top level
gmp/mpfr handling, for instance.

I didn't know about python-config. Is that new? Could we specify a
path to python-config instead of messing with cflags/ldflags directly?

> The other caveat is that I had trouble with gnulib configure script
> when using this patch. I didn't investigate the issue yet, and to test
> this code I removed gnulib from the compilation. Maybe I didn't
> regenerate the configure scripts properly.

And we sorted this out on IRC.

The special_processing argument to read_next_line / read_command_lines
is not commented and the name doesn't say much of anything about what
it's for. It looks like this changes the behavior of document versus
define, probably deliberately if it does what I think it does. So
this would be a good patch to split out separately. In general, any
bits that can go in separately should; it makes the whole pile easier
to manage :-)

> Index: tromey.git/gdb/c-exp.y
> ===================================================================
> --- tromey.git.orig/gdb/c-exp.y 2008-04-29 10:57:39.000000000 -0300
> +++ tromey.git/gdb/c-exp.y 2008-04-29 10:58:06.000000000 -0300
> @@ -1636,6 +1636,31 @@ yylex ()
> /* We must have come across a bad character (e.g. ';'). */
> error ("Invalid character '%c' in expression.", c);
>
> + if (c == '$' && tokstart[1] == '(')
> + {
> + /* Convenience function call. */
> + int cparen_depth = 0;
> + int i;
> + for (i = 1; tokstart[i]; ++i)
> + {
> + if (tokstart[i] == '(')
> + ++cparen_depth;
> + else if (tokstart[i] == ')')
> + {
> + if (--cparen_depth == 0)
> + break;
> + }
> + }
> + if (cparen_depth != 0)
> + error ("Unmatched parentheses in convenience function invocation.");
> + yylval.sval.ptr = &tokstart[2];
> + yylval.sval.length = i - 2;
> + write_dollar_funcall (yylval.sval);
> + lexptr = &tokstart[i + 1];
> + /* FIXME: not exactly right. */
> + return VARIABLE;
> + }
> +

VARIABLE seems fine. The parenthesis handling is a little hokey
because we do not lex the body of the scripting expression; you can't
use ')' or '(' unmatched in the expression... I'm not sure we can do
better.

> Index: tromey.git/gdb/cli/cli-script.c
> ===================================================================
> --- tromey.git.orig/gdb/cli/cli-script.c 2008-04-29 10:57:39.000000000 -0300
> +++ tromey.git/gdb/cli/cli-script.c 2008-04-29 11:03:58.000000000 -0300
> @@ -34,6 +34,10 @@
> #include "cli/cli-script.h"
> #include "gdb_assert.h"
>
> +#ifdef HAVE_PYTHON
> +#include "python/python.h"
> +#endif
> +

There are too many #ifdef's in the rest of GDB in this patch, IMO.
Can python/python.h be included unconditionally, not include
python-internal.h, and have the exposed interfaces error() when Python
support is not compiled in? That's the approach I took for
xml-support.h.

Similarly we should recognize python_control regardless of
HAVE_PYTHON. That way we can parse a .gdbinit which defines Python
commands and give a sensible error only if they are invoked.

> +#ifdef HAVE_PYTHON
> + {
> + eval_python_from_control_command (cmd);
> + ret = simple_control;
> + break;
> + }
> +#else
> + warning (_("Python scripting is not supported in this copy of GDB."));
> + break;
> +#endif

So, e.g., this would be error() but it would be pushed into
eval_python_from_control_command.

> + /* FIXME: handling noside == EVAL_AVOID_SIDE_EFFECTS? */
> + if (noside == EVAL_SKIP)
> + goto nosideret;
> + return value_of_python (&exp->elts[pc + 2].string, tem);

I think either a void value, or an error, for EVAL_AVOID_SIDE_EFFECTS.
If we need more we can return to it later.

> +/* Use this after a TRY_EXCEPT to throw the appropriate Python
> + exception. FIXME: throw different errors depending on
> + Exception.error? */
> +#define GDB_PY_HANDLE_EXCEPTION(Exception)

Yes, at some point it seems like we will want some Python exceptions
corresponding to memory read errors and so forth... no need for that
now, though.

In general I would prefer we not commit any FIXMEs. We can decide now
what the right thing to do is and if there is room for future
enhancement, that's not a FIXME any more :-)

> +
> +void
> +demand_python ()

Most functions need a comment. Also, please (void) instead of ().

> +static char *
> +compute_python_string (struct command_line *l)
> +{
> + char *script = NULL;
> + for (;l; l = l->next)
> + {
> + /* FIXME: inefficient. */
> + if (script)
> + {
> + char *save = script;
> + script = concat (script, l->line, "\n", NULL);
> + xfree (save);
> + }
> + else
> + script = concat (l->line, "\n", NULL);
> + }
> + return script;
> +}

So, fix it... :-)

> +
> +void
> +eval_python_from_control_command (struct command_line *cmd)
> +{
> + char *script;
> +
> + if (cmd->body_count != 1)
> + error (_("Invalid \"python\" block structure."));
> +
> + demand_python ();
> +
> + script = compute_python_string (cmd->body_list[0]);
> + PyRun_SimpleString (script);
> + xfree (script);
> + if (PyErr_Occurred ())
> + {
> + PyErr_Print ();
> + error ("error while executing Python code");
> + }
> +}

_() around arguments to error. Also, in general PyErr_Print is
probably dumping to stderr? Output should go through GDB's error
mechanisms, and in this case probably be part of the argument to
error. Can you get it to return an error string instead?

python_command can share the code in execute_control_command; see
if_command for a similar case.

> + return PyString_Decode (str, strlen (str), host_charset (),
> + NULL /* FIXME */);

Yes, do fix it :-)

> + case var_integer:
> + case var_zinteger:
> + return PyLong_FromLong (* (int *) cmd->var);
> +
> + case var_uinteger:
> + return PyLong_FromUnsignedLong (* (unsigned int *) cmd->var);

uinteger and integer both have sentinel values; you used None for auto
booleans so it is probably appropriate here too.

> +static PyObject *
> +execute_gdb_command (PyObject *self, PyObject *args)
> +{
> + struct cmd_list_element *alias, *prefix, *cmd;
> + char *arg, *newarg;
> + volatile struct gdb_exception except;
> +
> + if (! PyArg_ParseTuple (args, "s", &arg))
> + return NULL;
> +
> + TRY_CATCH (except, RETURN_MASK_ALL)
> + {
> + execute_command (arg, 0);
> + }
> + GDB_PY_HANDLE_EXCEPTION (except);
> +
> + /* gdbtk does bpstat_do_actions here... */

Please figure out if we need to or not. It looks like we do; running
the command "continue" to a breakpoint with actions will test it.

Speaking of which... a testsuite? :-)

> + result = PyRun_String (expr, Py_eval_input, global, NULL);
> + /* FIXME: do we need a decref here? */
> + if (PyErr_Occurred ())
> + {
> + /* FIXME -- probably should not do this. */
> + PyErr_Print ();
> + error ("python error while evaluating expression");
> + }

Fix? And various other things in this function.

> + add_com ("python", class_obscure, python_command, _("\
> +Blah, blah, blah."));

...

> +#include "defs.h"
> +#include "value.h"

No need for defs.h in a header; it's always included already and every
source file should include it explicitly.

> Index: tromey.git/gdb/value.h
> ===================================================================
> --- tromey.git.orig/gdb/value.h 2008-04-29 10:57:39.000000000 -0300
> +++ tromey.git/gdb/value.h 2008-04-29 10:58:06.000000000 -0300
> @@ -434,6 +434,8 @@ extern LONGEST parse_and_eval_long (char
>
> extern struct value *access_value_history (int num);
>
> +extern struct value *value_of_python (char *exp, int length);
> +
> extern struct value *value_of_internalvar (struct internalvar *var);
>
> extern void set_internalvar (struct internalvar *var, struct value *val);

All considered, probably belongs in python.h.

--
Daniel Jacobowitz
CodeSourcery