Re: [PATCH v1 02/13] script language API for GDB: scripting.[ch]

[Phil Muldoon <pmuldoon at redhat dot com>]

On 06/12/13 05:52, xdje42@gmail.com wrote:

> +struct script_language_defn
> +{
> +  /* Enum of the scripting language.  */
> +  enum script_language language;
> +
> +  /* The name of the scripting language, lowercase.  E.g., python.  */
> +  const char *name;
> +
> +  /* The capitalized name of the scripting language.  E.g., Python.  */
> +  const char *capitalized_name;

Why do we need a capitalized name and a lowercase name?  I am sure
there is a purpose in mind, but I could not see in the patch the
reason for both.

> +  /* The file suffix for this scripting language.  E.g., ".py".  */
> +  const char *suffix;

I am undecided if this should be an array.  I have seen .python
(though .py is far more common).  What do you think?

> +/* The interface for making calls from GDB to an external scripting
> +   language.  This is for non-script-loading related functionality,
> +   like pretty-printing, etc.  The reason these are separated out as
> +   GDB's own scripting language makes use of script_language_script_opts,
> +   but it makes no use of these.  There is no (current) intention to split
> +   script_language_ops up any further.  */

> +  /* Called before printing a type.  */
> +  void (*start_type_printers) (const struct script_language_defn *,
> +			       struct script_type_printers *);
> +
> +  /* Try to pretty-print TYPE.  If successful the pretty-printed type is
> +     returned.  Otherwise NULL is returned.
> +     This function has a bit of a funny name, since it actually applies
> +     recognizers, but this seemed clearer given the start_type_printers
> +     and free_type_printers functions.  */
> +  char *(*apply_type_printers) (const struct script_language_defn *,
> +				const struct script_type_printers *,
> +				struct type *);
> +
> +  /* Called after a type has been printed to give the type pretty-printer
> +     mechanism an opportunity to clean up.  */
> +  void (*free_type_printers) (const struct script_language_defn *,
> +			      struct script_type_printers *);
> +
> +  /* Try to pretty-print a value of type TYPE located at VALADDR
> +     + EMBEDDED_OFFSET, which came from the inferior at address ADDRESS
> +     + EMBEDDED_OFFSET, onto stdio stream STREAM according to OPTIONS.
> +     VAL is the whole object that came from ADDRESS.  VALADDR must point to
> +     the head of VAL's contents buffer.
> +     Returns non-zero if the value was successfully pretty-printed.  */
> +  int (*apply_val_pretty_printer)
> +    (const struct script_language_defn *,
> +     struct type *type, const gdb_byte *valaddr,
> +     int embedded_offset, CORE_ADDR address,
> +     struct ui_file *stream, int recurse,
> +     const struct value *val, const struct value_print_options *options,
> +     const struct language_defn *language);
> +
> +  /* GDB access to the "frame filter" feature.
> +     FRAME is the source frame to start frame-filter invocation.  FLAGS is an
> +     integer holding the flags for printing.  The following elements of
> +     the FRAME_FILTER_FLAGS enum denotes the make-up of FLAGS:
> +     PRINT_LEVEL is a flag indicating whether to print the frame's
> +     relative level in the output.  PRINT_FRAME_INFO is a flag that
> +     indicates whether this function should print the frame
> +     information, PRINT_ARGS is a flag that indicates whether to print
> +     frame arguments, and PRINT_LOCALS, likewise, with frame local
> +     variables.  ARGS_TYPE is an enumerator describing the argument
> +     format, OUT is the output stream to print.  FRAME_LOW is the
> +     beginning of the slice of frames to print, and FRAME_HIGH is the
> +     upper limit of the frames to count.  Returns SCR_BT_ERROR on error,
> +     or SCR_BT_COMPLETED on success.  */
> +  enum script_bt_status (*apply_frame_filter)
> +    (const struct script_language_defn *,
> +     struct frame_info *frame, int flags, enum script_frame_args args_type,
> +     struct ui_out *out, int frame_low, int frame_high);
> +
> +  /* Update values held by the scripting language when OBJFILE is discarded.
> +     New global types must be created for every such value, which must then be
> +     updated to use the new types.
> +     The function typically just iterates over all appropriate values and
> +     calls preserve_one_value for each one.
> +     COPIED_TYPES is used to prevent cycles / duplicates and is passed to
> +     preserve_one_value.  */
> +  void (*preserve_values) (const struct script_language_defn *,
> +			   struct objfile *objfile, htab_t copied_types);
> +
> +  /* Return non-zero if there is a stop condition for the breakpoint.  */
> +  int (*breakpoint_has_cond) (const struct script_language_defn *,
> +			      struct breakpoint *);

These features seem arbitrarily divided? Why no extended prompt
support here for example? I would have thought that callback would
have been something all of the languages would want access too.  I
guess I don't understand why some features are included here,
formally, and some are ad-hoc hooks? I understand that things like
convenience functions etc are not called via a central dispatch
(like the above), but I am unsure of having half the features formally
defined here, and the other half just left silently undeclared.

It gives me pause on the design.  If we are going to have a formalized
interface to scripting in GDB it seems it would be best to either be
ad-hoc, or completely formal.  Right now it feels in-between?

Anyway, I think a lot more documentation is needed regarding the
expectations of the API and the requirements to the scripting author.
Are all of these functions mandatory and need to be fully implemented?
Are NULLs allowed where the author has chosen not to implement that
feature if they are not mandatory? What about the behavior of NOOP
functions? (IE, the frame filter functions say for guile just returns
NO_FILTERS.  Will that deny the other scripting language the chance to
run their frame filters?

> +  /* Return non-zero if there is a stop condition for the breakpoint,
> +     and it indicates the program should stop.  */
> +  enum scr_bp_stop (*breakpoint_cond_says_stop)
> +    (const struct script_language_defn *, struct breakpoint *);

Why "scr" over script?


> +#include "defs.h"
> +#include "auto-load.h"
> +#include "breakpoint.h"
> +#include "scripting.h"
> +#include "scripting-priv.h"
> +#include "cli/cli-script.h"
> +#include "python/python.h"
> +
> +/* Iterate over all external scripting languages, regardless of whether the
> +   support has been compiled in or not.  */
> +#define ALL_EXT_SCRIPTING_LANGUAGES(i, slang) \
> +  for (/*int*/ i = 0, slang = external_scripting_languages[0]; \
> +       slang != NULL; \
> +       slang = external_scripting_languages[++i])
> +
> +/* Iterate over all external scripting languages that are supported.  */
> +#define ALL_ENABLED_EXT_SCRIPTING_LANGUAGES(i, slang) \
> +  for (/*int*/ i = 0, slang = external_scripting_languages[0]; \
> +       slang != NULL; \
> +       slang = external_scripting_languages[++i]) \
> +    if (slang->ops != NULL)

Tiny nit, and it is a personal thing.  If we are now asking for spaces
between function names and comments, surely these same should apply
for MACROS that are defined as, and called as, functions? Like I said,
tiny style issue.

> +/* Table of all external (non-native) scripting languages.  */
> +
> +static const struct script_language_defn * const
> +  external_scripting_languages[] =
> +{
> +  &script_language_python,
> +  NULL
> +};

Small comment.  If NULL is the sentinel value indicating the end of a
sequence, I normally put a comment saying so.  Otherwise it can get
replaced accidentally with the next entry.  Something like: "NULL /*
Sentinel */" so it is absolutely clear.

> +
> +/* Accessors for "public" attributes of struct script_language.
> +
> +   IWBN if we could use slang_foo here and elsewhere, but we can't for fear of
> +   confusing someone into thinking we might be referring to the Slang
> +   programming language.  */
> +
> +/* Return the "name" field of SLANG.  */

> +
> +const char *
> +script_lang_name (const struct script_language_defn *slang)
> +{
> +  return slang->name;
> +}

I'd go even further and remove all references to slang, and replace
them with scripting_lang, including parameters and variables.  It
would be more consistent. That's just my choice though, it is really
up to you.

> +
> +/* Return the "capitalized_name" field of SLANG.  */
> +
> +const char *
> +script_lang_capitalized_name (const struct script_language_defn *slang)
> +{
> +  return slang->capitalized_name;
> +}

I mentioned this earlier.  Not sure why we need it.  Perhaps instead
of capitalized name, you really mean the proper noun of the scripting
language? Still not sure why we need this and the lower-case version.
If terms of upper-case and lower-case, the lower-case can just be
extracted by conversion?  This might be a comment issue, and
these have real uses.  Maybe "generic" and "proper" or
something like this.

> +/* Return the "suffix" field of SLANG.  */

This is going to get really confusing if we ever have slang scripting
support ;)

> +
> +/* Return the script "sourcer" function for SLANG.
> +   This is the function that loads and processes a script.  */

The more I read the more I think which should be consistent with
script_* over slang.

> +/* Return the scripting language specified by CMD.
> +   Note the difference between this function and
> +   eval_script_from_control_command is that we loop on
> +   ALL_EXT_SCRIPTING_LANGUAGES whereas the latter loops on
> +   ALL_ENABLED_EXT_SCRIPTING_LANGUAGES.  */
> +
> +static const struct script_language_defn *
> +script_lang_from_control_command (struct command_line *cmd)
> +{
> +  int i;
> +  const struct script_language_defn *slang;
> +
> +  ALL_EXT_SCRIPTING_LANGUAGES (i, slang)
> +    {
> +      if (slang->cli_control_type == cmd->control_type)
> +	return slang;
> +    }

It is hard to work this out as there is only one language supported,
which is Python.  But are you planning to add info scripting_languages
and other assorted information commands?

It might even be that we need to add a priority to the language if
they exist in mutual exclusion for shared features.  (IE, if Python
has a pretty printer for std::list, and so does guile, which gets to
run?)


> +  struct script_type_printers *printers
> +    = XZALLOC (struct script_type_printers);
> +  int i;
> +  const struct script_language_defn *slang;
> +
> +  ALL_ENABLED_EXT_SCRIPTING_LANGUAGES (i, slang)
> +    {
> +      if (slang->ops->start_type_printers != NULL)
> +	slang->ops->start_type_printers (slang, printers);
> +    }
> +
> +  return printers;
> +}

I think this will run all type printers from all scripting languages.
If Python has already pretty printed a type does it make sense to run
it through other scripting languages (as an example)?

> +/* Try to pretty-print a value of type TYPE located at VALADDR
> +   + EMBEDDED_OFFSET, which came from the inferior at address ADDRESS
> +   + EMBEDDED_OFFSET, onto stdio stream STREAM according to OPTIONS.
> +   VAL is the whole object that came from ADDRESS.  VALADDR must point to
> +   the head of VAL's contents buffer.
> +   Returns non-zero if the value was successfully pretty-printed.  */
> +
> +int
> +apply_val_script_pretty_printer (struct type *type, const gdb_byte *valaddr,
> +				 int embedded_offset, CORE_ADDR address,
> +				 struct ui_file *stream, int recurse,
> +				 const struct value *val,
> +				 const struct value_print_options *options,
> +				 const struct language_defn *language)
> +{
> +  int i;
> +  const struct script_language_defn *slang;
> +
> +  ALL_ENABLED_EXT_SCRIPTING_LANGUAGES (i, slang)
> +    {
> +      if (slang->ops->apply_val_pretty_printer (slang, type, valaddr,
> +						embedded_offset, address,
> +						stream, recurse, val,
> +						options, language))
> +	return 1;
> +    }
> +
> +  return 0;
> +}

I think we definitely need a priority system for scripting languages.
If the user cannot define priority, it would seem chaotic when
multiple scripting languages are supported that have similar features
supported. I think it would be ideal if the user could specify which
scripting language has priority for printing a value where one or more
printers/frame-filters exist over the other.

I thought at first that the existing priority system in place for
frame-filters/type and value pretty printers would work.  But it
occurs to me it won't.  Each scripting language I presume keeps their
own list of printers.  (I don't see guile, for example, adding to a
python list).  These lists are compartmentalized to each language. So
the situation right now would be, if guile was listed above python,
and guile had a pretty printer for foo, and so did Python, there is
currently no way to use the Python pretty printer.  And vice
versa. And there is not way to get a grand list of pretty printers for
all implemented scripting languages.

In some distributions pretty/type/frame-filters are distributed via a
package, where the feature package installs the relevant bits. (IE in
Fedora libstdc++.rpm installs the libstdc++ pretty printers).  So in
these cases, the user might be unaware, and have no control over what
printers are installed.


> +/* GDB access to the "frame filter" feature.
> +   FRAME is the source frame to start frame-filter invocation.  FLAGS is an
> +   integer holding the flags for printing.  The following elements of
> +   the FRAME_FILTER_FLAGS enum denotes the make-up of FLAGS:
> +   PRINT_LEVEL is a flag indicating whether to print the frame's
> +   relative level in the output.  PRINT_FRAME_INFO is a flag that
> +   indicates whether this function should print the frame
> +   information, PRINT_ARGS is a flag that indicates whether to print
> +   frame arguments, and PRINT_LOCALS, likewise, with frame local
> +   variables.  ARGS_TYPE is an enumerator describing the argument
> +   format, OUT is the output stream to print.  FRAME_LOW is the
> +   beginning of the slice of frames to print, and FRAME_HIGH is the
> +   upper limit of the frames to count.  Returns SCR_BT_ERROR on error,
> +   or SCR_BT_COMPLETED on success.  */
> +
> +enum script_bt_status
> +apply_script_frame_filter (struct frame_info *frame, int flags,
> +			   enum script_frame_args args_type,
> +			   struct ui_out *out,
> +			   int frame_low, int frame_high)
> +{
> +  int i;
> +  const struct script_language_defn *slang;
> +
> +  ALL_ENABLED_EXT_SCRIPTING_LANGUAGES (i, slang)
> +    {
> +      enum script_bt_status status;
> +
> +      if (slang->ops->apply_frame_filter == NULL)
> +	continue;

I notice this can be NULL, but it seems type and value printers
cannot? Why?

> +      status = slang->ops->apply_frame_filter (slang, frame, flags,
> +					       args_type, out,
> +					       frame_low, frame_high);
> +      /* We use the filters from the first scripting language that has
> +	 applicable filters.  */
> +      if (status != SCR_BT_NO_FILTERS)
> +	return status;

Not sure why you use SCR here instead of SCRIPT

> +enum script_language
> +  {
> +    SCRIPT_LANG_NONE,
> +    SCRIPT_LANG_GDB,
> +    SCRIPT_LANG_PYTHON
> +  };
> +
> +/* Script frame-filter status return values.  */
> +
> +enum script_bt_status
> +  {
> +    /* Return when an error has occurred in processing frame filters,
> +       or when printing the stack.  */
> +    SCR_BT_ERROR = -1,

The enum name is script, but the constants are SCR_*

But in the enum script_language above the constants are SCRIPT.  I
think we should finalize on one name throughout the code.

> +/* Flags to pass to apply_frame_filter.  */
> +
> +enum frame_filter_flags
> +  {

Why no SCRIPT tag on the name?

> +    /* Set this flag if frame level is to be printed.  */
> +    PRINT_LEVEL = 1,
> +
> +    /* Set this flag if frame information is to be printed.  */
> +    PRINT_FRAME_INFO = 2,
> +
> +    /* Set this flag if frame arguments are to be printed.  */
> +    PRINT_ARGS = 4,
> +
> +    /* Set this flag if frame locals are to be printed.  */
> +    PRINT_LOCALS = 8,
> +  };
> +
> +/* A choice of the different frame argument printing strategies that
> +   can occur in different cases of frame filter instantiation.  */
> +
> +enum script_frame_args
> +  {
> +    /* Print no values for arguments when invoked from the MI. */

> +
> +/* The possible results of script_language_ops.breakpoint_cond_says_stop.  */
> +
> +enum scr_bp_stop
> +  {

SCRIPT. And others.
> +    /* No "stop" condition is set.  */
> +    SCR_BP_STOP_UNSET,