[PATCH] arc: Pass proper CPU value to disassembler

[PATCH] arc: Pass proper CPU value to disassembler

[Anton Kolesov]

There was a problem with generation of disassembler options for ARC in GDB,
because a BFD architecture name was used as a CPU name, but they have different
meaning even if some architectures have same name as respective CPUs.
Target description specifies a BFD architecture, which is different from ARC
CPU, as accepted by disassembler (and most other ARC tools), because CPU values
are much more fine grained - there can be multiple CPU values per single BFD
architecture.  As a result this code should translate architecture to some CPU
value.  Since there is no info on exact CPU configuration, it is best to use
the most feature-rich CPU, so that disassembler will recognize all instructions
available to the specified architecture.

gdb/ChangeLog
yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-tdep.c (arc_gdbarch_init): Pass proper cpu value to disassembler.
	* arc-tdep.h (arc_arch_is_em): New function.
	(arc_arch_is_hs): Likewise.

gdb/testsuite/ChangeLog
yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-tdesc-cpu.exp: New file.
	* arc-tdesc-cpu.xml: Likewise.
---
 gdb/arc-tdep.c                           | 34 ++++++++++++++++++--
 gdb/arc-tdep.h                           | 15 +++++++++
 gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp | 47 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml | 53 ++++++++++++++++++++++++++++++++
 4 files changed, 147 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp
 create mode 100644 gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml

diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 1d05c5a..d238ab7 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -2085,8 +2085,38 @@ arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	     existing gdbarches, which also can be problematic, if
 	     arc_gdbarch_init will start reusing existing gdbarch
 	     instances.  */
-	  arc_disassembler_options = xstrprintf ("cpu=%s",
-						 tdesc_arch->printable_name);
+	  /* Target description specifies a BFD architecture, which is
+	     different from ARC cpu, as accepted by disassembler (and most
+	     other ARC tools), because cpu values are much more fine grained -
+	     there can be multiple cpu values per single BFD architecture.  As
+	     a result this code should translate architecture to some cpu
+	     value.  Since there is no info on exact cpu configuration, it is
+	     best to use the most feature-rich CPU, so that disassembler will
+	     recognize all instructions available to the specified
+	     architecture.  */
+	  switch (tdesc_arch->mach)
+	    {
+	    case bfd_mach_arc_arc601:
+	      arc_disassembler_options = xstrdup ("cpu=arc601");
+	      break;
+	    case bfd_mach_arc_arc600:
+	      arc_disassembler_options = xstrdup ("cpu=arc600");
+	      break;
+	    case bfd_mach_arc_arc700:
+	      arc_disassembler_options = xstrdup ("cpu=arc700");
+	      break;
+	    case bfd_mach_arc_arcv2:
+	      /* Machine arcv2 has three arches: ARCv2, EM and HS; where ARCv2
+	         is treated as EM.  */
+	      if (arc_arch_is_hs (tdesc_arch))
+		arc_disassembler_options = xstrdup ("cpu=hs38_linux");
+	      else
+		arc_disassembler_options = xstrdup ("cpu=em4_fpuda");
+	      break;
+	    default:
+	      arc_disassembler_options = NULL;
+	      break;
+	    }
 	  set_gdbarch_disassembler_options (gdbarch,
 					    &arc_disassembler_options);
 	}
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index 1bf1817..07c76b6 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -123,6 +123,21 @@ arc_mach_is_arcv2 (struct gdbarch *gdbarch)
   return gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arcv2;
 }
 
+/* ARC EM and ARC HS are unique BFD arches, however they share the same machine
+   number as "ARCv2".  */
+
+static inline bool
+arc_arch_is_hs (const struct bfd_arch_info* arch)
+{
+  return CONST_STRNEQ (arch->printable_name, "HS");
+}
+
+static inline bool
+arc_arch_is_em (const struct bfd_arch_info* arch)
+{
+  return CONST_STRNEQ (arch->printable_name, "EM");
+}
+
 /* Function to access ARC disassembler.  Underlying opcodes disassembler will
    print an instruction into stream specified in the INFO, so if it is
    undesired, then this stream should be set to some invisible stream, but it
diff --git a/gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp
new file mode 100644
index 0000000..5246427
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp
@@ -0,0 +1,47 @@
+# Copyright 2017 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 {[gdb_skip_xml_test]} {
+    unsupported "arc-tdesc-cpu.exp"
+    return -1
+}
+
+gdb_start
+
+# Test whether it ok to have `arc:HS` in target description architecture.
+# `HS` is a valid BFD architecture name, however disassembler doesn't accept
+# it as a CPU name.  This test checks that GDB doesn't pass architecture from
+# target description directly to disassembler and instead uses one of the
+# valid CPU names.
+
+set filename $srcdir/$subdir/arc-tdesc-cpu.xml
+
+set cmd "set tdesc filename $filename"
+gdb_test $cmd
+
+# Error message is emitted by disassembler, therefore it is not shown unless
+# disassembler is actually invoked.  Address "0" is not invalid, but that
+# doesn't matter for this test case, because it is only disassembler error
+# message that is interesting.
+set cmd "x /i 0"
+set msg "setting HS architecture"
+gdb_test_multiple $cmd $msg {
+    -re "Unrecognised disassembler CPU option: HS" {
+	fail $msg
+    }
+    -re "\r\n$gdb_prompt" {
+	pass $msg
+    }
+}
diff --git a/gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml
new file mode 100644
index 0000000..fe158ae
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml
@@ -0,0 +1,53 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2017 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 target SYSTEM "gdb-target.dtd">
+<target>
+  <architecture>arc:HS</architecture>
+
+  <feature name="org.gnu.gdb.arc.core.v2">
+    <reg name="r0"  bitsize="32"/>
+    <reg name="r1"  bitsize="32"/>
+    <reg name="r2"  bitsize="32"/>
+    <reg name="r3"  bitsize="32"/>
+    <reg name="r4"  bitsize="32"/>
+    <reg name="r5"  bitsize="32"/>
+    <reg name="r6"  bitsize="32"/>
+    <reg name="r7"  bitsize="32"/>
+    <reg name="r8"  bitsize="32"/>
+    <reg name="r9"  bitsize="32"/>
+    <reg name="r10" bitsize="32"/>
+    <reg name="r11" bitsize="32"/>
+    <reg name="r12" bitsize="32"/>
+    <reg name="r13" bitsize="32"/>
+    <reg name="r14" bitsize="32"/>
+    <reg name="r15" bitsize="32"/>
+    <reg name="r16" bitsize="32"/>
+    <reg name="r17" bitsize="32"/>
+    <reg name="r18" bitsize="32"/>
+    <reg name="r19" bitsize="32"/>
+    <reg name="r20" bitsize="32"/>
+    <reg name="r21" bitsize="32"/>
+    <reg name="r22" bitsize="32"/>
+    <reg name="r23" bitsize="32"/>
+    <reg name="r24" bitsize="32"/>
+    <reg name="r25" bitsize="32"/>
+    <reg name="gp"  bitsize="32"/>
+    <reg name="fp"  bitsize="32"/>
+    <reg name="sp"  bitsize="32"/>
+    <reg name="ilink" bitsize="32"/>
+    <reg name="r30"   bitsize="32"/>
+    <reg name="blink" bitsize="32"/>
+    <reg name="lp_count" bitsize="32"/>
+    <reg name="pcl" bitsize="32"/>
+  </feature>
+
+  <feature name="org.gnu.gdb.arc.aux-minimal">
+    <reg name="pc"       bitsize="32"/>
+    <reg name="status32" bitsize="32"/>
+  </feature>
+</target>
-- 
2.8.3

Re: [PATCH] arc: Pass proper CPU value to disassembler

[Pedro Alves]

On 10/10/2017 08:22 PM, Anton Kolesov wrote:

> +/* ARC EM and ARC HS are unique BFD arches, however they share the same machine
> +   number as "ARCv2".  */
> +
> +static inline bool
> +arc_arch_is_hs (const struct bfd_arch_info* arch)
> +{
> +  return CONST_STRNEQ (arch->printable_name, "HS");
> +}
> +
> +static inline bool
> +arc_arch_is_em (const struct bfd_arch_info* arch)
> +{
> +  return CONST_STRNEQ (arch->printable_name, "EM");
> +}

I'd prefer if you used startswith instead.  There's not much
point in using CONST_STRNEQ nowadays, and we don't tend to
use it in GDB -- compilers have no trouble constant folding
the length of string literals.

> +gdb_start
> +
> +# Test whether it ok to have `arc:HS` in target description architecture.

"it's OK"  ... "in the target"

> +# `HS` is a valid BFD architecture name, however disassembler doesn't accept

"the disassembler"

> +# it as a CPU name.  This test checks that GDB doesn't pass architecture from
> +# target description directly to disassembler and instead uses one of the

"the target description" ... "the disassembler"

> +# valid CPU names.
> +
> +set filename $srcdir/$subdir/arc-tdesc-cpu.xml
> +
> +set cmd "set tdesc filename $filename"
> +gdb_test $cmd
> +
> +# Error message is emitted by disassembler, therefore it is not shown unless

"An error" ... "the disassembler" (x2).

> +# disassembler is actually invoked.  Address "0" is not invalid, but that
> +# doesn't matter for this test case, because it is only disassembler error
> +# message that is interesting.

"the disassembler"

> +set cmd "x /i 0"
> +set msg "setting HS architecture"
> +gdb_test_multiple $cmd $msg {
> +    -re "Unrecognised disassembler CPU option: HS" {

Must match $gdb_prompt too, otherwise the prompt is left
in the expect buffer and confuses
following gdb_test/gdb_test_multiple calls.

> +	fail $msg
> +    }
> +    -re "\r\n$gdb_prompt" {

This seems a bit fragile.  If the error output ever changes,
then this will match, and thus will always pass.  Can this
regex be tightened a bit to include something more than
just the prompt?

> +	pass $msg
> +    }
> +}

Thanks,
Pedro Alves

[PATCH v2] arc: Pass proper CPU value to the disassembler

[Anton Kolesov]

Changes in v2:

* Add missing articles in the comments;
* Use startswith () instead of CONST_STRNEQ ();
* Stricter pattern matching in test case.

---

There was a problem with generation of the disassembler options for ARC in GDB,
because a BFD architecture name was used as a CPU name, but they have different
meaning even if some architectures have same name as respective CPUs.  Target
description specifies a BFD architecture, which is different from ARC CPU, as
accepted by the disassembler (and most other ARC tools), because CPU values are
much more fine grained - there can be multiple CPU values per single BFD
architecture.  As a result this code should translate architecture to some CPU
value.  Since there is no info on exact CPU configuration, it is best to use
the most feature-rich CPU, so that the disassembler will recognize all
instructions available to the specified architecture.

gdb/ChangeLog
yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-tdep.c (arc_gdbarch_init): Pass proper cpu value to disassembler.
	* arc-tdep.h (arc_arch_is_em): New function.
	(arc_arch_is_hs): Likewise.

gdb/testsuite/ChangeLog
yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-tdesc-cpu.exp: New file.
	* arc-tdesc-cpu.xml: Likewise.
---
 gdb/arc-tdep.c                           | 34 ++++++++++++++++++--
 gdb/arc-tdep.h                           | 15 +++++++++
 gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp | 48 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml | 53 ++++++++++++++++++++++++++++++++
 4 files changed, 148 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp
 create mode 100644 gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml

diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 1d05c5a..d238ab7 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -2085,8 +2085,38 @@ arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	     existing gdbarches, which also can be problematic, if
 	     arc_gdbarch_init will start reusing existing gdbarch
 	     instances.  */
-	  arc_disassembler_options = xstrprintf ("cpu=%s",
-						 tdesc_arch->printable_name);
+	  /* Target description specifies a BFD architecture, which is
+	     different from ARC cpu, as accepted by disassembler (and most
+	     other ARC tools), because cpu values are much more fine grained -
+	     there can be multiple cpu values per single BFD architecture.  As
+	     a result this code should translate architecture to some cpu
+	     value.  Since there is no info on exact cpu configuration, it is
+	     best to use the most feature-rich CPU, so that disassembler will
+	     recognize all instructions available to the specified
+	     architecture.  */
+	  switch (tdesc_arch->mach)
+	    {
+	    case bfd_mach_arc_arc601:
+	      arc_disassembler_options = xstrdup ("cpu=arc601");
+	      break;
+	    case bfd_mach_arc_arc600:
+	      arc_disassembler_options = xstrdup ("cpu=arc600");
+	      break;
+	    case bfd_mach_arc_arc700:
+	      arc_disassembler_options = xstrdup ("cpu=arc700");
+	      break;
+	    case bfd_mach_arc_arcv2:
+	      /* Machine arcv2 has three arches: ARCv2, EM and HS; where ARCv2
+	         is treated as EM.  */
+	      if (arc_arch_is_hs (tdesc_arch))
+		arc_disassembler_options = xstrdup ("cpu=hs38_linux");
+	      else
+		arc_disassembler_options = xstrdup ("cpu=em4_fpuda");
+	      break;
+	    default:
+	      arc_disassembler_options = NULL;
+	      break;
+	    }
 	  set_gdbarch_disassembler_options (gdbarch,
 					    &arc_disassembler_options);
 	}
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index 1bf1817..580ccb7 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -123,6 +123,21 @@ arc_mach_is_arcv2 (struct gdbarch *gdbarch)
   return gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arcv2;
 }
 
+/* ARC EM and ARC HS are unique BFD arches, however they share the same machine
+   number as "ARCv2".  */
+
+static inline bool
+arc_arch_is_hs (const struct bfd_arch_info* arch)
+{
+  return startswith (arch->printable_name, "HS");
+}
+
+static inline bool
+arc_arch_is_em (const struct bfd_arch_info* arch)
+{
+  return startswith (arch->printable_name, "EM");
+}
+
 /* Function to access ARC disassembler.  Underlying opcodes disassembler will
    print an instruction into stream specified in the INFO, so if it is
    undesired, then this stream should be set to some invisible stream, but it
diff --git a/gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp
new file mode 100644
index 0000000..f1c009d
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.exp
@@ -0,0 +1,48 @@
+# Copyright 2017 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 {[gdb_skip_xml_test]} {
+    unsupported "arc-tdesc-cpu.exp"
+    return -1
+}
+
+gdb_start
+
+# Test whether it is OK to have `arc:HS` in the target description
+# architecture.  `HS` is a valid BFD architecture name, however the
+# disassembler doesn't accept it as a CPU name.  This test checks that GDB
+# doesn't pass architecture from the target description directly to the
+# disassembler and instead uses one of the valid CPU names.
+
+set filename $srcdir/$subdir/arc-tdesc-cpu.xml
+
+set cmd "set tdesc filename $filename"
+gdb_test $cmd
+
+# An error message is emitted by the disassembler, therefore it is not shown
+# unless the disassembler is actually invoked.  Address "0" is not invalid,
+# but that doesn't matter for this test case, because it is only the
+# disassembler error message that is interesting.
+set cmd "x /i 0"
+set msg "setting HS architecture"
+gdb_test_multiple $cmd $msg {
+    -re "Unrecognised disassembler CPU option: HS.*$gdb_prompt" {
+	fail $msg
+    }
+    -re "^$cmd\r\n\\s*$hex:\\s+Cannot access memory at address $hex\r\n$gdb_prompt"
+    {
+	pass $msg
+    }
+}
diff --git a/gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml
new file mode 100644
index 0000000..fe158ae
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arc-tdesc-cpu.xml
@@ -0,0 +1,53 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2017 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 target SYSTEM "gdb-target.dtd">
+<target>
+  <architecture>arc:HS</architecture>
+
+  <feature name="org.gnu.gdb.arc.core.v2">
+    <reg name="r0"  bitsize="32"/>
+    <reg name="r1"  bitsize="32"/>
+    <reg name="r2"  bitsize="32"/>
+    <reg name="r3"  bitsize="32"/>
+    <reg name="r4"  bitsize="32"/>
+    <reg name="r5"  bitsize="32"/>
+    <reg name="r6"  bitsize="32"/>
+    <reg name="r7"  bitsize="32"/>
+    <reg name="r8"  bitsize="32"/>
+    <reg name="r9"  bitsize="32"/>
+    <reg name="r10" bitsize="32"/>
+    <reg name="r11" bitsize="32"/>
+    <reg name="r12" bitsize="32"/>
+    <reg name="r13" bitsize="32"/>
+    <reg name="r14" bitsize="32"/>
+    <reg name="r15" bitsize="32"/>
+    <reg name="r16" bitsize="32"/>
+    <reg name="r17" bitsize="32"/>
+    <reg name="r18" bitsize="32"/>
+    <reg name="r19" bitsize="32"/>
+    <reg name="r20" bitsize="32"/>
+    <reg name="r21" bitsize="32"/>
+    <reg name="r22" bitsize="32"/>
+    <reg name="r23" bitsize="32"/>
+    <reg name="r24" bitsize="32"/>
+    <reg name="r25" bitsize="32"/>
+    <reg name="gp"  bitsize="32"/>
+    <reg name="fp"  bitsize="32"/>
+    <reg name="sp"  bitsize="32"/>
+    <reg name="ilink" bitsize="32"/>
+    <reg name="r30"   bitsize="32"/>
+    <reg name="blink" bitsize="32"/>
+    <reg name="lp_count" bitsize="32"/>
+    <reg name="pcl" bitsize="32"/>
+  </feature>
+
+  <feature name="org.gnu.gdb.arc.aux-minimal">
+    <reg name="pc"       bitsize="32"/>
+    <reg name="status32" bitsize="32"/>
+  </feature>
+</target>
-- 
2.8.3

Re: [PATCH v2] arc: Pass proper CPU value to the disassembler

[Pedro Alves]

On 10/11/2017 12:55 PM, Anton Kolesov wrote:
> Changes in v2:
> 
> * Add missing articles in the comments;
> * Use startswith () instead of CONST_STRNEQ ();
> * Stricter pattern matching in test case.

OK.

Thanks,
Pedro Alves