[RFC] rl78: vtables are code addresses

[RFC] rl78: vtables are code addresses

[Kevin Buettner]

This patch, for rl78 using the default multilib, fixes 5 failures in
gdb.cp/casts.exp, 2 failures in gdb.cp/class2.exp, 115 failures in
gdb.mi/mi-var-rtti.exp, and 2 failures in gdb.python/py-value.exp.

It introduces 9 failures (regressions) in gdb.mi/mi-var-rtti.exp.

One of the regressions is:

FAIL: gdb.mi/mi-var-rtti.exp: list children of s.public in type_update_when_use_rtti

The relevant lines from the log file from a pre-patch test run are as
follows:

-var-list-children  S.public  
^done,numchild="1",children=[child={name="S.public.ptr",exp="ptr",numchild="1",type="Base *",thread-id="1"}],has_more="0"
(gdb) 
PASS: gdb.mi/mi-var-rtti.exp: list children of s.public in type_update_when_use_rtti
Expecting: \^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"
Expecting: ^(-var-list-children  S\.public\.ptr  [
]+)?(\^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"[
]+[(]gdb[)] 
[ ]*)

------

The same set of lines for the failing (post-patch) run are instead:

-var-list-children  S.public  
&"warning: can't find linker symbol for virtual table for `Base' value\n"
&"warning:   found `typeinfo for __cxxabiv1::__vmi_class_type_info' instead\n"
&"warning: can't find linker symbol for virtual table for `Base' value\n"
&"warning:   found `typeinfo for __cxxabiv1::__vmi_class_type_info' instead\n"
&"warning: can't find linker symbol for virtual table for `Base' value\n"
&"warning:   found `typeinfo for __cxxabiv1::__vmi_class_type_info' instead\n"
^done,numchild="1",children=[child={name="S.public.ptr",exp="ptr",numchild="1",type="Base *",thread-id="1"}],has_more="0"
(gdb) 
FAIL: gdb.mi/mi-var-rtti.exp: list children of s.public in type_update_when_use_rtti
Expecting: \^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"
Expecting: ^(-var-list-children  S\.public\.ptr  [
]+)?(\^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"[
]+[(]gdb[)] 
[ ]*)

------

Note that the difference between these are the warnings regarding
linker symbols.  Aside from the warnings, the result is the same. 
I.e.  gdb produces the correct answer despite the warnings.  The
reason for the other 8 failures is the same:  the test harness is not
expecting these warnings.

I spent some time (a while ago) looking at the reason for these
warnings.  As I recall, we are now getting further along in the type
resolution process than we were without my patch.  I.e.  for the
example above, the code in question never got to the point where it
was looking for the specified linker symbol.  So it seems to me that,
at worst, my patch exposes some other problem, but is not directly
the cause of the problem.

I would like to commit this patch because, on balance, it markedly
improves the test results.  Without objection(s), I'll commit this
patch in a few days time.

Kevin

	* rl78-tdep.c (rl78_pointer_to_address): Add logic so that
	vtables are considered to be code addresses.

diff --git a/gdb/rl78-tdep.c b/gdb/rl78-tdep.c
index c28db4b..d067b43 100644
--- a/gdb/rl78-tdep.c
+++ b/gdb/rl78-tdep.c
@@ -764,14 +764,27 @@ rl78_pointer_to_address (struct gdbarch *gdbarch,
                          struct type *type, const gdb_byte *buf)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  int vtable = 0;
   CORE_ADDR addr
     = extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);
 
+  /* See if it's a vtable.  */
+  if (TYPE_CODE (type) == TYPE_CODE_PTR)
+    {
+      struct type *targ_type = TYPE_TARGET_TYPE (type);
+       
+      if (TYPE_CODE (targ_type) == TYPE_CODE_STRUCT
+          && TYPE_TAG_NAME (targ_type) != NULL
+	  && strcmp (TYPE_TAG_NAME (targ_type), "gdb_gnu_v3_abi_vtable") == 0)
+	vtable = 1;
+    }
+
   /* Is it a code address?  */
   if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
       || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
       || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type))
-      || TYPE_LENGTH (type) == 4)
+      || TYPE_LENGTH (type) == 4
+      || vtable)
     return rl78_make_instruction_address (addr);
   else
     return rl78_make_data_address (addr);

Re: [RFC] rl78: vtables are code addresses

[Joel Brobecker]

> I would like to commit this patch because, on balance, it markedly
> improves the test results.  Without objection(s), I'll commit this
> patch in a few days time.

FWIW, I agree. Let's not allow best to be the enemy of good...

> +  if (TYPE_CODE (type) == TYPE_CODE_PTR)
> +    {
> +      struct type *targ_type = TYPE_TARGET_TYPE (type);
> +       
   ^^^^^^^

"trailing" spaces there...

-- 
Joel

Re: [RFC] rl78: vtables are code addresses

[Pedro Alves]

On 02/11/2014 06:44 AM, Kevin Buettner wrote:

> I would like to commit this patch because, on balance, it markedly
> improves the test results.  Without objection(s), I'll commit this
> patch in a few days time.
> 
> Kevin
> 
> 	* rl78-tdep.c (rl78_pointer_to_address): Add logic so that
> 	vtables are considered to be code addresses.

Isn't this going to be needed for all Harvard targets?
Or rather, can't we always consider this to be code for
all targets?

> 
> diff --git a/gdb/rl78-tdep.c b/gdb/rl78-tdep.c
> index c28db4b..d067b43 100644
> --- a/gdb/rl78-tdep.c
> +++ b/gdb/rl78-tdep.c
> @@ -764,14 +764,27 @@ rl78_pointer_to_address (struct gdbarch *gdbarch,
>                           struct type *type, const gdb_byte *buf)
>  {
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  int vtable = 0;
>    CORE_ADDR addr
>      = extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);
>  
> +  /* See if it's a vtable.  */
> +  if (TYPE_CODE (type) == TYPE_CODE_PTR)
> +    {
> +      struct type *targ_type = TYPE_TARGET_TYPE (type);
> +       
> +      if (TYPE_CODE (targ_type) == TYPE_CODE_STRUCT
> +          && TYPE_TAG_NAME (targ_type) != NULL
> +	  && strcmp (TYPE_TAG_NAME (targ_type), "gdb_gnu_v3_abi_vtable") == 0)

Hmm.  gdb_gnu_v3_abi_vtable is a type baked by GDB itself.

> +	vtable = 1;
> +    }
> +
>    /* Is it a code address?  */
>    if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>        || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
>        || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type))

and we already check that the target type is code here.  But
the type system didn't know that.  But, shouldn't it?

> -      || TYPE_LENGTH (type) == 4)
> +      || TYPE_LENGTH (type) == 4
> +      || vtable)
>      return rl78_make_instruction_address (addr);
>    else
>      return rl78_make_data_address (addr);
> 

Thus, wouldn't it be a little better to mark the baked in struct
type as code to begin with?

---
 gdb/gnu-v3-abi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index a08dc36..ceccbe5 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -171,7 +171,7 @@ build_gdb_vtable_type (struct gdbarch *arch)
   TYPE_TAG_NAME (t) = "gdb_gnu_v3_abi_vtable";
   INIT_CPLUS_SPECIFIC (t);

-  return t;
+  return make_type_with_address_space (t, TYPE_INSTANCE_FLAG_CODE_SPACE);
 }


I believe this makes no difference for archs with flat address space.
(testing on x86-64 GNU/Linux shows no changes).

-- 
1.7.11.7

Re: [RFC] rl78: vtables are code addresses

[Kevin Buettner]

On Tue, 11 Feb 2014 10:42:37 +0000
Pedro Alves <palves@redhat.com> wrote:

> Thus, wouldn't it be a little better to mark the baked in struct
> type as code to begin with?
> 
> ---
>  gdb/gnu-v3-abi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
> index a08dc36..ceccbe5 100644
> --- a/gdb/gnu-v3-abi.c
> +++ b/gdb/gnu-v3-abi.c
> @@ -171,7 +171,7 @@ build_gdb_vtable_type (struct gdbarch *arch)
>    TYPE_TAG_NAME (t) = "gdb_gnu_v3_abi_vtable";
>    INIT_CPLUS_SPECIFIC (t);
> 
> -  return t;
> +  return make_type_with_address_space (t, TYPE_INSTANCE_FLAG_CODE_SPACE);
>  }
> 
> 
> I believe this makes no difference for archs with flat address space.
> (testing on x86-64 GNU/Linux shows no changes).

Very nice.  I've tested your patch and find that it produces
results identical to what I was getting with my patch.  Please
check it in...

(Or, if you'd prefer that I check it in, please let me know.)

Thanks!

Kevin

Re: [RFC] rl78: vtables are code addresses

[Pedro Alves]

On 02/11/2014 04:38 PM, Kevin Buettner wrote:

> Very nice.  I've tested your patch and find that it produces
> results identical to what I was getting with my patch.  Please
> check it in...

Alright, I've pushed it now, as below.

----------
Subject: [PATCH] Explicitly mark vtables as code space

Ports for Hardvard architectures will typically have in their
pointer_to_address hook a check for TYPE_CODE_SPACE in their
"pointer_to_address" gdbarch method.  E.g., rl78's:

  /* Is it a code address?  */
  if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
      || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
      || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type))
      || TYPE_LENGTH (type) == 4)
    return rl78_make_instruction_address (addr);
  else
    return rl78_make_data_address (addr);

The avr port is similar.

The vtable type is a struct type gdb itself bakes.  Being neither a
function, nor a method, and absent explicit flagging as residing in
code space, ends up being considered data.

This patch marks the type as code when it is created, on the theory
that this is needed for all Hardvard architectures.  I believe this
should make no difference on archs with flat address space.  Testing
on x86-64 GNU/Linux shows no changes.

gdb/
2014-02-12  Pedro Alves  <palves@redhat.com>
	    Kevin Buettner <kevinb@redhat.com>

	* gnu-v3-abi.c (build_gdb_vtable_type): Return a type marked with
	TYPE_INSTANCE_FLAG_CODE_SPACE.

Kevin Buettner, at
<https://sourceware.org/ml/gdb-patches/2014-02/msg00338.html>, writes,
re. rl78:

This patch, for rl78 using the default multilib, fixes 5 failures in
gdb.cp/casts.exp, 2 failures in gdb.cp/class2.exp, 115 failures in
gdb.mi/mi-var-rtti.exp, and 2 failures in gdb.python/py-value.exp.

It introduces 9 failures (regressions) in gdb.mi/mi-var-rtti.exp.

One of the regressions is:

 FAIL: gdb.mi/mi-var-rtti.exp: list children of s.public in type_update_when_use_rtti

The relevant lines from the log file from a pre-patch test run are as
follows:

 -var-list-children  S.public
 ^done,numchild="1",children=[child={name="S.public.ptr",exp="ptr",numchild="1",type="Base *",thread-id="1"}],has_more="0"
 (gdb)
 PASS: gdb.mi/mi-var-rtti.exp: list children of s.public in type_update_when_use_rtti
 Expecting: \^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"
 Expecting: ^(-var-list-children  S\.public\.ptr  [
 ]+)?(\^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"[
 ]+[(]gdb[)]
 [ ]*)

The same set of lines for the failing (post-patch) run are instead:

 -var-list-children  S.public
 &"warning: can't find linker symbol for virtual table for `Base' value\n"
 &"warning:   found `typeinfo for __cxxabiv1::__vmi_class_type_info' instead\n"
 &"warning: can't find linker symbol for virtual table for `Base' value\n"
 &"warning:   found `typeinfo for __cxxabiv1::__vmi_class_type_info' instead\n"
 &"warning: can't find linker symbol for virtual table for `Base' value\n"
 &"warning:   found `typeinfo for __cxxabiv1::__vmi_class_type_info' instead\n"
 ^done,numchild="1",children=[child={name="S.public.ptr",exp="ptr",numchild="1",type="Base *",thread-id="1"}],has_more="0"
 (gdb)
 FAIL: gdb.mi/mi-var-rtti.exp: list children of s.public in type_update_when_use_rtti
 Expecting: \^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"
 Expecting: ^(-var-list-children  S\.public\.ptr  [
 ]+)?(\^done,numchild=".*",children=\[child={name="S.public.ptr.public",exp="public",numchild="1"(,thread-id="[0-9]+")?}.*\],has_more="0"[
 ]+[(]gdb[)]
 [ ]*)

Note that the difference between these are the warnings regarding
linker symbols.  Aside from the warnings, the result is the same.
I.e.  gdb produces the correct answer despite the warnings.  The
reason for the other 8 failures is the same: the test harness is not
expecting these warnings.

I spent some time (a while ago) looking at the reason for these
warnings.  As I recall, we are now getting further along in the type
resolution process than we were without my patch.  I.e.  for the
example above, the code in question never got to the point where it
was looking for the specified linker symbol.  So it seems to me that,
at worst, my patch exposes some other problem, but is not directly the
cause of the problem.
---
 gdb/ChangeLog    | 6 ++++++
 gdb/gnu-v3-abi.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5e9e9b8..41397b2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,10 @@
 2014-02-12  Pedro Alves  <palves@redhat.com>
+	    Kevin Buettner <kevinb@redhat.com>
+
+	* gnu-v3-abi.c (build_gdb_vtable_type): Return a type marked with
+	TYPE_INSTANCE_FLAG_CODE_SPACE.
+
+2014-02-12  Pedro Alves  <palves@redhat.com>
 
 	* h8300-tdep.c (pseudo_from_raw_register)
 	(raw_from_pseudo_register): New functions.
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index a08dc36..ceccbe5 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -171,7 +171,7 @@ build_gdb_vtable_type (struct gdbarch *arch)
   TYPE_TAG_NAME (t) = "gdb_gnu_v3_abi_vtable";
   INIT_CPLUS_SPECIFIC (t);
 
-  return t;
+  return make_type_with_address_space (t, TYPE_INSTANCE_FLAG_CODE_SPACE);
 }
 
 
-- 
1.7.11.7

Re: [RFC] rl78: vtables are code addresses

[Kevin Buettner]

On Wed, 12 Feb 2014 13:32:24 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 02/11/2014 04:38 PM, Kevin Buettner wrote:
> 
> > Very nice.  I've tested your patch and find that it produces
> > results identical to what I was getting with my patch.  Please
> > check it in...
> 
> Alright, I've pushed it now, as below.

Thanks!

Kevin