[RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Paul Pluzhnikov]

Greetings,

Long time GDB users are somewhat used to 'p->x' and 'p.x' meaning
the same thing. But, as test case below demonstrates, they don't
mean the same thing when 'set print object on' is in effect.

I believe it is desirable to make the behavior consistent -- either
always accept 'p.x' when 'p->x' was meant, or always insist on
correct form (which is what VisualStudio does).

Attached patch fixes the problem for field access (but not for
mem-function access -- that goes through different path), and
eliminates some redundant code.

Comments?

Test case demonstrating the problem:

--- cut ---
struct Foo {
 int x;
 Foo() : x(1) { }
 virtual ~Foo() { }
};

struct Bar : public Foo {
 Bar() : y(2), z(3) { }
 int y, z;
};

int fn(Foo *f)
{
 return f->x;
}

int main()
{
 Bar b;
 return fn(&b);
}
--- cut ---

(gdb) b fn
Breakpoint 1 at 0x4002e0: file print-obj.cc, line 14.
(gdb) r

Breakpoint 1, fn (f=0x7fffffffe690) at print-obj.cc:14
14        return f->x;
(gdb) p f
$1 = (Foo *) 0x7fffffffe690
(gdb) set print object on
(gdb) p f
$2 = (Bar *) 0x7fffffffe690
(gdb) p f->z          # works
$3 = 3
(gdb) p f.z           # doesn't work -- bug in gdb
There is no member or method named z.

# For members of 'Foo' it works either way:

(gdb) p f->x
$4 = 1
(gdb) p f.x
$5 = 1


Thanks,

--
Paul Pluzhnikov

2008-07-17  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* eval.c (evaluate_subexp_standard): Treat 'p.x' and 'p->x' the same.


Index: eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.85
diff -u -p -u -r1.85 eval.c
--- eval.c	6 Jun 2008 20:58:08 -0000	1.85
+++ eval.c	17 Jul 2008 21:32:49 -0000
@@ -1374,23 +1374,6 @@ evaluate_subexp_standard (struct type *e
       return value_literal_complex (arg1, arg2, builtin_type_f_complex_s16);
 
     case STRUCTOP_STRUCT:
-      tem = longest_to_int (exp->elts[pc + 1].longconst);
-      (*pos) += 3 + BYTES_TO_EXP_ELEM (tem + 1);
-      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
-      if (noside == EVAL_SKIP)
-	goto nosideret;
-      if (noside == EVAL_AVOID_SIDE_EFFECTS)
-	return value_zero (lookup_struct_elt_type (value_type (arg1),
-						   &exp->elts[pc + 2].string,
-						   0),
-			   lval_memory);
-      else
-	{
-	  struct value *temp = arg1;
-	  return value_struct_elt (&temp, NULL, &exp->elts[pc + 2].string,
-				   NULL, "structure");
-	}
-
     case STRUCTOP_PTR:
       tem = longest_to_int (exp->elts[pc + 1].longconst);
       (*pos) += 3 + BYTES_TO_EXP_ELEM (tem + 1);
@@ -1430,8 +1413,10 @@ evaluate_subexp_standard (struct type *e
       else
 	{
 	  struct value *temp = arg1;
+	  char *const name =
+              (op == STRUCTOP_STRUCT) ? "structure" : "structure pointer";
 	  return value_struct_elt (&temp, NULL, &exp->elts[pc + 2].string,
-				   NULL, "structure pointer");
+				   NULL, name);
 	}
 
     case STRUCTOP_MEMBER:

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Long time GDB users are somewhat used to 'p->x' and 'p.x' meaning
Paul> the same thing. But, as test case below demonstrates, they don't
Paul> mean the same thing when 'set print object on' is in effect.

FWIW, I like this.  I was always confused when I printed an object and
saw a field, but was then unable to print the field directly.

However, I recall bringing this up once, in the distant past.  Someone
pointed out a difficulty with this approach: there might be one field
'x' that would be used with the declared type, but another field, also
named 'x', that would be used with the actual type.  So, this could
potentially be confusing (albeit in an already confusing situation).

Maybe this can be documented or otherwise resolved.

Also, IMO this patch could use an associated test case.

Tom

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Paul Pluzhnikov]

On Mon, Jul 21, 2008 at 5:52 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>
> Paul> Long time GDB users are somewhat used to 'p->x' and 'p.x' meaning
> Paul> the same thing. But, as test case below demonstrates, they don't
> Paul> mean the same thing when 'set print object on' is in effect.
>
> FWIW, I like this.  I was always confused when I printed an object and
> saw a field, but was then unable to print the field directly.
>
> However, I recall bringing this up once, in the distant past.  Someone
> pointed out a difficulty with this approach: there might be one field
> 'x' that would be used with the declared type, but another field, also
> named 'x', that would be used with the actual type.  So, this could
> potentially be confusing (albeit in an already confusing situation).

Thanks for bringing that up. I've modified the test case to have
a field 'y' in both the base and derived classes (attached at the
end), and the current situation is (IMHO) just as confusing before
the patch as it is after :(

Before patch (current behavior):

  Breakpoint 1, fn (f=0x7fffffffe690) at print-obj.cc:16
  16        return f->y;
  (gdb) p *f
  $3 = {_vptr.Foo = 0x400570, x = 1, y = 2}
  (gdb) p f->y
  $4 = 2
  (gdb) p f.y
  $5 = 2
  (gdb) set print object on
  (gdb) p *f
  $6 = (Bar) {<Foo> = {_vptr.Foo = 0x400570, x = 1, y = 2}, y = 20, z = 30}
  (gdb) p f->y
  $7 = 20
  (gdb) p f.y
  $8 = 2

// f.y and f->y do not mean the same thing :(

  (gdb) finish
  0x0000000000400305 in main () at print-obj.cc:22
  22        return fn(&b);
  Value returned is $9 = 2

// Confusing: 'print f->y' says '20'; but 'finish' says '2'

After:

  Breakpoint 1, fn (f=0x7fffffffe660) at print-obj.cc:16
  16        return f->y;
  (gdb) p *f
  $1 = {_vptr.Foo = 0x400570, x = 1, y = 2}
  (gdb) p f->y
  $2 = 2
  (gdb) p f.y
  $3 = 2

// without "print object on" patch makes no difference

  (gdb) set print object on
  (gdb) p *f
  $4 = (Bar) {<Foo> = {_vptr.Foo = 0x400570, x = 1, y = 2}, y = 20, z = 30}
  (gdb) p f->y
  $5 = 20
  (gdb) p f.y
  $6 = 20

// with "print object on" GDB is consistently treating f.y and f->y
// as the same expression.

  (gdb) finish
  0x0000000000400305 in main () at print-obj.cc:22
  22        return fn(&b);
  Value returned is $7 = 2

// Still confusing ...

> Maybe this can be documented or otherwise resolved.

Some other alternatives (hand-simulated):

A) Print both:

  (gbd) p f->y
  $14 = 2      (f->Foo::y)
  $15 = 20     (f->Bar::y)

  (gbd) p f.y
  $16 = 2      (f->Foo::y)
  $17 = 20     (f->Bar::y)

B) Print warning:

  (gdb) p f.y
  Warning: more than one field 'y' resulted from lookup in dynamic
  type of 'f'; set 'print object off' to see the other 'y'.
  $18 = 20

C) Do what the language does: lookup field 'x' in the static type,
   and only try dynamic type if the first lookup failed:

  (gdb) p f.y
  $19 = 2
  (gdb) p f->y
  $20 = 2

// The other field still accessible:
  (gdb) p f->Bar::y
  $21 = 20

// Lookup in Foo fails; try Bar
  (gdb) p f->z
  $22 = 30


I think "C" is the least confusing alternative.
It may actually be good to do "C" independent of the 'print object'
setting.

> Also, IMO this patch could use an associated test case.

Yes, I just wanted to see what people think about this before
creating the test case.

Thanks again.
-- 
Paul Pluzhnikov

// test case
struct Foo {
  int x, y;
  Foo() : x(1), y(2) { }
  virtual ~Foo() { }
  virtual int method() = 0;
};

struct Bar : public Foo {
  Bar() : y(20), z(30) { }
  int method() { return 42; }
  int y, z;
};

int fn(Foo *f)
{
  return f->y;
}

int main()
{
  Bar b;
  return fn(&b);
}

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Thanks for bringing that up. I've modified the test case to have
Paul> a field 'y' in both the base and derived classes (attached at the
Paul> end), and the current situation is (IMHO) just as confusing before
Paul> the patch as it is after :(

Thanks for doing this.

Paul> C) Do what the language does: lookup field 'x' in the static type,
Paul>    and only try dynamic type if the first lookup failed:

Paul> I think "C" is the least confusing alternative.
Paul> It may actually be good to do "C" independent of the 'print object'
Paul> setting.

I agree.  This does sound better.

Paul> Yes, I just wanted to see what people think about this before
Paul> creating the test case.

FWIW I'm definitely in favor.

Tom

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Daniel Jacobowitz]

On Thu, Aug 07, 2008 at 11:31:14AM -0600, Tom Tromey wrote:
> Paul> C) Do what the language does: lookup field 'x' in the static type,
> Paul>    and only try dynamic type if the first lookup failed:
> 
> Paul> I think "C" is the least confusing alternative.
> Paul> It may actually be good to do "C" independent of the 'print object'
> Paul> setting.
> 
> I agree.  This does sound better.

I agree too.  We have an extension that allows you to use "." on
pointers with the implicit dereference; but it shouldn't change the
type of the pointer, nor should ->.  It's a reasonably well-defined
extension as such things go.  At least, until you involve user-defined
operators.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> C) Do what the language does: lookup field 'x' in the static type,
Paul>    and only try dynamic type if the first lookup failed:

It occurred to me last night that we must be careful not to do this in
the overload resolution case.  Looking at extra overloads from the
dynamic type will yield the wrong answer.  I think these code paths
are separate in gdb, so that should not be a big deal, but I thought
it would be good to be explicit about it.

Tom

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Paul Pluzhnikov]

On Mon, Aug 11, 2008 at 8:08 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>
> Paul> C) Do what the language does: lookup field 'x' in the static type,
> Paul>    and only try dynamic type if the first lookup failed:
>
> It occurred to me last night that we must be careful not to do this in
> the overload resolution case.

Actually, I think there are 2 separate cases: virtual and non-virtual
function.

We must not do this in the non-virtual case, but *should* do that
in virtual case. IOW, 'print basep->virtfn()' and 'print
basep.virtfn()' should be the same for consistency.

> Looking at extra overloads from the
> dynamic type will yield the wrong answer.  I think these code paths
> are separate in gdb, so that should not be a big deal, but I thought
> it would be good to be explicit about it.

I've added tests for these cases.

Unfortunately, as I was adding tests, I discovered 4 new bugs :(
The patch does not introduce any new failures for me.

-- 
Paul Pluzhnikov


gdb/ChangeLog

2008-08-19  Paul Pluzhnikov  <ppluzhnikov@google.com>

	Changes to treat 'p.x' and 'p->x' the same.
	* eval.c (value_static_or_dynamic_member): New.
	(evaluate_subexp_standard): Call it.
	
gdb/testsuite/ChangeLog

2008-08-19  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb.cp/class3.exp: New test case.
	* gdb.cp/class3.cc: New source file.
	
diff -Npur gdb.orig/eval.c gdb/eval.c
--- gdb.orig/eval.c	2008-08-19 09:48:39.000000000 -0700
+++ gdb/eval.c	2008-08-19 10:14:04.000000000 -0700
@@ -44,10 +44,6 @@
 /* This is defined in valops.c */
 extern int overload_resolution;

-/* JYG: lookup rtti type of STRUCTOP_PTR when this is set to continue
-   on with successful lookup for member/method of the rtti type. */
-extern int objectprint;
-
 /* Prototypes for local functions. */

 static struct value *evaluate_subexp_for_sizeof (struct expression *, int *);
@@ -438,6 +434,51 @@ value_f90_subarray (struct value *array,
   return value_slice (array, low_bound, high_bound - low_bound + 1);
 }

+static struct value *
+value_static_or_dynamic_member(struct value *arg, char *string,
+			       char *name, enum noside noside)
+{
+  struct type *type = value_type (arg);
+  struct value *temp;
+
+  if (noside == EVAL_AVOID_SIDE_EFFECTS)
+    return value_zero (lookup_struct_elt_type (type, string, 0),
+		       lval_memory);
+  temp = arg;
+  {
+    volatile struct gdb_exception except;
+    struct value *v = NULL;
+    TRY_CATCH (except, RETURN_MASK_ERROR)
+      {
+	v = value_struct_elt (&temp, NULL, string, NULL, name);
+      }
+    if (v)
+      return v;
+  }
+
+  /* If we got here, value_struct_elt() above must have thrown,
+     and there is no field 'name' in 'type'.
+     Try dynamic type of 'arg' instead. */
+
+  if (TYPE_TARGET_TYPE(type)
+      && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_CLASS))
+    {
+      struct type *real_type;
+      int full, top, using_enc;
+      real_type = value_rtti_target_type (arg, &full, &top, &using_enc);
+      if (real_type)
+	{
+	  if (TYPE_CODE (type) == TYPE_CODE_PTR)
+	    real_type = lookup_pointer_type (real_type);
+	  else
+	    real_type = lookup_reference_type (real_type);
+
+	  temp = arg = value_cast (real_type, arg);
+	}
+    }
+  return value_struct_elt (&temp, NULL, string, NULL, name);
+}
+
 struct value *
 evaluate_subexp_standard (struct type *expect_type,
 			  struct expression *exp, int *pos,
@@ -1374,23 +1415,6 @@ evaluate_subexp_standard (struct type *e
       return value_literal_complex (arg1, arg2, builtin_type_f_complex_s16);

     case STRUCTOP_STRUCT:
-      tem = longest_to_int (exp->elts[pc + 1].longconst);
-      (*pos) += 3 + BYTES_TO_EXP_ELEM (tem + 1);
-      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
-      if (noside == EVAL_SKIP)
-	goto nosideret;
-      if (noside == EVAL_AVOID_SIDE_EFFECTS)
-	return value_zero (lookup_struct_elt_type (value_type (arg1),
-						   &exp->elts[pc + 2].string,
-						   0),
-			   lval_memory);
-      else
-	{
-	  struct value *temp = arg1;
-	  return value_struct_elt (&temp, NULL, &exp->elts[pc + 2].string,
-				   NULL, "structure");
-	}
-
     case STRUCTOP_PTR:
       tem = longest_to_int (exp->elts[pc + 1].longconst);
       (*pos) += 3 + BYTES_TO_EXP_ELEM (tem + 1);
@@ -1398,41 +1422,10 @@ evaluate_subexp_standard (struct type *e
       if (noside == EVAL_SKIP)
 	goto nosideret;

-      /* JYG: if print object is on we need to replace the base type
-	 with rtti type in order to continue on with successful
-	 lookup of member / method only available in the rtti type. */
-      {
-        struct type *type = value_type (arg1);
-        struct type *real_type;
-        int full, top, using_enc;
-
-        if (objectprint && TYPE_TARGET_TYPE(type) &&
-            (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_CLASS))
-          {
-            real_type = value_rtti_target_type (arg1, &full, &top, &using_enc);
-            if (real_type)
-              {
-                if (TYPE_CODE (type) == TYPE_CODE_PTR)
-                  real_type = lookup_pointer_type (real_type);
-                else
-                  real_type = lookup_reference_type (real_type);
-
-                arg1 = value_cast (real_type, arg1);
-              }
-          }
-      }
-
-      if (noside == EVAL_AVOID_SIDE_EFFECTS)
-	return value_zero (lookup_struct_elt_type (value_type (arg1),
-						   &exp->elts[pc + 2].string,
-						   0),
-			   lval_memory);
-      else
-	{
-	  struct value *temp = arg1;
-	  return value_struct_elt (&temp, NULL, &exp->elts[pc + 2].string,
-				   NULL, "structure pointer");
-	}
+      return value_static_or_dynamic_member(arg1, &exp->elts[pc + 2].string,
+					    (op == STRUCTOP_STRUCT)
+					    ? "structure" : "structure pointer",
+					    noside);

     case STRUCTOP_MEMBER:
     case STRUCTOP_MPTR:
diff -Npur gdb.orig/testsuite/gdb.cp/class3.cc gdb/testsuite/gdb.cp/class3.cc
--- gdb.orig/testsuite/gdb.cp/class3.cc	1969-12-31 16:00:00.000000000 -0800
+++ gdb/testsuite/gdb.cp/class3.cc	2008-08-19 10:06:13.000000000 -0700
@@ -0,0 +1,64 @@
+struct Base {
+  int x, y;
+  Base() : x(1), y(2) { }
+  virtual ~Base() { }
+          int  fn() { return x + y; }
+  virtual int vfn() { return x + y; }
+};
+
+struct D1 : public Base {
+  D1() : y(20), d1(30) { }
+          int  fn() { return x + y; }
+  virtual int vfn() { return x + y; }
+  int y, d1;
+};
+
+
+struct D2 : public Base {
+  D2() : y(200), d2(300) { }
+          int  fn() { return x + y; }
+  virtual int vfn() { return x + y; }
+  int y, d2;
+};
+
+struct D2v : public virtual Base {
+  D2v() : y(220), d2v(330) { }
+          int  fn() { return x + y; }
+  virtual int vfn() { return x + y; }
+  int y, d2v;
+};
+
+struct D3: public virtual D1, public virtual D2 {
+  D3() : y(2000), d3(3000) { }
+          int  fn() { return this->D1::x + y; }
+  virtual int vfn() { return this->D1::x + y; }
+  D1 *getD1() { return (D1*)this; }
+  D2 *getD2() { return (D2*)this; }
+  int y, d3;
+};
+
+int fn(Base *b)
+{
+  return b->y;
+}
+
+int main()
+{
+  Base b;
+  D1  d1;
+  D2  d2;
+  D2v d2v;
+  D3  d3;
+  return (fn(&b)            == 2
+	  && fn(&d1)        == 2
+	  && fn(&d2)        == 2
+	  && fn(&d2v)       == 2
+	  && fn(d3.getD1()) == 2
+	  && fn(d3.getD2()) == 2) ? 0 : 1;
+}
+
+int no_optimize(Base *b, D1 *d1, D2 *d2, D3 *d3)
+{
+  return b->fn() + d1->fn() + d2->fn() + d3->fn() +
+    b->vfn() + d1->vfn() + d2->vfn() + d3->vfn();
+}
diff -Npur gdb.orig/testsuite/gdb.cp/class3.exp gdb/testsuite/gdb.cp/class3.exp
--- gdb.orig/testsuite/gdb.cp/class3.exp	1969-12-31 16:00:00.000000000 -0800
+++ gdb/testsuite/gdb.cp/class3.exp	2008-08-19 10:12:52.000000000 -0700
@@ -0,0 +1,114 @@
+# Copyright 2008 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/>.
+
+if $tracelevel then {
+    strace $tracelevel
+    }
+
+if { [skip_cplus_tests] } { continue }
+
+set prms_id 0
+set bug_id 0
+
+set testfile "class3"
+set srcfile ${testfile}.cc
+set binfile ${objdir}/${subdir}/${testfile}
+
+# Create and source the file that provides information about the compiler
+# used to compile the test case.
+if [get_compiler_info ${binfile} "c++"] {
+    return -1
+}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
executable {debug c++}] != "" } {
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# ARGS is a list of quads:
+#   field
+#   its expected value
+#   whether b.field  is a known failure
+#   whether b->field is a known failure
+proc one_test {name args} {
+    foreach on_off {"on" "off"} {
+	gdb_test "set print object $on_off" ""
+	foreach extra $args {
+	    set f [lindex $extra 0]
+	    set v [lindex $extra 1]
+	    set kfail1 [lindex $extra 2]
+	    set kfail2 [lindex $extra 3]
+
+	    if {$kfail1 != ""} {
+		setup_kfail "*-*-*" $kfail1
+	    }
+	    gdb_test "print b.$f"  ".* = $v" "$name b.$f (print object $on_off)"
+
+	    if {$kfail2 != ""} {
+		setup_kfail "*-*-*" $kfail2
+	    }
+	    gdb_test "print b->$f" ".* = $v" "$name b->$f (print object $on_off)"
+	}
+    }
+}
+
+
+if ![runto_main] then {
+    perror "couldn't run to main"
+    continue
+}
+
+gdb_test "break fn" \
+    "Breakpoint.*at.* file .*" ""
+
+gdb_test "continue" "Breakpoint .* at .*" ""
+
+# Looking at Base*
+one_test "Base" {x 1} {y 2} { "fn()" 3 "gdb/NNN0" } \
+    { "vfn()" 3 "gdb/NNN0" }
+gdb_test "continue" "Breakpoint .* at .*" ""
+
+# Looking at D1*
+one_test "D1" {x 1} {y 2} {"d1" 30} {"D1::y" 20 "gdb/NNN1"} \
+    { "fn()" 3 "gdb/NNN0" } { "vfn()" 21 "gdb/NNN0" }
+gdb_test "continue" "Breakpoint .* at .*" ""
+
+# Looking at D2*
+one_test "D2" {x 1} {y 2} {"d2" 300} {"D2::y" 200 "gdb/NNN1"} \
+    { "fn()" 3 "gdb/NNN0" } { "vfn()" 201 "gdb/NNN0" }
+gdb_test "continue" "Breakpoint .* at .*" ""
+
+# Looking at D2v*
+one_test "D2v" {x 1} {y 2} {"d2v" 330 "gdb/NNN2" "gdb/NNN2"} \
+    {"D2::y" 220 "gdb/NNN2" "gdb/NNN2"} \
+    { "fn()" 3 "gdb/NNN0" } { "vfn()" 221 "gdb/NNN0" }
+gdb_test "continue" "Breakpoint .* at .*" ""
+
+# Looking at D1* part of D3
+one_test "D3::D1" {x 1} {y 2} {"d1" 30 "gdb/NNN3" "gdb/NNN3"} \
+    {"D1::y" 20 "gdb/NNN3" "gdb/NNN3"} \
+    { "fn()" 3 "gdb/NNN0" } { "vfn()" 21 "gdb/NNN0" "gdb/NNN0a" }
+gdb_test "continue" "Breakpoint .* at .*" ""
+
+# Looking at D2* part of D3
+one_test "D3::D2" {x 1} {y 2} {"d2" 300 "gdb/NNN3" "gdb/NNN3"} \
+    {"D2::y" 200 "gdb/NNN3" "gdb/NNN3"} \
+    { "fn()" 3 "gdb/NNN0" } { "vfn()" 201 "gdb/NNN0" "gdb/NNN0a" }
+gdb_test "continue" "Program exited normally\." ""

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Paul Pluzhnikov]

Ping:

http://sourceware.org/ml/gdb-patches/2008-08/msg00525.html

-- 
Paul Pluzhnikov

Re: [RFC] [patch] 'p->x' vs. 'p.x' and 'print object on'

[Ulrich Weigand]

Paul Pluzhnikov wrote:

> gdb/ChangeLog
> 
> 2008-08-19  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	Changes to treat 'p.x' and 'p->x' the same.
> 	* eval.c (value_static_or_dynamic_member): New.
> 	(evaluate_subexp_standard): Call it.
> 	
> gdb/testsuite/ChangeLog
> 
> 2008-08-19  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* gdb.cp/class3.exp: New test case.
> 	* gdb.cp/class3.cc: New source file.


Sorry for the late review.  Tom just pointed me to this
patch in the context of a different discussion ...

In general, I think the approach looks right to me.

However, the current patch does seem to have a couple
of issues:

> +static struct value *
> +value_static_or_dynamic_member(struct value *arg, char *string,
> +			       char *name, enum noside noside)
> +{
> +  struct type *type = value_type (arg);
> +  struct value *temp;
> +
> +  if (noside == EVAL_AVOID_SIDE_EFFECTS)
> +    return value_zero (lookup_struct_elt_type (type, string, 0),
> +		       lval_memory);
> +  temp = arg;
> +  {
> +    volatile struct gdb_exception except;
> +    struct value *v = NULL;
> +    TRY_CATCH (except, RETURN_MASK_ERROR)
> +      {
> +	v = value_struct_elt (&temp, NULL, string, NULL, name);
> +      }
> +    if (v)
> +      return v;
> +  }
> +
> +  /* If we got here, value_struct_elt() above must have thrown,
> +     and there is no field 'name' in 'type'.
> +     Try dynamic type of 'arg' instead. */
> +
> +  if (TYPE_TARGET_TYPE(type)
> +      && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_CLASS))
> +    {
> +      struct type *real_type;
> +      int full, top, using_enc;
> +      real_type = value_rtti_target_type (arg, &full, &top, &using_enc);
> +      if (real_type)
> +	{
> +	  if (TYPE_CODE (type) == TYPE_CODE_PTR)
> +	    real_type = lookup_pointer_type (real_type);
> +	  else
> +	    real_type = lookup_reference_type (real_type);
> +
> +	  temp = arg = value_cast (real_type, arg);
> +	}
> +    }
> +  return value_struct_elt (&temp, NULL, string, NULL, name);
> +}

You're treating looking up for type (EVAL_AVOID_SIDE_EFFECTS) 
differently from looking up for value; in the first case, you
*never* look at the dynamic type -- that doesn't seem right
to me (and in fact differs from what the original code does).
Maybe you should add some "ptype p->x" tests to exercise
this path as well.

Also, you're calling value_static_or_dynamic_member both for
class types *and* for pointer types -- but the whole dynamic
type lookup as written only works for pointer types (starting
with the if (TYPE_TARGET_TYPE)).

If you really want to treat p.x and p->x equivalent in every
case, why don't you call value_static_or_dynamic_member *always*
with the class type (using a value_ind in the STRUCTOP_PTR case)?
Then you'd use value_rtti_type instead of value_rtti_target_type.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com