This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix crash with empty Rust enum


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]