* [patch] function eval cleanup
@ 2010-07-12 18:51 sami wagiaalla
2010-07-14 23:00 ` Tom Tromey
0 siblings, 1 reply; 5+ messages in thread
From: sami wagiaalla @ 2010-07-12 18:51 UTC (permalink / raw)
To: gdb-patches
[-- 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);
+ }
}
}
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] function eval cleanup
2010-07-12 18:51 [patch] function eval cleanup sami wagiaalla
@ 2010-07-14 23:00 ` Tom Tromey
2010-07-19 15:28 ` sami wagiaalla
0 siblings, 1 reply; 5+ messages in thread
From: Tom Tromey @ 2010-07-14 23:00 UTC (permalink / raw)
To: sami wagiaalla; +Cc: gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] function eval cleanup
2010-07-14 23:00 ` Tom Tromey
@ 2010-07-19 15:28 ` sami wagiaalla
2010-07-20 19:05 ` sami wagiaalla
0 siblings, 1 reply; 5+ messages in thread
From: sami wagiaalla @ 2010-07-19 15:28 UTC (permalink / raw)
To: gdb-patches
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] function eval cleanup
2010-07-19 15:28 ` sami wagiaalla
@ 2010-07-20 19:05 ` sami wagiaalla
2010-07-20 19:57 ` Tom Tromey
0 siblings, 1 reply; 5+ messages in thread
From: sami wagiaalla @ 2010-07-20 19:05 UTC (permalink / raw)
To: gdb-patches
[-- 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"
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] function eval cleanup
2010-07-20 19:05 ` sami wagiaalla
@ 2010-07-20 19:57 ` Tom Tromey
0 siblings, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2010-07-20 19:57 UTC (permalink / raw)
To: sami wagiaalla; +Cc: gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-07-20 19:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-12 18:51 [patch] function eval cleanup sami wagiaalla
2010-07-14 23:00 ` Tom Tromey
2010-07-19 15:28 ` sami wagiaalla
2010-07-20 19:05 ` sami wagiaalla
2010-07-20 19:57 ` Tom Tromey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).