[patch] function eval cleanup

[patch] function eval cleanup

[sami wagiaalla]

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

This code performs function evaluation before overload resolution completes.

Currently, for C++, this is a waste if overload resolution results in a 
different function than the one which was evaluated but it breaks a 
future patch of mine which introduces a fake place holding variable 
needed for proper template function evaluation.

This patch cleans up the above

Sami Wagiaalla


[-- Attachment #2: overload-cleanup.patch --]
[-- Type: text/plain, Size: 1886 bytes --]

commit 9ae843b0376c83096f8d2d0fad5c94142209e8c3
Author: Sami Wagiaalla <swagiaal@redhat.com>
Date:   Tue Jun 22 12:19:08 2010 -0400

    Corrected pre-overload-resolution function evaluation.
    
    2010-07-12  Sami Wagiaalla  <swagiaal@redhat.com>
    
    	* eval.c (evaluate_subexp_standard): Disabled evaluation of C++
    	function symbols before overload resolution.

diff --git a/gdb/eval.c b/gdb/eval.c
index 9a60616..ea3d8a0 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -1491,19 +1491,28 @@ evaluate_subexp_standard (struct type *expect_type,
 	{
 	  /* Non-method function call */
 	  save_pos1 = *pos;
-	  argvec[0] = evaluate_subexp_with_coercion (exp, pos, noside);
 	  tem = 1;
-	  type = value_type (argvec[0]);
-	  if (type && TYPE_CODE (type) == TYPE_CODE_PTR)
-	    type = TYPE_TARGET_TYPE (type);
-	  if (type && TYPE_CODE (type) == TYPE_CODE_FUNC)
+
+	  /* If this is a C++ function wait until overload resolution.  */
+	  if (overload_resolution
+	      && (exp->language_defn->la_language == language_cplus))
 	    {
-	      for (; tem <= nargs && tem <= TYPE_NFIELDS (type); tem++)
+	      (*pos) += 4; /* Skip the evaluation of the symbol.  */
+	      argvec[0] = NULL;
+	    }
+	  else
+	    {
+	      argvec[0] = evaluate_subexp_with_coercion (exp, pos, noside);
+	      type = value_type (argvec[0]);
+	      if (type && TYPE_CODE (type) == TYPE_CODE_PTR)
+		type = TYPE_TARGET_TYPE (type);
+	      if (type && TYPE_CODE (type) == TYPE_CODE_FUNC)
 		{
-		  /* pai: FIXME This seems to be coercing arguments before
-		   * overload resolution has been done! */
-		  argvec[tem] = evaluate_subexp (TYPE_FIELD_TYPE (type, tem - 1),
-						 exp, pos, noside);
+		  for (; tem <= nargs && tem <= TYPE_NFIELDS (type); tem++)
+		    {
+		      argvec[tem] = evaluate_subexp (TYPE_FIELD_TYPE (type, tem - 1),
+						     exp, pos, noside);
+		    }
 		}
 	    }
 	}

Re: [patch] function eval cleanup

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> This code performs function evaluation before overload resolution
Sami> completes.  Currently, for C++, this is a waste if overload
Sami> resolution results in a different function than the one which was
Sami> evaluated but it breaks a future patch of mine which introduces a
Sami> fake place holding variable needed for proper template function
Sami> evaluation.

You didn't mention how you tested this...

Sami> +	  /* If this is a C++ function wait until overload resolution.  */
Sami> +	  if (overload_resolution
Sami> +	      && (exp->language_defn->la_language == language_cplus))
Sami>  	    {
Sami> -	      for (; tem <= nargs && tem <= TYPE_NFIELDS (type); tem++)
Sami> +	      (*pos) += 4; /* Skip the evaluation of the symbol.  */

Why is it ok to add 4 here?
I think this assumes that the function in question is a symbol.
But is that always the case?  What about something like calling via a
function pointer?  Or some other more complicated subexpression?

If there is a failing case here, and it was not caught by the test
suite, please also add a regression test.

Some day we should refactor this whole case, it is really hard to
understand.  And 'overload_resolution' -- another global affecting
things randomly throughout gdb.  Sigh.  I wonder if anybody would
complain if we just got rid of it.

Tom

Re: [patch] function eval cleanup

[sami wagiaalla]

> You didn't mention how you tested this...
>

This was tested by running the test suit on Fedora 13 with gcc 4.4.4 on 
x8664.

> Sami>  +	  /* If this is a C++ function wait until overload resolution.  */
> Sami>  +	  if (overload_resolution
> Sami>  +	&&  (exp->language_defn->la_language == language_cplus))
> Sami>   	{
> Sami>  -	      for (; tem<= nargs&&  tem<= TYPE_NFIELDS (type); tem++)
> Sami>  +	      (*pos) += 4; /* Skip the evaluation of the symbol.  */
>
> Why is it ok to add 4 here?
> I think this assumes that the function in question is a symbol.
> But is that always the case?  What about something like calling via a
> function pointer?  Or some other more complicated subexpression?
>

In this particular location and if the language is C++ we are dealing 
with an OP_VAR_VALUE since all the other constructs were eliminated by 
the if else chain. So it is always a symbol, but I could add a check for 
OP_VAR_VALUE before skipping evaluation.

> If there is a failing case here, and it was not caught by the test
> suite, please also add a regression test.
>

No, no failing case. Just the FIXME by pai. It is kind of tricky to test 
because whatever symbol is evaluated here is replace by overload 
reslution and quietly re-evaluated.

It only causes problems when meta-symbol for templates is introduced as 
it is not meant to be evaluated see patch 2 of Template Lookup.

Re: [patch] function eval cleanup

[sami wagiaalla]

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


>
> In this particular location and if the language is C++ we are dealing
> with an OP_VAR_VALUE since all the other constructs were eliminated by
> the if else chain. So it is always a symbol, but I could add a check for
> OP_VAR_VALUE before skipping evaluation.
>

Tom pointed out that the catch all is also responsible for evaluating 
complex expressions describing function pointers. So I added a check for 
OP_VAR_VALUE, and a test-case for the missed scenario.


[-- Attachment #2: overload-cleanup.patch --]
[-- Type: text/plain, Size: 4315 bytes --]

Corrected pre-overload-resolution function evaluation.

2010-07-20  Sami Wagiaalla  <swagiaal@redhat.com>

	* eval.c (evaluate_subexp_standard): Disabled evaluation of C++
	function symbols before overload resolution.

2010-07-20  Sami Wagiaalla  <swagiaal@redhat.com>

	* gdb.cp/fpointer.cc: New test.
	* gdb.cp/fpointer.exp: New test.

diff --git a/gdb/eval.c b/gdb/eval.c
index 9a60616..ba75952 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -1491,19 +1491,29 @@ evaluate_subexp_standard (struct type *expect_type,
 	{
 	  /* Non-method function call */
 	  save_pos1 = *pos;
-	  argvec[0] = evaluate_subexp_with_coercion (exp, pos, noside);
 	  tem = 1;
-	  type = value_type (argvec[0]);
-	  if (type && TYPE_CODE (type) == TYPE_CODE_PTR)
-	    type = TYPE_TARGET_TYPE (type);
-	  if (type && TYPE_CODE (type) == TYPE_CODE_FUNC)
+
+	  /* If this is a C++ function wait until overload resolution.  */
+	  if (op == OP_VAR_VALUE
+	      && overload_resolution
+	      && (exp->language_defn->la_language == language_cplus))
 	    {
-	      for (; tem <= nargs && tem <= TYPE_NFIELDS (type); tem++)
+	      (*pos) += 4; /* Skip the evaluation of the symbol.  */
+	      argvec[0] = NULL;
+	    }
+	  else
+	    {
+	      argvec[0] = evaluate_subexp_with_coercion (exp, pos, noside);
+	      type = value_type (argvec[0]);
+	      if (type && TYPE_CODE (type) == TYPE_CODE_PTR)
+		type = TYPE_TARGET_TYPE (type);
+	      if (type && TYPE_CODE (type) == TYPE_CODE_FUNC)
 		{
-		  /* pai: FIXME This seems to be coercing arguments before
-		   * overload resolution has been done! */
-		  argvec[tem] = evaluate_subexp (TYPE_FIELD_TYPE (type, tem - 1),
-						 exp, pos, noside);
+		  for (; tem <= nargs && tem <= TYPE_NFIELDS (type); tem++)
+		    {
+		      argvec[tem] = evaluate_subexp (TYPE_FIELD_TYPE (type, tem - 1),
+						     exp, pos, noside);
+		    }
 		}
 	    }
 	}
diff --git a/gdb/testsuite/gdb.cp/fpointer.cc b/gdb/testsuite/gdb.cp/fpointer.cc
new file mode 100644
index 0000000..007ea39
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/fpointer.cc
@@ -0,0 +1,40 @@
+#include "stdio.h"
+
+int foo(){
+  return 11;
+}
+
+int bar(){
+  return 12;
+}
+
+int bar(int){
+  printf ("bar(int)\n");
+  return 13;
+}
+
+int bar(char){
+  printf ("bar(char)\n");
+  return 14;
+}
+
+int (*p1)() = &foo;
+int (*p2)() = &bar;
+int (*p[2])() = {p1,p2};
+
+int (*p3)(int) = &bar;
+int (*p4)(char) = &bar;
+
+int main ()
+{
+  p1 ();
+  p2 ();
+
+  p[0]();
+  p[1]();
+
+  p3 ('a');
+  p4 (1);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.cp/fpointer.exp b/gdb/testsuite/gdb.cp/fpointer.exp
new file mode 100644
index 0000000..6144bbe
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/fpointer.exp
@@ -0,0 +1,54 @@
+# Copyright 2008, 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/>.
+
+# This tests for evaluation of functions in the form of
+# complex expressions.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile fpointer
+set srcfile ${testfile}.cc
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
+      {debug c++}] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+# Get things started.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if ![runto_main] then {
+    perror "couldn't run to breakpoint main"
+    continue
+}
+
+# Test that function calls done through function pointers
+gdb_test "p p1()" "= 11"
+gdb_test "p p2()" "= 12"
+
+gdb_test {p p[0]()} "= 11"
+gdb_test {p p[1]()} "= 12"
+
+# Test that overload resolution is not performed for function
+# pointers.
+gdb_test "p p3('a')" "= 13"
+gdb_test "p p4(1)"   "= 14"

Re: [patch] function eval cleanup

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> Corrected pre-overload-resolution function evaluation.
Sami> 2010-07-20  Sami Wagiaalla  <swagiaal@redhat.com>
Sami> 	* eval.c (evaluate_subexp_standard): Disabled evaluation of C++
Sami> 	function symbols before overload resolution.

Looks good.

Just one detail...

Sami> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
Sami> +      {debug c++}] != "" } {
Sami> +    untested "Couldn't compile test program"
Sami> +    return -1
Sami> +}
Sami> +
Sami> +# Get things started.
Sami> +
Sami> +gdb_exit
Sami> +gdb_start
Sami> +gdb_reinitialize_dir $srcdir/$subdir
Sami> +gdb_load ${binfile}

These days, prepare_for_testing is preferred instead of all the
boilerplate.

Ok with that change.

Tom