diff mbox

Fix crash with empty Rust enum

Message ID 20180911213659.10366-1-tom@tromey.com
State New
Headers show

Commit Message

Tom Tromey Sept. 11, 2018, 9:36 p.m. UTC
While testing my Rust compiler patch to fix the DWARF representation
of Rust enums (https://github.com/rust-lang/rust/pull/54004), I found
a gdb crash coming from one of the Rust test cases.

The bug here is that the new variant support in gdb does not handle
the case where there are no variants in the enum.

This patch fixes the problem in a straightforward way.  Note that the
new tests are somewhat lax because I did not want to try to fully fix
this corner case for older compilers.  If you think that's
unacceptable, let meknow.

Tested on x86-64 Fedora 28 using several versions of the Rust
compiler.  I intend to push this to the 8.2 branch as well.

gdb/ChangeLog
2018-09-11  Tom Tromey  <tom@tromey.com>

	PR rust/23626:
	* rust-lang.c (rust_enum_variant): Now static.
	(rust_empty_enum_p): New function.
	(rust_print_enum, rust_evaluate_subexp, rust_print_struct_def):
	Handle empty enum.

gdb/testsuite/ChangeLog
2018-09-11  Tom Tromey  <tom@tromey.com>

	PR rust/23626:
	* gdb.rust/simple.rs (EmptyEnum): New type.
	(main): Use it.
	* gdb.rust/simple.exp (test_one_slice): Add empty enum test.
---
 gdb/ChangeLog                     |  8 ++++++
 gdb/rust-lang.c                   | 42 ++++++++++++++++++++++++++++++-
 gdb/testsuite/ChangeLog           |  7 ++++++
 gdb/testsuite/gdb.rust/simple.exp | 12 +++++++++
 gdb/testsuite/gdb.rust/simple.rs  |  4 +++
 5 files changed, 72 insertions(+), 1 deletion(-)

Comments

Simon Marchi Sept. 12, 2018, 9:47 p.m. UTC | #1
Hi Tom,

I don't have much experience with rust, so here is some "best-effort" 
kind of feedback.

> @@ -1604,6 +1636,10 @@ rust_evaluate_subexp (struct type *expect_type,
> struct expression *exp,
> 
>  	    if (rust_enum_p (type))
>  	      {
> +		if (rust_empty_enum_p (type))
> +		  error (_("Cannot access field of empty enum %s"),
> +			 TYPE_NAME (type));
> +
>  		const gdb_byte *valaddr = value_contents (lhs);
>  		struct field *variant_field = rust_enum_variant (type, valaddr);
> 
> @@ -1672,6 +1708,10 @@ tuple structs, and tuple-like enum variants"));
>          type = value_type (lhs);
>          if (TYPE_CODE (type) == TYPE_CODE_STRUCT && rust_enum_p 
> (type))
>  	  {
> +	    if (rust_empty_enum_p (type))
> +	      error (_("Cannot access field of empty enum %s"),
> +		     TYPE_NAME (type));
> +
>  	    const gdb_byte *valaddr = value_contents (lhs);
>  	    struct field *variant_field = rust_enum_variant (type, valaddr);

I was wondering about this case (the STRUCTOP_STRUCT one).  Would it not 
work to let code go in the standard code path and let GDB not find the 
field?  Wouldn't we end up in the catch below, with the message:

		error (_("Could not find field %s of struct variant %s::%s"),
		       field_name, TYPE_NAME (outer_type),
		       rust_last_path_segment (TYPE_NAME (type)));

?  Same could go for the STRUCTOP_ANONYMOUS version I guess.

With the calls to error() as you wrote it in your patch, one suggestion 
would be to include the name (or field number) of the field in the 
error.  In the case of a complex expression, it could help spot the 
mistake (since the name of the enum type is not likely to appear in the 
expression).

> 
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index a360c225ef..8c684520d2 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,10 @@
> +2018-09-11  Tom Tromey  <tom@tromey.com>
> +
> +	PR rust/23626:
> +	* gdb.rust/simple.rs (EmptyEnum): New type.
> +	(main): Use it.
> +	* gdb.rust/simple.exp (test_one_slice): Add empty enum test.
> +
>  2018-09-08  Tom Tromey  <tom@tromey.com>
> 
>  	* gdb.python/py-prettyprint.exp: Use with_test_prefix.
> diff --git a/gdb/testsuite/gdb.rust/simple.exp
> b/gdb/testsuite/gdb.rust/simple.exp
> index 20fe8dd0a8..07b2512220 100644
> --- a/gdb/testsuite/gdb.rust/simple.exp
> +++ b/gdb/testsuite/gdb.rust/simple.exp
> @@ -303,6 +303,18 @@ gdb_test_sequence "ptype/o SimpleLayout" "" {
>      "                         }"
>  }
> 
> +# PR rust/23626 - this used to crash.  Note that the results are
> +# fairly lax because most existing versions of Rust (those before the
> +# DW_TAG_variant patches) do not emit what gdb wants here; and there
> +# was little point fixing gdb to cope with these cases as the fixed
> +# compilers will be available soon
> +gdb_test "print empty_enum_value" \
> +    " = simple::EmptyEnum.*"
> +gdb_test "ptype empty_enum_value" "simple::EmptyEnum.*"
> +# Just make sure these don't crash, for the same reason.
> +gdb_test "print empty_enum_value.0" ""
> +gdb_test "print empty_enum_value.something" ""

Is it possible (and worth it) to put some tighter checks, but kfail them 
based on the compiler version?

Simon
Tom Tromey Sept. 13, 2018, 12:34 a.m. UTC | #2
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> +	    if (rust_empty_enum_p (type))
>> +	      error (_("Cannot access field of empty enum %s"),
>> +		     TYPE_NAME (type));
>> +
>> const gdb_byte *valaddr = value_contents (lhs);
>> struct field *variant_field = rust_enum_variant (type, valaddr);

Simon> I was wondering about this case (the STRUCTOP_STRUCT one).  Would it
Simon> not work to let code go in the standard code path and let GDB not find
Simon> the field?

No, because the crash happens in rust_enum_variant.

Maybe I could change rust_enum_variant so that the callers can do this.
I'll look.

Simon> With the calls to error() as you wrote it in your patch, one
Simon> suggestion would be to include the name (or field number) of the field
Simon> in the error.  In the case of a complex expression, it could help spot
Simon> the mistake (since the name of the enum type is not likely to appear
Simon> in the expression).

Good idea.

>> +# PR rust/23626 - this used to crash.  Note that the results are
>> +# fairly lax because most existing versions of Rust (those before the
>> +# DW_TAG_variant patches) do not emit what gdb wants here; and there
>> +# was little point fixing gdb to cope with these cases as the fixed
>> +# compilers will be available soon
>> +gdb_test "print empty_enum_value" \
>> +    " = simple::EmptyEnum.*"
>> +gdb_test "ptype empty_enum_value" "simple::EmptyEnum.*"
>> +# Just make sure these don't crash, for the same reason.
>> +gdb_test "print empty_enum_value.0" ""
>> +gdb_test "print empty_enum_value.something" ""

Simon> Is it possible (and worth it) to put some tighter checks, but kfail
Simon> them based on the compiler version?

Yeah.  I am actually going to do something like this soon for other
tests that are already failing.  I wasn't sure I wanted to put the test
suite changes into the 8.2 branch but I guess it wouldn't hurt.

Tom
Tom Tromey Sept. 13, 2018, 3:51 p.m. UTC | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> +gdb_test "print empty_enum_value" \
>> +    " = simple::EmptyEnum.*"
>> +gdb_test "ptype empty_enum_value" "simple::EmptyEnum.*"
>> +# Just make sure these don't crash, for the same reason.
>> +gdb_test "print empty_enum_value.0" ""
>> +gdb_test "print empty_enum_value.something" ""

Simon> Is it possible (and worth it) to put some tighter checks, but kfail
Simon> them based on the compiler version?

On reflection I think we need to wait for a version of rustc to come out
that has the enum debuginfo fixes.

Adding xfails to gdb.rust/*.exp is tracked here:
https://sourceware.org/bugzilla/show_bug.cgi?id=23624

So I am just going to move forward with the looser checks and fix this
part later.

Tom
Tom Tromey Sept. 13, 2018, 4:49 p.m. UTC | #4
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> Maybe I could change rust_enum_variant so that the callers can do this.
Tom> I'll look.

I looked and I think I will keep it as is.
The existing errors refer to variants, like:

		  error(_("Cannot access field %d of variant %s::%s, "
			  "there are only %d fields"),

This seems nicely specific, so it's worth preserving; but on the other
hand, in the new scenario, there are no variants, so mentioning the
variant does not make sense.

However I am making the other change you suggested, and I found another
buglet besides which I will tack on.

Tom
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7adb11f4f0..9d853ecbda 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@ 
+2018-09-11  Tom Tromey  <tom@tromey.com>
+
+	PR rust/23626:
+	* rust-lang.c (rust_enum_variant): Now static.
+	(rust_empty_enum_p): New function.
+	(rust_print_enum, rust_evaluate_subexp, rust_print_struct_def):
+	Handle empty enum.
+
 2018-09-10  Tom Tromey  <tom@tromey.com>
 
 	PR python/18380:
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 1871ec7df9..9c21995dc7 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -74,9 +74,22 @@  rust_enum_p (const struct type *type)
 	  && TYPE_FLAG_DISCRIMINATED_UNION (TYPE_FIELD_TYPE (type, 0)));
 }
 
+/* Return true if TYPE, which must be an enum type, has no
+   variants.  */
+
+static bool
+rust_empty_enum_p (const struct type *type)
+{
+  gdb_assert (rust_enum_p (type));
+  /* In Rust the enum always fills the containing structure.  */
+  gdb_assert (TYPE_FIELD_BITPOS (type, 0) == 0);
+
+  return TYPE_NFIELDS (TYPE_FIELD_TYPE (type, 0)) == 0;
+}
+
 /* Given an enum type and contents, find which variant is active.  */
 
-struct field *
+static struct field *
 rust_enum_variant (struct type *type, const gdb_byte *contents)
 {
   /* In Rust the enum always fills the containing structure.  */
@@ -429,6 +442,13 @@  rust_print_enum (struct type *type, int embedded_offset,
 
   opts.deref_ref = 0;
 
+  if (rust_empty_enum_p (type))
+    {
+      /* Print the enum type name here to be more clear.  */
+      fprintf_filtered (stream, _("%s {<No data fields>}"), TYPE_NAME (type));
+      return;
+    }
+
   const gdb_byte *valaddr = value_contents_for_printing (val);
   struct field *variant_field = rust_enum_variant (type, valaddr);
   embedded_offset += FIELD_BITPOS (*variant_field) / 8;
@@ -664,6 +684,18 @@  rust_print_struct_def (struct type *type, const char *varstring,
       if (is_enum)
 	{
 	  fputs_filtered ("enum ", stream);
+
+	  if (rust_empty_enum_p (type))
+	    {
+	      if (tagname != NULL)
+		{
+		  fputs_filtered (tagname, stream);
+		  fputs_filtered (" ", stream);
+		}
+	      fputs_filtered ("{}", stream);
+	      return;
+	    }
+
 	  type = TYPE_FIELD_TYPE (type, 0);
 
 	  struct dynamic_prop *discriminant_prop
@@ -1604,6 +1636,10 @@  rust_evaluate_subexp (struct type *expect_type, struct expression *exp,
 
 	    if (rust_enum_p (type))
 	      {
+		if (rust_empty_enum_p (type))
+		  error (_("Cannot access field of empty enum %s"),
+			 TYPE_NAME (type));
+
 		const gdb_byte *valaddr = value_contents (lhs);
 		struct field *variant_field = rust_enum_variant (type, valaddr);
 
@@ -1672,6 +1708,10 @@  tuple structs, and tuple-like enum variants"));
         type = value_type (lhs);
         if (TYPE_CODE (type) == TYPE_CODE_STRUCT && rust_enum_p (type))
 	  {
+	    if (rust_empty_enum_p (type))
+	      error (_("Cannot access field of empty enum %s"),
+		     TYPE_NAME (type));
+
 	    const gdb_byte *valaddr = value_contents (lhs);
 	    struct field *variant_field = rust_enum_variant (type, valaddr);
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index a360c225ef..8c684520d2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@ 
+2018-09-11  Tom Tromey  <tom@tromey.com>
+
+	PR rust/23626:
+	* gdb.rust/simple.rs (EmptyEnum): New type.
+	(main): Use it.
+	* gdb.rust/simple.exp (test_one_slice): Add empty enum test.
+
 2018-09-08  Tom Tromey  <tom@tromey.com>
 
 	* gdb.python/py-prettyprint.exp: Use with_test_prefix.
diff --git a/gdb/testsuite/gdb.rust/simple.exp b/gdb/testsuite/gdb.rust/simple.exp
index 20fe8dd0a8..07b2512220 100644
--- a/gdb/testsuite/gdb.rust/simple.exp
+++ b/gdb/testsuite/gdb.rust/simple.exp
@@ -303,6 +303,18 @@  gdb_test_sequence "ptype/o SimpleLayout" "" {
     "                         }"
 }
 
+# PR rust/23626 - this used to crash.  Note that the results are
+# fairly lax because most existing versions of Rust (those before the
+# DW_TAG_variant patches) do not emit what gdb wants here; and there
+# was little point fixing gdb to cope with these cases as the fixed
+# compilers will be available soon
+gdb_test "print empty_enum_value" \
+    " = simple::EmptyEnum.*"
+gdb_test "ptype empty_enum_value" "simple::EmptyEnum.*"
+# Just make sure these don't crash, for the same reason.
+gdb_test "print empty_enum_value.0" ""
+gdb_test "print empty_enum_value.something" ""
+
 load_lib gdb-python.exp
 if {[skip_python_tests]} {
     continue
diff --git a/gdb/testsuite/gdb.rust/simple.rs b/gdb/testsuite/gdb.rust/simple.rs
index 1bcc030d60..00a25e0828 100644
--- a/gdb/testsuite/gdb.rust/simple.rs
+++ b/gdb/testsuite/gdb.rust/simple.rs
@@ -92,6 +92,8 @@  struct SimpleLayout {
     f2: u16
 }
 
+enum EmptyEnum {}
+
 fn main () {
     let a = ();
     let b : [i32; 0] = [];
@@ -168,6 +170,8 @@  fn main () {
     let u = Union { f2: 255 };
     let simplelayout = SimpleLayout { f1: 8, f2: 9 };
 
+    let empty_enum_value: EmptyEnum = unsafe { ::std::mem::zeroed() };
+
     println!("{}, {}", x.0, x.1);        // set breakpoint here
     println!("{}", diff2(92, 45));
     empty();