[PATCH] Fix display of structures/bitfields in register description.

[PATCH] Fix display of structures/bitfields in register description.

[Walfred Tedeschi]

Add support for displaying structures and bitfields for registers when
executing the "maint print c-tdesc". Command is also used when
converting the xml target description file into c file.

Example of the behaviour is given below reporting a snipet of the xml file
and a snippet of the c code generated.

XML file contains:
...
    <union id="vecint">
      <field name="v4" type="v4int8"/>
      <field name="v2" type="v2int16"/>
    </union>

    <struct id="struct1">
      <field name="v4" type="v4int8"/>
      <field name="v2" type="v2int16"/>
    </struct>

    <struct id="struct2" size="8">
      <field name="f1" start="0" end="34"/>
      <field name="f2" start="63" end="63"/>
    </struct>
...

Setting this xml file as target description file and
issuing the maintenance print c-tdesc the following output
is obtained:

  feature = tdesc_create_feature (result, "extra");
  field_type = tdesc_named_type (feature, "int8");
  tdesc_create_vector (feature, "v4int8", field_type, 4);

  field_type = tdesc_named_type (feature, "int16");
  tdesc_create_vector (feature, "v2int16", field_type, 2);

  type = tdesc_create_union (feature, "vecint");
  field_type = tdesc_named_type (feature, "v4int8");
  tdesc_add_field (type, "v4", field_type);
  field_type = tdesc_named_type (feature, "v2int16");
  tdesc_add_field (type, "v2", field_type);

C output is not supported type "struct1".

This is finally the issue.

2013-03-27  Walfred Tedeschi  <walfred.tedeschi@intel.com>

        * target-descriptions.c (maint_print_c_tdesc_cmd):
        Add case to parse structures as register types and
        bitfields.

testsuite/gdb.xml/

	* maint_print_struct.exp: New file.
	* maint_print_struct.xml: New file.

Signed-off-by: Walfred Tedeschi <walfred.tedeschi@intel.com>
---
 gdb/target-descriptions.c                    |   30 +++++++++++++++++++++
 gdb/testsuite/gdb.xml/maint_print_struct.exp |   37 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.xml/maint_print_struct.xml |   26 ++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 gdb/testsuite/gdb.xml/maint_print_struct.exp
 create mode 100644 gdb/testsuite/gdb.xml/maint_print_struct.xml

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 95980c5..88a08ad 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1746,6 +1746,36 @@ feature = tdesc_create_feature (result, \"%s\");\n",
 		("  tdesc_create_vector (feature, \"%s\", field_type, %d);\n",
 		 type->name, type->u.v.count);
 	      break;
+	    case TDESC_TYPE_STRUCT:
+	      printf_unfiltered
+		("  type = tdesc_create_struct (feature, \"%s\");\n",
+		 type->name);
+	      if (type->u.u.size != 0)
+		printf_unfiltered
+		  ("  tdesc_set_struct_size (type, %s);\n",
+		   plongest (type->u.u.size));
+	      for (ix3 = 0;
+		   VEC_iterate (tdesc_type_field, type->u.u.fields, ix3, f);
+		   ix3++)
+		{
+		  /* Going first for implicitly sized types, else part handles
+		     bitfields.  As reported on xml-tdesc.c implicitly sized types
+		     cannot contain a bitfield.  */
+		  if (f->start == 0 && f->end == 0)
+		    {
+		      printf_unfiltered
+			("  field_type = tdesc_named_type (feature, \"%s\");\n",
+			 f->type->name);
+		      printf_unfiltered
+			("  tdesc_add_field (type, \"%s\", field_type);\n",
+			 f->name);
+		    }
+		  else
+		    printf_unfiltered
+		      ("  tdesc_add_bitfield (type, \"%s\", %d, %d);\n",
+		       f->name, f->start, f->end);
+		}
+	      break;
 	    case TDESC_TYPE_UNION:
 	      printf_unfiltered
 		("  type = tdesc_create_union (feature, \"%s\");\n",
diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp
new file mode 100644
index 0000000..f444451
--- /dev/null
+++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp
@@ -0,0 +1,37 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
+#
+# 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 {[gdb_skip_xml_test]} {
+    unsupported "maint_print_struct.exp"
+    return -1
+}
+
+gdb_start
+global srcdir
+global subdir
+
+# Required registers are not present so it is expected a warning.
+#
+gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" "
+warning:.*" "setting a new tdesc having only a structure"
+
+gdb_test "maint print c-tdesc" "
+.*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r
+.*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r
+.*" "printing tdesc with a structure and a bitfield"
diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.xml b/gdb/testsuite/gdb.xml/maint_print_struct.xml
new file mode 100644
index 0000000..dab71fa
--- /dev/null
+++ b/gdb/testsuite/gdb.xml/maint_print_struct.xml
@@ -0,0 +1,26 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2013 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<target>
+<feature name="test">
+
+  <struct id="two_fielded">
+       <field name="field1" type="data_ptr"/>
+       <field name="field2" type="data_ptr"/>
+  </struct>
+
+  <struct id="bitfield" size="8">
+    <field name="field1" start="24" end="63"/>
+    <field name="field2" start="16" end="24"/>
+  </struct>
+
+  <reg name="bad_reg1" bitsize="128" type="two_fielded"/>
+  <reg name="bad_reg2" bitsize="64"  type="bitfield"/>
+</feature>
+</target>
+
-- 
1.7.10.4

Re: [PATCH] Fix display of structures/bitfields in register description.

[Tom Tromey]

>>>>> "Walfred" == Walfred Tedeschi <walfred.tedeschi@intel.com> writes:

Walfred> Add support for displaying structures and bitfields for
Walfred> registers when executing the "maint print c-tdesc". Command is
Walfred> also used when converting the xml target description file into
Walfred> c file.

Thanks for the patch.

I think it looks pretty good.  I have a few notes.

Walfred> +	    case TDESC_TYPE_STRUCT:
Walfred> +	      printf_unfiltered
Walfred> +		("  type = tdesc_create_struct (feature, \"%s\");\n",

This emits code to assign to the variable "type".
But is this variable defined?
Earlier I see:

	  if (type->kind == TDESC_TYPE_UNION
	      && VEC_length (tdesc_type_field, type->u.u.fields) > 0)
	    {
	      printf_unfiltered ("  struct tdesc_type *type;\n");

... which I think is the only spot where it is defined.

If this is correct then I think changing this to also check
TDESC_TYPE_STRUCT would be appropriate.

If it isn't correct, then I'm curious to know what defines 'type' in the
output.

Walfred> +global srcdir
Walfred> +global subdir

I don't think these are needed.

Walfred> +<!-- Copyright (C) 2013 Free Software Foundation, Inc.
Walfred> +
Walfred> +     Copying and distribution of this file, with or without modification,
Walfred> +     are permitted in any medium without royalty provided the copyright
Walfred> +     notice and this notice are preserved.  -->

I think this should be the usual GPL comment.

Tom

RE: [PATCH] Fix display of structures/bitfields in register description.

[Tedeschi, Walfred]

[-- Attachment #1: smime.p7m --]
[-- Type: application/pkcs7-mime, Size: 11214 bytes --]

Re: [PATCH] Fix display of structures/bitfields in register description.

[Tom Tromey]

Walfred> Copyrights:
Walfred> I took a look on several xml files and all of them had the same
Walfred> copyright I have used.  Though, would you recommend to use the
Walfred> common GPL header?

I don't know why the other .xml files have this other header.
However, seeing as the new file is in the test suite, I think the plain
GPL comment is preferable.

Tom

RE: [PATCH] Fix display of structures/bitfields in register description.

[Tedeschi, Walfred]

[-- Attachment #1: smime.p7m --]
[-- Type: application/pkcs7-mime, Size: 9461 bytes --]

Re: [PATCH] Fix display of structures/bitfields in register description.

[Daniel Jacobowitz]

They have a different header because they're supposed to be embedded
in things that run on the target which are not necessarily
GPL-compatible.  I figured out with the FSF what an appropriate notice
would be for that case.  I'd generally rather we use that notice for
all target XML files, even in the testsuite, to prevent someone
copying the GPL'd version into something by accident - not a big deal,
though.

On Thu, Apr 25, 2013 at 9:49 AM, Tom Tromey <tromey@redhat.com> wrote:
> Walfred> Copyrights:
> Walfred> I took a look on several xml files and all of them had the same
> Walfred> copyright I have used.  Though, would you recommend to use the
> Walfred> common GPL header?
>
> I don't know why the other .xml files have this other header.
> However, seeing as the new file is in the test suite, I think the plain
> GPL comment is preferable.
>
> Tom



-- 
Thanks,
Daniel

Re: [PATCH] Fix display of structures/bitfields in register description.

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> They have a different header because they're supposed to be embedded
Daniel> in things that run on the target which are not necessarily
Daniel> GPL-compatible.  I figured out with the FSF what an appropriate notice
Daniel> would be for that case.  I'd generally rather we use that notice for
Daniel> all target XML files, even in the testsuite, to prevent someone
Daniel> copying the GPL'd version into something by accident - not a big deal,
Daniel> though.

It is totally fine with me.
Thanks for explaining this.

Tom

RE: [PATCH] Fix display of structures/bitfields in register description.

[Tedeschi, Walfred]

[-- Attachment #1: smime.p7m --]
[-- Type: application/pkcs7-mime, Size: 10233 bytes --]

Re: [PATCH] Fix display of structures/bitfields in register description.

[Tom Tromey]

>>>>> "Walfred" == Tedeschi, Walfred <walfred.tedeschi@intel.com> writes:

Walfred> Thanks for this clarification.  Since it's not checked-in yet,
Walfred> I think it would make sense using the usual xml header, or?
Walfred> (Keeping all xml files homogeneous)

Yeah, I think it is fine to change it back.
Sorry about that.

Tom

[PATCH] Fix display of structures/bitfields in register description.

[Walfred Tedeschi]

Add support for displaying structures and bitfields for registers when
executing "maint print c-tdesc". This command is also used when
converting the xml target description file into c file.

Example of the behaviour is given below reporting a snipet of the xml file
and a snippet of the c code generated.

XML file contains:
...
    <union id="vecint">
      <field name="v4" type="v4int8"/>
      <field name="v2" type="v2int16"/>
    </union>

    <struct id="struct1">
      <field name="v4" type="v4int8"/>
      <field name="v2" type="v2int16"/>
    </struct>

    <struct id="struct2" size="8">
      <field name="f1" start="0" end="34"/>
      <field name="f2" start="63" end="63"/>
    </struct>
...

Setting this xml file as target description file and
issuing the maintenance print c-tdesc the following output
is obtained:

  feature = tdesc_create_feature (result, "extra");
  field_type = tdesc_named_type (feature, "int8");
  tdesc_create_vector (feature, "v4int8", field_type, 4);

  field_type = tdesc_named_type (feature, "int16");
  tdesc_create_vector (feature, "v2int16", field_type, 2);

  type = tdesc_create_union (feature, "vecint");
  field_type = tdesc_named_type (feature, "v4int8");
  tdesc_add_field (type, "v4", field_type);
  field_type = tdesc_named_type (feature, "v2int16");
  tdesc_add_field (type, "v2", field_type);

C output is not supported type "struct1".

This is finally the issue.

2013-03-27  Walfred Tedeschi  <walfred.tedeschi@intel.com>

        * target-descriptions.c (maint_print_c_tdesc_cmd):
        Add case to parse structures as register types and
        bitfields.

testsuite/

	* gdb.xml/maint_print_struct.exp: New file.
	* gdb.xml/maint_print_struct.xml: New file.

Signed-off-by: Walfred Tedeschi <walfred.tedeschi@intel.com>
---
 gdb/target-descriptions.c                    |   33 +++++++++++++++++++++-
 gdb/testsuite/gdb.xml/maint_print_struct.exp |   35 ++++++++++++++++++++++++
 gdb/testsuite/gdb.xml/maint_print_struct.xml |   38 ++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.xml/maint_print_struct.exp
 create mode 100644 gdb/testsuite/gdb.xml/maint_print_struct.xml

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 95980c5..44ad401 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1675,7 +1675,8 @@ maint_print_c_tdesc_cmd (char *args, int from_tty)
 	      printed_field_type = 1;
 	    }
 
-	  if (type->kind == TDESC_TYPE_UNION
+	  if ((type->kind == TDESC_TYPE_UNION
+	      || type->kind == TDESC_TYPE_STRUCT)
 	      && VEC_length (tdesc_type_field, type->u.u.fields) > 0)
 	    {
 	      printf_unfiltered ("  struct tdesc_type *type;\n");
@@ -1746,6 +1747,36 @@ feature = tdesc_create_feature (result, \"%s\");\n",
 		("  tdesc_create_vector (feature, \"%s\", field_type, %d);\n",
 		 type->name, type->u.v.count);
 	      break;
+	    case TDESC_TYPE_STRUCT:
+	      printf_unfiltered
+		("  type = tdesc_create_struct (feature, \"%s\");\n",
+		 type->name);
+	      if (type->u.u.size != 0)
+		printf_unfiltered
+		  ("  tdesc_set_struct_size (type, %s);\n",
+		   plongest (type->u.u.size));
+	      for (ix3 = 0;
+		   VEC_iterate (tdesc_type_field, type->u.u.fields, ix3, f);
+		   ix3++)
+		{
+		  /* Going first for implicitly sized types, else part handles
+		     bitfields.  As reported on xml-tdesc.c implicitly sized types
+		     cannot contain a bitfield.  */
+		  if (f->start == 0 && f->end == 0)
+		    {
+		      printf_unfiltered
+			("  field_type = tdesc_named_type (feature, \"%s\");\n",
+			 f->type->name);
+		      printf_unfiltered
+			("  tdesc_add_field (type, \"%s\", field_type);\n",
+			 f->name);
+		    }
+		  else
+		    printf_unfiltered
+		      ("  tdesc_add_bitfield (type, \"%s\", %d, %d);\n",
+		       f->name, f->start, f->end);
+		}
+	      break;
 	    case TDESC_TYPE_UNION:
 	      printf_unfiltered
 		("  type = tdesc_create_union (feature, \"%s\");\n",
diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp
new file mode 100644
index 0000000..568d174
--- /dev/null
+++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp
@@ -0,0 +1,35 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
+#
+# 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 {[gdb_skip_xml_test]} {
+    unsupported "maint_print_struct.exp"
+    return -1
+}
+
+gdb_start
+
+# Required registers are not present so it is expected a warning.
+#
+gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" "
+warning:.*" "setting a new tdesc having only a structure"
+
+gdb_test "maint print c-tdesc" "
+.*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r
+.*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r
+.*" "printing tdesc with a structure and a bitfield"
diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.xml b/gdb/testsuite/gdb.xml/maint_print_struct.xml
new file mode 100644
index 0000000..b385769
--- /dev/null
+++ b/gdb/testsuite/gdb.xml/maint_print_struct.xml
@@ -0,0 +1,38 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2013 Free Software Foundation, Inc.
+
+     This file is part of GDB.
+
+     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/>.
+     -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<target>
+<feature name="test">
+
+  <struct id="two_fielded">
+       <field name="field1" type="data_ptr"/>
+       <field name="field2" type="data_ptr"/>
+  </struct>
+
+  <struct id="bitfield" size="8">
+    <field name="field1" start="24" end="63"/>
+    <field name="field2" start="16" end="24"/>
+  </struct>
+
+  <reg name="bad_reg1" bitsize="128" type="two_fielded"/>
+  <reg name="bad_reg2" bitsize="64"  type="bitfield"/>
+</feature>
+</target>
+
-- 
1.7.10.4