[RFC/RFA] Introduce new struct parse_context

[RFC/RFA] Introduce new struct parse_context

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 3319 bytes --]

all,

I would like to introduce a new structure called "struct parse_context"
which, for now, would contain the language and radix to use when doing
some parsing. This is something that we already discussed in a more
general way.

This patch introduces:

  - struct parse_context

  - A function called "build_parse_context" that builds a parse_context
    structure from a language and radix. Eventually, when the struct
    grows, we will add more parameters to that function.

  - A macro: current_parse_context which returns a parse context using
    the current_language and the input_radix.  This is the context that
    we should be using when parsing from a function that is a "command"
    (that is a function that implements a command)

  - A macro: wrong_parse_context, which is an alias to current_parse_context.
    This macro is useful to help breaking this task into incremental changes
    and is temporary, as the FIXME explains. I am using this macro when
    I am in a context where I don't have the parse_context to be used (yet),
    to make sure that I don't forget to fix it later. Will be removed
    when the task is complete.

2007-12-17  Joel Brobecker  <brobecker@adacore.com>

        * expression.h (parse_context): New struct.
        (build_parse_context): Add declaration.
        (current_parse_context, wrong_parse_context): New macros.
        * parse.c (build_parse_context): New function.

Tested on x86-linux.

Depending on this patch is the following one, which introduces the
use of that structure in the parse routines.  I am putting that second
patch with the first one in order to show how all this is going to be
used.

2007-12-17  Joel Brobecker  <brobecker@adacore.com>

        * expression.h (parse_expression): Add parse_context parameter.
        (parse_expression_in_context, parse_exp_1): Likewise.
        * parse.c (parse_exp_1): Add new parameter parse_context and use it.
        (parse_exp_in_context, parse_expression, parse_expression_in_context):
        Likewise.
        * ada-lang.c: Update calls to parse_expression and parse_exp_1.
        * objc-lang.c: Likewise.
        * breakpoint.c: Likewise.
        * value.c: Likewise.
        * ax-gdb.c: Likewise.
        * tracepoint.c: Likewise.
        * wrapper.c: Likewise.
        * eval.c: Likewise.
        * varobj.c: Likewise.
        * printcmd.c: Likewise.
        * cli/cli-script.c: Likewise.
        * mi/mi-main.c: Likewise.
        * typeprint.c (whatis_exp): Add new parameter parse_context.
        Update call to parse_expression.
        (whatis_command, ptype_command): Update call to whatis_exp.
        (maintenance_print_type): Update call to parse_expression.

Also tested on x86-linux.

I would like to commit these two patches now, and work progressively
directly in the HEAD branch.  All the changes that I am making should
either not change the behavior, or make it better.  Working on HEAD
directly allows me to work progressively as time permits - I can always
find 10-15 mins to work on one "wrong_parse_context" everyday. Finding
a couple of hours is not always that easy.  Also, I think that people
will be able to follow the changes a little more easily rather than
being bombarded by a truckload of patches.

Would it be OK if I worked directly on HEAD?

Thanks,
-- 
Joel

[-- Attachment #2: parse_context.diff --]
[-- Type: text/plain, Size: 2306 bytes --]

Index: expression.h
===================================================================
--- expression.h	(revision 2)
+++ expression.h	(revision 3)
@@ -387,6 +387,37 @@ struct expression
 
 /* From parse.c */
 
+/* Parsing an expression is dependent on a certain context such as
+   the language to use, or the input radix for numbers.  This structure
+   contains that context information.  */
+
+struct parse_context
+ {
+   /* The language to use when parsing an expression.  */
+   const struct language_defn *language;
+
+   /* The radix to use by default when parsing numbers.  */
+   int radix;
+ };
+
+extern struct parse_context
+  build_parse_context (const struct language_defn *language, int radix);
+
+#define current_parse_context \
+  (build_parse_context (current_language, input_radix))
+
+/* FIXME: brobecker/2007-12-09: We are currently introducing the addition
+   of a struct parse_context parameter to all the parsing routines.
+   The addition of this parameter means that we need to update all
+   the callers, which often means that the caller should also be added
+   a struct parse_context parameter, which in turn often means that
+   the caller's caller should also be updated, etc etc etc.  Rather than
+   updating the entire calling chains all at once, which would be a very
+   large change, we break the transition into smaller changes, with the use
+   of the following macro, which is a temporary use of current_parse_context.
+   This macro should be deleted once the transition is complete.  */
+#define wrong_parse_context (current_parse_context)
+
 extern struct expression *parse_expression (char *);
 
 extern struct expression *parse_expression_in_context (char *, int);
Index: parse.c
===================================================================
--- parse.c	(revision 2)
+++ parse.c	(revision 3)
@@ -911,6 +911,21 @@ prefixify_subexp (struct expression *ine
       outbeg += oplen;
     }
 }
+
+/* Return a parse_context structure using the given LANGUAGE
+   and RADIX.  */
+
+struct parse_context
+build_parse_context (const struct language_defn *language, int radix)
+{
+  struct parse_context result;
+
+  result.language = language;
+  result.radix = radix;
+
+  return result;
+}
+
 \f
 /* This page contains the two entry points to this file.  */
 

[-- Attachment #3: param_parse.diff --]
[-- Type: text/plain, Size: 18780 bytes --]

Index: ada-lang.c
===================================================================
--- ada-lang.c	(revision 6)
+++ ada-lang.c	(revision 7)
@@ -9675,7 +9675,8 @@ static struct expression *
 ada_parse_catchpoint_condition (char *cond_string,
                                 struct symtab_and_line sal)
 {
-  return (parse_exp_1 (&cond_string, block_for_pc (sal.pc), 0));
+  return (parse_exp_1 (&cond_string, block_for_pc (sal.pc), 0,
+                       wrong_parse_context));
 }
 
 /* Return the symtab_and_line that should be used to insert an exception
Index: objc-lang.c
===================================================================
--- objc-lang.c	(revision 6)
+++ objc-lang.c	(revision 7)
@@ -1371,7 +1371,7 @@ print_object_command (char *args, int fr
 "The 'print-object' command requires an argument (an Objective-C object)");
 
   {
-    struct expression *expr = parse_expression (args);
+    struct expression *expr = parse_expression (args, current_parse_context);
     struct cleanup *old_chain = 
       make_cleanup (free_current_contents, &expr);
     int pc = 0;
Index: breakpoint.c
===================================================================
--- breakpoint.c	(revision 6)
+++ breakpoint.c	(revision 7)
@@ -596,7 +596,13 @@ condition_command (char *arg, int from_t
 	  for (loc = b->loc; loc; loc = loc->next)
 	    {
 	      arg = p;
-	      loc->cond = parse_exp_1 (&arg, block_for_pc (loc->address), 0);
+              /* FIXME: brobecker/2007-12-09:  The current breakpoint
+                 structures are defined in a way that assumes that the
+                 parse_context used to express the condition is the same
+                 as the parse_context of the condition.  So use the
+                 breakpoint parse_context to parse the condition.  */
+	      loc->cond = parse_exp_1 (&arg, block_for_pc (loc->address), 0,
+                                       wrong_parse_context);
 	      if (*arg)
 		error (_("Junk at end of expression"));
 	    }
@@ -5126,7 +5132,8 @@ create_breakpoint (struct symtabs_and_li
       if (b->cond_string)
 	{
 	  char *arg = b->cond_string;
-	  loc->cond = parse_exp_1 (&arg, block_for_pc (loc->address), 0);
+	  loc->cond = parse_exp_1 (&arg, block_for_pc (loc->address), 0,
+                                   wrong_parse_context);
 	  if (*arg)
               error (_("Garbage %s follows condition"), arg);
 	}
@@ -5421,7 +5428,7 @@ find_condition_and_thread (char *tok, CO
       if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
 	{
 	  tok = cond_start = end_tok + 1;
-	  parse_exp_1 (&tok, block_for_pc (pc), 0);
+	  parse_exp_1 (&tok, block_for_pc (pc), 0, wrong_parse_context);
 	  cond_end = tok;
 	  *cond_string = savestring (cond_start, 
 				     cond_end - cond_start);
@@ -5900,7 +5907,7 @@ watch_command_1 (char *arg, int accessfl
   /* Parse arguments.  */
   innermost_block = NULL;
   exp_start = arg;
-  exp = parse_exp_1 (&arg, 0, 0);
+  exp = parse_exp_1 (&arg, 0, 0, current_parse_context);
   exp_end = arg;
   exp_valid_block = innermost_block;
   mark = value_mark ();
@@ -5921,7 +5928,7 @@ watch_command_1 (char *arg, int accessfl
   if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
     {
       tok = cond_start = end_tok + 1;
-      cond = parse_exp_1 (&tok, 0, 0);
+      cond = parse_exp_1 (&tok, 0, 0, current_parse_context);
       cond_end = tok;
     }
   if (*tok)
@@ -7381,7 +7388,7 @@ update_breakpoint_locations (struct brea
 	  TRY_CATCH (e, RETURN_MASK_ERROR)
 	    {
 	      new_loc->cond = parse_exp_1 (&s, block_for_pc (sals.sals[i].pc), 
-					   0);
+					   0, wrong_parse_context);
 	    }
 	  if (e.reason < 0)
 	    {
@@ -7552,7 +7559,7 @@ breakpoint_re_set_one (void *bint)
              parse_expression.  */
 	  b->exp = NULL;
 	}
-      b->exp = parse_expression (b->exp_string);
+      b->exp = parse_expression (b->exp_string, wrong_parse_context);
       b->exp_valid_block = innermost_block;
       mark = value_mark ();
       if (b->val)
@@ -7577,7 +7584,8 @@ breakpoint_re_set_one (void *bint)
 		 to parse_exp_1.  */
 	      b->loc->cond = NULL;
 	    }
-	  b->loc->cond = parse_exp_1 (&s, (struct block *) 0, 0);
+	  b->loc->cond = parse_exp_1 (&s, (struct block *) 0, 0,
+                                      wrong_parse_context);
 	}
       if (breakpoint_enabled (b))
 	mention (b);
Index: value.c
===================================================================
--- value.c	(revision 6)
+++ value.c	(revision 7)
@@ -714,7 +714,7 @@ init_if_undefined_command (char* args, i
   struct internalvar* intvar;
 
   /* Parse the expression - this is taken from set_command().  */
-  struct expression *expr = parse_expression (args);
+  struct expression *expr = parse_expression (args, current_parse_context);
   register struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
 
Index: ax-gdb.c
===================================================================
--- ax-gdb.c	(revision 6)
+++ ax-gdb.c	(revision 7)
@@ -1786,7 +1786,7 @@ agent_command (char *exp, int from_tty)
   if (exp == 0)
     error_no_arg (_("expression to translate"));
 
-  expr = parse_expression (exp);
+  expr = parse_expression (exp, current_parse_context);
   old_chain = make_cleanup (free_current_contents, &expr);
   agent = gen_trace_for_expr (get_frame_pc (fi), expr);
   make_cleanup_free_agent_expr (agent);
Index: tracepoint.c
===================================================================
--- tracepoint.c	(revision 6)
+++ tracepoint.c	(revision 7)
@@ -970,7 +970,9 @@ validate_actionline (char **line, struct
 		}
 	      /* else fall thru, treat p as an expression and parse it!  */
 	    }
-	  exp = parse_exp_1 (&p, block_for_pc (t->address), 1);
+	  exp = parse_exp_1 (&p, block_for_pc (t->address), 1,
+	                     build_parse_context (language_def (t->language),
+			                          t->input_radix));
 	  old_chain = make_cleanup (free_current_contents, &exp);
 
 	  if (exp->elts[0].opcode == OP_VAR_VALUE)
@@ -1598,7 +1600,10 @@ encode_actions (struct tracepoint *t, ch
 		  struct agent_reqs areqs;
 
 		  exp = parse_exp_1 (&action_exp, 
-				     block_for_pc (t->address), 1);
+				     block_for_pc (t->address), 1,
+				     build_parse_context
+				       (language_def (t->language),
+				        t->input_radix));
 		  old_chain = make_cleanup (free_current_contents, &exp);
 
 		  switch (exp->elts[0].opcode)
Index: wrapper.c
===================================================================
--- wrapper.c	(revision 6)
+++ wrapper.c	(revision 7)
@@ -29,7 +29,7 @@ gdb_parse_exp_1 (char **stringptr, struc
 
   TRY_CATCH (except, RETURN_MASK_ERROR)
     {
-      *expression = parse_exp_1 (stringptr, block, comma);
+      *expression = parse_exp_1 (stringptr, block, comma, wrong_parse_context);
     }
 
   if (except.reason < 0)
Index: eval.c
===================================================================
--- eval.c	(revision 6)
+++ eval.c	(revision 7)
@@ -82,7 +82,7 @@ evaluate_subexp (struct type *expect_typ
 CORE_ADDR
 parse_and_eval_address (char *exp)
 {
-  struct expression *expr = parse_expression (exp);
+  struct expression *expr = parse_expression (exp, wrong_parse_context);
   CORE_ADDR addr;
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
@@ -98,7 +98,8 @@ parse_and_eval_address (char *exp)
 CORE_ADDR
 parse_and_eval_address_1 (char **expptr)
 {
-  struct expression *expr = parse_exp_1 (expptr, (struct block *) 0, 0);
+  struct expression *expr = parse_exp_1 (expptr, (struct block *) 0, 0,
+                                         wrong_parse_context);
   CORE_ADDR addr;
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
@@ -113,7 +114,7 @@ parse_and_eval_address_1 (char **expptr)
 LONGEST
 parse_and_eval_long (char *exp)
 {
-  struct expression *expr = parse_expression (exp);
+  struct expression *expr = parse_expression (exp, wrong_parse_context);
   LONGEST retval;
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
@@ -126,7 +127,7 @@ parse_and_eval_long (char *exp)
 struct value *
 parse_and_eval (char *exp)
 {
-  struct expression *expr = parse_expression (exp);
+  struct expression *expr = parse_expression (exp, wrong_parse_context);
   struct value *val;
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
@@ -143,7 +144,8 @@ parse_and_eval (char *exp)
 struct value *
 parse_to_comma_and_eval (char **expp)
 {
-  struct expression *expr = parse_exp_1 (expp, (struct block *) 0, 1);
+  struct expression *expr = parse_exp_1 (expp, (struct block *) 0, 1,
+                                         wrong_parse_context);
   struct value *val;
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
@@ -2297,7 +2299,7 @@ parse_and_eval_type (char *p, int length
   tmp[length + 1] = ')';
   tmp[length + 2] = '0';
   tmp[length + 3] = '\0';
-  expr = parse_expression (tmp);
+  expr = parse_expression (tmp, wrong_parse_context);
   if (expr->elts[0].opcode != UNOP_CAST)
     error (_("Internal error in eval_type."));
   return expr->elts[1].type;
Index: typeprint.c
===================================================================
--- typeprint.c	(revision 6)
+++ typeprint.c	(revision 7)
@@ -45,7 +45,7 @@ static void ptype_command (char *, int);
 
 static void whatis_command (char *, int);
 
-static void whatis_exp (char *, int);
+static void whatis_exp (char *, int, struct parse_context parse_context);
 
 /* Print a description of a type in the format of a 
    typedef for the current language.
@@ -109,7 +109,7 @@ type_print (struct type *type, char *var
    show is passed to type_print.  */
 
 static void
-whatis_exp (char *exp, int show)
+whatis_exp (char *exp, int show, struct parse_context parse_context)
 {
   struct expression *expr;
   struct value *val;
@@ -122,7 +122,7 @@ whatis_exp (char *exp, int show)
 
   if (exp)
     {
-      expr = parse_expression (exp);
+      expr = parse_expression (exp, parse_context);
       old_chain = make_cleanup (free_current_contents, &expr);
       val = evaluate_type (expr);
     }
@@ -175,7 +175,7 @@ whatis_command (char *exp, int from_tty)
   /* Most of the time users do not want to see all the fields
      in a structure.  If they do they can use the "ptype" command.
      Hence the "-1" below.  */
-  whatis_exp (exp, -1);
+  whatis_exp (exp, -1, current_parse_context);
 }
 
 /* TYPENAME is either the name of a type, or an expression.  */
@@ -183,7 +183,7 @@ whatis_command (char *exp, int from_tty)
 static void
 ptype_command (char *typename, int from_tty)
 {
-  whatis_exp (typename, 1);
+  whatis_exp (typename, 1, current_parse_context);
 }
 
 /* Print integral scalar data VAL, of type TYPE, onto stdio stream STREAM.
@@ -283,7 +283,7 @@ maintenance_print_type (char *typename, 
 
   if (typename != NULL)
     {
-      expr = parse_expression (typename);
+      expr = parse_expression (typename, current_parse_context);
       old_chain = make_cleanup (free_current_contents, &expr);
       if (expr->elts[0].opcode == OP_TYPE)
 	{
Index: parse.c
===================================================================
--- parse.c	(revision 6)
+++ parse.c	(revision 7)
@@ -105,7 +105,7 @@ static void prefixify_subexp (struct exp
 			      int);
 
 static struct expression *parse_exp_in_context (char **, struct block *, int, 
-						int);
+						int, struct parse_context);
 
 void _initialize_parse (void);
 
@@ -940,9 +940,10 @@ build_parse_context (const struct langua
    If COMMA is nonzero, stop if a comma is reached.  */
 
 struct expression *
-parse_exp_1 (char **stringptr, struct block *block, int comma)
+parse_exp_1 (char **stringptr, struct block *block, int comma,
+             struct parse_context parse_context)
 {
-  return parse_exp_in_context (stringptr, block, comma, 0);
+  return parse_exp_in_context (stringptr, block, comma, 0, parse_context);
 }
 
 /* As for parse_exp_1, except that if VOID_CONTEXT_P, then
@@ -950,7 +951,7 @@ parse_exp_1 (char **stringptr, struct bl
 
 static struct expression *
 parse_exp_in_context (char **stringptr, struct block *block, int comma, 
-		      int void_context_p)
+		      int void_context_p, struct parse_context parse_context)
 {
   struct cleanup *old_chain;
 
@@ -994,11 +995,11 @@ parse_exp_in_context (char **stringptr, 
   expout_ptr = 0;
   expout = (struct expression *)
     xmalloc (sizeof (struct expression) + EXP_ELEM_TO_BYTES (expout_size));
-  expout->language_defn = current_language;
+  expout->language_defn = parse_context.language;
   make_cleanup (free_current_contents, &expout);
 
-  if (current_language->la_parser ())
-    current_language->la_error (NULL);
+  if (parse_context.language->la_parser ())
+    parse_context.language->la_error (NULL);
 
   discard_cleanups (old_chain);
 
@@ -1020,7 +1021,7 @@ parse_exp_in_context (char **stringptr, 
 
   prefixify_expression (expout);
 
-  current_language->la_post_parser (&expout, void_context_p);
+  parse_context.language->la_post_parser (&expout, void_context_p);
 
   if (expressiondebug)
     dump_prefix_expression (expout, gdb_stdlog);
@@ -1033,10 +1034,10 @@ parse_exp_in_context (char **stringptr, 
    to use up all of the contents of STRING.  */
 
 struct expression *
-parse_expression (char *string)
+parse_expression (char *string, struct parse_context parse_context)
 {
   struct expression *exp;
-  exp = parse_exp_1 (&string, 0, 0);
+  exp = parse_exp_1 (&string, 0, 0, parse_context);
   if (*string)
     error (_("Junk after end of expression."));
   return exp;
@@ -1047,10 +1048,11 @@ parse_expression (char *string)
    no value is expected from the expression.  */
 
 struct expression *
-parse_expression_in_context (char *string, int void_context_p)
+parse_expression_in_context (char *string, int void_context_p,
+                             struct parse_context parse_context)
 {
   struct expression *exp;
-  exp = parse_exp_in_context (&string, 0, 0, void_context_p);
+  exp = parse_exp_in_context (&string, 0, 0, void_context_p, parse_context);
   if (*string != '\000')
     error (_("Junk after end of expression."));
   return exp;
Index: mi/mi-main.c
===================================================================
--- mi/mi-main.c	(revision 6)
+++ mi/mi-main.c	(revision 7)
@@ -44,6 +44,7 @@
 #include "gdb.h"
 #include "frame.h"
 #include "mi-main.h"
+#include "language.h"
 
 #include <ctype.h>
 #include <sys/time.h>
@@ -693,7 +694,7 @@ mi_cmd_data_evaluate_expression (char *c
       return MI_CMD_ERROR;
     }
 
-  expr = parse_expression (argv[0]);
+  expr = parse_expression (argv[0], current_parse_context);
 
   old_chain = make_cleanup (free_current_contents, &expr);
 
Index: varobj.c
===================================================================
--- varobj.c	(revision 6)
+++ varobj.c	(revision 7)
@@ -877,7 +877,7 @@ varobj_set_value (struct varobj *var, ch
   gdb_assert (varobj_editable_p (var));
 
   input_radix = 10;		/* ALWAYS reset to decimal temporarily */
-  exp = parse_exp_1 (&s, 0, 0);
+  exp = parse_exp_1 (&s, 0, 0, wrong_parse_context);
   if (!gdb_evaluate_expression (exp, &value))
     {
       /* We cannot proceed without a valid expression. */
Index: cli/cli-script.c
===================================================================
--- cli/cli-script.c	(revision 6)
+++ cli/cli-script.c	(revision 7)
@@ -414,7 +414,7 @@ execute_control_command (struct command_
 	if (!new_line)
 	  break;
 	make_cleanup (free_current_contents, &new_line);
-	expr = parse_expression (new_line);
+	expr = parse_expression (new_line, current_parse_context);
 	make_cleanup (free_current_contents, &expr);
 
 	ret = simple_control;
@@ -481,7 +481,7 @@ execute_control_command (struct command_
 	  break;
 	make_cleanup (free_current_contents, &new_line);
 	/* Parse the conditional for the if statement.  */
-	expr = parse_expression (new_line);
+	expr = parse_expression (new_line, current_parse_context);
 	make_cleanup (free_current_contents, &expr);
 
 	current = NULL;
Index: expression.h
===================================================================
--- expression.h	(revision 6)
+++ expression.h	(revision 7)
@@ -419,11 +419,13 @@ extern struct parse_context
    This macro should be deleted once the transition is complete.  */
 #define wrong_parse_context (current_parse_context)
 
-extern struct expression *parse_expression (char *);
+extern struct expression *parse_expression (char *, struct parse_context);
 
-extern struct expression *parse_expression_in_context (char *, int);
+extern struct expression *
+  parse_expression_in_context (char *, int, struct parse_context);
 
-extern struct expression *parse_exp_1 (char **, struct block *, int);
+extern struct expression *
+  parse_exp_1 (char **, struct block *, int, struct parse_context);
 
 /* The innermost context required by the stack and register variables
    we've encountered so far.  To use this, set it to NULL, then call
Index: printcmd.c
===================================================================
--- printcmd.c	(revision 6)
+++ printcmd.c	(revision 7)
@@ -865,7 +865,7 @@ print_command_1 (char *exp, int inspect,
   if (exp && *exp)
     {
       struct type *type;
-      expr = parse_expression (exp);
+      expr = parse_expression (exp, wrong_parse_context);
       old_chain = make_cleanup (free_current_contents, &expr);
       cleanup = 1;
       val = evaluate_expression (expr);
@@ -950,7 +950,7 @@ output_command (char *exp, int from_tty)
       format = fmt.format;
     }
 
-  expr = parse_expression (exp);
+  expr = parse_expression (exp, current_parse_context);
   old_chain = make_cleanup (free_current_contents, &expr);
 
   val = evaluate_expression (expr);
@@ -970,7 +970,7 @@ output_command (char *exp, int from_tty)
 static void
 set_command (char *exp, int from_tty)
 {
-  struct expression *expr = parse_expression (exp);
+  struct expression *expr = parse_expression (exp, current_parse_context);
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
   evaluate_expression (expr);
@@ -1272,7 +1272,7 @@ x_command (char *exp, int from_tty)
 
   if (exp != 0 && *exp != 0)
     {
-      expr = parse_expression (exp);
+      expr = parse_expression (exp, current_parse_context);
       /* Cause expression not to be there any more if this command is
          repeated with Newline.  But don't clobber a user-defined
          command's definition.  */
@@ -1367,7 +1367,7 @@ display_command (char *exp, int from_tty
 	}
 
       innermost_block = 0;
-      expr = parse_expression (exp);
+      expr = parse_expression (exp, current_parse_context);
 
       new = (struct display *) xmalloc (sizeof (struct display));
 

Re: [RFC/RFA] Introduce new struct parse_context

[Daniel Jacobowitz]

On Mon, Dec 17, 2007 at 10:42:13AM +0400, Joel Brobecker wrote:
> Would it be OK if I worked directly on HEAD?

Well, I've no objection.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC/RFA] Introduce new struct parse_context

[Eli Zaretskii]

> Date: Mon, 17 Dec 2007 10:42:13 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> I would like to introduce a new structure called "struct parse_context"
> which, for now, would contain the language and radix to use when doing
> some parsing. This is something that we already discussed in a more
> general way.

Thanks!

> This patch introduces:
> 
>   - struct parse_context
> 
>   - A function called "build_parse_context" that builds a parse_context
>     structure from a language and radix. Eventually, when the struct
>     grows, we will add more parameters to that function.
> 
>   - A macro: current_parse_context which returns a parse context using
>     the current_language and the input_radix.  This is the context that
>     we should be using when parsing from a function that is a "command"
>     (that is a function that implements a command)
> 
>   - A macro: wrong_parse_context, which is an alias to current_parse_context.
>     This macro is useful to help breaking this task into incremental changes
>     and is temporary, as the FIXME explains. I am using this macro when
>     I am in a context where I don't have the parse_context to be used (yet),
>     to make sure that I don't forget to fix it later. Will be removed
>     when the task is complete.

How about if we try to document in gdbint.texinfo each new
infrastructure we introduce, from now on?  Would other maintainers
support such a requirement from contributors?  I can offer
text-to-Texinfo conversion services (and, of course, general help in
writing documentation), if someone does not feel up to speed with
Texinfo.

WDYT?

Re: [RFC/RFA] Introduce new struct parse_context

[Joel Brobecker]

Eli,

> How about if we try to document in gdbint.texinfo each new
> infrastructure we introduce, from now on?  Would other maintainers
> support such a requirement from contributors?  I can offer
> text-to-Texinfo conversion services (and, of course, general help in
> writing documentation), if someone does not feel up to speed with
> Texinfo.

Generally speaking, I am very committed to good documentation.
I would tend to say that your suggestion is a good one, and we should
request documentation when necessary. But I think that this should
be evaluated on a case-by-case basis, because there are some times
when external documents such as gdbint will be more useful, and other
times when it will be more appropriate to leave the documentation
as a comment embedded in the code.

Let's take the two patches of this thread as an example. Would you
suggest to move the documentation to gdbint, or leave it in the
code?  To me, it seems better to put keep the documentation with
the code. If we were to put some documentation in gdbint for this patch,
what would you put?

-- 
Joel

Re: [RFC/RFA] Introduce new struct parse_context

[Eli Zaretskii]

> Date: Fri, 21 Dec 2007 08:36:08 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: uweigand@de.ibm.com, gdb-patches@sourceware.org
> 
> I would tend to say that your suggestion is a good one, and we should
> request documentation when necessary. But I think that this should
> be evaluated on a case-by-case basis, because there are some times
> when external documents such as gdbint will be more useful, and other
> times when it will be more appropriate to leave the documentation
> as a comment embedded in the code.
> 
> Let's take the two patches of this thread as an example. Would you
> suggest to move the documentation to gdbint, or leave it in the
> code?  To me, it seems better to put keep the documentation with
> the code.

We had this discussion in the past, and I'm well aware that some of
the maintainers think that code comments are better than a manual that
documents the internals.  I can only reiterate what I wrote back then:
I think code comments are not a replacement for a good internals
manual.  Comments don't have a structure.  They lack good
cross-referencing facilities.  They don't have any efficient ways of
searching for text by a word or phrase that describes the topic or
subject we are looking for (something like index search in Info).

Comments also tend to describe details, but not the overall picture.

In sum, I think comments are not appropriate for someone who wants to
learn about a subject she knows next to nothing about.

> If we were to put some documentation in gdbint for this patch,
> what would you put?

Both.  This is a significant new feature, and at least its main
principles should be described in gdbint, enough to get the reader off
the ground with the knowledge she could use to find the parts of code
which implement this feature (and read the comments there ;-).

Re: [RFC/RFA] Introduce new struct parse_context

[Joel Brobecker]

> > If we were to put some documentation in gdbint for this patch,
> > what would you put?
> 
> Both.  This is a significant new feature, and at least its main
> principles should be described in gdbint, enough to get the reader off
> the ground with the knowledge she could use to find the parts of code
> which implement this feature (and read the comments there ;-).

I don't see what you would describe in the documentation that would
not be in the code as a comment. I think I don't see what to say because
I don't see this patch as a significant new feature, but as a modification
whose purpose is to fix a bug (the parser uses the global current_language)
to do the parsing, but this global gets overwritten as a side effect of
a function called between the moment we set the current_language and
the moment we actually do the parsing).

How about the following compromise: I describe again roughly what the
problem is and how I am going to address it. Then you can decide what
to add to gdbint, and where. I would appreciate your help in writing
that addition, because I just simply suck in writing documentations,
and as a result, I'm really slow.  Just the time it takes me to write
good comments makes me mad, sometimes.

-- 
Joel

Re: [RFC/RFA] Introduce new struct parse_context

[Eli Zaretskii]

> Date: Sat, 22 Dec 2007 10:22:23 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: uweigand@de.ibm.com, gdb-patches@sourceware.org
> 
> How about the following compromise: I describe again roughly what the
> problem is and how I am going to address it. Then you can decide what
> to add to gdbint, and where.

Okay, let's try that.

Thanks.

Re: [RFC/RFA] Introduce new struct parse_context

[Joel Brobecker]

> > How about the following compromise: I describe again roughly what the
> > problem is and how I am going to address it. Then you can decide what
> > to add to gdbint, and where.
> 
> Okay, let's try that.

Great! Here is the problem - It shows up more often on mips-irix.
Using an Ada program named foo:

    % gdb foo
    (gdb) b foo
    Breakpoint 1 at 0x1000278c: file foo.adb, line 4.
    (gdb) run
    Starting program: /kern.a/brobecke/head/ex/foo
    Error in re-setting breakpoint 1:
    Function "foo" not defined.

    Program exited normally.

procedure "Foo" is the name of the main subprogram, the equivalent
of "main" in C. The difference with C is that Ada entity names follow
an encoding - for instance, in our case, the Ada mode in GDB understood
that "foo" was the main subprogram, and that its real (aka "linkage")
name is actually "_ada_foo".

As soon as the program was started, the loader mapped the shared libraries,
and that caused all breakpoints to be re-evaluated. This is done using
the following sequence:

    breakpoint_re_set_one
       -> Set current_language global to breakpoint language
       -> Re-evaluation of the breakpoint location
       -> Reset current_language to intial value

This works well most of the time, but not always. What happens
sometimes is that the current_language gets unexpectedly switched
up from under our feet as a side-effect of calling select_frame().

I have faced this type of problem in the past before, and we discussed
the idea of having the language be a parameter of the parsing routine,
but an easy workaround was also available and so the work-around was
installed and the idea postponed.

Unfortunately in this case, I turned the problem in everyway possible,
and I couldn't find an easy work-around. I actually think that this is
a fortunate thing, as it forces us to move forward in fixing the
weakness in that part of the GDB technology. Later on, Ulrich also
mentioned that the language was not the only parameter that influences
the parsing, there is also the input_radix.

The solution is to get rid of using the global current_language to
parse any expression, but to pass it as a parameter of the parsing
routine. Same for the input_radix. Because there will be more than
one such parameters to pass, we decided to create a new structure
that holds them all, and pass that structure to the parse routine.

This is a simple change on paper, but will require large updates
in the sources, because most callers of the various routines have
also been assuming that the current_language should be used. So
these routines should also be updated to take an extra parameter
as the parse_context. This in turn means that the callers' callers
also need to be updated, etc etc etc, until we reach the command
routine.

I hope this clarifies the problem we're trying to solve, and
the designed escape. I am planning to start working on the code
aspect this week, while things are quiet at the office, and will
work on documentation with you in parallel.

-- 
Joel

Re: [RFC/RFA] Introduce new struct parse_context

[Eli Zaretskii]

> Date: Tue, 25 Dec 2007 22:00:22 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > > How about the following compromise: I describe again roughly what the
> > > problem is and how I am going to address it. Then you can decide what
> > > to add to gdbint, and where.
> > 
> > Okay, let's try that.
> 
> Great! Here is the problem - It shows up more often on mips-irix.

Thanks for the detailed description.

To document this, I'd go like this:

  Once upon a time, @value{GDBN} used a global variable called
  @code{current_language} to store the current source language.  But
  this didn't always work, because @code{current_language} would
  sometimes get unexpectedly switched under our feet as a side-effect
  of calling functions such as @code{select_frame}.  To resolve this
  difficulty, @value{GDBN} now passes the language as a parameter to
  routines that parse language-specific expressions.

I would then describe the structure that will hold the language and
the input radix, give a few examples of such routines, and explain
when they are used by GDB.  If there's some infrastructure involved
here, beyond the above-mentioned structure, it should be documented as
well.

As to where to document this, I think the node "Language Support" is
the right place.  It already mentions `current_language', so it should
be updated anyway if we are getting rid of that variable.