RFA: fix crash in expression evaluation

RFA: fix crash in expression evaluation

[Tom Tromey]

On irc, Vladimir pointed out this crash:

http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg13422.html

This problem seems to have snuck in via the recent pointer math
changes.

This patch fixes the crash by changing coerce_array to look through
typedefs.

Built and regtested on x86-64 (compile farm).
New test case included.

Please review.

Tom

:ADDPATCH expressions:

2008-10-02  Tom Tromey  <tromey@redhat.com>

	* value.c (coerce_array): Use check_typedef.

2008-10-02  Tom Tromey  <tromey@redhat.com>

	* gdb.base/pointers.exp: Add test.
	* gdb.base/pointers.c (k, S): New typedefs.
	(instance): New global.

diff --git a/gdb/testsuite/gdb.base/pointers.c b/gdb/testsuite/gdb.base/pointers.c
index 85bfdc9..4ee5e78 100644
--- a/gdb/testsuite/gdb.base/pointers.c
+++ b/gdb/testsuite/gdb.base/pointers.c
@@ -71,6 +71,15 @@ float ** ptr_to_ptr_to_float;
 
 int y;
 
+
+typedef long k[5];
+
+typedef struct {
+  k array_variable;
+} S;
+
+S instance;
+
 /* Do nothing function used for forcing some of the above variables to
    be referenced by the program source.  If the variables are not
    referenced, some linkers will remove the symbol from the symbol
diff --git a/gdb/testsuite/gdb.base/pointers.exp b/gdb/testsuite/gdb.base/pointers.exp
index 5532140..d7d17e7 100644
--- a/gdb/testsuite/gdb.base/pointers.exp
+++ b/gdb/testsuite/gdb.base/pointers.exp
@@ -596,3 +596,7 @@ gdb_expect {
     timeout             { fail "(timeout) ptype ppppppC" }
 }
 
+# Regression test for a crash.
+
+gdb_test "p instance.array_variable + 0" \
+  " = \\(long int \\*\\) 0x\[0-9a-f\]*"
diff --git a/gdb/value.c b/gdb/value.c
index f3f2c72..0c33959 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1692,11 +1692,16 @@ coerce_ref (struct value *arg)
 struct value *
 coerce_array (struct value *arg)
 {
+  struct type *type;
   arg = coerce_ref (arg);
+  type = check_typedef (value_type (arg));
   if (current_language->c_style_arrays
-      && TYPE_CODE (value_type (arg)) == TYPE_CODE_ARRAY)
-    arg = value_coerce_array (arg);
-  if (TYPE_CODE (value_type (arg)) == TYPE_CODE_FUNC)
+      && TYPE_CODE (type) == TYPE_CODE_ARRAY)
+    {
+      arg = value_coerce_array (arg);
+      type = check_typedef (value_type (arg));
+    }
+  if (TYPE_CODE (type) == TYPE_CODE_FUNC)
     arg = value_coerce_function (arg);
   return arg;
 }

Re: RFA: fix crash in expression evaluation

[Joel Brobecker]

Hi Tom,

> 2008-10-02  Tom Tromey  <tromey@redhat.com>
> 
> 	* value.c (coerce_array): Use check_typedef.

Almost OK. Just a question...

> 2008-10-02  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.base/pointers.exp: Add test.
> 	* gdb.base/pointers.c (k, S): New typedefs.
> 	(instance): New global.

This part looks fine and is OK to commit once we have clarified
my questions.

> @@ -1692,11 +1692,16 @@ coerce_ref (struct value *arg)
>  struct value *
>  coerce_array (struct value *arg)
>  {
> +  struct type *type;

Nit-picking. I think that the general convention here is to separate
local declarations from the rest of the block with an empty line.
I wonder if this is documented anywhere...

>    arg = coerce_ref (arg);
> +  type = check_typedef (value_type (arg));
>    if (current_language->c_style_arrays
> -      && TYPE_CODE (value_type (arg)) == TYPE_CODE_ARRAY)
> -    arg = value_coerce_array (arg);
> -  if (TYPE_CODE (value_type (arg)) == TYPE_CODE_FUNC)
> +      && TYPE_CODE (type) == TYPE_CODE_ARRAY)
> +    {
> +      arg = value_coerce_array (arg);
> +      type = check_typedef (value_type (arg));

I don't think the second line is useful, is it? type should necessarily
be a TYPE_CODE_PTR, if I understand value_coerce_array correctly. So
it goes from being a TYPE_CODE_ARRAY to a TYPE_CODE_PTR. In neither case
will it cause the check just below to be true.

> +    }
> +  if (TYPE_CODE (type) == TYPE_CODE_FUNC)
>      arg = value_coerce_function (arg);
>    return arg;
>  }

Honestly, I think that the code is poorly written. How about using
a case statement or at least a if/else if sequence. Would something
like this work?

    struct type *type;

    arg = coerce_ref (arg);
    type = check_typedef (value_type (arg))

    switch (TYPE_CODE (type))
      {
        case TYPE_CODE_ARRAY:
          if (current_language->c_style_arrays)
            arg = value_coerce_array (arg);
          break;
        case TYPE_CODE_FUNC:
          arg = value_coerce_function (arg);
          break
      }
    return arg;

What do you think?

-- 
Joel

:REVIEWMAIL:

Re: RFA: fix crash in expression evaluation

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Tom> +  struct type *type;

Joel> Nit-picking. I think that the general convention here is to separate
Joel> local declarations from the rest of the block with an empty line.

Ok, fixed.

Joel> I don't think the second line is useful, is it? type should
Joel> necessarily be a TYPE_CODE_PTR, if I understand
Joel> value_coerce_array correctly. So it goes from being a
Joel> TYPE_CODE_ARRAY to a TYPE_CODE_PTR. In neither case will it
Joel> cause the check just below to be true.

Yeah, good point.

Joel> Honestly, I think that the code is poorly written. How about using
Joel> a case statement or at least a if/else if sequence. Would something
Joel> like this work?

Looks good to me.
I'll send it through the tester.

Tom

Re: RFA: fix crash in expression evaluation

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I'll send it through the tester.

It passed.

Here is the new patch.

Tom

2008-10-02  Tom Tromey  <tromey@redhat.com>

	* value.c (coerce_array): Use check_typedef.

diff --git a/gdb/testsuite/gdb.base/pointers.c b/gdb/testsuite/gdb.base/pointers.c
index 85bfdc9..4ee5e78 100644
--- a/gdb/testsuite/gdb.base/pointers.c
+++ b/gdb/testsuite/gdb.base/pointers.c
@@ -71,6 +71,15 @@ float ** ptr_to_ptr_to_float;
 
 int y;
 
+
+typedef long k[5];
+
+typedef struct {
+  k array_variable;
+} S;
+
+S instance;
+
 /* Do nothing function used for forcing some of the above variables to
    be referenced by the program source.  If the variables are not
    referenced, some linkers will remove the symbol from the symbol
diff --git a/gdb/testsuite/gdb.base/pointers.exp b/gdb/testsuite/gdb.base/pointers.exp
index 5532140..d7d17e7 100644
--- a/gdb/testsuite/gdb.base/pointers.exp
+++ b/gdb/testsuite/gdb.base/pointers.exp
@@ -596,3 +596,7 @@ gdb_expect {
     timeout             { fail "(timeout) ptype ppppppC" }
 }
 
+# Regression test for a crash.
+
+gdb_test "p instance.array_variable + 0" \
+  " = \\(long int \\*\\) 0x\[0-9a-f\]*"
diff --git a/gdb/value.c b/gdb/value.c
index 1fa376d..695aa33 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1728,12 +1728,21 @@ coerce_ref (struct value *arg)
 struct value *
 coerce_array (struct value *arg)
 {
+  struct type *type;
+
   arg = coerce_ref (arg);
-  if (current_language->c_style_arrays
-      && TYPE_CODE (value_type (arg)) == TYPE_CODE_ARRAY)
-    arg = value_coerce_array (arg);
-  if (TYPE_CODE (value_type (arg)) == TYPE_CODE_FUNC)
-    arg = value_coerce_function (arg);
+  type = check_typedef (value_type (arg));
+
+  switch (TYPE_CODE (type))
+    {
+    case TYPE_CODE_ARRAY:
+      if (current_language->c_style_arrays)
+	arg = value_coerce_array (arg);
+      break;
+    case TYPE_CODE_FUNC:
+      arg = value_coerce_function (arg);
+      break;
+    }
   return arg;
 }
 \f

Re: RFA: fix crash in expression evaluation

[Joel Brobecker]

> It passed.

Cool! :)

> 2008-10-02  Tom Tromey  <tromey@redhat.com>
> 
> 	* value.c (coerce_array): Use check_typedef.

Approved, please go ahead and apply.

Thank you,
-- 
Joel