Patchwork gdb/rust: Handle printing structures containing strings

login
register
mail settings
Submitter Andrew Burgess
Date May 1, 2019, 11:52 p.m.
Message ID <20190501235246.7982-1-andrew.burgess@embecosm.com>
Download mbox | patch
Permalink /patch/32481/
State New
Headers show

Comments

Andrew Burgess - May 1, 2019, 11:52 p.m.
When printing a rust structure that contains a string GDB can
currently fail to read the fields that define the string. This is
because GDB mistakenly treats a value that is the parent structure as
though it is the structure that defines the string, and then fails to
find the fields needed to extract a string.

The solution is to create a new value to represent the string field of
the parent value.

gdb/ChangeLog:

	* rust-lang.c (val_print_struct): Handle printing structures
	containing strings.

gdb/testsuite/ChangeLog:

	* gdb.rust/simple.exp: Add new test case.
	* gdb.rust/simple.rs (struct StringAtOffset): New struct.
	(main): Initialise an instance of the new struct.
---
 gdb/ChangeLog                     | 5 +++++
 gdb/rust-lang.c                   | 8 ++++++++
 gdb/testsuite/ChangeLog           | 6 ++++++
 gdb/testsuite/gdb.rust/simple.exp | 2 ++
 gdb/testsuite/gdb.rust/simple.rs  | 8 ++++++++
 5 files changed, 29 insertions(+)
Tom Tromey - May 2, 2019, 1:44 p.m.
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> When printing a rust structure that contains a string GDB can
Andrew> currently fail to read the fields that define the string. This is
Andrew> because GDB mistakenly treats a value that is the parent structure as
Andrew> though it is the structure that defines the string, and then fails to
Andrew> find the fields needed to extract a string.

Andrew> The solution is to create a new value to represent the string field of
Andrew> the parent value.

Thank you for doing this.

Andrew>    if (rust_slice_type_p (type) && strcmp (TYPE_NAME (type), "&str") == 0)
Andrew>      {
Andrew> +      /* If what we are printing here is actually a string within a
Andrew> +	 structure then VAL will be the original parent value, while TYPE
Andrew> +	 will be the type of the structure representing the string we want
Andrew> +	 to print.
Andrew> +	 However, RUST_VAL_PRINT_STR looks up the fields of the string
Andrew> +	 inside VAL, assuming that VAL is the string.
Andrew> +	 So, recreate VAL as a value representing just the string.  */
Andrew> +      val = value_at_lazy (type, value_address (val) + embedded_offset);
Andrew>        rust_val_print_str (stream, val, options);
Andrew>        return;

It took me a while to understand this, but I get it now.  At first it
looked like this code was misplaced, but what's going on is that
rust_val_print_str only takes a value, so in a "val_print" context we
must reconstruct one.

We really ought to get rid of the val_print / value_print distinction
someday.  It doesn't provide any benefit and it leads to bugs like this.

Andrew> +gdb_test "print st" " = simple::StringAtOffset {field1: \"hello\", field2: 1, field3: \"world\"}"

I think this line should probably be split after the first argument.
Ok with this change.

Tom
Andrew Burgess - May 2, 2019, 2:56 p.m.
* Tom Tromey <tom@tromey.com> [2019-05-02 07:44:04 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> When printing a rust structure that contains a string GDB can
> Andrew> currently fail to read the fields that define the string. This is
> Andrew> because GDB mistakenly treats a value that is the parent structure as
> Andrew> though it is the structure that defines the string, and then fails to
> Andrew> find the fields needed to extract a string.
> 
> Andrew> The solution is to create a new value to represent the string field of
> Andrew> the parent value.
> 
> Thank you for doing this.
> 
> Andrew>    if (rust_slice_type_p (type) && strcmp (TYPE_NAME (type), "&str") == 0)
> Andrew>      {
> Andrew> +      /* If what we are printing here is actually a string within a
> Andrew> +	 structure then VAL will be the original parent value, while TYPE
> Andrew> +	 will be the type of the structure representing the string we want
> Andrew> +	 to print.
> Andrew> +	 However, RUST_VAL_PRINT_STR looks up the fields of the string
> Andrew> +	 inside VAL, assuming that VAL is the string.
> Andrew> +	 So, recreate VAL as a value representing just the string.  */
> Andrew> +      val = value_at_lazy (type, value_address (val) + embedded_offset);
> Andrew>        rust_val_print_str (stream, val, options);
> Andrew>        return;
> 
> It took me a while to understand this, but I get it now.  At first it
> looked like this code was misplaced, but what's going on is that
> rust_val_print_str only takes a value, so in a "val_print" context we
> must reconstruct one.

If you'd like to suggest some new words I'd be happy to include them.
I spent too long trying to write a helpful comment here, and like you
I wasn't really happy with what I ended up with....

> 
> We really ought to get rid of the val_print / value_print distinction
> someday.  It doesn't provide any benefit and it leads to bugs like this.
> 
> Andrew> +gdb_test "print st" " = simple::StringAtOffset {field1: \"hello\", field2: 1, field3: \"world\"}"
> 
> I think this line should probably be split after the first argument.
> Ok with this change.
> 
> Tom
Tom Tromey - May 2, 2019, 3:50 p.m.
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

>> It took me a while to understand this, but I get it now.  At first it
>> looked like this code was misplaced, but what's going on is that
>> rust_val_print_str only takes a value, so in a "val_print" context we
>> must reconstruct one.

Andrew> If you'd like to suggest some new words I'd be happy to include them.
Andrew> I spent too long trying to write a helpful comment here, and like you
Andrew> I wasn't really happy with what I ended up with....

I think it's fine and it was just me being a bit obtuse :-)

Tom

Patch

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 2fada465d65..79f13311cd8 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -378,6 +378,14 @@  val_print_struct (struct type *type, int embedded_offset,
 
   if (rust_slice_type_p (type) && strcmp (TYPE_NAME (type), "&str") == 0)
     {
+      /* If what we are printing here is actually a string within a
+	 structure then VAL will be the original parent value, while TYPE
+	 will be the type of the structure representing the string we want
+	 to print.
+	 However, RUST_VAL_PRINT_STR looks up the fields of the string
+	 inside VAL, assuming that VAL is the string.
+	 So, recreate VAL as a value representing just the string.  */
+      val = value_at_lazy (type, value_address (val) + embedded_offset);
       rust_val_print_str (stream, val, options);
       return;
     }
diff --git a/gdb/testsuite/gdb.rust/simple.exp b/gdb/testsuite/gdb.rust/simple.exp
index 91afe85cee7..869b16ce33c 100644
--- a/gdb/testsuite/gdb.rust/simple.exp
+++ b/gdb/testsuite/gdb.rust/simple.exp
@@ -239,6 +239,8 @@  gdb_test "print custom_some" \
     " = simple::NonZeroOptimized::Value\\(\[a-z\]+::string::String .*"
 gdb_test "print custom_none" " = simple::NonZeroOptimized::Empty"
 
+gdb_test "print st" " = simple::StringAtOffset {field1: \"hello\", field2: 1, field3: \"world\"}"
+
 proc test_one_slice {svar length base range} {
     global hex
 
diff --git a/gdb/testsuite/gdb.rust/simple.rs b/gdb/testsuite/gdb.rust/simple.rs
index 19f5ef92241..e6e0efd3b16 100644
--- a/gdb/testsuite/gdb.rust/simple.rs
+++ b/gdb/testsuite/gdb.rust/simple.rs
@@ -85,6 +85,12 @@  union Union {
     f2: u8,
 }
 
+struct StringAtOffset {
+    pub field1: &'static str,
+    pub field2: i32,
+    pub field3: &'static str,
+}
+
 // A simple structure whose layout won't be changed by the compiler,
 // so that ptype/o testing will work on any platform.
 struct SimpleLayout {
@@ -146,6 +152,8 @@  fn main () {
     let to1 = &w[..3];
     let to2 = &slice[..1];
 
+    let st = StringAtOffset { field1: "hello", field2: 1, field3: "world" };
+
     // tests for enum optimizations
 
     let str_some = Some("hi".to_string());