[v4,2/3] arc: Add hardware loop detection

Message ID 20200723193532.25812-3-shahab.vahedi@gmail.com
State New
Headers show
Series
  • arc: Add GNU/Linux support
Related show

Commit Message

Kevin Buettner via Gdb-patches July 23, 2020, 7:35 p.m.
From: Shahab Vahedi <shahab@synopsys.com>


For ARC there are registers that are not part of a required set in XML
target descriptions by default, but are almost always present on ARC
targets and are universally exposed by the ptrace interface.  Hardware
loop registers being one of them.

LP_START and LP_END auxiliary registers are hardware loop start and end.
Formally, they are optional, but it is hard to find an ARC configuration
that doesn't have them.  They are always present in processors that can
run GNU/Linux.  GDB needs to know about those registers to implement
proper software single stepping, since they affect  what instruction
will be next.

This commit adds the code to check for the existance of "lp_start" and
"lp_end" in XML target descriptions. If they exist, then the function
reports that the target supports hardware loops.

gdb/ChangeLog:
2020-07-22  Shahab Vahedi  <shahab@synopsys.com>

	* arc-tdep.c (arc_check_for_hardware_loop): New.
	* arc-tdep.h (gdbarch_tdep): New field has_hw_loops.

gdb/doc/ChangeLog:
2020-07-22  Anton Kolesov  <anton.kolesov@synopsys.com>

	* gdb.texinfo (Synopsys ARC): Document LP_START, LP_END and BTA.
---
 gdb/arc-tdep.c      | 39 +++++++++++++++++++++++++++++++++++----
 gdb/arc-tdep.h      |  4 ++++
 gdb/doc/gdb.texinfo |  3 ++-
 3 files changed, 41 insertions(+), 5 deletions(-)

-- 
2.27.0

Comments

Simon Marchi Aug. 1, 2020, 2:53 p.m. | #1
This patch is also OK with these nits fixed.

On 2020-07-23 3:35 p.m., Shahab Vahedi wrote:
> diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c

> index 0c7e045e54b..59e698bb18b 100644

> --- a/gdb/arc-tdep.c

> +++ b/gdb/arc-tdep.c

> @@ -2006,6 +2006,35 @@ arc_check_tdesc_feature (struct tdesc_arch_data *tdesc_data,

>    return true;

>  }

>  

> +/* Check for the existance of "lp_start" and "lp_end" in target description.

> +   If both are present, assume there is hardware loop support in the target.

> +   This can be improved by looking into "lpc_size" field of "isa_config"

> +   auxiliary register.  */

> +

> +static bool

> +arc_check_for_hw_loops (const struct target_desc *tdesc,

> +			struct tdesc_arch_data *data)

> +{

> +  const auto feature_aux = tdesc_find_feature (tdesc, ARC_AUX_FEATURE_NAME);

> +  const auto *aux_regset = determine_aux_reg_feature_set ();


To be consistent, I'd suggest using the star in both places or not use it in both places.

> +

> +  if (feature_aux == nullptr || aux_regset == nullptr)

> +    return false;


If a function (I'm thinking of determine_aux_reg_feature_set) can't (according to its contract)
return nullptr, don't check for nullptr, that's:

(1) unnecessary
(2) confusing, since by reading this code my mind goes searching for a reason why
    determine_aux_reg_feature_set might return nullptr

I suppose that tdesc_find_feature can return nullptr, if the feature is not present.

> diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h

> index 6ca759a661d..e752348a262 100644

> --- a/gdb/arc-tdep.h

> +++ b/gdb/arc-tdep.h

> @@ -23,6 +23,7 @@

>  

>  /* Need disassemble_info.  */

>  #include "dis-asm.h"

> +#include "gdbarch.h"


I suppose that adding this is correct, as this file uses some things from gdbarch.h.

It's a change unrelated to the topic of this patch, so it's preferable to send it as
a separate patch, with its own explanation.

Simon

Patch

diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 0c7e045e54b..59e698bb18b 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -2006,6 +2006,35 @@  arc_check_tdesc_feature (struct tdesc_arch_data *tdesc_data,
   return true;
 }
 
+/* Check for the existance of "lp_start" and "lp_end" in target description.
+   If both are present, assume there is hardware loop support in the target.
+   This can be improved by looking into "lpc_size" field of "isa_config"
+   auxiliary register.  */
+
+static bool
+arc_check_for_hw_loops (const struct target_desc *tdesc,
+			struct tdesc_arch_data *data)
+{
+  const auto feature_aux = tdesc_find_feature (tdesc, ARC_AUX_FEATURE_NAME);
+  const auto *aux_regset = determine_aux_reg_feature_set ();
+
+  if (feature_aux == nullptr || aux_regset == nullptr)
+    return false;
+
+  bool hw_loop_p = false;
+  const auto lp_start_name =
+    aux_regset->registers[ARC_LP_START_REGNUM - ARC_FIRST_AUX_REGNUM].names[0];
+  const auto lp_end_name =
+    aux_regset->registers[ARC_LP_END_REGNUM - ARC_FIRST_AUX_REGNUM].names[0];
+
+  hw_loop_p = tdesc_numbered_register (feature_aux, data,
+				       ARC_LP_START_REGNUM, lp_start_name);
+  hw_loop_p &= tdesc_numbered_register (feature_aux, data,
+				       ARC_LP_END_REGNUM, lp_end_name);
+
+  return hw_loop_p;
+}
+
 /* Initialize target description for the ARC.
 
    Returns true if input TDESC was valid and in this case it will assign TDESC
@@ -2125,13 +2154,15 @@  arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
     debug_printf ("arc: Architecture initialization.\n");
 
   if (!arc_tdesc_init (info, &tdesc, &tdesc_data))
-    return NULL;
+    return nullptr;
 
   /* Allocate the ARC-private target-dependent information structure, and the
      GDB target-independent information structure.  */
-  struct gdbarch_tdep *tdep = XCNEW (struct gdbarch_tdep);
+  gdb::unique_xmalloc_ptr<struct gdbarch_tdep> tdep
+    (XCNEW (struct gdbarch_tdep));
   tdep->jb_pc = -1; /* No longjmp support by default.  */
-  struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep);
+  tdep->has_hw_loops = arc_check_for_hw_loops (tdesc, tdesc_data);
+  struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep.release ());
 
   /* Data types.  */
   set_gdbarch_short_bit (gdbarch, 16);
@@ -2212,7 +2243,7 @@  arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
      It can override functions set earlier.  */
   gdbarch_init_osabi (info, gdbarch);
 
-  if (tdep->jb_pc >= 0)
+  if (gdbarch_tdep (gdbarch)->jb_pc >= 0)
     set_gdbarch_get_longjmp_target (gdbarch, arc_get_longjmp_target);
 
   /* Disassembler options.  Enforce CPU if it was specified in XML target
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index 6ca759a661d..e752348a262 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -23,6 +23,7 @@ 
 
 /* Need disassemble_info.  */
 #include "dis-asm.h"
+#include "gdbarch.h"
 #include "arch/arc.h"
 
 /* To simplify GDB code this enum assumes that internal regnums should be same
@@ -110,6 +111,9 @@  struct gdbarch_tdep
   /* Offset to PC value in jump buffer.  If this is negative, longjmp
      support will be disabled.  */
   int jb_pc;
+
+  /* Whether target has hardware (aka zero-delay) loops.  */
+  bool has_hw_loops;
 };
 
 /* Utility functions used by other ARC-specific modules.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a002084d5b9..9502992b9af 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -45238,7 +45238,8 @@  difference with @samp{org.gnu.gdb.arc.core.v2} feature is in the names of
 ARC v2, but @samp{ilink2} is optional on ARCompact.
 
 The @samp{org.gnu.gdb.arc.aux-minimal} feature is required for all ARC
-targets.  It should contain registers @samp{pc} and @samp{status32}.
+targets.  It should contain registers @samp{pc} and @samp{status32}.  It may
+contain registers @samp{lp_start}, @samp{lp_end} and @samp{bta}.
 
 @node ARM Features
 @subsection ARM Features