[patch] pr11594

[patch] pr11594

[Chris Moller]

[-- Attachment #1: Type: text/plain, Size: 178 bytes --]

I'm fairly sure that the

    type2 = check_typedef (type1);

at valarith.c:277 is a typo and treally should be

    type2 = check_typedef (type2);

but, with gdb, who knows?




[-- Attachment #2: pr11594.patch --]
[-- Type: text/x-patch, Size: 6386 bytes --]

? testsuite/gdb.cp/pr11594
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.11916
diff -u -r1.11916 ChangeLog
--- ChangeLog	22 Jun 2010 00:09:08 -0000	1.11916
+++ ChangeLog	22 Jun 2010 02:38:33 -0000
@@ -1,3 +1,10 @@
+2010-06-21  Chris Moller  <cmoller@redhat.com>
+
+	* eval.c (evaluate_subexp_standard): Add a test for an overloaded
+	comma operator.
+	* valarith.c (binop_types_user_defined_p): Fix a typo.
+	(value_x_binop): Synthesise an "operator," function name.
+
 2010-06-21  Doug Evans  <dje@google.com>
 
 	* i386-tdep.h (i386_displaced_step_copy_insn): Declare.
Index: eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.136
diff -u -r1.136 eval.c
--- eval.c	7 Jun 2010 16:11:31 -0000	1.136
+++ eval.c	22 Jun 2010 02:38:37 -0000
@@ -2421,8 +2421,23 @@
 	return value_repeat (arg1, longest_to_int (value_as_long (arg2)));
 
     case BINOP_COMMA:
-      evaluate_subexp (NULL_TYPE, exp, pos, noside);
-      return evaluate_subexp (NULL_TYPE, exp, pos, noside);
+      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
+      arg2 = evaluate_subexp (value_type (arg1), exp, pos, noside);
+      if (noside == EVAL_SKIP)
+        goto nosideret;
+      if (current_language->la_language == language_cplus 
+	  && binop_user_defined_p (op, arg1, arg2))
+	{
+	  struct value *rc;
+	  
+	  rc = value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  if (rc != NULL)
+	    return rc;
+	  else
+	    return arg2;
+	}
+      else
+	return arg2;
 
     case UNOP_PLUS:
       arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
Index: valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.85
diff -u -r1.85 valarith.c
--- valarith.c	7 Jun 2010 16:11:31 -0000	1.85
+++ valarith.c	22 Jun 2010 02:38:38 -0000
@@ -274,7 +274,7 @@
   if (TYPE_CODE (type1) == TYPE_CODE_REF)
     type1 = check_typedef (TYPE_TARGET_TYPE (type1));
 
-  type2 = check_typedef (type1);
+  type2 = check_typedef (type2);
   if (TYPE_CODE (type2) == TYPE_CODE_REF)
     type2 = check_typedef (TYPE_TARGET_TYPE (type2));
 
@@ -418,6 +418,9 @@
   ptr = tstr + 8;
   switch (op)
     {
+    case BINOP_COMMA:
+      strcpy (ptr, ",");
+      break;
     case BINOP_ADD:
       strcpy (ptr, "+");
       break;
Index: testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.2344
diff -u -r1.2344 ChangeLog
--- testsuite/ChangeLog	22 Jun 2010 00:02:07 -0000	1.2344
+++ testsuite/ChangeLog	22 Jun 2010 02:38:55 -0000
@@ -1,3 +1,9 @@
+2010-06-21  Chris Moller  <moller@mollerware.com>
+
+	* gdb.cp/Makefile.in (EXECUTABLES):  Added pr11594.
+	* gdb.cp/pr11594.cc: New File.
+	* gdb.cp/pr11594.exp: New File.
+
 2010-06-21  Doug Evans  <dje@google.com>
 
 	* gdb.gdb/selftest.exp: Remove support for gpl v1 and v2 gdb's.
Index: testsuite/gdb.cp/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/Makefile.in,v
retrieving revision 1.11
diff -u -r1.11 Makefile.in
--- testsuite/gdb.cp/Makefile.in	21 Apr 2010 17:33:54 -0000	1.11
+++ testsuite/gdb.cp/Makefile.in	22 Jun 2010 02:38:56 -0000
@@ -5,7 +5,7 @@
 	derivation inherit local member-ptr method misc \
         overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
 	ref-types ref-params method2 pr9594 gdb2495 virtfunc2 pr9067 \
-	pr1072 pr10687 pr9167
+	pr1072 pr10687 pr9167 pr11594
 
 all info install-info dvi install uninstall installcheck check:
 	@echo "Nothing to be done for $@..."
Index: testsuite/gdb.cp/pr11594.cc
===================================================================
RCS file: testsuite/gdb.cp/pr11594.cc
diff -N testsuite/gdb.cp/pr11594.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11594.cc	22 Jun 2010 02:38:57 -0000
@@ -0,0 +1,46 @@
+class complx
+{
+  double real, imag;
+public:
+  complx( double real = 0., double imag = 0.); // constructor
+  complx operator,(const complx&) const;       // operator,()
+};
+
+// define constructor
+complx::complx( double r, double i )
+{
+  real = r; imag = i;
+}
+
+// define overloaded , (comma) operator
+complx
+complx::operator, (const complx& c) const
+{
+  complx result;
+  result.real = (this->real + c.real);
+  result.imag = (this->imag - c.imag);
+  return result;
+}
+
+int
+main()
+{
+  complx x(4,5);
+  complx y(6,7);
+  complx z;
+
+  return 0;	// break
+}
+
+#if 0
+(gdb) p x,y
+$1 = {real = 10, imag = -2}
+(gdb) p y,x
+$2 = {real = 10, imag = 2}
+(gdb) set variable z = x,y
+(gdb) p z
+$3 = {real = 4, imag = 5}
+(gdb) set variable z = (x,y)
+(gdb) p z
+$4 = {real = 10, imag = -2}
+#endif
Index: testsuite/gdb.cp/pr11594.exp
===================================================================
RCS file: testsuite/gdb.cp/pr11594.exp
diff -N testsuite/gdb.cp/pr11594.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr11594.exp	22 Jun 2010 02:38:57 -0000
@@ -0,0 +1,37 @@
+#Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile pr11594
+set srcfile ${testfile}.cc
+if [prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}] {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return
+}
+
+gdb_breakpoint [gdb_get_line_number "break"]
+gdb_continue_to_breakpoint "break"
+
+gdb_test "p x,y" "{real = 10, imag = -2}.*"
+
+gdb_test "set variable z = x,y" ""
+gdb_test "p z" "{real = 4, imag = 5}"
+
+gdb_test "set variable z = (x,y)" ""
+gdb_test "p z" "{real = 10, imag = -2}"
+

[-- Attachment #3: pr11594-gdb-sum.diff --]
[-- Type: text/x-patch, Size: 701 bytes --]

1c1
< Test Run By moller on Mon Jun 21 21:38:39 2010
---
> Test Run By moller on Mon Jun 21 22:12:55 2010
12245a12246,12252
> Running ../../../src/gdb/testsuite/gdb.cp/pr11594.exp ...
> PASS: gdb.cp/pr11594.exp: continue to breakpoint: break
> PASS: gdb.cp/pr11594.exp: p x,y
> PASS: gdb.cp/pr11594.exp: set variable z = x,y
> PASS: gdb.cp/pr11594.exp: p z
> PASS: gdb.cp/pr11594.exp: set variable z = (x,y)
> PASS: gdb.cp/pr11594.exp: p z
16610c16617
< KFAIL: gdb.threads/watchthreads2.exp: gdb can drop watchpoints in multithreaded app (PRMS: gdb/10116)
---
> PASS: gdb.threads/watchthreads2.exp: all threads incremented x
16873c16880
< # of expected passes		15991
---
> # of expected passes		15998

Re: [patch] pr11594

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> I'm fairly sure that the
Chris>    type2 = check_typedef (type1);
Chris> at valarith.c:277 is a typo and treally should be
Chris>    type2 = check_typedef (type2);

Yes.

Chris> but, with gdb, who knows?

The maintainers are here to help.  Feel free to ask questions either on
the gdb list or #gdb.

Chris> +2010-06-21  Chris Moller  <cmoller@redhat.com>
Chris> +
Chris> +	* eval.c (evaluate_subexp_standard): Add a test for an overloaded
Chris> +	comma operator.

Put the PR info at the top of the ChangeLog entry.
See ChangeLog for examples of the format.

Chris>      case BINOP_COMMA:
Chris> -      evaluate_subexp (NULL_TYPE, exp, pos, noside);
Chris> -      return evaluate_subexp (NULL_TYPE, exp, pos, noside);
Chris> +      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
Chris> +      arg2 = evaluate_subexp (value_type (arg1), exp, pos, noside);
Chris> +      if (noside == EVAL_SKIP)
Chris> +        goto nosideret;
Chris> +      if (current_language->la_language == language_cplus 
Chris> +	  && binop_user_defined_p (op, arg1, arg2))
Chris> +	{
Chris> +	  struct value *rc;
Chris> +	  
Chris> +	  rc = value_x_binop (arg1, arg2, op, OP_NULL, noside);
Chris> +	  if (rc != NULL)
Chris> +	    return rc;
Chris> +	  else
Chris> +	    return arg2;

I think I understand why the current_language check and the result check
of value_x_binop are needed.

However, I think it would be better to do all the work in value_x_binop
and also remove the current_language check.  This is more similar to
what other code does and it consolidates the (broken) current_language
checks in the value code.

I'm guessing this means a change to value_user_defined_op.

Chris> Index: testsuite/gdb.cp/pr11594.cc
Chris> ===================================================================
Chris> RCS file: testsuite/gdb.cp/pr11594.cc
Chris> diff -N testsuite/gdb.cp/pr11594.cc
Chris> --- /dev/null	1 Jan 1970 00:00:00 -0000
Chris> +++ testsuite/gdb.cp/pr11594.cc	22 Jun 2010 02:38:57 -0000
Chris> @@ -0,0 +1,46 @@
Chris> +class complx

Needs a GPL header.

Tom

Re: [patch] pr11594

[Chris Moller]

On 06/22/10 14:38, Tom Tromey wrote:
>
> Chris>       case BINOP_COMMA:
> Chris>  -      evaluate_subexp (NULL_TYPE, exp, pos, noside);
> Chris>  -      return evaluate_subexp (NULL_TYPE, exp, pos, noside);
> Chris>  +      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
> Chris>  +      arg2 = evaluate_subexp (value_type (arg1), exp, pos, noside);
> Chris>  +      if (noside == EVAL_SKIP)
> Chris>  +        goto nosideret;
> Chris>  +      if (current_language->la_language == language_cplus
> Chris>  +	&&  binop_user_defined_p (op, arg1, arg2))
> Chris>  +	{
> Chris>  +	  struct value *rc;
> Chris>  +	
> Chris>  +	  rc = value_x_binop (arg1, arg2, op, OP_NULL, noside);
> Chris>  +	  if (rc != NULL)
> Chris>  +	    return rc;
> Chris>  +	  else
> Chris>  +	    return arg2;
>
> I think I understand why the current_language check and the result check
> of value_x_binop are needed.
>
> However, I think it would be better to do all the work in value_x_binop
> and also remove the current_language check.  This is more similar to
> what other code does and it consolidates the (broken) current_language
> checks in the value code.
>
> I'm guessing this means a change to value_user_defined_op.
>    

I went down that route--that's what's taken so long about getting this 
patch out.  Under some circumstances, you wind up special-case checking 
for BINOP_COMMA and strcmp("operator,"   ) all over the place and 
eventually wind up in a place where there's not enough contextual 
information left to determine if you're dealing with an overloaded comma 
and call value_addr() that tries to take the target address of a 
register value.

The value_x_binop() result is checked because binop_user_defined_p() 
only asserts that it's possible for the operands to represent an 
overload, not that there is in fact an overload in this instance.  
That's apparently determined in value_x_binop() and if the operator 
isn't overloaded, I'm defaulting back to non-overloaded behaviour.  (It 
seems to me that the language  check ought to be in 
binop_user_defined_p() or binop_types_user_defined_p()--is there ever a 
circumstance other than language_cplus  where you can have overloaded 
operators?)

If the language check really is unacceptable, I'll revisit this, but the 
patch gets significantly more complicated.

I'm going to back-burner this, though--I've gone back to the matrix 
pretty-print stuff for a while.

-cm

Re: [patch] pr11594

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> The value_x_binop() result is checked because binop_user_defined_p()
Chris> only asserts that it's possible for the operands to represent an
Chris> overload, not that there is in fact an overload in this instance.
Chris> That's apparently determined in value_x_binop() and if the operator
Chris> isn't overloaded, I'm defaulting back to non-overloaded behaviour.
Chris> (It seems to me that the language  check ought to be in
Chris> binop_user_defined_p() or binop_types_user_defined_p()--is there ever
Chris> a circumstance other than language_cplus  where you can have
Chris> overloaded operators?)

I don't know of a case.

I couldn't see when value_x_binop can actually return NULL.  It didn't
happen for any of the tests in your .exp.

Another thing to consider are cases like "print 5,x" and "print x,5" in
your test.  These are both valid expressions (g++ accepts them), but
with your patch gdb errors out on at least the first one.

I think your test should also include a test of non-member "operator,".

Tom