[RFA] Patch for PR gdb/2477

[RFA] Patch for PR gdb/2477

[Paul Pluzhnikov]

Greetings,

Attached patch fixes error in printing NULL pointers when "set print object on".
 http://sourceware.org/cgi-bin/gnatsweb.pl?cmd=view&database=gdb&pr=2477
and adds a test case for it.

Ok to commit?

--
Paul Pluzhnikov


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

	PR gdb/2477
	* cp-abi.c (value_virtual_fn_field): Handle NULL pointers.
	
testsuite/ChangeLog
2008-07-14  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb.cp/class2.exp, gdb.cp/class2.cc: Test for PR2477.
	
Index: cp-abi.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-abi.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 cp-abi.c
--- cp-abi.c	1 Jan 2008 22:53:09 -0000	1.21
+++ cp-abi.c	14 Jul 2008 17:23:56 -0000
@@ -89,7 +89,8 @@ value_virtual_fn_field (struct value **a
 struct type *
 value_rtti_type (struct value *v, int *full, int *top, int *using_enc)
 {
-  if ((current_cp_abi.rtti_type) == NULL)
+  if ((current_cp_abi.rtti_type) == NULL
+      || VALUE_ADDRESS (v) == 0)
     return NULL;
   return (*current_cp_abi.rtti_type) (v, full, top, using_enc);
 }
Index: testsuite/gdb.cp/class2.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.cc,v
retrieving revision 1.6
diff -u -p -u -r1.6 class2.cc
--- testsuite/gdb.cp/class2.cc	1 Jan 2008 22:53:19 -0000	1.6
+++ testsuite/gdb.cp/class2.cc	14 Jul 2008 17:23:56 -0000
@@ -41,6 +41,11 @@ B::~B()
   b2 = 902;
 }
 
+struct C : public B
+{
+  A *c1;
+};
+
 // Stop the compiler from optimizing away data.
 void refer (A *)
 {
@@ -57,16 +62,19 @@ void refer (empty *)
 
 int main (void)
 {
-  A alpha, *aap, *abp;
+  A alpha, *aap, *abp, *acp;
   B beta, *bbp;
+  C gamma;
   empty e;
 
   alpha.a1 = 100;
   beta.a1 = 200; beta.b1 = 201; beta.b2 = 202;
+  gamma.c1 = 0;
 
   aap = &alpha; refer (aap);
   abp = &beta;  refer (abp);
   bbp = &beta;  refer (bbp);
+  acp = &gamma; refer (acp);
   refer (&e);
 
   return 0;  // marker return 0
Index: testsuite/gdb.cp/class2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.exp,v
retrieving revision 1.6
diff -u -p -u -r1.6 class2.exp
--- testsuite/gdb.cp/class2.exp	1 Jan 2008 22:53:19 -0000	1.6
+++ testsuite/gdb.cp/class2.exp	14 Jul 2008 17:23:56 -0000
@@ -117,3 +117,9 @@ gdb_test "print * (B *) abp" \
 # Printing the value of an object containing no data fields:
 
 gdb_test "p e" "= \{<No data fields>\}" "print object with no data fields"
+
+# Printing NULL pointers with "set print object on"
+
+gdb_test "set print object on" ""
+gdb_test "p acp" "= \\(C \\*\\) 0x\[a-f0-9\]+"
+gdb_test "p acp->c1" "\\(A \\*\\) 0x0"

Re: [RFA] Patch for PR gdb/2477

[Daniel Jacobowitz]

On Mon, Jul 14, 2008 at 10:31:49AM -0700, Paul Pluzhnikov wrote:
> Greetings,
> 
> Attached patch fixes error in printing NULL pointers when "set print object on".
>  http://sourceware.org/cgi-bin/gnatsweb.pl?cmd=view&database=gdb&pr=2477
> and adds a test case for it.
> 
> Ok to commit?

Thanks for fixing this.  The patch isn't OK, but it points to the
right solution so we can get it fixed up in no time.  The test is OK.

> @@ -89,7 +89,8 @@ value_virtual_fn_field (struct value **a
>  struct type *
>  value_rtti_type (struct value *v, int *full, int *top, int *using_enc)
>  {
> -  if ((current_cp_abi.rtti_type) == NULL)
> +  if ((current_cp_abi.rtti_type) == NULL
> +      || VALUE_ADDRESS (v) == 0)
>      return NULL;
>    return (*current_cp_abi.rtti_type) (v, full, top, using_enc);
>  }

There's nothing special about zero.  It's just an unreadable memory
address; the same problem will reappear with any other invalid
pointer.  So what we need is to catch memory errors.  There's no way
at present to separate them from other errors, but that's not a big
problem.

So if we use TRY_CATCH and RETURN_MASK_ERROR around the call to
current_cp_abi.rtti_type, we can return NULL in the error case.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Patch for PR gdb/2477

[Paul Pluzhnikov]

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

On Mon, Jul 14, 2008 at 10:42 AM, Daniel Jacobowitz <drow@false.org> wrote:

> There's nothing special about zero.  It's just an unreadable memory
> address; the same problem will reappear with any other invalid
> pointer.

There is something special about zero -- it very frequently occurs
in correct C++ programs :)

> So if we use TRY_CATCH and RETURN_MASK_ERROR around the call to
> current_cp_abi.rtti_type, we can return NULL in the error case.

Done.

I've also added a "non-zero but invalid" pointer test at ~0UL.
Is there a "canonical" invalid address I should be testing instead?

Thanks,
-- 
Paul Pluzhnikov

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

       PR gdb/2477
       * cp-abi.c (value_virtual_fn_field): Handle invalid pointers.

testsuite/ChangeLog
2008-07-14  Paul Pluzhnikov  <ppluzhnikov@google.com>

       * gdb.cp/class2.exp, gdb.cp/class2.cc: Test for PR2477.

[-- Attachment #2: gdb-patch-2477-20080714-2.txt --]
[-- Type: text/plain, Size: 2645 bytes --]

Index: gdb/cp-abi.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-abi.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 cp-abi.c
--- gdb/cp-abi.c	1 Jan 2008 22:53:09 -0000	1.21
+++ gdb/cp-abi.c	14 Jul 2008 18:01:19 -0000
@@ -22,6 +22,7 @@
 #include "value.h"
 #include "cp-abi.h"
 #include "command.h"
+#include "exceptions.h"
 #include "gdbcmd.h"
 #include "ui-out.h"
 
@@ -89,9 +90,17 @@ value_virtual_fn_field (struct value **a
 struct type *
 value_rtti_type (struct value *v, int *full, int *top, int *using_enc)
 {
+  struct type *ret = NULL;
+  struct gdb_exception e;
   if ((current_cp_abi.rtti_type) == NULL)
     return NULL;
-  return (*current_cp_abi.rtti_type) (v, full, top, using_enc);
+  TRY_CATCH (e, RETURN_MASK_ERROR)
+    {
+      ret = (*current_cp_abi.rtti_type) (v, full, top, using_enc);
+    }
+  if (e.reason < 0)
+    return NULL;
+  return ret;
 }
 
 void
Index: gdb/testsuite/gdb.cp/class2.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.cc,v
retrieving revision 1.6
diff -u -p -u -r1.6 class2.cc
--- gdb/testsuite/gdb.cp/class2.cc	1 Jan 2008 22:53:19 -0000	1.6
+++ gdb/testsuite/gdb.cp/class2.cc	14 Jul 2008 18:01:19 -0000
@@ -41,6 +41,12 @@ B::~B()
   b2 = 902;
 }
 
+struct C : public B
+{
+  A *c1;
+  A *c2;
+};
+
 // Stop the compiler from optimizing away data.
 void refer (A *)
 {
@@ -57,16 +63,19 @@ void refer (empty *)
 
 int main (void)
 {
-  A alpha, *aap, *abp;
+  A alpha, *aap, *abp, *acp;
   B beta, *bbp;
+  C gamma;
   empty e;
 
   alpha.a1 = 100;
   beta.a1 = 200; beta.b1 = 201; beta.b2 = 202;
+  gamma.c1 = 0; gamma.c2 = (A *) ~0UL;
 
   aap = &alpha; refer (aap);
   abp = &beta;  refer (abp);
   bbp = &beta;  refer (bbp);
+  acp = &gamma; refer (acp);
   refer (&e);
 
   return 0;  // marker return 0
Index: gdb/testsuite/gdb.cp/class2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.exp,v
retrieving revision 1.6
diff -u -p -u -r1.6 class2.exp
--- gdb/testsuite/gdb.cp/class2.exp	1 Jan 2008 22:53:19 -0000	1.6
+++ gdb/testsuite/gdb.cp/class2.exp	14 Jul 2008 18:01:19 -0000
@@ -117,3 +117,10 @@ gdb_test "print * (B *) abp" \
 # Printing the value of an object containing no data fields:
 
 gdb_test "p e" "= \{<No data fields>\}" "print object with no data fields"
+
+# Printing NULL pointers with "set print object on"
+
+gdb_test "set print object on" ""
+gdb_test "p acp" "= \\(C \\*\\) 0x\[a-f0-9\]+"
+gdb_test "p acp->c1" "\\(A \\*\\) 0x0"
+gdb_test "p acp->c2" "\\(A \\*\\) 0xf+"

Re: [RFA] Patch for PR gdb/2477

[Daniel Jacobowitz]

On Mon, Jul 14, 2008 at 11:11:59AM -0700, Paul Pluzhnikov wrote:
> There is something special about zero -- it very frequently occurs
> in correct C++ programs :)

Well, true :-)

> I've also added a "non-zero but invalid" pointer test at ~0UL.
> Is there a "canonical" invalid address I should be testing instead?

Nope, this is fine.

The patch is OK - if you'll update the dependencies for cp-abi.o in
Makefile.in also.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Patch for PR gdb/2477

[Paul Pluzhnikov]

On Mon, Jul 14, 2008 at 11:20 AM, Daniel Jacobowitz <drow@false.org> wrote:

> The patch is OK - if you'll update the dependencies for cp-abi.o in
> Makefile.in also.

Done and committed.

Thanks,
-- 
Paul Pluzhnikov

Re: [RFA] Patch for PR gdb/2477

[Ulrich Weigand]

Paul Pluzhnikov wrote:

> I've also added a "non-zero but invalid" pointer test at ~0UL.
> Is there a "canonical" invalid address I should be testing instead?

This test fails on spu-elf:

+  gamma.c1 = 0; gamma.c2 = (A *) ~0UL;

+gdb_test "p acp->c2" "\\(A \\*\\) 0xf+"

because our pointer-to-address conversion mimics the wrap-around
behaviour of the SPU hardware's local store access: all addresses
will wrap around modulo 256 KB.

The c2 pointer will therefore show up as 0x3ffff:

p acp->c2^M
$10 = (A *) 0x3ffff^M
(gdb) FAIL: gdb.cp/class2.exp: p acp->c2

Any suggestions how to fix this?

Bye,
Ulrich

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

Re: [RFA] Patch for PR gdb/2477

[Daniel Jacobowitz]

On Tue, Jul 15, 2008 at 08:52:14PM +0200, Ulrich Weigand wrote:
> The c2 pointer will therefore show up as 0x3ffff:
> 
> p acp->c2^M
> $10 = (A *) 0x3ffff^M
> (gdb) FAIL: gdb.cp/class2.exp: p acp->c2
> 
> Any suggestions how to fix this?

Ignoring the number entirely should preserve the intention of the test
- we're interested in the "A *" and in there being no error message.
"${hex}f"?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Patch for PR gdb/2477

[Paul Pluzhnikov]

On Tue, Jul 15, 2008 at 11:52 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:

> p acp->c2
> $10 = (A *) 0x3ffff
> (gdb) FAIL: gdb.cp/class2.exp: p acp->c2
>
> Any suggestions how to fix this?

Well, since we are just testing absence of gdb error, the test can
be relaxed:

Index: testsuite/gdb.cp/class2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.exp,v
retrieving revision 1.7
diff -u -p -u -r1.7 class2.exp
--- testsuite/gdb.cp/class2.exp	14 Jul 2008 18:28:57 -0000	1.7
+++ testsuite/gdb.cp/class2.exp	15 Jul 2008 19:01:37 -0000
@@ -123,4 +123,4 @@ gdb_test "p e" "= \{<No data fields>\}"
 gdb_test "set print object on" ""
 gdb_test "p acp" "= \\(C \\*\\) 0x\[a-f0-9\]+"
 gdb_test "p acp->c1" "\\(A \\*\\) 0x0"
-gdb_test "p acp->c2" "\\(A \\*\\) 0xf+"
+gdb_test "p acp->c2" "\\(A \\*\\) 0x\[a-f0-9\]+"


I don't know if that's the correct solution though.


-- 
Paul Pluzhnikov

Re: [RFA] Patch for PR gdb/2477

[Paul Pluzhnikov]

On Tue, Jul 15, 2008 at 12:03 PM, Daniel Jacobowitz <drow@false.org> wrote:

> Ignoring the number entirely should preserve the intention of the test
> - we're interested in the "A *" and in there being no error message.
> "${hex}f"?

I didn't know about ${hex}.
Committed as below.

Thanks,

-- 
Paul Pluzhnikov

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

	* gdb.cp/class2.exp: fix for failure on spu-elf
	

Index: testsuite/gdb.cp/class2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/class2.exp,v
retrieving revision 1.7
diff -u -p -u -r1.7 class2.exp
--- testsuite/gdb.cp/class2.exp	14 Jul 2008 18:28:57 -0000	1.7
+++ testsuite/gdb.cp/class2.exp	15 Jul 2008 19:08:07 -0000
@@ -121,6 +121,6 @@ gdb_test "p e" "= \{<No data fields>\}"
 # Printing NULL pointers with "set print object on"

 gdb_test "set print object on" ""
-gdb_test "p acp" "= \\(C \\*\\) 0x\[a-f0-9\]+"
+gdb_test "p acp" "= \\(C \\*\\) ${hex}"
 gdb_test "p acp->c1" "\\(A \\*\\) 0x0"
-gdb_test "p acp->c2" "\\(A \\*\\) 0xf+"
+gdb_test "p acp->c2" "\\(A \\*\\) ${hex}f"

Re: [RFA] Patch for PR gdb/2477

[Ulrich Weigand]

Paul Pluzhnikov wrote:
> On Tue, Jul 15, 2008 at 12:03 PM, Daniel Jacobowitz <drow@false.org> wrote:
> 
> > Ignoring the number entirely should preserve the intention of the test
> > - we're interested in the "A *" and in there being no error message.
> > "${hex}f"?
> 
> I didn't know about ${hex}.
> Committed as below.

This works for me, thanks!

Bye,
Ulrich

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