[RFA] Fix breakpoint condition that use member variables.

[RFA] Fix breakpoint condition that use member variables.

[Vladimir Prus]

Suppose that foo.cpp:10 is a location inside member function of a class,
and that said class has member variable i_, and that we've just started
a program and are in main. Then:

    break foo.cpp:10 if i_ == 10

will not work, claiming that i_ does not exist. The problem is that
lookup_symbol_aux uses value_of_this, which uses value_of_local and
that totally ignores the block that is passed to parse_exp_1 by
the breakpoint code and uses the block of the selected frame. Of course,
that either does not have "this", or has wrong "this".

This patch fixes the problem by looking in the right block directly,
and also by looking for the field in the type of "this", without
trying to get the value.

It should be noted that the existing check_field/check_field_in
functions are very similar in spirit to lookup_struct_elt_type,
so I've tried to kill check_field completely, but presently 
lookup_struct_elt_type does not handle member functions, so not
immediately suitable.

I've also changed language definitions for fortran and scheme not
to claim they have "this" pointer, as to the best of my knowledge
they don't.

No regressions on x86. OK?

- Volodya

	[gdb]
	Fix breakpoint condition that use member variables.
	* valops.c (check_field): Remove.
	(check_field_in): Rename to check_field.
	(value_of_this): Use la_name_of_this.
	* value.h (check_field): Adjust prototype.

	* language.h (la_value_of_this): Rename to la_name_of_this.
	* language.c (unknown_language_defn): Specify "this" for
	name_of_this.
	(auto_language_defn): Likewise.
	(local_language_defn): Likewise.
	* ada-lang.c (ada_language_defn): Adjust comment.
	* c-lang.c (c_language_defn): Adjust comment.
	(cplus_language_defn): Specify "this" for name_of_this.
	(asm_language_defn): Adjust comment.
	(minimal_language_defn): Adjust comment.
	* f-lang.c (f_language_defn): Specify NULL for name_of_this.
	* jv-lang.c (java_language_defn): Specify "this" for name_of_this.
	* m2-lang.c (m2_language_defn): Specify "this" for name_of_this.
	* objc-lang.c (objc_language_defn): Specify "self" for
	name_of_this.
	* p-lang.c (pascal_language_defn): Specify "this" for
	name_of_this.
	* scm-lang.c (scm_language_defn): Specify NULL for name_of_this.

	* symtab.c (lookup_symbol_aux): Lookup "this" in the
	proper scope, and check for field in type of "this", without
	trying to create a value.

	[gdb/testsuite]
	* gdb.cp/breakpoint.cc: New code to test conditions involving
	member variables.
	* gdb.cp/breakpoint.exp: Test condition involving member
	variables.
---
 gdb/ada-lang.c                      |    2 +-
 gdb/c-lang.c                        |    8 +++---
 gdb/f-lang.c                        |    2 +-
 gdb/jv-lang.c                       |    2 +-
 gdb/language.c                      |    6 ++--
 gdb/language.h                      |   10 ++-----
 gdb/m2-lang.c                       |    2 +-
 gdb/objc-lang.c                     |    2 +-
 gdb/p-lang.c                        |    2 +-
 gdb/scm-lang.c                      |    2 +-
 gdb/symtab.c                        |   35 +++++++++++++++++++------
 gdb/testsuite/gdb.cp/breakpoint.cc  |   16 +++++++++++-
 gdb/testsuite/gdb.cp/breakpoint.exp |    5 +++
 gdb/valops.c                        |   47 
+++-------------------------------
 gdb/value.h                         |    2 +-
 15 files changed, 69 insertions(+), 74 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 2f0f55f..8cfc2c8 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10999,7 +10999,7 @@ const struct language_defn ada_language_defn = {
   ada_val_print,                /* Print a value using appropriate 
syntax */
   ada_value_print,              /* Print a top-level value */
   NULL,                         /* Language specific skip_trampoline */
-  NULL,                         /* value_of_this */
+  NULL,                         /* name_of_this */
   ada_lookup_symbol_nonlocal,   /* Looking up non-local symbols.  */
   basic_lookup_transparent_type,        /* lookup_transparent_type */
   ada_la_decode,                /* Language specific symbol demangler 
*/
diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index c4ef9d6..29aa765 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -414,7 +414,7 @@ const struct language_defn c_language_defn =
   c_val_print,			/* Print a value using appropriate syntax */
   c_value_print,		/* Print a top-level value */
   NULL,				/* Language specific skip_trampoline */
-  NULL,				/* value_of_this */
+  NULL,				/* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,				/* Language specific symbol demangler */
@@ -527,7 +527,7 @@ const struct language_defn cplus_language_defn =
   c_val_print,			/* Print a value using appropriate syntax */
   c_value_print,		/* Print a top-level value */
   cplus_skip_trampoline,	/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "this",                       /* name_of_this */
   cp_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   cp_lookup_transparent_type,   /* lookup_transparent_type */
   cplus_demangle,		/* Language specific symbol demangler */
@@ -562,7 +562,7 @@ const struct language_defn asm_language_defn =
   c_val_print,			/* Print a value using appropriate syntax */
   c_value_print,		/* Print a top-level value */
   NULL,				/* Language specific skip_trampoline */
-  NULL,				/* value_of_this */
+  NULL,				/* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,				/* Language specific symbol demangler */
@@ -602,7 +602,7 @@ const struct language_defn minimal_language_defn =
   c_val_print,			/* Print a value using appropriate syntax */
   c_value_print,		/* Print a top-level value */
   NULL,				/* Language specific skip_trampoline */
-  NULL,				/* value_of_this */
+  NULL,				/* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,				/* Language specific symbol demangler */
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 038126c..5dcbd33 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -324,7 +324,7 @@ const struct language_defn f_language_defn =
   f_val_print,			/* Print a value using appropriate syntax */
   c_value_print,		/* FIXME */
   NULL,				/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  NULL,                    	/* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,				/* Language specific symbol demangler */
diff --git a/gdb/jv-lang.c b/gdb/jv-lang.c
index e6c6f7d..b72b90c 100644
--- a/gdb/jv-lang.c
+++ b/gdb/jv-lang.c
@@ -1070,7 +1070,7 @@ const struct language_defn java_language_defn =
   java_val_print,		/* Print a value using appropriate syntax */
   java_value_print,		/* Print a top-level value */
   NULL,				/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "this",	                /* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   java_demangle,		/* Language specific symbol demangler */
diff --git a/gdb/language.c b/gdb/language.c
index a26218d..80f6961 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -1192,7 +1192,7 @@ const struct language_defn unknown_language_defn =
   unk_lang_val_print,		/* Print a value using appropriate syntax */
   unk_lang_value_print,		/* Print a top-level value */
   unk_lang_trampoline,		/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "this",        	    	/* name_of_this */
   basic_lookup_symbol_nonlocal, /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   unk_lang_demangle,		/* Language specific symbol demangler */
@@ -1228,7 +1228,7 @@ const struct language_defn auto_language_defn =
   unk_lang_val_print,		/* Print a value using appropriate syntax */
   unk_lang_value_print,		/* Print a top-level value */
   unk_lang_trampoline,		/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "this",		        /* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   unk_lang_demangle,		/* Language specific symbol demangler */
@@ -1263,7 +1263,7 @@ const struct language_defn local_language_defn =
   unk_lang_val_print,		/* Print a value using appropriate syntax */
   unk_lang_value_print,		/* Print a top-level value */
   unk_lang_trampoline,		/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "this", 		        /* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   unk_lang_demangle,		/* Language specific symbol demangler */
diff --git a/gdb/language.h b/gdb/language.h
index f7e654d..233a5a3 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -206,14 +206,10 @@ struct language_defn
 
     /* Now come some hooks for lookup_symbol.  */
 
-    /* If this is non-NULL, lookup_symbol will do the 'field_of_this'
-       check, using this function to find the value of this.  */
+    /* If this is non-NULL, specifies the name that of the implicit
+       local variable that refers to the current object instance.  */
 
-    /* FIXME: carlton/2003-05-19: Audit all the language_defn structs
-       to make sure we're setting this appropriately: I'm sure it
-       could be NULL in more languages.  */
-
-    struct value *(*la_value_of_this) (int complain);
+    char *la_name_of_this;
 
     /* This is a function that lookup_symbol will call when it gets to
        the part of symbol lookup where C looks up static and global
diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c
index 6b51fd5..400338e 100644
--- a/gdb/m2-lang.c
+++ b/gdb/m2-lang.c
@@ -375,7 +375,7 @@ const struct language_defn m2_language_defn =
   m2_val_print,			/* Print a value using appropriate syntax */
   c_value_print,		/* Print a top-level value */
   NULL,				/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "this",		        /* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,				/* Language specific symbol demangler */
diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index ccf8068..08a6bb8 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -509,7 +509,7 @@ const struct language_defn objc_language_defn = {
   c_val_print,			/* Print a value using appropriate syntax */
   c_value_print,		/* Print a top-level value */
   objc_skip_trampoline, 	/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "self",		        /* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   objc_demangle,		/* Language specific symbol demangler */
diff --git a/gdb/p-lang.c b/gdb/p-lang.c
index f7b901f..2accf35 100644
--- a/gdb/p-lang.c
+++ b/gdb/p-lang.c
@@ -414,7 +414,7 @@ const struct language_defn pascal_language_defn =
   pascal_val_print,		/* Print a value using appropriate syntax */
   pascal_value_print,		/* Print a top-level value */
   NULL,				/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  "this",		        /* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,				/* Language specific symbol demangler */
diff --git a/gdb/scm-lang.c b/gdb/scm-lang.c
index 339c614..0692d43 100644
--- a/gdb/scm-lang.c
+++ b/gdb/scm-lang.c
@@ -253,7 +253,7 @@ const struct language_defn scm_language_defn =
   scm_val_print,		/* Print a value using appropriate syntax */
   scm_value_print,		/* Print a top-level value */
   NULL,				/* Language specific skip_trampoline */
-  value_of_this,		/* value_of_this */
+  NULL,	                        /* name_of_this */
   basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,				/* Language specific symbol demangler */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index ddd2310..7d9b4ea 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1198,17 +1198,34 @@ lookup_symbol_aux (const char *name, const char 
*linkage_name,
 
   langdef = language_def (language);
 
-  if (langdef->la_value_of_this != NULL
-      && is_a_field_of_this != NULL)
+  if (langdef->la_name_of_this != NULL && is_a_field_of_this != NULL
+      && block != NULL && !dict_empty (BLOCK_DICT (block)))
     {
-      struct value *v = langdef->la_value_of_this (0);
-
-      if (v && check_field (v, name))
+      struct symbol *sym = lookup_block_symbol (block, 
langdef->la_name_of_this,
+						NULL, VAR_DOMAIN);
+      if (sym)
 	{
-	  *is_a_field_of_this = 1;
-	  if (symtab != NULL)
-	    *symtab = NULL;
-	  return NULL;
+	  struct type *t = sym->type;
+
+	  /* I'm not really sure that type of this can ever
+	     be typedefed; just be safe.  */
+	  CHECK_TYPEDEF (t);
+	  if (TYPE_CODE (t) == TYPE_CODE_PTR
+	      || TYPE_CODE (t) == TYPE_CODE_REF)
+	    t = TYPE_TARGET_TYPE (t);
+
+	  if (TYPE_CODE (t) != TYPE_CODE_STRUCT
+	      && TYPE_CODE (t) != TYPE_CODE_UNION)
+	    error (_("Internal error: `%s' is not an aggregate"), 
+		   langdef->la_name_of_this);
+	  
+	  if (check_field (t, name))
+	    {
+	      *is_a_field_of_this = 1;
+	      if (symtab != NULL)
+		*symtab = NULL;
+	      return NULL;
+	    }
 	}
     }
 
diff --git a/gdb/testsuite/gdb.cp/breakpoint.cc 
b/gdb/testsuite/gdb.cp/breakpoint.cc
index c719af2..e479efd 100644
--- a/gdb/testsuite/gdb.cp/breakpoint.cc
+++ b/gdb/testsuite/gdb.cp/breakpoint.cc
@@ -19,6 +19,13 @@
 
 class C1 {
 public:
+  C1(int i) : i_(i) {}
+
+  int foo ()
+  {
+    return 1; // conditional breakpoint in method
+  }
+
   class Nested {
   public:
     int
@@ -27,13 +34,20 @@ public:
       return 1;
     }
   };
+
+private:
+  int i_;
 };
 
 int main ()
 {
   C1::Nested c1;
 
-  c1.foo();
+  c1.foo ();
+  
+  C1 c2 (2), c3 (3);
+  c2.foo ();
+  c3.foo ();
 
   return 0;
 }
diff --git a/gdb/testsuite/gdb.cp/breakpoint.exp 
b/gdb/testsuite/gdb.cp/breakpoint.exp
index d75e0f7..3c176e9 100644
--- a/gdb/testsuite/gdb.cp/breakpoint.exp
+++ b/gdb/testsuite/gdb.cp/breakpoint.exp
@@ -61,5 +61,10 @@ proc test_breakpoint {name} {
 
 test_breakpoint "C1::Nested::foo"
 
+set bp_location1 [gdb_get_line_number "conditional breakpoint in 
method"]
+gdb_test "break $bp_location1 if i_==3" ".*Breakpoint.*" "conditional 
breakpoint in method"
+gdb_test "continue" ".*Breakpoint.*C1::foo.*" "continue to breakpoint"
+gdb_test "print i_" "\\\$1 = 3" "check the member variable"
+
 gdb_exit
 return 0
diff --git a/gdb/valops.c b/gdb/valops.c
index 80bee1e..0ef8df4 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -80,8 +80,6 @@ static enum
 oload_classification classify_oload_match (struct badness_vector *,
 					   int, int);
 
-static int check_field_in (struct type *, const char *);
-
 static struct value *value_struct_elt_for_reference (struct type *,
 						     int, struct type *,
 						     char *,
@@ -2296,12 +2294,12 @@ destructor_name_p (const char *name, const 
struct type *type)
   return 0;
 }
 
-/* Helper function for check_field: Given TYPE, a structure/union,
+/* Given TYPE, a structure/union,
    return 1 if the component named NAME from the ultimate target
    structure/union is defined, otherwise, return 0.  */
 
-static int
-check_field_in (struct type *type, const char *name)
+int
+check_field (struct type *type, const char *name)
 {
   int i;
 
@@ -2330,44 +2328,12 @@ check_field_in (struct type *type, const char 
*name)
     }
 
   for (i = TYPE_N_BASECLASSES (type) - 1; i >= 0; i--)
-    if (check_field_in (TYPE_BASECLASS (type, i), name))
+    if (check_field (TYPE_BASECLASS (type, i), name))
       return 1;
 
   return 0;
 }
 
-
-/* C++: Given ARG1, a value of type (pointer to a)* structure/union,
-   return 1 if the component named NAME from the ultimate target
-   structure/union is defined, otherwise, return 0.  */
-
-int
-check_field (struct value *arg1, const char *name)
-{
-  struct type *t;
-
-  arg1 = coerce_array (arg1);
-
-  t = value_type (arg1);
-
-  /* Follow pointers until we get to a non-pointer.  */
-
-  for (;;)
-    {
-      CHECK_TYPEDEF (t);
-      if (TYPE_CODE (t) != TYPE_CODE_PTR 
-	  && TYPE_CODE (t) != TYPE_CODE_REF)
-	break;
-      t = TYPE_TARGET_TYPE (t);
-    }
-
-  if (TYPE_CODE (t) != TYPE_CODE_STRUCT
-      && TYPE_CODE (t) != TYPE_CODE_UNION)
-    error (_("Internal error: `this' is not an aggregate"));
-
-  return check_field_in (t, name);
-}
-
 /* C++: Given an aggregate type CURTYPE, and a member name NAME,
    return the appropriate member (or the address of the member, if
    WANT_ADDRESS).  This function is used to resolve user expressions
@@ -2773,10 +2739,7 @@ value_of_local (const char *name, int complain)
 struct value *
 value_of_this (int complain)
 {
-  if (current_language->la_language == language_objc)
-    return value_of_local ("self", complain);
-  else
-    return value_of_local ("this", complain);
+  return value_of_local (current_language->la_name_of_this, complain);
 }
 
 /* Create a slice (sub-string, sub-array) of ARRAY, that is LENGTH
diff --git a/gdb/value.h b/gdb/value.h
index ba226e5..5f8fe58 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -530,7 +530,7 @@ extern void print_variable_value (struct symbol 
*var,
 				  struct frame_info *frame,
 				  struct ui_file *stream);
 
-extern int check_field (struct value *, const char *);
+extern int check_field (struct type *, const char *);
 
 extern void typedef_print (struct type *type, struct symbol *news,
 			   struct ui_file *stream);
-- 
1.5.3.5

Re: [RFA] Fix breakpoint condition that use member variables.

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sat, 22 Mar 2008 12:40:15 +0300
> 
> Suppose that foo.cpp:10 is a location inside member function of a class,
> and that said class has member variable i_, and that we've just started
> a program and are in main. Then:
> 
>     break foo.cpp:10 if i_ == 10
> 
> will not work, claiming that i_ does not exist. The problem is that
> lookup_symbol_aux uses value_of_this, which uses value_of_local and
> that totally ignores the block that is passed to parse_exp_1 by
> the breakpoint code and uses the block of the selected frame. Of course,
> that either does not have "this", or has wrong "this".
> 
> This patch fixes the problem by looking in the right block directly,
> and also by looking for the field in the type of "this", without
> trying to get the value.

What will happen after your patch if there's also a variable i_ in the
selected frame?

Re: [RFA] Fix breakpoint condition that use member variables.

[Vladimir Prus]

On Saturday 22 March 2008 15:06:35 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Sat, 22 Mar 2008 12:40:15 +0300
> >
> > Suppose that foo.cpp:10 is a location inside member function of a
> > class, and that said class has member variable i_, and that we've
> > just started a program and are in main. Then:
> >
> >     break foo.cpp:10 if i_ == 10
> >
> > will not work, claiming that i_ does not exist. The problem is that
> > lookup_symbol_aux uses value_of_this, which uses value_of_local and
> > that totally ignores the block that is passed to parse_exp_1 by
> > the breakpoint code and uses the block of the selected frame. Of
> > course, that either does not have "this", or has wrong "this".
> >
> > This patch fixes the problem by looking in the right block
> > directly, and also by looking for the field in the type of "this",
> > without trying to get the value.
>
> What will happen after your patch if there's also a variable i_ in
> the selected frame?

The i_ in the breakpoint condition will be associated with the member
variable, not with the variable with selected frame. FWIW, it's exactly
as happens with local variables -- say, with gdb 6.6, if I have ordinary 
local variable i in selected frame, and set breakpoint at 
whatever.cpp:10, and there's local variable i there, then breakpoint 
condition using 'i' will evaluate i at the breakpoint location, not
from the currently selected frame.

When lookup_symbol_aux is asked to find a symbol in a block, it will
carefully look in that block, not using selected frame in any way.
It's only the lookup for "this" that relied on selected frame.

I have modified my testcase to include a local variable called i_,
to make sure this does not confuse anything.

- Volodya

Re: [RFA] Fix breakpoint condition that use member variables.

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sat, 22 Mar 2008 15:36:06 +0300
> Cc: gdb-patches@sources.redhat.com
> 
> > What will happen after your patch if there's also a variable i_ in
> > the selected frame?
> 
> The i_ in the breakpoint condition will be associated with the member
> variable, not with the variable with selected frame.

That could surprise the user.  Is it possible to make an additional
change to look for possible other interpretations of i_ which are
currently in scope, and display a warning of some kind if such
possibilities are found?

Re: [RFA] Fix breakpoint condition that use member variables.

[Vladimir Prus]

On Saturday 22 March 2008 15:55:42 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Sat, 22 Mar 2008 15:36:06 +0300
> > Cc: gdb-patches@sources.redhat.com
> >
> > > What will happen after your patch if there's also a variable i_
> > > in the selected frame?
> >
> > The i_ in the breakpoint condition will be associated with the
> > member variable, not with the variable with selected frame.
>
> That could surprise the user.  Is it possible to make an additional
> change to look for possible other interpretations of i_ which are
> currently in scope, and display a warning of some kind if such
> possibilities are found?

The current behaviour was around for some time, and my patch only
fixes one corner case that is not consistent with the existing 
behaviour.

To answer your question -- I'm not sure. I think it would be
wrong for parse_exp_1 -- which is passed explicit block, to
go around looking at other blocks it was not asked for. We probably
can get block of selected frame, call parse_exp_1 again, in that block,
and then walk over two parse results, reporting variables which are
bound to different blocks. This sounds like some
substantial amount of work, though.

- Volodya

Re: [RFA] Fix breakpoint condition that use member variables.

[Daniel Jacobowitz]

On Sat, Mar 22, 2008 at 02:55:42PM +0200, Eli Zaretskii wrote:
> That could surprise the user.  Is it possible to make an additional
> change to look for possible other interpretations of i_ which are
> currently in scope, and display a warning of some kind if such
> possibilities are found?

This is how breakpoint conditions have worked as long as I can
remember: they are evaluated at the time the breakpoint is hit,
not at the time they are typed.  So whatever they reference has
to be in scope then.

The early parsing is mainly to give early warning if something
doesn't parse.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Fix breakpoint condition that use member variables.

[Eli Zaretskii]

> Date: Sat, 22 Mar 2008 10:49:31 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Vladimir Prus <vladimir@codesourcery.com>,
> 	gdb-patches@sources.redhat.com
> 
> On Sat, Mar 22, 2008 at 02:55:42PM +0200, Eli Zaretskii wrote:
> > That could surprise the user.  Is it possible to make an additional
> > change to look for possible other interpretations of i_ which are
> > currently in scope, and display a warning of some kind if such
> > possibilities are found?
> 
> This is how breakpoint conditions have worked as long as I can
> remember

But we could try make it better, couldn't we?

Re: [RFA] Fix breakpoint condition that use member variables.

[Michael Snyder]

On Sat, 2008-03-22 at 19:09 +0200, Eli Zaretskii wrote:
> > Date: Sat, 22 Mar 2008 10:49:31 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Vladimir Prus <vladimir@codesourcery.com>,
> > 	gdb-patches@sources.redhat.com
> > 
> > On Sat, Mar 22, 2008 at 02:55:42PM +0200, Eli Zaretskii wrote:
> > > That could surprise the user.  Is it possible to make an additional
> > > change to look for possible other interpretations of i_ which are
> > > currently in scope, and display a warning of some kind if such
> > > possibilities are found?
> > 
> > This is how breakpoint conditions have worked as long as I can
> > remember
> 
> But we could try make it better, couldn't we?

I should think that the "i_" that's chosen should be the
one in the scope of the breakpoint, not the one that is in
scope when the breakpoint is created.

Is that not what Vladimir is saying?

Re: [RFA] Fix breakpoint condition that use member variables.

[Eli Zaretskii]

> From: Michael Snyder <msnyder@specifix.com>
> Cc: Daniel Jacobowitz <drow@false.org>, vladimir@codesourcery.com,  gdb-patches@sources.redhat.com
> Date: Mon, 24 Mar 2008 11:05:27 -0700
> 
> On Sat, 2008-03-22 at 19:09 +0200, Eli Zaretskii wrote:
> > > Date: Sat, 22 Mar 2008 10:49:31 -0400
> > > From: Daniel Jacobowitz <drow@false.org>
> > > Cc: Vladimir Prus <vladimir@codesourcery.com>,
> > > 	gdb-patches@sources.redhat.com
> > > 
> > > On Sat, Mar 22, 2008 at 02:55:42PM +0200, Eli Zaretskii wrote:
> > > > That could surprise the user.  Is it possible to make an additional
> > > > change to look for possible other interpretations of i_ which are
> > > > currently in scope, and display a warning of some kind if such
> > > > possibilities are found?
> > > 
> > > This is how breakpoint conditions have worked as long as I can
> > > remember
> > 
> > But we could try make it better, couldn't we?
> 
> I should think that the "i_" that's chosen should be the
> one in the scope of the breakpoint, not the one that is in
> scope when the breakpoint is created.
> 
> Is that not what Vladimir is saying?

I don't know (I'm not Vladimir), but that's not *exactly* what I was
saying.  Imagine the situation where: you have the inferior stopped in
a function that lives in a module which has a variable i_ in global
scope.  You decide to place a breakpoint in some other function in the
same module, and condition it on i_.  So far so good; however,
unbeknownst to you, there's also a local variable i_ in the function
where you put the breakpoint.  How can this be unbeknownst, you ask?
easy: suppose you not really look into that function, just see its
call, and want to step through it.

IOW, the use case I was thinking of is when i_ happens to be in scope
both when you set the breakpoint _and_ when it breaks.

Re: [RFA] Fix breakpoint condition that use member variables.

[Michael Snyder]

On Mon, 2008-03-24 at 22:17 +0200, Eli Zaretskii wrote:
> > From: Michael Snyder <msnyder@specifix.com>
> > Cc: Daniel Jacobowitz <drow@false.org>, vladimir@codesourcery.com,  gdb-patches@sources.redhat.com
> > Date: Mon, 24 Mar 2008 11:05:27 -0700
> > 
> > On Sat, 2008-03-22 at 19:09 +0200, Eli Zaretskii wrote:
> > > > Date: Sat, 22 Mar 2008 10:49:31 -0400
> > > > From: Daniel Jacobowitz <drow@false.org>
> > > > Cc: Vladimir Prus <vladimir@codesourcery.com>,
> > > > 	gdb-patches@sources.redhat.com
> > > > 
> > > > On Sat, Mar 22, 2008 at 02:55:42PM +0200, Eli Zaretskii wrote:
> > > > > That could surprise the user.  Is it possible to make an additional
> > > > > change to look for possible other interpretations of i_ which are
> > > > > currently in scope, and display a warning of some kind if such
> > > > > possibilities are found?
> > > > 
> > > > This is how breakpoint conditions have worked as long as I can
> > > > remember
> > > 
> > > But we could try make it better, couldn't we?
> > 
> > I should think that the "i_" that's chosen should be the
> > one in the scope of the breakpoint, not the one that is in
> > scope when the breakpoint is created.
> > 
> > Is that not what Vladimir is saying?
> 
> I don't know (I'm not Vladimir), but that's not *exactly* what I was
> saying.  Imagine the situation where: you have the inferior stopped in
> a function that lives in a module which has a variable i_ in global
> scope.  You decide to place a breakpoint in some other function in the
> same module, and condition it on i_.  So far so good; however,
> unbeknownst to you, there's also a local variable i_ in the function
> where you put the breakpoint.  How can this be unbeknownst, you ask?
> easy: suppose you not really look into that function, just see its
> call, and want to step through it.
> 
> IOW, the use case I was thinking of is when i_ happens to be in scope
> both when you set the breakpoint _and_ when it breaks.

OK, but if that's an issue, it's always been an issue.
The present behavior is that the 'i_' that's local to 
the scope of the breakpoint wins.

Vladimir's change just makes it more consistant, in that
the 'i_ that's in the breakpoint scope still wins, but now
it wins even if it's in the "class" scope of the breakpoint, 
not just the automatic scope.

Re: [RFA] Fix breakpoint condition that use member variables.

[Daniel Jacobowitz]

Eli's raised a valid concern, but I think it's unrelated to this
change, so I will review this patch independently of it.

On Sat, Mar 22, 2008 at 12:40:15PM +0300, Vladimir Prus wrote:
> -    /* If this is non-NULL, lookup_symbol will do the 'field_of_this'
> -       check, using this function to find the value of this.  */
> +    /* If this is non-NULL, specifies the name that of the implicit
> +       local variable that refers to the current object instance.  */

Drop "that" in the first line.

> diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c
> index 6b51fd5..400338e 100644
> --- a/gdb/m2-lang.c
> +++ b/gdb/m2-lang.c
> @@ -375,7 +375,7 @@ const struct language_defn m2_language_defn =
>    m2_val_print,			/* Print a value using appropriate syntax */
>    c_value_print,		/* Print a top-level value */
>    NULL,				/* Language specific skip_trampoline */
> -  value_of_this,		/* value_of_this */
> +  "this",		        /* name_of_this */
>    basic_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
>    basic_lookup_transparent_type,/* lookup_transparent_type */
>    NULL,				/* Language specific symbol demangler */

This can be NULL.

> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index ddd2310..7d9b4ea 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -1198,17 +1198,34 @@ lookup_symbol_aux (const char *name, const char 
> *linkage_name,
>  
>    langdef = language_def (language);
>  
> -  if (langdef->la_value_of_this != NULL
> -      && is_a_field_of_this != NULL)
> +  if (langdef->la_name_of_this != NULL && is_a_field_of_this != NULL
> +      && block != NULL && !dict_empty (BLOCK_DICT (block)))
>      {
> -      struct value *v = langdef->la_value_of_this (0);
> -
> -      if (v && check_field (v, name))
> +      struct symbol *sym = lookup_block_symbol (block, 
> langdef->la_name_of_this,
> +						NULL, VAR_DOMAIN);

value_of_local always gets the function block.  This might have an
inner block that does not correspond to the function.  I think you
need to find the enclosing function before checking dict_empty or
calling lookup_block_symbol.  You could use block_function to get
the symbol and then SYMBOL_BLOCK_VALUE to get the containing
block, or just walk up BLOCK_SUPERBLOCK looking for BLOCK_FUNCTION.

> @@ -2773,10 +2739,7 @@ value_of_local (const char *name, int complain)
>  struct value *
>  value_of_this (int complain)
>  {
> -  if (current_language->la_language == language_objc)
> -    return value_of_local ("self", complain);
> -  else
> -    return value_of_local ("this", complain);
> +  return value_of_local (current_language->la_name_of_this, complain);
>  }
>  
>  /* Create a slice (sub-string, sub-array) of ARRAY, that is LENGTH

Is this going to crash if we get here for a language where it is NULL?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Fix breakpoint condition that use member variables.

[Vladimir Prus]

On Tuesday 01 April 2008 17:59:52 Daniel Jacobowitz wrote:

> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index ddd2310..7d9b4ea 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -1198,17 +1198,34 @@ lookup_symbol_aux (const char *name, const char 
> > *linkage_name,
> >  
> >    langdef = language_def (language);
> >  
> > -  if (langdef->la_value_of_this != NULL
> > -      && is_a_field_of_this != NULL)
> > +  if (langdef->la_name_of_this != NULL && is_a_field_of_this != NULL
> > +      && block != NULL && !dict_empty (BLOCK_DICT (block)))
> >      {
> > -      struct value *v = langdef->la_value_of_this (0);
> > -
> > -      if (v && check_field (v, name))
> > +      struct symbol *sym = lookup_block_symbol (block, 
> > langdef->la_name_of_this,
> > +						NULL, VAR_DOMAIN);
> 
> value_of_local always gets the function block.  This might have an
> inner block that does not correspond to the function.  I think you
> need to find the enclosing function before checking dict_empty or
> calling lookup_block_symbol.  You could use block_function to get
> the symbol and then SYMBOL_BLOCK_VALUE to get the containing
> block, or just walk up BLOCK_SUPERBLOCK looking for BLOCK_FUNCTION.

Done, and added a new test that setting a breakpoint inside an inner
block of a function actually finds this's fields.

> > @@ -2773,10 +2739,7 @@ value_of_local (const char *name, int complain)
> >  struct value *
> >  value_of_this (int complain)
> >  {
> > -  if (current_language->la_language == language_objc)
> > -    return value_of_local ("self", complain);
> > -  else
> > -    return value_of_local ("this", complain);
> > +  return value_of_local (current_language->la_name_of_this, complain);
> >  }
> >  
> >  /* Create a slice (sub-string, sub-array) of ARRAY, that is LENGTH
> 
> Is this going to crash if we get here for a language where it is NULL?

Yes. It's not very good idea to call value_of_this in a language that
has no this, but for safety, I've added a check.

I attach the revised patch, OK?

- Volodya

        [gdb]
        * valops.c (check_field): Remove.
        (check_field_in): Rename to check_field.
        (value_of_this): Use la_name_of_this.
        * value.h (check_field): Adjust prototype.

        * language.h (la_value_of_this): Rename to la_name_of_this.
        * language.c (unknown_language_defn): Specify "this" for
        name_of_this.
        (auto_language_defn): Likewise.
        (local_language_defn): Likewise.
        * ada-lang.c (ada_language_defn): Adjust comment.
        * c-lang.c (c_language_defn): Adjust comment.
        (cplus_language_defn): Specify "this" for name_of_this.
        (asm_language_defn): Adjust comment.
        (minimal_language_defn): Adjust comment.
        * f-lang.c (f_language_defn): Specify NULL for name_of_this.
        * jv-lang.c (java_language_defn): Specify "this" for name_of_this.
        * m2-lang.c (m2_language_defn): Specify "this" for name_of_this.
        * objc-lang.c (objc_language_defn): Specify "self" for
        name_of_this.
        * p-lang.c (pascal_language_defn): Specify "this" for
        name_of_this.
        * scm-lang.c (scm_language_defn): Specify NULL for name_of_this.

        * symtab.c (lookup_symbol_aux): Lookup "this" in the
        proper scope, and check for field in type of "this", without
        trying to create a value.

        [gdb/testsuite]
        * gdb.cp/breakpoint.cc: New code to test conditions involving
        member variables.
        * gdb.cp/breakpoint.exp: Test condition involving member
        variables.
---
 gdb/ada-lang.c                      |    2 +-
 gdb/c-lang.c                        |    8 +++---
 gdb/f-lang.c                        |    2 +-
 gdb/jv-lang.c                       |    2 +-
 gdb/language.c                      |    6 ++--
 gdb/language.h                      |   10 ++-----
 gdb/m2-lang.c                       |    2 +-
 gdb/objc-lang.c                     |    2 +-
 gdb/p-lang.c                        |    2 +-
 gdb/scm-lang.c                      |    2 +-
 gdb/symtab.c                        |   42 +++++++++++++++++++++++------
 gdb/testsuite/gdb.cp/breakpoint.cc  |   29 ++++++++++++++++++++-
 gdb/testsuite/gdb.cp/breakpoint.exp |   10 +++++++
 gdb/valops.c                        |   49 +++++------------------------------
 gdb/value.h                         |    2 +-
 15 files changed, 96 insertions(+), 74 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 2f0f55f..8cfc2c8 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10999,7 +10999,7 @@ const struct language_defn ada_language_defn = {
   ada_val_print,                /* Print a value using appropriate syntax */
   ada_value_print,              /* Print a top-level value */
   NULL,                         /* Language specific skip_trampoline */
-  NULL,                         /* value_of_this */
+  NULL,                         /* name_of_this */
   ada_lookup_symbol_nonlocal,   /* Looking up non-local symbols.  */
   basic_lookup_transparent_type,        /* lookup_transparent_type */
   ada_la_decode,                /* Language specific symbol demangler */
diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index c4ef9d6..29aa765 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -414,7 +414,7 @@ const struct language_defn c_language_defn =
   c_val_print,                 /* Print a value using appropriate syntax */
   c_value_print,               /* Print a top-level value */
   NULL,                                /* Language specific skip_trampoline */
-  NULL,                                /* value_of_this */
+  NULL,                                /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,                                /* Language specific symbol demangler */
@@ -527,7 +527,7 @@ const struct language_defn cplus_language_defn =
   c_val_print,                 /* Print a value using appropriate syntax */
   c_value_print,               /* Print a top-level value */
   cplus_skip_trampoline,       /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  "this",                       /* name_of_this */
   cp_lookup_symbol_nonlocal,   /* lookup_symbol_nonlocal */
   cp_lookup_transparent_type,   /* lookup_transparent_type */
   cplus_demangle,              /* Language specific symbol demangler */
@@ -562,7 +562,7 @@ const struct language_defn asm_language_defn =
   c_val_print,                 /* Print a value using appropriate syntax */
   c_value_print,               /* Print a top-level value */
   NULL,                                /* Language specific skip_trampoline */
-  NULL,                                /* value_of_this */
+  NULL,                                /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,                                /* Language specific symbol demangler */
@@ -602,7 +602,7 @@ const struct language_defn minimal_language_defn =
   c_val_print,                 /* Print a value using appropriate syntax */
   c_value_print,               /* Print a top-level value */
   NULL,                                /* Language specific skip_trampoline */
-  NULL,                                /* value_of_this */
+  NULL,                                /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,                                /* Language specific symbol demangler */
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 038126c..5dcbd33 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -324,7 +324,7 @@ const struct language_defn f_language_defn =
   f_val_print,                 /* Print a value using appropriate syntax */
   c_value_print,               /* FIXME */
   NULL,                                /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  NULL,                        /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,                                /* Language specific symbol demangler */
diff --git a/gdb/jv-lang.c b/gdb/jv-lang.c
index e6c6f7d..b72b90c 100644
--- a/gdb/jv-lang.c
+++ b/gdb/jv-lang.c
@@ -1070,7 +1070,7 @@ const struct language_defn java_language_defn =
   java_val_print,              /* Print a value using appropriate syntax */
   java_value_print,            /* Print a top-level value */
   NULL,                                /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  "this",                      /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   java_demangle,               /* Language specific symbol demangler */
diff --git a/gdb/language.c b/gdb/language.c
index a26218d..80f6961 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -1192,7 +1192,7 @@ const struct language_defn unknown_language_defn =
   unk_lang_val_print,          /* Print a value using appropriate syntax */
   unk_lang_value_print,                /* Print a top-level value */
   unk_lang_trampoline,         /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  "this",                      /* name_of_this */
   basic_lookup_symbol_nonlocal, /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   unk_lang_demangle,           /* Language specific symbol demangler */
@@ -1228,7 +1228,7 @@ const struct language_defn auto_language_defn =
   unk_lang_val_print,          /* Print a value using appropriate syntax */
   unk_lang_value_print,                /* Print a top-level value */
   unk_lang_trampoline,         /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  "this",                      /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   unk_lang_demangle,           /* Language specific symbol demangler */
@@ -1263,7 +1263,7 @@ const struct language_defn local_language_defn =
   unk_lang_val_print,          /* Print a value using appropriate syntax */
   unk_lang_value_print,                /* Print a top-level value */
   unk_lang_trampoline,         /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  "this",                      /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   unk_lang_demangle,           /* Language specific symbol demangler */
diff --git a/gdb/language.h b/gdb/language.h
index f7e654d..233a5a3 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -206,14 +206,10 @@ struct language_defn
 
     /* Now come some hooks for lookup_symbol.  */
 
-    /* If this is non-NULL, lookup_symbol will do the 'field_of_this'
-       check, using this function to find the value of this.  */
+    /* If this is non-NULL, specifies the name that of the implicit
+       local variable that refers to the current object instance.  */
 
-    /* FIXME: carlton/2003-05-19: Audit all the language_defn structs
-       to make sure we're setting this appropriately: I'm sure it
-       could be NULL in more languages.  */
-
-    struct value *(*la_value_of_this) (int complain);
+    char *la_name_of_this;
 
     /* This is a function that lookup_symbol will call when it gets to
        the part of symbol lookup where C looks up static and global
diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c
index 6b51fd5..bb205ad 100644
--- a/gdb/m2-lang.c
+++ b/gdb/m2-lang.c
@@ -375,7 +375,7 @@ const struct language_defn m2_language_defn =
   m2_val_print,                        /* Print a value using appropriate syntax */
   c_value_print,               /* Print a top-level value */
   NULL,                                /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  NULL,                                /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,                                /* Language specific symbol demangler */
diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index ccf8068..08a6bb8 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -509,7 +509,7 @@ const struct language_defn objc_language_defn = {
   c_val_print,                 /* Print a value using appropriate syntax */
   c_value_print,               /* Print a top-level value */
   objc_skip_trampoline,        /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  "self",                      /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   objc_demangle,               /* Language specific symbol demangler */
diff --git a/gdb/p-lang.c b/gdb/p-lang.c
index f7b901f..2accf35 100644
--- a/gdb/p-lang.c
+++ b/gdb/p-lang.c
@@ -414,7 +414,7 @@ const struct language_defn pascal_language_defn =
   pascal_val_print,            /* Print a value using appropriate syntax */
   pascal_value_print,          /* Print a top-level value */
   NULL,                                /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  "this",                      /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,                                /* Language specific symbol demangler */
diff --git a/gdb/scm-lang.c b/gdb/scm-lang.c
index 339c614..0692d43 100644
--- a/gdb/scm-lang.c
+++ b/gdb/scm-lang.c
@@ -253,7 +253,7 @@ const struct language_defn scm_language_defn =
   scm_val_print,               /* Print a value using appropriate syntax */
   scm_value_print,             /* Print a top-level value */
   NULL,                                /* Language specific skip_trampoline */
-  value_of_this,               /* value_of_this */
+  NULL,                                /* name_of_this */
   basic_lookup_symbol_nonlocal,        /* lookup_symbol_nonlocal */
   basic_lookup_transparent_type,/* lookup_transparent_type */
   NULL,                                /* Language specific symbol demangler */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index ddd2310..6a452f7 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1198,17 +1198,41 @@ lookup_symbol_aux (const char *name, const char *linkage_name,
 
   langdef = language_def (language);
 
-  if (langdef->la_value_of_this != NULL
-      && is_a_field_of_this != NULL)
+  if (langdef->la_name_of_this != NULL && is_a_field_of_this != NULL
+      && block != NULL)
     {
-      struct value *v = langdef->la_value_of_this (0);
-
-      if (v && check_field (v, name))
+      struct symbol *sym = NULL;
+      /* 'this' is only defined in the function's block, so find the
+        enclosing function block.  */
+      for (; block && !BLOCK_FUNCTION (block); 
+          block = BLOCK_SUPERBLOCK (block));
+
+      if (block && !dict_empty (BLOCK_DICT (block)))
+       sym = lookup_block_symbol (block, langdef->la_name_of_this,
+                                  NULL, VAR_DOMAIN);
+      if (sym)
        {
-         *is_a_field_of_this = 1;
-         if (symtab != NULL)
-           *symtab = NULL;
-         return NULL;
+         struct type *t = sym->type;
+         
+         /* I'm not really sure that type of this can ever
+            be typedefed; just be safe.  */
+         CHECK_TYPEDEF (t);
+         if (TYPE_CODE (t) == TYPE_CODE_PTR
+             || TYPE_CODE (t) == TYPE_CODE_REF)
+           t = TYPE_TARGET_TYPE (t);
+         
+         if (TYPE_CODE (t) != TYPE_CODE_STRUCT
+             && TYPE_CODE (t) != TYPE_CODE_UNION)
+           error (_("Internal error: `%s' is not an aggregate"), 
+                  langdef->la_name_of_this);
+         
+         if (check_field (t, name))
+           {
+             *is_a_field_of_this = 1;
+             if (symtab != NULL)
+               *symtab = NULL;
+             return NULL;
+           }
        }
     }
 
diff --git a/gdb/testsuite/gdb.cp/breakpoint.cc b/gdb/testsuite/gdb.cp/breakpoint.cc
index c719af2..98b7891 100644
--- a/gdb/testsuite/gdb.cp/breakpoint.cc
+++ b/gdb/testsuite/gdb.cp/breakpoint.cc
@@ -17,8 +17,26 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+int g = 0;
+
 class C1 {
 public:
+  C1(int i) : i_(i) {}
+
+  int foo ()
+  {
+    return 1; // conditional breakpoint in method
+  }
+
+  int bar ()
+  {
+    for (int i = 0; i < 1; ++i)
+      {
+       int t = i * 2;
+       g += t; // conditional breakpoint in method 2
+      }
+  }
+
   class Nested {
   public:
     int
@@ -27,13 +45,22 @@ public:
       return 1;
     }
   };
+
+private:
+  int i_;
 };
 
 int main ()
 {
   C1::Nested c1;
 
-  c1.foo();
+  c1.foo ();
+  
+  C1 c2 (2), c3 (3);
+  c2.foo ();
+  c2.bar ();
+  c3.foo ();
+  c3.bar ();
 
   return 0;
 }
diff --git a/gdb/testsuite/gdb.cp/breakpoint.exp b/gdb/testsuite/gdb.cp/breakpoint.exp
index d75e0f7..57f2fd4 100644
--- a/gdb/testsuite/gdb.cp/breakpoint.exp
+++ b/gdb/testsuite/gdb.cp/breakpoint.exp
@@ -61,5 +61,15 @@ proc test_breakpoint {name} {
 
 test_breakpoint "C1::Nested::foo"
 
+set bp_location1 [gdb_get_line_number "conditional breakpoint in method"]
+set bp_location2 [gdb_get_line_number "conditional breakpoint in method 2"]
+gdb_test "break $bp_location1 if i_==3" ".*Breakpoint.*" "conditional breakpoint in method"
+gdb_test "break $bp_location2 if i_==3" ".*Breakpoint.*" "conditional breakpoint in method 2"
+gdb_test "continue" ".*Breakpoint.*C1::foo.*" "continue to breakpoint"
+gdb_test "print i_" "\\\$1 = 3" "check the member variable"
+gdb_test "continue" ".*Breakpoint.*C1::bar.*" "continue to breakpoint"
+gdb_test "print i_" "\\\$2 = 3" "check the member variable"
+
+
 gdb_exit
 return 0
diff --git a/gdb/valops.c b/gdb/valops.c
index 80bee1e..5b05035 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -80,8 +80,6 @@ static enum
 oload_classification classify_oload_match (struct badness_vector *,
                                           int, int);
 
-static int check_field_in (struct type *, const char *);
-
 static struct value *value_struct_elt_for_reference (struct type *,
                                                     int, struct type *,
                                                     char *,
@@ -2296,12 +2294,12 @@ destructor_name_p (const char *name, const struct type *type)
   return 0;
 }
 
-/* Helper function for check_field: Given TYPE, a structure/union,
+/* Given TYPE, a structure/union,
    return 1 if the component named NAME from the ultimate target
    structure/union is defined, otherwise, return 0.  */
 
-static int
-check_field_in (struct type *type, const char *name)
+int
+check_field (struct type *type, const char *name)
 {
   int i;
 
@@ -2330,44 +2328,12 @@ check_field_in (struct type *type, const char *name)
     }
 
   for (i = TYPE_N_BASECLASSES (type) - 1; i >= 0; i--)
-    if (check_field_in (TYPE_BASECLASS (type, i), name))
+    if (check_field (TYPE_BASECLASS (type, i), name))
       return 1;
 
   return 0;
 }
 
-
-/* C++: Given ARG1, a value of type (pointer to a)* structure/union,
-   return 1 if the component named NAME from the ultimate target
-   structure/union is defined, otherwise, return 0.  */
-
-int
-check_field (struct value *arg1, const char *name)
-{
-  struct type *t;
-
-  arg1 = coerce_array (arg1);
-
-  t = value_type (arg1);
-
-  /* Follow pointers until we get to a non-pointer.  */
-
-  for (;;)
-    {
-      CHECK_TYPEDEF (t);
-      if (TYPE_CODE (t) != TYPE_CODE_PTR 
-         && TYPE_CODE (t) != TYPE_CODE_REF)
-       break;
-      t = TYPE_TARGET_TYPE (t);
-    }
-
-  if (TYPE_CODE (t) != TYPE_CODE_STRUCT
-      && TYPE_CODE (t) != TYPE_CODE_UNION)
-    error (_("Internal error: `this' is not an aggregate"));
-
-  return check_field_in (t, name);
-}
-
 /* C++: Given an aggregate type CURTYPE, and a member name NAME,
    return the appropriate member (or the address of the member, if
    WANT_ADDRESS).  This function is used to resolve user expressions
@@ -2773,10 +2739,9 @@ value_of_local (const char *name, int complain)
 struct value *
 value_of_this (int complain)
 {
-  if (current_language->la_language == language_objc)
-    return value_of_local ("self", complain);
-  else
-    return value_of_local ("this", complain);
+  if (!current_language->la_name_of_this)
+    return 0;
+  return value_of_local (current_language->la_name_of_this, complain);
 }
 
 /* Create a slice (sub-string, sub-array) of ARRAY, that is LENGTH
diff --git a/gdb/value.h b/gdb/value.h
index ba226e5..5f8fe58 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -530,7 +530,7 @@ extern void print_variable_value (struct symbol *var,
                                  struct frame_info *frame,
                                  struct ui_file *stream);
 
-extern int check_field (struct value *, const char *);
+extern int check_field (struct type *, const char *);
 
 extern void typedef_print (struct type *type, struct symbol *news,
                           struct ui_file *stream);
-- 
1.5.3.5
 

Re: [RFA] Fix breakpoint condition that use member variables.

[Daniel Jacobowitz]

On Thu, Apr 03, 2008 at 04:11:19PM +0400, Vladimir Prus wrote:
> 
> I attach the revised patch, OK?

OK.

-- 
Daniel Jacobowitz
CodeSourcery