RFC: fix `gdb -write' case

RFC: fix `gdb -write' case

[Tom Tromey]

I'd appreciate comments on this.  In the absence of comments I will
check it in.  I plan to put it on the 7.3 branch as well.

This comes from:

    https://bugzilla.redhat.com/show_bug.cgi?id=696148

The bug is that you cannot change a string in an executable with `gdb
-write'.  Jan tracked this down to the wide string change; the bug is
that evaluate_subexp_c did not examine `expect_type', leading to an
attempt to coerce the string to memory.

Built and regtested by our internal buildbot.

Tom

2011-05-06  Tom Tromey  <tromey@redhat.com>

	* c-lang.c (evaluate_subexp_c): Use expect_type if it is not
	NULL.

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index ad770bf..64c4d79 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -1061,9 +1061,39 @@ evaluate_subexp_c (struct type *expect_type, struct expression *exp,
 	    /* Write the terminating character.  */
 	    for (i = 0; i < TYPE_LENGTH (type); ++i)
 	      obstack_1grow (&output, 0);
-	    result = value_cstring (obstack_base (&output),
-				    obstack_object_size (&output),
-				    type);
+
+	    if (expect_type && TYPE_CODE (expect_type) == TYPE_CODE_ARRAY)
+	      {
+		LONGEST low_bound, high_bound;
+		struct type *element_type
+		  = check_typedef (TYPE_TARGET_TYPE (expect_type));
+		int element_size = TYPE_LENGTH (element_type);
+
+		if (TYPE_CODE (element_type) != TYPE_CODE_INT
+		    && (TYPE_CODE (element_type) != TYPE_CODE_CHAR))
+		  error (_("wrong expected type for string constant"));
+		if (TYPE_LENGTH (element_type) != TYPE_LENGTH (type))
+		  error (_("expected type of string constant has wrong "
+			   "character width"));
+
+		if (get_discrete_bounds (TYPE_INDEX_TYPE (expect_type),
+					 &low_bound, &high_bound) < 0)
+		  {
+		    low_bound = 0;
+		    high_bound = (TYPE_LENGTH (expect_type) / element_size) - 1;
+		  }
+		if (obstack_object_size (&output) / TYPE_LENGTH (type)
+		    > (high_bound - low_bound + 1))
+		  error (_("Too many array elements"));
+
+		result = allocate_value (expect_type);
+		memcpy (value_contents_raw (result), obstack_base (&output),
+			obstack_object_size (&output));
+	      }
+	    else
+	      result = value_cstring (obstack_base (&output),
+				      obstack_object_size (&output),
+				      type);
 	  }
 	do_cleanups (cleanup);
 	return result;

Re: RFC: fix `gdb -write' case

[Yao Qi]

On 05/07/2011 01:12 AM, Tom Tromey wrote:
> I'd appreciate comments on this.  In the absence of comments I will
> check it in.  I plan to put it on the 7.3 branch as well.
> 
> This comes from:
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=696148
> 

FYI, I can't access this bug.  I am told "You are not authorized to
access bug #696148."

> +
> +		result = allocate_value (expect_type);
> +		memcpy (value_contents_raw (result), obstack_base (&output),
> +			obstack_object_size (&output));

Compared with value_cstring, the difference is that expect_type is used
here, while type used in value_cstring is got from lookup_array_range_type.

struct type *stringtype
    = lookup_array_range_type (char_type, lowbound, highbound + lowbound
- 1);

Any difference on these two type variables (expect_type vs. stringtype)?

> +	      }
> +	    else
> +	      result = value_cstring (obstack_base (&output),
> +				      obstack_object_size (&output),
> +				      type);
>  	  }
>  	do_cleanups (cleanup);
>  	return result;

-- 
Yao (齐尧)

Re: RFC: fix `gdb -write' case

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Tom> https://bugzilla.redhat.com/show_bug.cgi?id=696148

Yao> FYI, I can't access this bug.  I am told "You are not authorized to
Yao> access bug #696148."

Oops, sorry.  It is a private bug -- not for any good reason AFAICT, but
I can't change it.

Here is Jan's one-line reproducer:

echo 'char s[]="a";main(){return s[0]=='"'a';}"|gcc -o v -x c - -g;./v;echo $?;~/redhat/gdb-6.8/gdb/gdb -nx -write ./v -ex 'set var s="b"' -ex q;./v;echo $?

Tom> +		result = allocate_value (expect_type);
Tom> +		memcpy (value_contents_raw (result), obstack_base (&output),
Tom> +			obstack_object_size (&output));

Yao> Compared with value_cstring, the difference is that expect_type is used
Yao> here, while type used in value_cstring is got from lookup_array_range_type.

Yao> struct type *stringtype
Yao>     = lookup_array_range_type (char_type, lowbound, highbound + lowbound
Yao> - 1);

Yao> Any difference on these two type variables (expect_type vs. stringtype)?

Yes.  You can wind up with a different "char" type following the logic
in c-lang.c.  In this case some higher level will try to cast the string
to the correct type, which will try to force it to memory, leading to
the bad result.

The checks in the patch are intended to ensure that the expected type
isn't "too weird" -- that we don't do something nonsensical.

Tom

Re: RFC: fix `gdb -write' case

[Yao Qi]

On 05/09/2011 10:53 PM, Tom Tromey wrote:
> Here is Jan's one-line reproducer:
> 
> echo 'char s[]="a";main(){return s[0]=='"'a';}"|gcc -o v -x c - -g;./v;echo $?;~/redhat/gdb-6.8/gdb/gdb -nx -write ./v -ex 'set var s="b"' -ex q;./v;echo $?

Is this patch to fix bugs against gdb 6.8?  I am still unable to
reproduce this problem on gdb 20110505-cvs.

$ ./gdb/gdb -nx -write ~/Work/gdb-write
(gdb) set var s="b"
evaluation of this expression requires the target program to be active
(gdb) b main
Breakpoint 1 at 0x80483b7: file gdb-write.c, line 3.
(gdb) run
Starting program: /home/yao/Work/gdb-write
BFD: reopening /home/yao/Work/gdb-write: Text file busy

[This message above repeats]
Breakpoint 1, main () at gdb-write.c:3
3	main(){return s[0]=='a';}
(gdb) set var s="b"
BFD: reopening /home/yao/Work/gdb-write: Text file busy
....
Invalid cast.

Is it a separate issue?

-- 
Yao (齐尧)

Re: RFC: fix `gdb -write' case

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Is this patch to fix bugs against gdb 6.8?

Yeah, it is a regression since 6.8.

Yao> $ ./gdb/gdb -nx -write ~/Work/gdb-write
Yao> (gdb) set var s="b"
Yao> evaluation of this expression requires the target program to be active

This is the bug -- before the charset changes, this worked.

Yao> (gdb) set var s="b"
Yao> BFD: reopening /home/yao/Work/gdb-write: Text file busy
Yao> ....
Yao> Invalid cast.
Yao> Is it a separate issue?

I don't know.

Tom

Re: RFC: fix `gdb -write' case

[Jan Kratochvil]

On Tue, 10 May 2011 04:02:47 +0200, Yao Qi wrote:
> Is this patch to fix bugs against gdb 6.8?

No.  In FSF gdb-6.8 it worked.  In FSF GDB HEAD it is broken.  It is
a regression.


> I am still unable to reproduce this problem on gdb 20110505-cvs.
> 
> $ ./gdb/gdb -nx -write ~/Work/gdb-write
> (gdb) set var s="b"
> evaluation of this expression requires the target program to be active

This worked with gdb-6.8, therefore you have reproduced the problem.


> (gdb) b main
> Breakpoint 1 at 0x80483b7: file gdb-write.c, line 3.
> (gdb) run
> Starting program: /home/yao/Work/gdb-write
> BFD: reopening /home/yao/Work/gdb-write: Text file busy
> 
> [This message above repeats]
> Breakpoint 1, main () at gdb-write.c:3
> 3	main(){return s[0]=='a';}
> (gdb) set var s="b"
> BFD: reopening /home/yao/Work/gdb-write: Text file busy
> ....
> Invalid cast.
> 
> Is it a separate issue?

This breaks for me a similar (but exactly the same) way with gdb-6.8.
I find this part as offtopic for this Bug.


Thanks,
Jan

Re: RFC: fix `gdb -write' case

[Jan Kratochvil]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Tom> https://bugzilla.redhat.com/show_bug.cgi?id=696148

Yao> FYI, I can't access this bug.  I am told "You are not authorized to
Yao> access bug #696148."

In that Bug I made a comment
# evaluate_subexp_c->OP_STRING ignores expect_type, GDB then tries to convert
# the non-matching type using malloc.

which I advocate below.


On Mon, 09 May 2011 16:53:38 +0200, Tom Tromey wrote:
> Yao> Any difference on these two type variables (expect_type vs. stringtype)?
> 
> Yes.  You can wind up with a different "char" type following the logic
> in c-lang.c.  In this case some higher level will try to cast the string
> to the correct type, which will try to force it to memory, leading to
> the bad result.
> 
> The checks in the patch are intended to ensure that the expected type
> isn't "too weird" -- that we don't do something nonsensical.

The approach you took is not in the style of current GDB:

#include <wchar.h>
short a_short[2];
long a_long[2];
char a_char[3];
wchar_t a_wchar[3];
int main (void) { return 0; }

# With gcc only - without libstdc++ linked - GDB complains despite the code
# compiled and runs fine; but that is a different Bug.
# (gdb) p U"x"
# No type named char32_t.
g++ -g

(gdb) start
(gdb) set a_short={1L,2LL}
(gdb) p a_short
$1 = {1, 2}
(gdb) ptype a_short
type = short [2]
(gdb) set a_long={1L,2LL}
(gdb) p a_long
$2 = {1, 2}
(gdb) ptype a_long
type = long [2]
(gdb) set a_char=U"x"
expected type of string constant has wrong character width
(gdb) set a_wchar="x"
expected type of string constant has wrong character width
(gdb) 

I believe if GDB adjusts the short/long types shouldn't it adjust also
char/whar_t types?  That is now the user must know the type of string in
advance which wasn't the case for arrays before.

But it is not a real regression as GDB had not supported Unicode strings.

This is why I made the comment above.


Thanks,
Jan

Re: RFC: fix `gdb -write' case

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> In that Bug I made a comment
Jan> # evaluate_subexp_c->OP_STRING ignores expect_type, GDB then tries to convert
Jan> # the non-matching type using malloc.
Jan> which I advocate below.

Jan> I believe if GDB adjusts the short/long types shouldn't it adjust also
Jan> char/whar_t types?  That is now the user must know the type of string in
Jan> advance which wasn't the case for arrays before.

Ok, I understand now.

What do you think of this?  It uses the expected type to construct the
array's values when it makes sense.

This version adds a test case for this too.

Tom

2011-05-11  Tom Tromey  <tromey@redhat.com>

	* c-lang.c (evaluate_subexp_c): Use expect_type if it is not
	NULL.

2011-05-11  Tom Tromey  <tromey@redhat.com>

	* gdb.base/charset.exp (string_display): Add tests to assign to
	arrays.
	* gdb.base/charset.c (short_array, int_array, long_array): New.

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index ad770bf..255fabe 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -978,6 +978,7 @@ evaluate_subexp_c (struct type *expect_type, struct expression *exp,
 	struct value *result;
 	enum c_string_type dest_type;
 	const char *dest_charset;
+	int satisfy_expected = 0;
 
 	obstack_init (&output);
 	cleanup = make_cleanup_obstack_free (&output);
@@ -1014,6 +1015,22 @@ evaluate_subexp_c (struct type *expect_type, struct expression *exp,
 	/* Ensure TYPE_LENGTH is valid for TYPE.  */
 	check_typedef (type);
 
+	/* If the caller expects an array of some integral type,
+	   satisfy them.  If something odder is expected, rely on the
+	   caller to cast.  */
+	if (expect_type && TYPE_CODE (expect_type) == TYPE_CODE_ARRAY)
+	  {
+	    struct type *element_type
+	      = check_typedef (TYPE_TARGET_TYPE (expect_type));
+
+	    if (TYPE_CODE (element_type) == TYPE_CODE_INT
+		|| TYPE_CODE (element_type) == TYPE_CODE_CHAR)
+	      {
+		type = element_type;
+		satisfy_expected = 1;
+	      }
+	  }
+
 	dest_charset = charset_for_string_type (dest_type, exp->gdbarch);
 
 	++*pos;
@@ -1036,7 +1053,9 @@ evaluate_subexp_c (struct type *expect_type, struct expression *exp,
 	if (noside == EVAL_SKIP)
 	  {
 	    /* Return a dummy value of the appropriate type.  */
-	    if ((dest_type & C_CHAR) != 0)
+	    if (expect_type != NULL)
+	      result = allocate_value (expect_type);
+	    else if ((dest_type & C_CHAR) != 0)
 	      result = allocate_value (type);
 	    else
 	      result = value_cstring ("", 0, type);
@@ -1061,9 +1080,30 @@ evaluate_subexp_c (struct type *expect_type, struct expression *exp,
 	    /* Write the terminating character.  */
 	    for (i = 0; i < TYPE_LENGTH (type); ++i)
 	      obstack_1grow (&output, 0);
-	    result = value_cstring (obstack_base (&output),
-				    obstack_object_size (&output),
-				    type);
+
+	    if (satisfy_expected)
+	      {
+		LONGEST low_bound, high_bound;
+		int element_size = TYPE_LENGTH (type);
+
+		if (get_discrete_bounds (TYPE_INDEX_TYPE (expect_type),
+					 &low_bound, &high_bound) < 0)
+		  {
+		    low_bound = 0;
+		    high_bound = (TYPE_LENGTH (expect_type) / element_size) - 1;
+		  }
+		if (obstack_object_size (&output) / element_size
+		    > (high_bound - low_bound + 1))
+		  error (_("Too many array elements"));
+
+		result = allocate_value (expect_type);
+		memcpy (value_contents_raw (result), obstack_base (&output),
+			obstack_object_size (&output));
+	      }
+	    else
+	      result = value_cstring (obstack_base (&output),
+				      obstack_object_size (&output),
+				      type);
 	  }
 	do_cleanups (cleanup);
 	return result;
diff --git a/gdb/testsuite/gdb.base/charset.c b/gdb/testsuite/gdb.base/charset.c
index 644fe47..df56c45 100644
--- a/gdb/testsuite/gdb.base/charset.c
+++ b/gdb/testsuite/gdb.base/charset.c
@@ -73,6 +73,11 @@ char32_t *String32;
 typedef wchar_t my_wchar_t;
 my_wchar_t myvar;
 
+/* Some arrays for simple assignment tests.  */
+short short_array[3];
+int int_array[3];
+long long_array[3];
+
 void
 init_string (char string[],
              char x,
diff --git a/gdb/testsuite/gdb.base/charset.exp b/gdb/testsuite/gdb.base/charset.exp
index 4e4cf09..b558bb2 100644
--- a/gdb/testsuite/gdb.base/charset.exp
+++ b/gdb/testsuite/gdb.base/charset.exp
@@ -617,4 +617,13 @@ if {$wchar_size == 4} {
 }
 
 
+foreach name {short int long} {
+    # We're really just checking to make sure this doesn't give an
+    # error.
+    gdb_test "print ${name}_array = \"hi\"" \
+	" = {.*}" \
+	"assign string to $name array"
+}
+
+
 gdb_exit 

Re: RFC: fix `gdb -write' case

[Jan Kratochvil]

On Wed, 11 May 2011 21:41:47 +0200, Tom Tromey wrote:
> What do you think of this?  It uses the expected type to construct the
> array's values when it makes sense.

OK, I like it now.


Thanks,
Jan

Re: RFC: fix `gdb -write' case

[Tom Tromey]

Tom> What do you think of this?  It uses the expected type to construct the
Tom> array's values when it makes sense.

Jan> OK, I like it now.

I am checking it in.  Thanks.

Tom