diff mbox

[RFA] Use watchpoint's language when re-parsing expression

Message ID 20170518023916.19913-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey May 18, 2017, 2:39 a.m. UTC 
  PR rust/21484 notes that watch -location does not work with Rust:

    (gdb) watch -location a
    syntax error in expression, near `) 0x00007fffffffe0f4'.

update_watchpoint tries to tell gdb that the new expression it creates
has C syntax:

      /* The above expression is in C.  */
      b->language = language_c;

However, update_watchpoint doesn't actually use this language when
re-parsing the expression.

Originally I was going to fix this by saving and restoring the
language in update_watchpoint, but this regressed
gdb.dlang/watch-loc.exp, because the constructed expression actually
has D syntax (specifically the name is not parseable by C).

Next I looked at directly constructing an expression, and not relying
on the parser at all; but it seemed to me that upon a re-set, we'd
want to reparse the type, and there is no existing API to do this
correctly.

So, in the end I made a hook to let each language choose what
expression to use.  I made all the languages other than Rust use the C
expression, because that is the status quo ante.  However, this is
probably not truly correct.  After this patch, at least, it is easy to
correct by someone who knows the language(s) in question.

Regtested by the buildbot.

gdb/ChangeLog
2017-05-17  Tom Tromey  <tom@tromey.com>

	PR rust/21484:
	* rust-lang.c (exp_descriptor_rust): New function.
	(rust_language_defn): Use it.
	* p-lang.c (pascal_language_defn): Update.
	* opencl-lang.c (opencl_language_defn): Update.
	* objc-lang.c (objc_language_defn): Update.
	* m2-lang.c (m2_language_defn): Update.
	* language.h (struct language_defn)
	<la_watch_location_expression>: New member.
	* language.c (unknown_language_defn, auto_language_defn)
	(local_language_defn): Update.
	* go-lang.c (go_language_defn): Update.
	* f-lang.c (f_language_defn): Update.
	* d-lang.c (d_language_defn): Update.
	* c-lang.h (c_watch_location_expression): Declare.
	* c-lang.c (c_watch_location_expression): New function.
	(c_language_defn, cplus_language_defn, asm_language_defn)
	(minimal_language_defn): Use it.
	* breakpoint.c (watch_command_1): Call
	la_watch_location_expression.
	* ada-lang.c (ada_language_defn): Update.

gdb/testsuite/ChangeLog
2017-05-17  Tom Tromey  <tom@tromey.com>

	PR rust/21484:
	* gdb.rust/watch.exp: New file.
	* gdb.rust/watch.rs: New file.
---
 gdb/ChangeLog                    | 24 ++++++++++++++++++++++++
 gdb/ada-lang.c                   |  1 +
 gdb/breakpoint.c                 | 11 ++---------
 gdb/c-lang.c                     | 14 ++++++++++++++
 gdb/c-lang.h                     |  3 +++
 gdb/d-lang.c                     |  1 +
 gdb/f-lang.c                     |  1 +
 gdb/go-lang.c                    |  1 +
 gdb/language.c                   |  4 ++++
 gdb/language.h                   |  6 ++++++
 gdb/m2-lang.c                    |  1 +
 gdb/objc-lang.c                  |  1 +
 gdb/opencl-lang.c                |  1 +
 gdb/p-lang.c                     |  2 ++
 gdb/rust-lang.c                  | 15 +++++++++++++++
 gdb/testsuite/ChangeLog          |  6 ++++++
 gdb/testsuite/gdb.rust/watch.exp | 35 +++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.rust/watch.rs  | 24 ++++++++++++++++++++++++
 18 files changed, 142 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.rust/watch.exp
 create mode 100644 gdb/testsuite/gdb.rust/watch.rs

Comments

Andreas Schwab May 18, 2017, 7:37 a.m. UTC | #1 
On Mai 17 2017, Tom Tromey <tom@tromey.com> wrote:

> So, in the end I made a hook to let each language choose what
> expression to use.  I made all the languages other than Rust use the C
> expression, because that is the status quo ante.  However, this is
> probably not truly correct.  After this patch, at least, it is easy to
> correct by someone who knows the language(s) in question.

The docs say that gdb is supposed to support C style casts in every
language.  Also, there is the {type} operator that is also supported in
every language.

Andreas.
Tom Tromey May 18, 2017, 9:41 p.m. UTC | #2 
>>>>> "Andreas" == Andreas Schwab <schwab@suse.de> writes:

Andreas> On Mai 17 2017, Tom Tromey <tom@tromey.com> wrote:
>> So, in the end I made a hook to let each language choose what
>> expression to use.  I made all the languages other than Rust use the C
>> expression, because that is the status quo ante.  However, this is
>> probably not truly correct.  After this patch, at least, it is easy to
>> correct by someone who knows the language(s) in question.

Andreas> The docs say that gdb is supposed to support C style casts in every
Andreas> language.  Also, there is the {type} operator that is also supported in
Andreas> every language.

I never noticed that before.  However, I don't think it is a good reason
to reject this patch.

First, the docs are simply incorrect now, as the Rust parser doesn't
support this, and I don't think it ought to.  The docs state the
rationale for this as being the usefulness of casting -- and while it's
indeed useful, Rust already supports a syntax for this, so implementing
the C syntax additionally seems strange.

Second, while the cast is supported, nothing in the docs claims that the
prefix "*" is supported by all languages.  So, even if you accept the
cast part of the expression (which you shouldn't), the expression as a
whole is still wrong.

Finally, I'm not at all certain that C-style casts or the {type}
operator can be added to the Rust expression parser without breaking
something else.  And, that seems like a complex request just to unbreak
what amount to an implementation detail.

Tom
Joel Brobecker May 19, 2017, 1:51 p.m. UTC | #3 
> Andreas> The docs say that gdb is supposed to support C style casts in every
> Andreas> language.  Also, there is the {type} operator that is also supported in
> Andreas> every language.
[...]
> Finally, I'm not at all certain that C-style casts or the {type}
> operator can be added to the Rust expression parser without breaking
> something else.  And, that seems like a complex request just to unbreak
> what amount to an implementation detail.

FWIW, I also never noticed this, and I share the concerns about
making this a requirement for all languages. It really depends
on the language itself whether it's easy to implement such feature,
and I would hate for language parsers to be harder to maintain
just so we can provide a bit of a C-centric view to every language
we support.

I vote for downgrading the requirement and amending the documentation
instead.
Pedro Alves May 19, 2017, 2:32 p.m. UTC | #4 
On 05/19/2017 02:51 PM, Joel Brobecker wrote:
>> Andreas> The docs say that gdb is supposed to support C style casts in every
>> Andreas> language.  Also, there is the {type} operator that is also supported in
>> Andreas> every language.
> [...]
>> Finally, I'm not at all certain that C-style casts or the {type}
>> operator can be added to the Rust expression parser without breaking
>> something else.  And, that seems like a complex request just to unbreak
>> what amount to an implementation detail.
> 
> FWIW, I also never noticed this, and I share the concerns about
> making this a requirement for all languages. It really depends
> on the language itself whether it's easy to implement such feature,
> and I would hate for language parsers to be harder to maintain
> just so we can provide a bit of a C-centric view to every language
> we support.
> 
> I vote for downgrading the requirement and amending the documentation
> instead.
> 

I agree.  The patch LGTM.

Thanks,
Pedro Alves
Tom Tromey May 20, 2017, 4:07 a.m. UTC | #5 
Pedro> I agree.  The patch LGTM.

Thanks, I'm checking it in.

Tom
Andreas Schwab May 20, 2017, 8:24 a.m. UTC | #6 
Please also update the documentation.

Andreas.
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 980bd3f..7fa861c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,27 @@ 
+2017-05-17  Tom Tromey  <tom@tromey.com>
+
+	PR rust/21484:
+	* rust-lang.c (exp_descriptor_rust): New function.
+	(rust_language_defn): Use it.
+	* p-lang.c (pascal_language_defn): Update.
+	* opencl-lang.c (opencl_language_defn): Update.
+	* objc-lang.c (objc_language_defn): Update.
+	* m2-lang.c (m2_language_defn): Update.
+	* language.h (struct language_defn)
+	<la_watch_location_expression>: New member.
+	* language.c (unknown_language_defn, auto_language_defn)
+	(local_language_defn): Update.
+	* go-lang.c (go_language_defn): Update.
+	* f-lang.c (f_language_defn): Update.
+	* d-lang.c (d_language_defn): Update.
+	* c-lang.h (c_watch_location_expression): Declare.
+	* c-lang.c (c_watch_location_expression): New function.
+	(c_language_defn, cplus_language_defn, asm_language_defn)
+	(minimal_language_defn): Use it.
+	* breakpoint.c (watch_command_1): Call
+	la_watch_location_expression.
+	* ada-lang.c (ada_language_defn): Update.
+
 2017-05-15  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	* procfs.c (procfs_create_inferior): Change prototype to match
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 58c8a2e..25c49c5 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -14047,6 +14047,7 @@  const struct language_defn ada_language_defn = {
   ada_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   ada_get_symbol_name_cmp,	/* la_get_symbol_name_cmp */
   ada_iterate_over_symbols,
   &ada_varobj_ops,
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 9d6a2f4..150b08c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11256,18 +11256,11 @@  watch_command_1 (const char *arg, int accessflag, int from_tty,
       struct type *t = value_type (val);
       CORE_ADDR addr = value_as_address (val);
 
-      t = check_typedef (TYPE_TARGET_TYPE (check_typedef (t)));
-
-      std::string name = type_to_string (t);
-
-      w->exp_string_reparse = xstrprintf ("* (%s *) %s", name.c_str (),
-					  core_addr_to_string (addr));
+      w->exp_string_reparse
+	= current_language->la_watch_location_expression (t, addr).release ();
 
       w->exp_string = xstrprintf ("-location %.*s",
 				  (int) (exp_end - exp_start), exp_start);
-
-      /* The above expression is in C.  */
-      b->language = language_c;
     }
   else
     w->exp_string = savestring (exp_start, exp_end - exp_start);
diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 19a8608..a6d533d 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -712,7 +712,17 @@  evaluate_subexp_c (struct type *expect_type, struct expression *exp,
     }
   return evaluate_subexp_standard (expect_type, exp, pos, noside);
 }
+
+/* la_watch_location_expression for C.  */
 
+gdb::unique_xmalloc_ptr<char>
+c_watch_location_expression (struct type *type, CORE_ADDR addr)
+{
+  type = check_typedef (TYPE_TARGET_TYPE (check_typedef (type)));
+  std::string name = type_to_string (type);
+  return gdb::unique_xmalloc_ptr<char>
+    (xstrprintf ("* (%s *) %s", name.c_str (), core_addr_to_string (addr)));
+}
 
 
 /* Table mapping opcodes into strings for printing operators
@@ -865,6 +875,7 @@  const struct language_defn c_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &c_varobj_ops,
@@ -1008,6 +1019,7 @@  const struct language_defn cplus_language_defn =
   default_print_array_index,
   cp_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &cplus_varobj_ops,
@@ -1060,6 +1072,7 @@  const struct language_defn asm_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
@@ -1112,6 +1125,7 @@  const struct language_defn minimal_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/c-lang.h b/gdb/c-lang.h
index 9746238..5ce6486 100644
--- a/gdb/c-lang.h
+++ b/gdb/c-lang.h
@@ -112,6 +112,9 @@  extern void c_emit_char (int c, struct type *type,
 
 extern const struct op_print c_op_print_tab[];
 
+extern gdb::unique_xmalloc_ptr<char> c_watch_location_expression
+     (struct type *type, CORE_ADDR addr);
+
 /* These are in c-typeprint.c: */
 
 extern void c_type_print_base (struct type *, struct ui_file *,
diff --git a/gdb/d-lang.c b/gdb/d-lang.c
index 3ffdbdf..434b2e6 100644
--- a/gdb/d-lang.c
+++ b/gdb/d-lang.c
@@ -244,6 +244,7 @@  static const struct language_defn d_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 3c30d75..a9663d4 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -287,6 +287,7 @@  const struct language_defn f_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   default_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/go-lang.c b/gdb/go-lang.c
index 88ddfb6..3b80cec 100644
--- a/gdb/go-lang.c
+++ b/gdb/go-lang.c
@@ -605,6 +605,7 @@  static const struct language_defn go_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   NULL,
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/language.c b/gdb/language.c
index ee0101d..e9d9803 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -43,6 +43,7 @@ 
 #include "symfile.h"
 #include "cp-support.h"
 #include "frame.h"
+#include "c-lang.h"
 
 extern void _initialize_language (void);
 
@@ -859,6 +860,7 @@  const struct language_defn unknown_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   default_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
@@ -908,6 +910,7 @@  const struct language_defn auto_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   default_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
@@ -955,6 +958,7 @@  const struct language_defn local_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   default_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/language.h b/gdb/language.h
index 58bcbe8..cc0df8c 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -358,6 +358,12 @@  struct language_defn
     void (*la_get_string) (struct value *value, gdb_byte **buffer, int *length,
 			   struct type **chartype, const char **charset);
 
+    /* Return an expression that can be used for a location
+       watchpoint.  TYPE is a pointer type that points to the memory
+       to watch, and ADDR is the address of the watched memory.  */
+    gdb::unique_xmalloc_ptr<char> (*la_watch_location_expression)
+         (struct type *type, CORE_ADDR addr);
+
     /* Return a pointer to the function that should be used to match
        a symbol name against LOOKUP_NAME. This is mostly for languages
        such as Ada where the matching algorithm depends on LOOKUP_NAME.
diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c
index 03b955c..35840bf 100644
--- a/gdb/m2-lang.c
+++ b/gdb/m2-lang.c
@@ -393,6 +393,7 @@  const struct language_defn m2_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   default_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index e102b80..934d9c6 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -402,6 +402,7 @@  const struct language_defn objc_language_defn = {
   default_print_array_index,
   default_pass_by_reference,
   default_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/opencl-lang.c b/gdb/opencl-lang.c
index 6e4c185..df57b61 100644
--- a/gdb/opencl-lang.c
+++ b/gdb/opencl-lang.c
@@ -1082,6 +1082,7 @@  const struct language_defn opencl_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/p-lang.c b/gdb/p-lang.c
index 1b069ef..c4fe2f6 100644
--- a/gdb/p-lang.c
+++ b/gdb/p-lang.c
@@ -30,6 +30,7 @@ 
 #include "valprint.h"
 #include "value.h"
 #include <ctype.h>
+#include "c-lang.h"
 
 extern void _initialize_pascal_language (void);
 
@@ -455,6 +456,7 @@  const struct language_defn pascal_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   default_get_string,
+  c_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 898edda..dce184c 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -2122,6 +2122,20 @@  rust_sniff_from_mangled_name (const char *mangled, char **demangled)
 
 
 
+/* la_watch_location_expression for Rust.  */
+
+static gdb::unique_xmalloc_ptr<char>
+rust_watch_location_expression (struct type *type, CORE_ADDR addr)
+{
+  type = check_typedef (TYPE_TARGET_TYPE (check_typedef (type)));
+  std::string name = type_to_string (type);
+  return gdb::unique_xmalloc_ptr<char>
+    (xstrprintf ("*(%s as *mut %s)", core_addr_to_string (addr),
+		 name.c_str ()));
+}
+
+
+
 static const struct exp_descriptor exp_descriptor_rust = 
 {
   rust_print_subexp,
@@ -2176,6 +2190,7 @@  static const struct language_defn rust_language_defn =
   default_print_array_index,
   default_pass_by_reference,
   c_get_string,
+  rust_watch_location_expression,
   NULL,				/* la_get_symbol_name_cmp */
   iterate_over_symbols,
   &default_varobj_ops,
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b672df3..aad2778 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2017-05-17  Tom Tromey  <tom@tromey.com>
+
+	PR rust/21484:
+	* gdb.rust/watch.exp: New file.
+	* gdb.rust/watch.rs: New file.
+
 2017-05-04  Pedro Alves  <palves@redhat.com>
 
 	* gdb.python/py-record-btrace-threads.exp (check_insn_for_thread):
diff --git a/gdb/testsuite/gdb.rust/watch.exp b/gdb/testsuite/gdb.rust/watch.exp
new file mode 100644
index 0000000..0a3c055
--- /dev/null
+++ b/gdb/testsuite/gdb.rust/watch.exp
@@ -0,0 +1,35 @@ 
+# Copyright (C) 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test watch -location with Rust.
+
+load_lib rust-support.exp
+if {[skip_rust_tests]} {
+    continue
+}
+
+standard_testfile .rs
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug rust}]} {
+    return -1
+}
+
+set line [gdb_get_line_number "set breakpoint here"]
+if {![runto ${srcfile}:$line]} {
+    untested "could not run to breakpoint"
+    return -1
+}
+
+# Just setting a watchpoint was enough to trigger the bug.
+gdb_test "watch -location y" ".*watchpoint .* -location .*"
diff --git a/gdb/testsuite/gdb.rust/watch.rs b/gdb/testsuite/gdb.rust/watch.rs
new file mode 100644
index 0000000..4146ecb
--- /dev/null
+++ b/gdb/testsuite/gdb.rust/watch.rs
@@ -0,0 +1,24 @@ 
+// Copyright (C) 2017 Free Software Foundation, Inc.
+
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+#![allow(dead_code)]
+#![allow(unused_variables)]
+#![allow(unused_assignments)]
+
+fn main() {
+    let mut y = 7;
+    y = y + 1;                  // set breakpoint here
+    y = y + 1;
+}