[PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Antoine Tremblay]

This patch series enables GDBServer to trace an ARM target on linux.

Patches 1-3: Fixes collection failures in certain cases.

Patch 4: Enables tracepoints on ARM and introduces the new TracepointKinds
feature and 'K' parameter to the QTDP packet.

[PATCH 1/4] Teach arm unwinders to terminate gracefully

[Antoine Tremblay]

When examining a trace buffer we have the following issue:

~~~
tfind start
Register 13 is not available
Found trace frame 0, tracepoint 2
#-1 0x40123556 in pendfunc2
^^^
~~~

The reason for this is that the target's stack pointer is unavailable
when examining the trace buffer.  What we are seeing is due to the
'tfind' command creating a sentinel frame and unwinding it.  If an
exception is thrown, we are left with the sentinel frame being displayed
at level #-1.  The exception is thrown when the prologue unwinder tries
to read the stack pointer to construct an ID for the frame.

This patch fixes this and similar issues by making all the arm unwinders
catch NOT_AVAILABLE_ERROR exceptions when either register or memory is
unreadable and report back to the frame core code with UNWIND_UNAVAILABLE.

Note this commit log adapted from 7dfa3edc033c443036d9f2a3e01120f7fb54f498
which fixed a similar issue for aarch64.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (struct arm_prologue_cache) <available_p>: New field.
	(arm_make_prologue_cache): Swallow NOT_AVAIABLE_ERROR or set
	available_p.
	(arm_prologue_unwind_stop_reason): Return UNWIND_UNAVAILABLE if
	available_p is not set.
	(arm_prologue_this_id): Call frame_id_build_unavailable_stack if
	available_p is not set.
	(arm_make_stub_cache): Swallow NOT_AVAIABLE_ERROR or set
	available_p.
	(arm_stub_this_id): Call frame_id_build_unavailable_stack if
	available_p is not set.
	(arm_m_exception_cache): Swallow NOT_AVAIABLE_ERROR or set
	available_p.
	(arm_m_exception_this_id): Call frame_id_build_unavailable_stack if
	available_p is not set.
---
 gdb/arm-tdep.c | 142 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 95 insertions(+), 47 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 05d60bb..5ee7fb0 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -252,6 +252,9 @@ struct arm_prologue_cache
      to identify this frame.  */
   CORE_ADDR prev_sp;
 
+  /* Is the target available to read from ?  */
+  int available_p;
+
   /* The frame base for this frame is just prev_sp - frame size.
      FRAMESIZE is the distance from the frame pointer to the
      initial stack pointer.  */
@@ -1793,19 +1796,29 @@ arm_make_prologue_cache (struct frame_info *this_frame)
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
 
-  arm_scan_prologue (this_frame, cache);
+  TRY
+    {
+      arm_scan_prologue (this_frame, cache);
+      unwound_fp = get_frame_register_unsigned (this_frame, cache->framereg);
+      if (unwound_fp == 0)
+	return cache;
 
-  unwound_fp = get_frame_register_unsigned (this_frame, cache->framereg);
-  if (unwound_fp == 0)
-    return cache;
+      cache->prev_sp = unwound_fp + cache->framesize;
 
-  cache->prev_sp = unwound_fp + cache->framesize;
+      /* Calculate actual addresses of saved registers using offsets
+	 determined by arm_scan_prologue.  */
+      for (reg = 0; reg < gdbarch_num_regs (get_frame_arch (this_frame)); reg++)
+	if (trad_frame_addr_p (cache->saved_regs, reg))
+	  cache->saved_regs[reg].addr += cache->prev_sp;
 
-  /* Calculate actual addresses of saved registers using offsets
-     determined by arm_scan_prologue.  */
-  for (reg = 0; reg < gdbarch_num_regs (get_frame_arch (this_frame)); reg++)
-    if (trad_frame_addr_p (cache->saved_regs, reg))
-      cache->saved_regs[reg].addr += cache->prev_sp;
+      cache->available_p = 1;
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
 
   return cache;
 }
@@ -1823,6 +1836,9 @@ arm_prologue_unwind_stop_reason (struct frame_info *this_frame,
     *this_cache = arm_make_prologue_cache (this_frame);
   cache = (struct arm_prologue_cache *) *this_cache;
 
+  if (!cache->available_p)
+    return UNWIND_UNAVAILABLE;
+
   /* This is meant to halt the backtrace at "_start".  */
   pc = get_frame_pc (this_frame);
   if (pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
@@ -1851,16 +1867,23 @@ arm_prologue_this_id (struct frame_info *this_frame,
     *this_cache = arm_make_prologue_cache (this_frame);
   cache = (struct arm_prologue_cache *) *this_cache;
 
-  /* Use function start address as part of the frame ID.  If we cannot
-     identify the start address (due to missing symbol information),
-     fall back to just using the current PC.  */
-  pc = get_frame_pc (this_frame);
-  func = get_frame_func (this_frame);
-  if (!func)
-    func = pc;
+  if (!cache->available_p)
+    {
+      *this_id = frame_id_build_unavailable_stack (cache->prev_sp);
+    }
+  else
+    {
+      /* Use function start address as part of the frame ID.  If we cannot
+	 identify the start address (due to missing symbol information),
+	 fall back to just using the current PC.  */
+      pc = get_frame_pc (this_frame);
+      func = get_frame_func (this_frame);
+      if (!func)
+	func = pc;
 
-  id = frame_id_build (cache->prev_sp, func);
-  *this_id = id;
+      id = frame_id_build (cache->prev_sp, func);
+      *this_id = id;
+    }
 }
 
 static struct value *
@@ -2738,7 +2761,17 @@ arm_make_stub_cache (struct frame_info *this_frame)
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
 
-  cache->prev_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+  TRY
+    {
+      cache->prev_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+      cache->available_p = 1;
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
 
   return cache;
 }
@@ -2756,7 +2789,10 @@ arm_stub_this_id (struct frame_info *this_frame,
     *this_cache = arm_make_stub_cache (this_frame);
   cache = (struct arm_prologue_cache *) *this_cache;
 
-  *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
+  if (!cache->available_p)
+    *this_id = frame_id_build_unavailable_stack (cache->prev_sp);
+  else
+    *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
 }
 
 static int
@@ -2809,29 +2845,38 @@ arm_m_exception_cache (struct frame_info *this_frame)
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
 
-  unwound_sp = get_frame_register_unsigned (this_frame,
-					    ARM_SP_REGNUM);
-
-  /* The hardware saves eight 32-bit words, comprising xPSR,
-     ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
-     "B1.5.6 Exception entry behavior" in
-     "ARMv7-M Architecture Reference Manual".  */
-  cache->saved_regs[0].addr = unwound_sp;
-  cache->saved_regs[1].addr = unwound_sp + 4;
-  cache->saved_regs[2].addr = unwound_sp + 8;
-  cache->saved_regs[3].addr = unwound_sp + 12;
-  cache->saved_regs[12].addr = unwound_sp + 16;
-  cache->saved_regs[14].addr = unwound_sp + 20;
-  cache->saved_regs[15].addr = unwound_sp + 24;
-  cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
-
-  /* If bit 9 of the saved xPSR is set, then there is a four-byte
-     aligner between the top of the 32-byte stack frame and the
-     previous context's stack pointer.  */
-  cache->prev_sp = unwound_sp + 32;
-  if (safe_read_memory_integer (unwound_sp + 28, 4, byte_order, &xpsr)
-      && (xpsr & (1 << 9)) != 0)
-    cache->prev_sp += 4;
+  TRY
+    {
+      unwound_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+      /* The hardware saves eight 32-bit words, comprising xPSR,
+	 ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
+	 "B1.5.6 Exception entry behavior" in
+	 "ARMv7-M Architecture Reference Manual".  */
+      cache->saved_regs[0].addr = unwound_sp;
+      cache->saved_regs[1].addr = unwound_sp + 4;
+      cache->saved_regs[2].addr = unwound_sp + 8;
+      cache->saved_regs[3].addr = unwound_sp + 12;
+      cache->saved_regs[12].addr = unwound_sp + 16;
+      cache->saved_regs[14].addr = unwound_sp + 20;
+      cache->saved_regs[15].addr = unwound_sp + 24;
+      cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
+
+      /* If bit 9 of the saved xPSR is set, then there is a four-byte
+	 aligner between the top of the 32-byte stack frame and the
+	 previous context's stack pointer.  */
+      cache->prev_sp = unwound_sp + 32;
+      if (safe_read_memory_integer (unwound_sp + 28, 4, byte_order, &xpsr)
+	  && (xpsr & (1 << 9)) != 0)
+	cache->prev_sp += 4;
+
+      cache->available_p = 1;
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
 
   return cache;
 }
@@ -2850,9 +2895,12 @@ arm_m_exception_this_id (struct frame_info *this_frame,
     *this_cache = arm_m_exception_cache (this_frame);
   cache = (struct arm_prologue_cache *) *this_cache;
 
-  /* Our frame ID for a stub frame is the current SP and LR.  */
-  *this_id = frame_id_build (cache->prev_sp,
-			     get_frame_pc (this_frame));
+  if (!cache->available_p)
+    *this_id = frame_id_build_unavailable_stack (cache->prev_sp);
+  else
+    /* Our frame ID for a stub frame is the current SP and LR.  */
+    *this_id = frame_id_build (cache->prev_sp,
+			       get_frame_pc (this_frame));
 }
 
 /* Implementation of function hook 'prev_register' in
-- 
2.6.4

[PATCH 4/4] Support tracepoints for ARM linux in GDBServer

[Antoine Tremblay]

This patch adds support for tracepoints for ARM linux in GDBServer.

To enable this, this patch introduces a new :K (kind) field in the
QTDP packet to encode the breakpoint kind, this is the same kind as a z0
packet.

This is the new qSupported feature: TracepointKinds

This field is decoded by sw_breakpoint_from_kind target ops in linux-low.

A note about tests :

New tests passing: All of gdb.trace except below tests.

Failing tests:

new FAIL: gdb.trace/unavailable.exp: unavailable locals:
register locals: tfile: info locals
new FAIL: gdb.trace/unavailable.exp: unavailable locals:
register locals: tfile: print locd
new FAIL: gdb.trace/unavailable.exp: unavailable locals:
register locals: tfile: print locf

These tests are failing since we would need the proper gdbarch containing
the vfp registers when trying to read pseudo-registers.  However, we would
need to have an inferior running to have this and since we don't, the tests
fail.

Should these be set as expected fail ?

Tested on Ubuntu 14.04 ARMv7 and x86 with no regression.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* NEWS: Add news for tracepoins on ARM.

gdb/doc/ChangeLog:

	* gdb.texinfo (General Query Packets): Add TracepointKinds packet.
	(ARM Breakpoint Kinds): Add QTDP reference.
	(Tracepoint Packets): Add kind parameter to QTDP packet.

gdb/gdbserver/ChangeLog:

	* linux-arm-low.c (arm_supports_tracepoints): New function.
	(struct linux_target_ops) <supports_tracepoints>: Initialize.
	* mem-break.c (set_breakpoint_at_with_kind): New function.
	* mem-break.h (set_breakpoint_at_with_kind): New function declaration.
	* server.c (handle_query): Add TracepointsKinds feature.
	* tracepoint.c (struct tracepoint) <kind>: New field.
	(add_tracepoint): Initialize kind field.
	(cmd_qtdp): Handle kind field 'K'.
	(install_tracepoint): Use set_breakpoint_at_with_kind when kind is
	present.
	(cmd_qtstart): Likewise.

gdb/ChangeLog:

	* remote.c (remote_supports_tracepoint_kinds): New function declaration.
	(PACKET_TracepointKinds): New enum field.
	(remote_protocol_features[]): New TracepointKinds element.
	(remote_supports_tracepoint_kinds): New function.
	(remote_download_tracepoint): Fetch the breakpoint kind and send
	it as K parameter to QTDP packet.
	(_initialize_remote): Add TracepointKinds packet_config_cmd.

gdb/testsuite/ChangeLog:

	* gdb.trace/collection.exp (gdb_collect_return_test): Set test
	unsupported for arm/aarch32 targets as it's not supported by the arch.
	* gdb.trace/trace-common.h: Add ARM fast tracepoint label to allow
	tracepoints tests.
	* lib/trace-support.exp: Add arm/aarch32 target support.
---
 gdb/NEWS                               |  2 ++
 gdb/doc/gdb.texinfo                    | 22 +++++++++++++----
 gdb/gdbserver/linux-arm-low.c          | 10 +++++++-
 gdb/gdbserver/mem-break.c              | 13 ++++++++++
 gdb/gdbserver/mem-break.h              |  7 ++++++
 gdb/gdbserver/server.c                 |  1 +
 gdb/gdbserver/tracepoint.c             | 43 ++++++++++++++++++++++++++++------
 gdb/remote.c                           | 27 +++++++++++++++++++++
 gdb/testsuite/gdb.trace/collection.exp |  7 +++++-
 gdb/testsuite/gdb.trace/trace-common.h | 10 +++++++-
 gdb/testsuite/lib/trace-support.exp    |  4 ++++
 11 files changed, 131 insertions(+), 15 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 484d98d..28110cc 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 7.10
 
+* Support for tracepoints on arm-linux was added in GDBServer.
+
 * Record btrace now supports non-stop mode.
 
 * Support for tracepoints on aarch64-linux was added in GDBserver.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 0778383..cffad48 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -36633,6 +36633,11 @@ These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab No
 
+@item @samp{TracepointKinds}
+@tab No
+@tab @samp{-}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -36851,6 +36856,9 @@ The remote stub understands the @samp{QThreadEvents} packet.
 @item no-resumed
 The remote stub reports the @samp{N} stop reply.
 
+@item TracepointKinds
+The remote stub reports the @samp{:K} kind parameter for @samp{QTDP} packets.
+
 @end table
 
 @item qSymbol::
@@ -37361,7 +37369,8 @@ details of XML target descriptions for each architecture.
 @subsubsection @acronym{ARM} Breakpoint Kinds
 @cindex breakpoint kinds, @acronym{ARM}
 
-These breakpoint kinds are defined for the @samp{Z0} and @samp{Z1} packets.
+These breakpoint kinds are defined for the @samp{Z0}, @samp{Z1}
+and @samp{QTDP} packets.
 
 @table @r
 
@@ -37441,7 +37450,7 @@ tracepoints (@pxref{Tracepoints}).
 
 @table @samp
 
-@item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}[:F@var{flen}][:X@var{len},@var{bytes}]@r{[}-@r{]}
+@item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}[:F@var{flen}][:X@var{len},@var{bytes}][:K@var{kind}]@r{[}-@r{]}
 @cindex @samp{QTDP} packet
 Create a new tracepoint, number @var{n}, at @var{addr}.  If @var{ena}
 is @samp{E}, then the tracepoint is enabled; if it is @samp{D}, then
@@ -37452,9 +37461,12 @@ the number of bytes that the target should copy elsewhere to make room
 for the tracepoint.  If an @samp{X} is present, it introduces a
 tracepoint condition, which consists of a hexadecimal length, followed
 by a comma and hex-encoded bytes, in a manner similar to action
-encodings as described below.  If the trailing @samp{-} is present,
-further @samp{QTDP} packets will follow to specify this tracepoint's
-actions.
+encodings as described below. If a @samp{K} is present, it
+indicates a target specific breakpoint length.  E.g., the arm and mips
+can insert either a 2 or 4 byte breakpoint. Some architectures have
+additional meanings for kind see @ref{Architecture-Specific Protocol
+Details}. If the trailing @samp{-} is present, further @samp{QTDP}
+packets will follow to specify this tracepoint's actions.
 
 Replies:
 @table @samp
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index d967e58..76d0e32 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -1005,6 +1005,14 @@ arm_regs_info (void)
     return &regs_info_arm;
 }
 
+/* Implementation of the linux_target_ops method "support_tracepoints".  */
+
+static int
+arm_supports_tracepoints (void)
+{
+  return 1;
+}
+
 struct linux_target_ops the_low_target = {
   arm_arch_setup,
   arm_regs_info,
@@ -1031,7 +1039,7 @@ struct linux_target_ops the_low_target = {
   arm_new_fork,
   arm_prepare_to_resume,
   NULL, /* process_qsupported */
-  NULL, /* supports_tracepoints */
+  arm_supports_tracepoints,
   NULL, /* get_thread_area */
   NULL, /* install_fast_tracepoint_jump_pad */
   NULL, /* emit_ops */
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 2e220b8..3a9a816 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -791,6 +791,19 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
 			 &err_ignored);
 }
 
+/* See mem-break.h  */
+
+struct breakpoint *
+set_breakpoint_at_with_kind (CORE_ADDR where,
+			     int (*handler) (CORE_ADDR),
+			     int kind)
+{
+  int err_ignored;
+
+  return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
+			 where, kind, handler,
+			 &err_ignored);
+}
 
 static int
 delete_raw_breakpoint (struct process_info *proc, struct raw_breakpoint *todel)
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 4d9a76c..02a1038 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -146,6 +146,13 @@ int gdb_breakpoint_here (CORE_ADDR where);
 struct breakpoint *set_breakpoint_at (CORE_ADDR where,
 				      int (*handler) (CORE_ADDR));
 
+/* Same as set_breakpoint_at but allow the kind to be specified */
+
+struct breakpoint *set_breakpoint_at_with_kind (CORE_ADDR where,
+						int (*handler)(CORE_ADDR),
+						int kind);
+
+
 /* Delete a breakpoint.  */
 
 int delete_breakpoint (struct breakpoint *bkpt);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index fe7195d..059a373 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2269,6 +2269,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	  strcat (own_buf, ";EnableDisableTracepoints+");
 	  strcat (own_buf, ";QTBuffer:size+");
 	  strcat (own_buf, ";tracenz+");
+	  strcat (own_buf, ";TracepointKinds+");
 	}
 
       if (target_supports_hardware_single_step ()
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 40d0da9..3a50f47 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -754,6 +754,11 @@ struct tracepoint
   /* Link to the next tracepoint in the list.  */
   struct tracepoint *next;
 
+  /* Optional kind of the breakpoint to be used.  Note this can mean
+     different things for different archs as z0 breakpoint command.
+     Value is -1 if not persent.  */
+  int32_t kind;
+
 #ifndef IN_PROCESS_AGENT
   /* The list of actions to take when the tracepoint triggers, in
      string/packet form.  */
@@ -1820,6 +1825,7 @@ add_tracepoint (int num, CORE_ADDR addr)
   tpoint->compiled_cond = 0;
   tpoint->handle = NULL;
   tpoint->next = NULL;
+  tpoint->kind = -1;
 
   /* Find a place to insert this tracepoint into list in order to keep
      the tracepoint list still in the ascending order.  There may be
@@ -2495,6 +2501,7 @@ cmd_qtdp (char *own_buf)
   ULONGEST num;
   ULONGEST addr;
   ULONGEST count;
+  ULONGEST kind;
   struct tracepoint *tpoint;
   char *actparm;
   char *packet = own_buf;
@@ -2561,6 +2568,12 @@ cmd_qtdp (char *own_buf)
 	      tpoint->cond = gdb_parse_agent_expr (&actparm);
 	      packet = actparm;
 	    }
+	  else if (*packet == 'K')
+	    {
+	      ++packet;
+	      packet = unpack_varlen_hex (packet, &kind);
+	      tpoint->kind = kind;
+	    }
 	  else if (*packet == '-')
 	    break;
 	  else if (*packet == '\0')
@@ -2575,11 +2588,13 @@ cmd_qtdp (char *own_buf)
 	}
 
       trace_debug ("Defined %stracepoint %d at 0x%s, "
-		   "enabled %d step %" PRIu64 " pass %" PRIu64,
+		   "enabled %d step %" PRIu64 " pass %" PRIu64
+		   " kind %" PRId32,
 		   tpoint->type == fast_tracepoint ? "fast "
 		   : tpoint->type == static_tracepoint ? "static " : "",
 		   tpoint->number, paddress (tpoint->address), tpoint->enabled,
-		   tpoint->step_count, tpoint->pass_count);
+		   tpoint->step_count, tpoint->pass_count,
+		   tpoint->kind);
     }
   else if (tpoint)
     add_tracepoint_action (tpoint, packet);
@@ -3153,9 +3168,17 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf)
       /* Tracepoints are installed as memory breakpoints.  Just go
 	 ahead and install the trap.  The breakpoints module
 	 handles duplicated breakpoints, and the memory read
-	 routine handles un-patching traps from memory reads.  */
-      tpoint->handle = set_breakpoint_at (tpoint->address,
-					  tracepoint_handler);
+	 routine handles un-patching traps from memory reads.
+	 If tracepoint kind is not set, use the default values
+	 otherwise what was set from the gdb client will be used.  */
+      if (tpoint->kind == -1)
+	  tpoint->handle = set_breakpoint_at (tpoint->address,
+					      tracepoint_handler);
+      else
+	  tpoint->handle =
+	    set_breakpoint_at_with_kind (tpoint->address,
+					 tracepoint_handler,
+					 tpoint->kind);
     }
   else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint)
     {
@@ -3248,8 +3271,14 @@ cmd_qtstart (char *packet)
 	     ahead and install the trap.  The breakpoints module
 	     handles duplicated breakpoints, and the memory read
 	     routine handles un-patching traps from memory reads.  */
-	  tpoint->handle = set_breakpoint_at (tpoint->address,
-					      tracepoint_handler);
+	  if (tpoint->kind == -1)
+	    tpoint->handle = set_breakpoint_at (tpoint->address,
+						tracepoint_handler);
+	  else
+	    tpoint->handle =
+	      set_breakpoint_at_with_kind (tpoint->address,
+					   tracepoint_handler,
+					   tpoint->kind);
 	}
       else if (tpoint->type == fast_tracepoint
 	       || tpoint->type == static_tracepoint)
diff --git a/gdb/remote.c b/gdb/remote.c
index 528d863..17581e2 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -241,6 +241,8 @@ static int stop_reply_queue_length (void);
 
 static void readahead_cache_invalidate (void);
 
+static int remote_supports_tracepoint_kinds (void);
+
 /* For "remote".  */
 
 static struct cmd_list_element *remote_cmdlist;
@@ -1491,6 +1493,9 @@ enum {
   /* Support TARGET_WAITKIND_NO_RESUMED.  */
   PACKET_no_resumed,
 
+  /* Support target dependant tracepoint kinds.  */
+  PACKET_TracepointKinds,
+
   PACKET_MAX
 };
 
@@ -4534,6 +4539,8 @@ static const struct protocol_feature remote_protocol_features[] = {
   { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported },
   { "QThreadEvents", PACKET_DISABLE, remote_supported_packet, PACKET_QThreadEvents },
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
+  { "TracepointKinds", PACKET_DISABLE, remote_supported_packet,
+    PACKET_TracepointKinds }
 };
 
 static char *remote_support_xml;
@@ -11692,6 +11699,12 @@ remote_can_run_breakpoint_commands (struct target_ops *self)
   return packet_support (PACKET_BreakpointCommands) == PACKET_ENABLE;
 }
 
+static int
+remote_supports_tracepoint_kinds (void)
+{
+  return packet_support (PACKET_TracepointKinds) == PACKET_ENABLE;
+}
+
 static void
 remote_trace_init (struct target_ops *self)
 {
@@ -11780,6 +11793,7 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
   char *pkt;
   struct breakpoint *b = loc->owner;
   struct tracepoint *t = (struct tracepoint *) b;
+  int kind;
 
   encode_actions_rsp (loc, &tdp_actions, &stepping_actions);
   old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
@@ -11788,6 +11802,10 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
 		       stepping_actions);
 
   tpaddr = loc->address;
+
+  /* Fetch the proper tracepoint kind.  */
+  gdbarch_remote_breakpoint_from_pc (target_gdbarch (), &tpaddr, &kind);
+
   sprintf_vma (addrbuf, tpaddr);
   xsnprintf (buf, BUF_SIZE, "QTDP:%x:%s:%c:%lx:%x", b->number,
 	     addrbuf, /* address */
@@ -11862,6 +11880,11 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
 		   "ignoring tp %d cond"), b->number);
     }
 
+  /* Tracepoint Kinds are modeled after the breakpoint Z0 kind packet.
+     Send the tracepoint kind if we support it.  */
+  if (remote_supports_tracepoint_kinds ())
+    xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":K%x", kind);
+
   if (b->commands || *default_collect)
     strcat (buf, "-");
   putpkt (buf);
@@ -13767,6 +13790,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_no_resumed],
 			 "N stop reply", "no-resumed-stop-reply", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_TracepointKinds],
+			 "TracepointKinds",
+			 "tracepoint-kinds", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
diff --git a/gdb/testsuite/gdb.trace/collection.exp b/gdb/testsuite/gdb.trace/collection.exp
index f225429..a30234f 100644
--- a/gdb/testsuite/gdb.trace/collection.exp
+++ b/gdb/testsuite/gdb.trace/collection.exp
@@ -764,7 +764,12 @@ proc gdb_trace_collection_test {} {
     gdb_collect_expression_test globals_test_func \
 	    "globalarr\[\(l6, l7\)\]" "7"    "a\[\(b, c\)\]"
 
-    gdb_collect_return_test
+    #This architecture has no method to collect a return address.
+    if { [is_aarch32_target] } {
+	unsupported "collect \$_ret: This architecture has no method to collect a return address"
+    } else {
+	gdb_collect_return_test
+    }
 
     gdb_collect_strings_test strings_test_func "locstr" "abcdef" "" \
 	    "local string"
diff --git a/gdb/testsuite/gdb.trace/trace-common.h b/gdb/testsuite/gdb.trace/trace-common.h
index eceb182..4f05423 100644
--- a/gdb/testsuite/gdb.trace/trace-common.h
+++ b/gdb/testsuite/gdb.trace/trace-common.h
@@ -40,7 +40,7 @@ x86_trace_dummy ()
        "    call " SYMBOL(x86_trace_dummy) "\n" \
        )
 
-#elif (defined __aarch64__)
+#elif (defined __aarch64__ || (defined __arm__ && !defined __thumb__))
 
 #define FAST_TRACEPOINT_LABEL(name) \
   asm ("    .global " SYMBOL(name) "\n" \
@@ -48,6 +48,14 @@ x86_trace_dummy ()
        "    nop\n" \
        )
 
+#elif (defined __arm__ && defined __thumb2__)
+
+#define FAST_TRACEPOINT_LABEL(name) \
+  asm ("    .global " SYMBOL(name) "\n" \
+       SYMBOL(name) ":\n" \
+       "    nop.w\n" \
+       )
+
 #else
 
 #error "unsupported architecture for trace tests"
diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp
index f593c43..ef63f7a 100644
--- a/gdb/testsuite/lib/trace-support.exp
+++ b/gdb/testsuite/lib/trace-support.exp
@@ -36,6 +36,10 @@ if [is_amd64_regs_target] {
     set fpreg "x29"
     set spreg "sp"
     set pcreg "pc"
+} elseif [is_aarch32_target] {
+    set fpreg "sp"
+    set spreg "sp"
+    set pcreg "pc"
 } else {
     set fpreg "fp"
     set spreg "sp"
-- 
2.6.4

[PATCH 3/4] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.
---
 gdb/arm-tdep.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 5ee7fb0..562fb2b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8752,6 +8752,65 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int rawnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      rawnum = (reg - num_regs) / 2 + 26;
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      rawnum = (reg - num_regs - 32) * 2 + 26;
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  return rawnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9413,6 +9472,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
-- 
2.6.4

[PATCH 2/4] Use the target architecture when encoding tracepoint actions

[Antoine Tremblay]

This patch uses the target architecture rather then the objfile
architecture when encoding tracepoint actions.

The target architecture may contain additional registers. E.g. ARM VFP
registers. This information is needed to allow their collection. Since we
can never know whether the registers numbers in the target match the
binary's we have to use tdesc here.

One note about combined debuggers / multi-inferior from Pedro Alves:

In the combined debugger case taking Cell as the practical example that
gdb supports currently:

In that case, the main target_gdbarch() will be powerpc, but you may have set a
tracepoint on _spu_ code, which has a different gdbarch.  so for that case,
target_gdbarch would be wrong.  I think that in that case, we'd need to
find __the_ target/tdesc gdbarch that is (bfd) compatible with the
objfile's gdbarch.

I think cell/spu gdbserver doesn't support tracepoints, so we can ignore
this for now.

The multi-inferior/process case is somewhat related, but its simpler.
each inferior has its own gdbarch.

That is, target_gdbarch depends on the current inferior selected.
In fact, that just returns inferior->gdbarch nowaways.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
	than loc->gdbarch.
---
 gdb/tracepoint.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 55b2e8e..f7f5736 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1428,14 +1428,14 @@ encode_actions_1 (struct command_line *action,
 
 	      if (0 == strncasecmp ("$reg", action_exp, 4))
 		{
-		  for (i = 0; i < gdbarch_num_regs (tloc->gdbarch); i++)
+		  for (i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++)
 		    add_register (collect, i);
 		  action_exp = strchr (action_exp, ',');	/* more? */
 		}
 	      else if (0 == strncasecmp ("$arg", action_exp, 4))
 		{
 		  add_local_symbols (collect,
-				     tloc->gdbarch,
+				     target_gdbarch (),
 				     tloc->address,
 				     frame_reg,
 				     frame_offset,
@@ -1446,7 +1446,7 @@ encode_actions_1 (struct command_line *action,
 	      else if (0 == strncasecmp ("$loc", action_exp, 4))
 		{
 		  add_local_symbols (collect,
-				     tloc->gdbarch,
+				     target_gdbarch (),
 				     tloc->address,
 				     frame_reg,
 				     frame_offset,
@@ -1459,7 +1459,7 @@ encode_actions_1 (struct command_line *action,
 		  struct cleanup *old_chain1 = NULL;
 
 		  aexpr = gen_trace_for_return_address (tloc->address,
-							tloc->gdbarch,
+							target_gdbarch (),
 							trace_string);
 
 		  old_chain1 = make_cleanup_free_agent_expr (aexpr);
@@ -1513,7 +1513,7 @@ encode_actions_1 (struct command_line *action,
 		      {
 			const char *name = &exp->elts[2].string;
 
-			i = user_reg_map_name_to_regnum (tloc->gdbarch,
+			i = user_reg_map_name_to_regnum (target_gdbarch (),
 							 name, strlen (name));
 			if (i == -1)
 			  internal_error (__FILE__, __LINE__,
@@ -1543,7 +1543,7 @@ encode_actions_1 (struct command_line *action,
 
 			collect_symbol (collect,
 					exp->elts[2].symbol,
-					tloc->gdbarch,
+					target_gdbarch (),
 					frame_reg,
 					frame_offset,
 					tloc->address,
-- 
2.6.4

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> This patch series enables GDBServer to trace an ARM target on linux.
>
> Patches 1-3: Fixes collection failures in certain cases.
>
> Patch 4: Enables tracepoints on ARM and introduces the new TracepointKinds
> feature and 'K' parameter to the QTDP packet.

Hi Antoine,
First of all, thanks for patches.  I am afraid can't review them soon
because 1) I find there are some places in arm software single step code
can be improved, so I'd like to clean the room first before we move in
new furniture, 2) I have to fix AArch64 and ARM test fails before 7.11
branch/release.  I'll review them after I finish them above, if nobody
reviews them yet.

-- 
Yao (齐尧)

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Antoine Tremblay]

On 01/11/2016 07:17 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> This patch series enables GDBServer to trace an ARM target on linux.
>>
>> Patches 1-3: Fixes collection failures in certain cases.
>>
>> Patch 4: Enables tracepoints on ARM and introduces the new TracepointKinds
>> feature and 'K' parameter to the QTDP packet.
> Hi Antoine,
> First of all, thanks for patches.  I am afraid can't review them soon
> because 1) I find there are some places in arm software single step code
> can be improved, so I'd like to clean the room first before we move in
> new furniture,
Can I help with that ? Maybe it would free you some time for 2) ?

>   2) I have to fix AArch64 and ARM test fails before 7.11
> branch/release.  I'll review them after I finish them above, if nobody
> reviews them yet.
>

OK. I wish I could get these features in for 7.11 too.
It's not too ARM specific at least, so it's good that you're OK with 
another reviewer pitching in.

I hope it all works out for 7.11.

Thanks for letting me know.

Regards,
Antoine

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

>> Hi Antoine,
>> First of all, thanks for patches.  I am afraid can't review them soon
>> because 1) I find there are some places in arm software single step code
>> can be improved, so I'd like to clean the room first before we move in
>> new furniture,
> Can I help with that ? Maybe it would free you some time for 2) ?
>

Thanks for your offer.  Most of my patches are done in my tree, but I
need to polish them, and figure out some better solutions.

>>   2) I have to fix AArch64 and ARM test fails before 7.11
>> branch/release.  I'll review them after I finish them above, if nobody
>> reviews them yet.
>>
>
> OK. I wish I could get these features in for 7.11 too.
> It's not too ARM specific at least, so it's good that you're OK with
> another reviewer pitching in.
>
> I hope it all works out for 7.11.

Yes, it is good to ship it in 7.11, but it isn't a must for 7.11, IMO.
I don't mind that ARM linux tracepiont isn't in 7.11 release.  I'd like
to make these existing features working properly, and then look at the
new things.

-- 
Yao (齐尧)

Re: [PATCH 2/4] Use the target architecture when encoding tracepoint actions

[Marcin Kościelnicki]

On 07/01/16 18:44, Antoine Tremblay wrote:
> This patch uses the target architecture rather then the objfile
> architecture when encoding tracepoint actions.
>
> The target architecture may contain additional registers. E.g. ARM VFP
> registers. This information is needed to allow their collection. Since we
> can never know whether the registers numbers in the target match the
> binary's we have to use tdesc here.
>
> One note about combined debuggers / multi-inferior from Pedro Alves:
>
> In the combined debugger case taking Cell as the practical example that
> gdb supports currently:
>
> In that case, the main target_gdbarch() will be powerpc, but you may have set a
> tracepoint on _spu_ code, which has a different gdbarch.  so for that case,
> target_gdbarch would be wrong.  I think that in that case, we'd need to
> find __the_ target/tdesc gdbarch that is (bfd) compatible with the
> objfile's gdbarch.
>
> I think cell/spu gdbserver doesn't support tracepoints, so we can ignore
> this for now.
>
> The multi-inferior/process case is somewhat related, but its simpler.
> each inferior has its own gdbarch.
>
> That is, target_gdbarch depends on the current inferior selected.
> In fact, that just returns inferior->gdbarch nowaways.
>
> No regressions, tested on ubuntu 14.04 ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
>
> gdb/ChangeLog:
>
> 	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
> 	than loc->gdbarch.
 > [...]

Hey, could we get that one pushed soon?  I've made a patchset that adds 
tdesc information to tfile format, making it work properly for 
multiple-tdesc architectures 
(https://sourceware.org/ml/gdb-patches/2016-02/msg00161.html), and made 
x86_64 work with collecting AVX registers 
(https://sourceware.org/ml/gdb-patches/2016-02/msg00167.html), so that 
patch could do a lot of good already.

Marcin Kościelnicki

Re: [PATCH 4/4] Support tracepoints for ARM linux in GDBServer

[Antoine Tremblay]

On 01/07/2016 12:44 PM, Antoine Tremblay wrote:
> This patch adds support for tracepoints for ARM linux in GDBServer.
>
> To enable this, this patch introduces a new :K (kind) field in the
> QTDP packet to encode the breakpoint kind, this is the same kind as a z0
> packet.
>
> This is the new qSupported feature: TracepointKinds
>
> This field is decoded by sw_breakpoint_from_kind target ops in linux-low.
>
> A note about tests :
>
> New tests passing: All of gdb.trace except below tests.
>
> Failing tests:
>
> new FAIL: gdb.trace/unavailable.exp: unavailable locals:
> register locals: tfile: info locals
> new FAIL: gdb.trace/unavailable.exp: unavailable locals:
> register locals: tfile: print locd
> new FAIL: gdb.trace/unavailable.exp: unavailable locals:
> register locals: tfile: print locf
>
> These tests are failing since we would need the proper gdbarch containing
> the vfp registers when trying to read pseudo-registers.  However, we would
> need to have an inferior running to have this and since we don't, the tests
> fail.
>
>Should these be set as expected fail ?
>

Note that these tests no longer fail after this patch series :
https://sourceware.org/ml/gdb-patches/2016-02/msg00161.html

Thanks for that work Marcin Kościelnicki!

So I think we could leave them failing if this patch set goes in before, 
since they will get fixed...or I'll adapt the commit log if Marcin's 
patch set is first to be merged.

> Tested on Ubuntu 14.04 ARMv7 and x86 with no regression.
> With gdbserver-{native,extended} / { -marm -mthumb }
>
> gdb/ChangeLog:
>
> 	* NEWS: Add news for tracepoins on ARM.
>
> gdb/doc/ChangeLog:
>
> 	* gdb.texinfo (General Query Packets): Add TracepointKinds packet.
> 	(ARM Breakpoint Kinds): Add QTDP reference.
> 	(Tracepoint Packets): Add kind parameter to QTDP packet.
>
> gdb/gdbserver/ChangeLog:
>
> 	* linux-arm-low.c (arm_supports_tracepoints): New function.
> 	(struct linux_target_ops) <supports_tracepoints>: Initialize.
> 	* mem-break.c (set_breakpoint_at_with_kind): New function.
> 	* mem-break.h (set_breakpoint_at_with_kind): New function declaration.
> 	* server.c (handle_query): Add TracepointsKinds feature.
> 	* tracepoint.c (struct tracepoint) <kind>: New field.
> 	(add_tracepoint): Initialize kind field.
> 	(cmd_qtdp): Handle kind field 'K'.
> 	(install_tracepoint): Use set_breakpoint_at_with_kind when kind is
> 	present.
> 	(cmd_qtstart): Likewise.
>
> gdb/ChangeLog:
>
> 	* remote.c (remote_supports_tracepoint_kinds): New function declaration.
> 	(PACKET_TracepointKinds): New enum field.
> 	(remote_protocol_features[]): New TracepointKinds element.
> 	(remote_supports_tracepoint_kinds): New function.
> 	(remote_download_tracepoint): Fetch the breakpoint kind and send
> 	it as K parameter to QTDP packet.
> 	(_initialize_remote): Add TracepointKinds packet_config_cmd.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.trace/collection.exp (gdb_collect_return_test): Set test
> 	unsupported for arm/aarch32 targets as it's not supported by the arch.
> 	* gdb.trace/trace-common.h: Add ARM fast tracepoint label to allow
> 	tracepoints tests.
> 	* lib/trace-support.exp: Add arm/aarch32 target support.
> ---
>   gdb/NEWS                               |  2 ++
>   gdb/doc/gdb.texinfo                    | 22 +++++++++++++----
>   gdb/gdbserver/linux-arm-low.c          | 10 +++++++-
>   gdb/gdbserver/mem-break.c              | 13 ++++++++++
>   gdb/gdbserver/mem-break.h              |  7 ++++++
>   gdb/gdbserver/server.c                 |  1 +
>   gdb/gdbserver/tracepoint.c             | 43 ++++++++++++++++++++++++++++------
>   gdb/remote.c                           | 27 +++++++++++++++++++++
>   gdb/testsuite/gdb.trace/collection.exp |  7 +++++-
>   gdb/testsuite/gdb.trace/trace-common.h | 10 +++++++-
>   gdb/testsuite/lib/trace-support.exp    |  4 ++++
>   11 files changed, 131 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 484d98d..28110cc 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
>
>   *** Changes since GDB 7.10
>
> +* Support for tracepoints on arm-linux was added in GDBServer.
> +
>   * Record btrace now supports non-stop mode.
>
>   * Support for tracepoints on aarch64-linux was added in GDBserver.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 0778383..cffad48 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -36633,6 +36633,11 @@ These are the currently defined stub features and their properties:
>   @tab @samp{-}
>   @tab No
>
> +@item @samp{TracepointKinds}
> +@tab No
> +@tab @samp{-}
> +@tab No
> +
>   @end multitable
>
>   These are the currently defined stub features, in more detail:
> @@ -36851,6 +36856,9 @@ The remote stub understands the @samp{QThreadEvents} packet.
>   @item no-resumed
>   The remote stub reports the @samp{N} stop reply.
>
> +@item TracepointKinds
> +The remote stub reports the @samp{:K} kind parameter for @samp{QTDP} packets.
> +
>   @end table
>
>   @item qSymbol::
> @@ -37361,7 +37369,8 @@ details of XML target descriptions for each architecture.
>   @subsubsection @acronym{ARM} Breakpoint Kinds
>   @cindex breakpoint kinds, @acronym{ARM}
>
> -These breakpoint kinds are defined for the @samp{Z0} and @samp{Z1} packets.
> +These breakpoint kinds are defined for the @samp{Z0}, @samp{Z1}
> +and @samp{QTDP} packets.
>
>   @table @r
>
> @@ -37441,7 +37450,7 @@ tracepoints (@pxref{Tracepoints}).
>
>   @table @samp
>
> -@item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}[:F@var{flen}][:X@var{len},@var{bytes}]@r{[}-@r{]}
> +@item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}[:F@var{flen}][:X@var{len},@var{bytes}][:K@var{kind}]@r{[}-@r{]}
>   @cindex @samp{QTDP} packet
>   Create a new tracepoint, number @var{n}, at @var{addr}.  If @var{ena}
>   is @samp{E}, then the tracepoint is enabled; if it is @samp{D}, then
> @@ -37452,9 +37461,12 @@ the number of bytes that the target should copy elsewhere to make room
>   for the tracepoint.  If an @samp{X} is present, it introduces a
>   tracepoint condition, which consists of a hexadecimal length, followed
>   by a comma and hex-encoded bytes, in a manner similar to action
> -encodings as described below.  If the trailing @samp{-} is present,
> -further @samp{QTDP} packets will follow to specify this tracepoint's
> -actions.
> +encodings as described below. If a @samp{K} is present, it
> +indicates a target specific breakpoint length.  E.g., the arm and mips
> +can insert either a 2 or 4 byte breakpoint. Some architectures have
> +additional meanings for kind see @ref{Architecture-Specific Protocol
> +Details}. If the trailing @samp{-} is present, further @samp{QTDP}
> +packets will follow to specify this tracepoint's actions.
>
>   Replies:
>   @table @samp
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index d967e58..76d0e32 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -1005,6 +1005,14 @@ arm_regs_info (void)
>       return &regs_info_arm;
>   }
>
> +/* Implementation of the linux_target_ops method "support_tracepoints".  */
> +
> +static int
> +arm_supports_tracepoints (void)
> +{
> +  return 1;
> +}
> +
>   struct linux_target_ops the_low_target = {
>     arm_arch_setup,
>     arm_regs_info,
> @@ -1031,7 +1039,7 @@ struct linux_target_ops the_low_target = {
>     arm_new_fork,
>     arm_prepare_to_resume,
>     NULL, /* process_qsupported */
> -  NULL, /* supports_tracepoints */
> +  arm_supports_tracepoints,
>     NULL, /* get_thread_area */
>     NULL, /* install_fast_tracepoint_jump_pad */
>     NULL, /* emit_ops */
> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
> index 2e220b8..3a9a816 100644
> --- a/gdb/gdbserver/mem-break.c
> +++ b/gdb/gdbserver/mem-break.c
> @@ -791,6 +791,19 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
>   			 &err_ignored);
>   }
>
> +/* See mem-break.h  */
> +
> +struct breakpoint *
> +set_breakpoint_at_with_kind (CORE_ADDR where,
> +			     int (*handler) (CORE_ADDR),
> +			     int kind)
> +{
> +  int err_ignored;
> +
> +  return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
> +			 where, kind, handler,
> +			 &err_ignored);
> +}
>
>   static int
>   delete_raw_breakpoint (struct process_info *proc, struct raw_breakpoint *todel)
> diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
> index 4d9a76c..02a1038 100644
> --- a/gdb/gdbserver/mem-break.h
> +++ b/gdb/gdbserver/mem-break.h
> @@ -146,6 +146,13 @@ int gdb_breakpoint_here (CORE_ADDR where);
>   struct breakpoint *set_breakpoint_at (CORE_ADDR where,
>   				      int (*handler) (CORE_ADDR));
>
> +/* Same as set_breakpoint_at but allow the kind to be specified */
> +
> +struct breakpoint *set_breakpoint_at_with_kind (CORE_ADDR where,
> +						int (*handler)(CORE_ADDR),
> +						int kind);
> +
> +
>   /* Delete a breakpoint.  */
>
>   int delete_breakpoint (struct breakpoint *bkpt);
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index fe7195d..059a373 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -2269,6 +2269,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>   	  strcat (own_buf, ";EnableDisableTracepoints+");
>   	  strcat (own_buf, ";QTBuffer:size+");
>   	  strcat (own_buf, ";tracenz+");
> +	  strcat (own_buf, ";TracepointKinds+");
>   	}
>
>         if (target_supports_hardware_single_step ()
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 40d0da9..3a50f47 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -754,6 +754,11 @@ struct tracepoint
>     /* Link to the next tracepoint in the list.  */
>     struct tracepoint *next;
>
> +  /* Optional kind of the breakpoint to be used.  Note this can mean
> +     different things for different archs as z0 breakpoint command.
> +     Value is -1 if not persent.  */
> +  int32_t kind;
> +
>   #ifndef IN_PROCESS_AGENT
>     /* The list of actions to take when the tracepoint triggers, in
>        string/packet form.  */
> @@ -1820,6 +1825,7 @@ add_tracepoint (int num, CORE_ADDR addr)
>     tpoint->compiled_cond = 0;
>     tpoint->handle = NULL;
>     tpoint->next = NULL;
> +  tpoint->kind = -1;
>
>     /* Find a place to insert this tracepoint into list in order to keep
>        the tracepoint list still in the ascending order.  There may be
> @@ -2495,6 +2501,7 @@ cmd_qtdp (char *own_buf)
>     ULONGEST num;
>     ULONGEST addr;
>     ULONGEST count;
> +  ULONGEST kind;
>     struct tracepoint *tpoint;
>     char *actparm;
>     char *packet = own_buf;
> @@ -2561,6 +2568,12 @@ cmd_qtdp (char *own_buf)
>   	      tpoint->cond = gdb_parse_agent_expr (&actparm);
>   	      packet = actparm;
>   	    }
> +	  else if (*packet == 'K')
> +	    {
> +	      ++packet;
> +	      packet = unpack_varlen_hex (packet, &kind);
> +	      tpoint->kind = kind;
> +	    }
>   	  else if (*packet == '-')
>   	    break;
>   	  else if (*packet == '\0')
> @@ -2575,11 +2588,13 @@ cmd_qtdp (char *own_buf)
>   	}
>
>         trace_debug ("Defined %stracepoint %d at 0x%s, "
> -		   "enabled %d step %" PRIu64 " pass %" PRIu64,
> +		   "enabled %d step %" PRIu64 " pass %" PRIu64
> +		   " kind %" PRId32,
>   		   tpoint->type == fast_tracepoint ? "fast "
>   		   : tpoint->type == static_tracepoint ? "static " : "",
>   		   tpoint->number, paddress (tpoint->address), tpoint->enabled,
> -		   tpoint->step_count, tpoint->pass_count);
> +		   tpoint->step_count, tpoint->pass_count,
> +		   tpoint->kind);
>       }
>     else if (tpoint)
>       add_tracepoint_action (tpoint, packet);
> @@ -3153,9 +3168,17 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf)
>         /* Tracepoints are installed as memory breakpoints.  Just go
>   	 ahead and install the trap.  The breakpoints module
>   	 handles duplicated breakpoints, and the memory read
> -	 routine handles un-patching traps from memory reads.  */
> -      tpoint->handle = set_breakpoint_at (tpoint->address,
> -					  tracepoint_handler);
> +	 routine handles un-patching traps from memory reads.
> +	 If tracepoint kind is not set, use the default values
> +	 otherwise what was set from the gdb client will be used.  */
> +      if (tpoint->kind == -1)
> +	  tpoint->handle = set_breakpoint_at (tpoint->address,
> +					      tracepoint_handler);
> +      else
> +	  tpoint->handle =
> +	    set_breakpoint_at_with_kind (tpoint->address,
> +					 tracepoint_handler,
> +					 tpoint->kind);
>       }
>     else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint)
>       {
> @@ -3248,8 +3271,14 @@ cmd_qtstart (char *packet)
>   	     ahead and install the trap.  The breakpoints module
>   	     handles duplicated breakpoints, and the memory read
>   	     routine handles un-patching traps from memory reads.  */
> -	  tpoint->handle = set_breakpoint_at (tpoint->address,
> -					      tracepoint_handler);
> +	  if (tpoint->kind == -1)
> +	    tpoint->handle = set_breakpoint_at (tpoint->address,
> +						tracepoint_handler);
> +	  else
> +	    tpoint->handle =
> +	      set_breakpoint_at_with_kind (tpoint->address,
> +					   tracepoint_handler,
> +					   tpoint->kind);
>   	}
>         else if (tpoint->type == fast_tracepoint
>   	       || tpoint->type == static_tracepoint)
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 528d863..17581e2 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -241,6 +241,8 @@ static int stop_reply_queue_length (void);
>
>   static void readahead_cache_invalidate (void);
>
> +static int remote_supports_tracepoint_kinds (void);
> +
>   /* For "remote".  */
>
>   static struct cmd_list_element *remote_cmdlist;
> @@ -1491,6 +1493,9 @@ enum {
>     /* Support TARGET_WAITKIND_NO_RESUMED.  */
>     PACKET_no_resumed,
>
> +  /* Support target dependant tracepoint kinds.  */
> +  PACKET_TracepointKinds,
> +
>     PACKET_MAX
>   };
>
> @@ -4534,6 +4539,8 @@ static const struct protocol_feature remote_protocol_features[] = {
>     { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported },
>     { "QThreadEvents", PACKET_DISABLE, remote_supported_packet, PACKET_QThreadEvents },
>     { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
> +  { "TracepointKinds", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_TracepointKinds }
>   };
>
>   static char *remote_support_xml;
> @@ -11692,6 +11699,12 @@ remote_can_run_breakpoint_commands (struct target_ops *self)
>     return packet_support (PACKET_BreakpointCommands) == PACKET_ENABLE;
>   }
>
> +static int
> +remote_supports_tracepoint_kinds (void)
> +{
> +  return packet_support (PACKET_TracepointKinds) == PACKET_ENABLE;
> +}
> +
>   static void
>   remote_trace_init (struct target_ops *self)
>   {
> @@ -11780,6 +11793,7 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
>     char *pkt;
>     struct breakpoint *b = loc->owner;
>     struct tracepoint *t = (struct tracepoint *) b;
> +  int kind;
>
>     encode_actions_rsp (loc, &tdp_actions, &stepping_actions);
>     old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
> @@ -11788,6 +11802,10 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
>   		       stepping_actions);
>
>     tpaddr = loc->address;
> +
> +  /* Fetch the proper tracepoint kind.  */
> +  gdbarch_remote_breakpoint_from_pc (target_gdbarch (), &tpaddr, &kind);
> +
>     sprintf_vma (addrbuf, tpaddr);
>     xsnprintf (buf, BUF_SIZE, "QTDP:%x:%s:%c:%lx:%x", b->number,
>   	     addrbuf, /* address */
> @@ -11862,6 +11880,11 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
>   		   "ignoring tp %d cond"), b->number);
>       }
>
> +  /* Tracepoint Kinds are modeled after the breakpoint Z0 kind packet.
> +     Send the tracepoint kind if we support it.  */
> +  if (remote_supports_tracepoint_kinds ())
> +    xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":K%x", kind);
> +
>     if (b->commands || *default_collect)
>       strcat (buf, "-");
>     putpkt (buf);
> @@ -13767,6 +13790,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>     add_packet_config_cmd (&remote_protocol_packets[PACKET_no_resumed],
>   			 "N stop reply", "no-resumed-stop-reply", 0);
>
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_TracepointKinds],
> +			 "TracepointKinds",
> +			 "tracepoint-kinds", 0);
> +
>     /* Assert that we've registered "set remote foo-packet" commands
>        for all packet configs.  */
>     {
> diff --git a/gdb/testsuite/gdb.trace/collection.exp b/gdb/testsuite/gdb.trace/collection.exp
> index f225429..a30234f 100644
> --- a/gdb/testsuite/gdb.trace/collection.exp
> +++ b/gdb/testsuite/gdb.trace/collection.exp
> @@ -764,7 +764,12 @@ proc gdb_trace_collection_test {} {
>       gdb_collect_expression_test globals_test_func \
>   	    "globalarr\[\(l6, l7\)\]" "7"    "a\[\(b, c\)\]"
>
> -    gdb_collect_return_test
> +    #This architecture has no method to collect a return address.
> +    if { [is_aarch32_target] } {
> +	unsupported "collect \$_ret: This architecture has no method to collect a return address"
> +    } else {
> +	gdb_collect_return_test
> +    }
>
>       gdb_collect_strings_test strings_test_func "locstr" "abcdef" "" \
>   	    "local string"
> diff --git a/gdb/testsuite/gdb.trace/trace-common.h b/gdb/testsuite/gdb.trace/trace-common.h
> index eceb182..4f05423 100644
> --- a/gdb/testsuite/gdb.trace/trace-common.h
> +++ b/gdb/testsuite/gdb.trace/trace-common.h
> @@ -40,7 +40,7 @@ x86_trace_dummy ()
>          "    call " SYMBOL(x86_trace_dummy) "\n" \
>          )
>
> -#elif (defined __aarch64__)
> +#elif (defined __aarch64__ || (defined __arm__ && !defined __thumb__))
>
>   #define FAST_TRACEPOINT_LABEL(name) \
>     asm ("    .global " SYMBOL(name) "\n" \
> @@ -48,6 +48,14 @@ x86_trace_dummy ()
>          "    nop\n" \
>          )
>
> +#elif (defined __arm__ && defined __thumb2__)
> +
> +#define FAST_TRACEPOINT_LABEL(name) \
> +  asm ("    .global " SYMBOL(name) "\n" \
> +       SYMBOL(name) ":\n" \
> +       "    nop.w\n" \
> +       )
> +
>   #else
>
>   #error "unsupported architecture for trace tests"
> diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp
> index f593c43..ef63f7a 100644
> --- a/gdb/testsuite/lib/trace-support.exp
> +++ b/gdb/testsuite/lib/trace-support.exp
> @@ -36,6 +36,10 @@ if [is_amd64_regs_target] {
>       set fpreg "x29"
>       set spreg "sp"
>       set pcreg "pc"
> +} elseif [is_aarch32_target] {
> +    set fpreg "sp"
> +    set spreg "sp"
> +    set pcreg "pc"
>   } else {
>       set fpreg "fp"
>       set spreg "sp"
>

Re: [PATCH 2/4] Use the target architecture when encoding tracepoint actions

[Pedro Alves]

On 01/07/2016 05:44 PM, Antoine Tremblay wrote:

> gdb/ChangeLog:
> 
> 	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
> 	than loc->gdbarch.

OK, please push.

Thanks,
Pedro Alves

Re: [PATCH 2/4] Use the target architecture when encoding tracepoint actions

[Antoine Tremblay]

Pedro Alves writes:

> On 01/07/2016 05:44 PM, Antoine Tremblay wrote:
>
>> gdb/ChangeLog:
>> 
>> 	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
>> 	than loc->gdbarch.
>
> OK, please push.
>
> Thanks,
> Pedro Alves

Pushed in.

Thanks,
Antoine

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

Hi Antoine,

> The reason for this is that the target's stack pointer is unavailable
> when examining the trace buffer.  What we are seeing is due to the
> 'tfind' command creating a sentinel frame and unwinding it.  If an
> exception is thrown, we are left with the sentinel frame being displayed
> at level #-1.  The exception is thrown when the prologue unwinder tries
> to read the stack pointer to construct an ID for the frame.
>
> This patch fixes this and similar issues by making all the arm unwinders
> catch NOT_AVAILABLE_ERROR exceptions when either register or memory is
> unreadable and report back to the frame core code with UNWIND_UNAVAILABLE.
>
> Note this commit log adapted from 7dfa3edc033c443036d9f2a3e01120f7fb54f498
> which fixed a similar issue for aarch64.

It is right to follow aarch64 patch, but I am wondering whether we can
do it better.

Nowadays, the unwind termination due to unavailable memory is handled in
unwinders in each arch backend.  However, as we support more and more
arch for tracepoint, can we handle the unwind termination in target
independent code?

The initial work of unwind termination due to unavailable memory was
done by Pedro https://www.sourceware.org/ml/gdb-patches/2011-02/msg00611.html
in a way that each unwinder was taught to terminate with
UNWIND_UNAVAILABLE.  At that moment, only x86 supports tracepoint, so it
was reasonable to handle UNWIND_UNAVAILABLE inside unwinders of one arch.  Now,
the situation changes, because we have more and more arch need
tracepoint support, if we can handle UNWIND_UNAVAILABLE in the callers
of each unwinder, each unwinder doesn't have to worry about the
unavailable at all.  In fact, GDB has done that way when calling unwinder->sniffer,
in frame_unwind_try_unwinder

  TRY
    {
      res = unwinder->sniffer (unwinder, this_frame, this_cache);
    }
  CATCH (ex, RETURN_MASK_ERROR)
    {
      if (ex.error == NOT_AVAILABLE_ERROR)
	{
	  /* This usually means that not even the PC is available,
	     thus most unwinders aren't able to determine if they're
	     the best fit.  Keep trying.  Fallback prologue unwinders
	     should always accept the frame.  */
	  do_cleanups (old_cleanup);
	  return 0;
	}
      throw_exception (ex);
    }
  END_CATCH

we can wrap methods of 'struct frame_unwind' with try/catch, and handle
NOT_AVAILABLE_ERROR properly.  In this way, each unwinder doesn't have
to worry about unavailable memory at all.

Pedro, what do you think?  Did you try this approach in the rest of 9
different ways :) (you said you "implemented this differently in about
10 different ways" in your email) ?

-- 
Yao (齐尧)

Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> +/* Map the pseudo register number REG to the proper register number.  */
> +
> +static int
> +arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
> +{
> +  int rawnum = 0;
> +  int num_regs = gdbarch_num_regs (gdbarch);
> +
> +  /* Single precision pseudo registers. s0-s31.  */
> +  if (reg >= num_regs && reg < num_regs + 32)
> +    {
> +      rawnum = (reg - num_regs) / 2 + 26;

We should get double register number via user_reg_map_name_to_regnum,

      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
						   strlen (name_buf));

> +    }
> +  /* Quadruple precision pseudo regisers. q0-q15.  */
> +  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
> +    {
> +      rawnum = (reg - num_regs - 32) * 2 + 26;

Likewise,

      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) * 2);
      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
						   strlen (name_buf));

additionally, we need to check gdbarch_tdep (gdbarch)->have_neon_pseudos,

> +    }
> +  /* Error bad register number.  */
> +  else
> +    return -1;
> +
> +  return rawnum;
> +}

We also need a test case, and you can extend gdb.trace/tfile-avx.exp.
Probably, it can be renamed to gdb.trace/tracefile-pseudo-reg.exp, and
put x86 and arm tests in it.

-- 
Yao (齐尧)

Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM

[Marcin Kościelnicki]

On 12/02/16 16:13, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> +/* Map the pseudo register number REG to the proper register number.  */
>> +
>> +static int
>> +arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
>> +{
>> +  int rawnum = 0;
>> +  int num_regs = gdbarch_num_regs (gdbarch);
>> +
>> +  /* Single precision pseudo registers. s0-s31.  */
>> +  if (reg >= num_regs && reg < num_regs + 32)
>> +    {
>> +      rawnum = (reg - num_regs) / 2 + 26;
>
> We should get double register number via user_reg_map_name_to_regnum,
>
>        xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
>        double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
> 						   strlen (name_buf));
> .
>> +    }
>> +  /* Quadruple precision pseudo regisers. q0-q15.  */
>> +  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
>> +    {
>> +      rawnum = (reg - num_regs - 32) * 2 + 26;
>
> Likewise,
>
>        xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) * 2);
>        double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
> 						   strlen (name_buf));
>
> additionally, we need to check gdbarch_tdep (gdbarch)->have_neon_pseudos,
>
>> +    }
>> +  /* Error bad register number.  */
>> +  else
>> +    return -1;
>> +
>> +  return rawnum;
>> +}
>
> We also need a test case, and you can extend gdb.trace/tfile-avx.exp.
> Probably, it can be renamed to gdb.trace/tracefile-pseudo-reg.exp, and
> put x86 and arm tests in it.
>

I'd like to point out that this testcase is near-useless for testing 
ax_pseudo_register_collect or pseudo_register_to_register at the moment 
- while gdb computes a mask of what registers need to be collected, 
gdbserver just ignores it and collects all registers if any register at 
all is to be collected.  In turn, gdb allows you to display the state of 
all registers, even ones not included in the mask.  In fact, the 
tfile-avx.exp test passes just fine if you change it to collect any 
unrelated register.  My commit with ax_pseudo_register_collect only made 
it work because gdb needs to have that function return success, the 
actual returned mask could just as well be wrong...

The other hook, pseudo_register_push_stack, is much easier to test - 
it's invoked when a pseudo is used in an actual agent expression, eg. if 
you use it in a tracepoint condition, or as part of the address of 
collected memory area.  However, it cannot be used on SIMD registers (at 
least on x86, I don't know much about arm), as they don't fit in an 
ULONGEST...

Matter of fact, our support for >64-bit quantities in tracepoints is 
very poor at the moment - they can only be collected wholesale when 
they're single registers or contig memory areas.  Use in expressions is 
out (if you happen to have something interesting in low 32 bits of a 
vector reg, sorry).  Likewise, stiching them together with DW_op_piece 
(or whatever that was called) also fails (see 
https://sourceware.org/bugzilla/show_bug.cgi?id=17015).  We could 
definitely use some improvement there...

Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM

[Yao Qi]

Marcin Kościelnicki <koriakin@0x04.net> writes:

> I'd like to point out that this testcase is near-useless for testing
> ax_pseudo_register_collect or pseudo_register_to_register at the
> moment - while gdb computes a mask of what registers need to be
> collected, gdbserver just ignores it and collects all registers if any
> register at all is to be collected.  In turn, gdb allows you to
> display the state of all registers, even ones not included in the
> mask.  In fact, the tfile-avx.exp test passes just fine if you change
> it to collect any unrelated register.  My commit with
> ax_pseudo_register_collect only made it work because gdb needs to have
> that function return success, the actual returned mask could just as
> well be wrong...

The usefulness I can think of is that GDB can check whether the pseudo
register exists to collect.  User may want to collect Q registers, but
they don't exist on the target.

>
> The other hook, pseudo_register_push_stack, is much easier to test - 
> it's invoked when a pseudo is used in an actual agent expression,
> eg. if you use it in a tracepoint condition, or as part of the address
> of collected memory area.  However, it cannot be used on SIMD
> registers (at least on x86, I don't know much about arm), as they
> don't fit in an ULONGEST...

The same issue on both arm and aarch64, AFAIK.

>
> Matter of fact, our support for >64-bit quantities in tracepoints is
> very poor at the moment - they can only be collected wholesale when
> they're single registers or contig memory areas.  Use in expressions
> is out (if you happen to have something interesting in low 32 bits of
> a vector reg, sorry).  Likewise, stiching them together with
> DW_op_piece (or whatever that was called) also fails (see
> https://sourceware.org/bugzilla/show_bug.cgi?id=17015).  We could
> definitely use some improvement there...

Yeah, agreed.

-- 
Yao (齐尧)

Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/15/2016 10:27 AM, Yao Qi wrote:
> Marcin Kościelnicki <koriakin@0x04.net> writes:
> 

>>
>> Matter of fact, our support for >64-bit quantities in tracepoints is
>> very poor at the moment - they can only be collected wholesale when
>> they're single registers or contig memory areas.  Use in expressions
>> is out (if you happen to have something interesting in low 32 bits of
>> a vector reg, sorry).  Likewise, stiching them together with
>> DW_op_piece (or whatever that was called) also fails (see
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17015).  We could
>> definitely use some improvement there...
> 
> Yeah, agreed.
> 

I think the that ultimate long term solution would pass actual DWARF
expressions to the target side as collect actions.  AX predates DWARF; probably
if we were starting now we'd base it on DWARF.  Then for the most part,
we'd stop getting into trouble with mapping DWARF constructs to AX.

I imagine we'd reuse gdb/dwarf2expr.c somehow, similarly to get-next-pcs,
and that we'd maybe lower/rewrite some of the the DWARF before passing
it to the target, to e.g., maybe avoid relying on debug info types or
the frame/unwind machinery.

+Gary, since given Infinity is based on DWARF expressions, it may
be Gary's already looked at factoring out gdb/dwarf2expr.c.

Not a trivial project though...

Thanks,
Pedro Alves

[PATCH v2] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

In this v2:
 Use user_reg_map_name_to_regnum.
 Add testcase. Note that this testcase needs the tracepoint patch applyed to work
 we can however keep the series order, the test will be untested until the tracepoint
 patch is present.

-

This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.

gdb/testsuite/ChangeLog:

	* gdb.trace/tfile-avx.c: Move to...
	* gdb.trace/tracefile-pseudo-reg.c: Here.
	* gdb.trace/tfile-avx.exp: Move to...
	* gdb.trace/tracefile-pseudo-reg.exp: Here.
---
 gdb/arm-tdep.c                                   | 68 +++++++++++++++++
 gdb/testsuite/gdb.trace/tfile-avx.c              | 51 -------------
 gdb/testsuite/gdb.trace/tfile-avx.exp            | 73 ------------------
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c   | 63 ++++++++++++++++
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp | 94 ++++++++++++++++++++++++
 5 files changed, 225 insertions(+), 124 deletions(-)
 delete mode 100644 gdb/testsuite/gdb.trace/tfile-avx.c
 delete mode 100644 gdb/testsuite/gdb.trace/tfile-avx.exp
 create mode 100644 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
 create mode 100644 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ccfefa8..0f6d88c 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8718,6 +8718,70 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int double_regnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+  char name_buf[4];
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs - 32) * 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  return double_regnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9379,6 +9443,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.c b/gdb/testsuite/gdb.trace/tfile-avx.c
deleted file mode 100644
index 212c556..0000000
--- a/gdb/testsuite/gdb.trace/tfile-avx.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/* This testcase is part of GDB, the GNU debugger.
-
-   Copyright 2016 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/>.  */
-
-/*
- * Test program for reading target description from tfile: collects AVX
- * registers on x86_64.
- */
-
-#include <immintrin.h>
-
-void
-dummy (void)
-{
-}
-
-static void
-end (void)
-{
-}
-
-int
-main (void)
-{
-  register __v8si a asm("ymm15") = {
-    0x12340001,
-    0x12340002,
-    0x12340003,
-    0x12340004,
-    0x12340005,
-    0x12340006,
-    0x12340007,
-    0x12340008,
-  };
-  asm volatile ("traceme: call dummy" : : "x" (a));
-  end ();
-  return 0;
-}
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.exp b/gdb/testsuite/gdb.trace/tfile-avx.exp
deleted file mode 100644
index 4c52c64..0000000
--- a/gdb/testsuite/gdb.trace/tfile-avx.exp
+++ /dev/null
@@ -1,73 +0,0 @@
-# Copyright 2016 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 { ! [is_amd64_regs_target] } {
-    verbose "Skipping tfile AVX test (target is not x86_64)."
-    return
-}
-
-load_lib "trace-support.exp"
-
-standard_testfile
-
-if {[prepare_for_testing $testfile.exp $testfile $srcfile \
-     [list debug additional_flags=-mavx]]} {
-    return -1
-}
-
-if ![runto_main] {
-    fail "Can't run to main to check for trace support"
-    return -1
-}
-
-if ![gdb_target_supports_trace] {
-    unsupported "target does not support trace"
-    return -1
-}
-
-gdb_test_multiple "print \$ymm15" "check for AVX support" {
-    -re " = void.*$gdb_prompt $" {
-	verbose "Skipping tfile AVX test (target doesn't support AVX)."
-	return
-    }
-    -re " = \\{.*}.*$gdb_prompt $" {
-	# All is well.
-    }
-}
-
-gdb_test "trace traceme" ".*"
-
-gdb_trace_setactions "set actions for tracepoint" "" \
-	"collect \$ymm15" "^$"
-
-gdb_breakpoint "end"
-
-gdb_test_no_output "tstart"
-
-gdb_test "continue" ".*Breakpoint $decimal, end .*"
-
-set tracefile [standard_output_file ${testfile}]
-
-# Save trace frames to tfile.
-gdb_test "tsave ${tracefile}.tf" \
-    "Trace data saved to file '${tracefile}.tf'.*" \
-    "save tfile trace file"
-
-# Change target to tfile.
-gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
-  "A program is being debugged already.  Kill it. .y or n. $" "y"
-
-gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
-
-gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
new file mode 100644
index 0000000..e8f66f7
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
@@ -0,0 +1,63 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 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/>.  */
+
+/*
+ * Test program for reading target description from tfile: collects AVX
+ * registers on x86_64.
+ */
+
+#if (defined __x86_64__)
+#include <immintrin.h>
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+#include <arm_neon.h>
+#endif
+
+void
+dummy (void)
+{
+}
+
+static void
+end (void)
+{
+}
+
+int
+main (void)
+{
+#if (defined __x86_64__)
+  register __v8si a asm("xmm15") = {
+    0x12340001,
+    0x12340002,
+    0x12340003,
+    0x12340004,
+    0x12340005,
+    0x12340006,
+    0x12340007,
+    0x12340008,
+  };
+  asm volatile ("traceme: call dummy" : : "x" (a));
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+  register uint32_t a asm("s5") = {
+    0x2
+  };
+  asm volatile ("traceme: bl dummy" : : "x" (a));
+#endif
+
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
new file mode 100644
index 0000000..12a2740
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
@@ -0,0 +1,94 @@
+# Copyright 2016 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 { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
+    verbose "Skipping tracefile pseudo register tests, target is not supported."
+    return
+}
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if { [is_amd64_regs_target] } {
+ set add_flags "-mavx"
+} elseif { [istarget "arm*-*-*"] } {
+ set add_flags "-mfpu=neon"
+}
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile \
+     [list debug additional_flags=$add_flags]]} {
+    return -1
+}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+if { [is_amd64_regs_target] } {
+    set reg "\$ymm15"
+    set reg_message "check for AVX support"
+} elseif { [istarget "arm*-*-*"] } {
+    set reg "\$s5"
+    set reg_message "check for Neon support"
+}
+
+gdb_test_multiple "print $reg" $reg_message {
+    -re " = void.*$gdb_prompt $" {
+	verbose "Skipping tracefile pseudo register tests, target is not supported."
+	return
+    }
+    -re " = \\{.*}.*$gdb_prompt $" {
+	# All is well.
+    }
+    -re " = 0.*$gdb_prompt $" {
+	# All is well.
+    }
+}
+
+gdb_test "trace traceme" ".*"
+
+gdb_trace_setactions "set actions for tracepoint" "" \
+	"collect $reg" "^$"
+
+gdb_breakpoint "end"
+
+gdb_test_no_output "tstart"
+
+gdb_test "continue" ".*Breakpoint $decimal, end .*"
+
+set tracefile [standard_output_file ${testfile}]
+
+# Save trace frames to tfile.
+gdb_test "tsave ${tracefile}.tf" \
+    "Trace data saved to file '${tracefile}.tf'.*" \
+    "save tfile trace file"
+
+# Change target to tfile.
+gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
+  "A program is being debugged already.  Kill it. .y or n. $" "y"
+
+gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
+
+if { [is_amd64_regs_target] } {
+    gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+} elseif { [istarget "arm*-*-*"] } {
+    gdb_test "print \$s5" "2.80259693e-45"
+}
-- 
2.6.4

Re: [PATCH v2] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Antoine Tremblay writes:

> In this v2:
>  Use user_reg_map_name_to_regnum.

Oops this is actually wrong.

I forgot that the reason I had it as :

+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      rawnum = (reg - num_regs) / 2 + 26;
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      rawnum = (reg - num_regs - 32) * 2 + 26;
+    }

In order to map from the GDB internal regnum to a real regnum to be used
by GDBServer in gdbserver/ax.c as :

case gdb_agent_op_reg:
	  /* Flush the cached stack top.  */
	  stack[sp++] = top;
	  arg = aexpr->bytes[pc++];
	  arg = (arg << 8) + aexpr->bytes[pc++];
	  {
	    int regnum = arg;
	    struct regcache *regcache = ctx->regcache;

	    switch (register_size (regcache->tdesc, regnum))

Here regnum is expected to be the real register number.

As example if I get arm_pseudo_register_to_register with register 109

This is actually register 35, but user_reg_map_name_to_regnum will still
map it as a GDB internal register number 67.

It's 32 regs off because user_reg_map_name_to_regnum iterates over all
regs as num_regs + pseudo_regs, and in this case num_regs is 91 since in
arm.h the regnum enums gives a GDB internal register number so that all
registers numbers are unique independantly from the arch in use.

Maybe there's a better way to map these internal gdb registers to actual
register numbers ? Yao?

[PATCH v3] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

In this v3:
* Use gdbarch_remote_register_number to get the remote/tsec register number
Thanks to Pedro for pointing me in the right direction.
-

This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.

gdb/testsuite/ChangeLog:

	* gdb.trace/tfile-avx.c: Move to...
	* gdb.trace/tracefile-pseudo-reg.c: Here.
	* gdb.trace/tfile-avx.exp: Move to...
	* gdb.trace/tracefile-pseudo-reg.exp: Here.
---
 gdb/arm-tdep.c                                   | 71 ++++++++++++++++++
 gdb/testsuite/gdb.trace/tfile-avx.c              | 53 -------------
 gdb/testsuite/gdb.trace/tfile-avx.exp            | 73 ------------------
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c   | 65 ++++++++++++++++
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp | 94 ++++++++++++++++++++++++
 5 files changed, 230 insertions(+), 126 deletions(-)
 delete mode 100644 gdb/testsuite/gdb.trace/tfile-avx.c
 delete mode 100644 gdb/testsuite/gdb.trace/tfile-avx.exp
 create mode 100644 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
 create mode 100644 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ccfefa8..1728de1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8718,6 +8718,73 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int double_regnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+  char name_buf[4];
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs - 32) * 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  /* Get the remote/tdesc register number.  */
+  double_regnum = gdbarch_remote_register_number (gdbarch, double_regnum);
+
+  return double_regnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9379,6 +9446,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.c b/gdb/testsuite/gdb.trace/tfile-avx.c
deleted file mode 100644
index 3cc3ec0..0000000
--- a/gdb/testsuite/gdb.trace/tfile-avx.c
+++ /dev/null
@@ -1,53 +0,0 @@
-/* This testcase is part of GDB, the GNU debugger.
-
-   Copyright 2016 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/>.  */
-
-/*
- * Test program for reading target description from tfile: collects AVX
- * registers on x86_64.
- */
-
-#include <immintrin.h>
-
-void
-dummy (void)
-{
-}
-
-static void
-end (void)
-{
-}
-
-int
-main (void)
-{
-  /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
-     than 4.9 doesn't recognize "ymm15" as a valid register name.  */
-  register __v8si a asm("xmm15") = {
-    0x12340001,
-    0x12340002,
-    0x12340003,
-    0x12340004,
-    0x12340005,
-    0x12340006,
-    0x12340007,
-    0x12340008,
-  };
-  asm volatile ("traceme: call dummy" : : "x" (a));
-  end ();
-  return 0;
-}
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.exp b/gdb/testsuite/gdb.trace/tfile-avx.exp
deleted file mode 100644
index 4c52c64..0000000
--- a/gdb/testsuite/gdb.trace/tfile-avx.exp
+++ /dev/null
@@ -1,73 +0,0 @@
-# Copyright 2016 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 { ! [is_amd64_regs_target] } {
-    verbose "Skipping tfile AVX test (target is not x86_64)."
-    return
-}
-
-load_lib "trace-support.exp"
-
-standard_testfile
-
-if {[prepare_for_testing $testfile.exp $testfile $srcfile \
-     [list debug additional_flags=-mavx]]} {
-    return -1
-}
-
-if ![runto_main] {
-    fail "Can't run to main to check for trace support"
-    return -1
-}
-
-if ![gdb_target_supports_trace] {
-    unsupported "target does not support trace"
-    return -1
-}
-
-gdb_test_multiple "print \$ymm15" "check for AVX support" {
-    -re " = void.*$gdb_prompt $" {
-	verbose "Skipping tfile AVX test (target doesn't support AVX)."
-	return
-    }
-    -re " = \\{.*}.*$gdb_prompt $" {
-	# All is well.
-    }
-}
-
-gdb_test "trace traceme" ".*"
-
-gdb_trace_setactions "set actions for tracepoint" "" \
-	"collect \$ymm15" "^$"
-
-gdb_breakpoint "end"
-
-gdb_test_no_output "tstart"
-
-gdb_test "continue" ".*Breakpoint $decimal, end .*"
-
-set tracefile [standard_output_file ${testfile}]
-
-# Save trace frames to tfile.
-gdb_test "tsave ${tracefile}.tf" \
-    "Trace data saved to file '${tracefile}.tf'.*" \
-    "save tfile trace file"
-
-# Change target to tfile.
-gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
-  "A program is being debugged already.  Kill it. .y or n. $" "y"
-
-gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
-
-gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
new file mode 100644
index 0000000..473d805
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 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/>.  */
+
+/*
+ * Test program for reading target description from tfile: collects AVX
+ * registers on x86_64.
+ */
+
+#if (defined __x86_64__)
+#include <immintrin.h>
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+#include <arm_neon.h>
+#endif
+
+void
+dummy (void)
+{
+}
+
+static void
+end (void)
+{
+}
+
+int
+main (void)
+{
+  /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
+     than 4.9 doesn't recognize "ymm15" as a valid register name.  */
+#if (defined __x86_64__)
+  register __v8si a asm("xmm15") = {
+    0x12340001,
+    0x12340002,
+    0x12340003,
+    0x12340004,
+    0x12340005,
+    0x12340006,
+    0x12340007,
+    0x12340008,
+  };
+  asm volatile ("traceme: call dummy" : : "x" (a));
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+  register uint32_t a asm("s5") = {
+    0x2
+  };
+  asm volatile ("traceme: bl dummy" : : "x" (a));
+#endif
+
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
new file mode 100644
index 0000000..12a2740
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
@@ -0,0 +1,94 @@
+# Copyright 2016 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 { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
+    verbose "Skipping tracefile pseudo register tests, target is not supported."
+    return
+}
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if { [is_amd64_regs_target] } {
+ set add_flags "-mavx"
+} elseif { [istarget "arm*-*-*"] } {
+ set add_flags "-mfpu=neon"
+}
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile \
+     [list debug additional_flags=$add_flags]]} {
+    return -1
+}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+if { [is_amd64_regs_target] } {
+    set reg "\$ymm15"
+    set reg_message "check for AVX support"
+} elseif { [istarget "arm*-*-*"] } {
+    set reg "\$s5"
+    set reg_message "check for Neon support"
+}
+
+gdb_test_multiple "print $reg" $reg_message {
+    -re " = void.*$gdb_prompt $" {
+	verbose "Skipping tracefile pseudo register tests, target is not supported."
+	return
+    }
+    -re " = \\{.*}.*$gdb_prompt $" {
+	# All is well.
+    }
+    -re " = 0.*$gdb_prompt $" {
+	# All is well.
+    }
+}
+
+gdb_test "trace traceme" ".*"
+
+gdb_trace_setactions "set actions for tracepoint" "" \
+	"collect $reg" "^$"
+
+gdb_breakpoint "end"
+
+gdb_test_no_output "tstart"
+
+gdb_test "continue" ".*Breakpoint $decimal, end .*"
+
+set tracefile [standard_output_file ${testfile}]
+
+# Save trace frames to tfile.
+gdb_test "tsave ${tracefile}.tf" \
+    "Trace data saved to file '${tracefile}.tf'.*" \
+    "save tfile trace file"
+
+# Change target to tfile.
+gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
+  "A program is being debugged already.  Kill it. .y or n. $" "y"
+
+gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
+
+if { [is_amd64_regs_target] } {
+    gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+} elseif { [istarget "arm*-*-*"] } {
+    gdb_test "print \$s5" "2.80259693e-45"
+}
-- 
2.6.4

[PATCH v4] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

In this v4:
* Use patch -M to reflect the test file rename
-
This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.

gdb/testsuite/ChangeLog:

	* gdb.trace/tfile-avx.c: Move to...
	* gdb.trace/tracefile-pseudo-reg.c: Here.
	* gdb.trace/tfile-avx.exp: Move to...
	* gdb.trace/tracefile-pseudo-reg.exp: Here.
---
 gdb/arm-tdep.c                                     | 71 ++++++++++++++++++++++
 .../{tfile-avx.c => tracefile-pseudo-reg.c}        | 12 ++++
 .../{tfile-avx.exp => tracefile-pseudo-reg.exp}    | 35 ++++++++---
 3 files changed, 111 insertions(+), 7 deletions(-)
 rename gdb/testsuite/gdb.trace/{tfile-avx.c => tracefile-pseudo-reg.c} (80%)
 rename gdb/testsuite/gdb.trace/{tfile-avx.exp => tracefile-pseudo-reg.exp} (65%)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ccfefa8..1728de1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8718,6 +8718,73 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int double_regnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+  char name_buf[4];
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs - 32) * 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  /* Get the remote/tdesc register number.  */
+  double_regnum = gdbarch_remote_register_number (gdbarch, double_regnum);
+
+  return double_regnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9379,6 +9446,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
similarity index 80%
rename from gdb/testsuite/gdb.trace/tfile-avx.c
rename to gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
index 3cc3ec0..473d805 100644
--- a/gdb/testsuite/gdb.trace/tfile-avx.c
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
@@ -20,7 +20,11 @@
  * registers on x86_64.
  */
 
+#if (defined __x86_64__)
 #include <immintrin.h>
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+#include <arm_neon.h>
+#endif
 
 void
 dummy (void)
@@ -37,6 +41,7 @@ main (void)
 {
   /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
      than 4.9 doesn't recognize "ymm15" as a valid register name.  */
+#if (defined __x86_64__)
   register __v8si a asm("xmm15") = {
     0x12340001,
     0x12340002,
@@ -48,6 +53,13 @@ main (void)
     0x12340008,
   };
   asm volatile ("traceme: call dummy" : : "x" (a));
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+  register uint32_t a asm("s5") = {
+    0x2
+  };
+  asm volatile ("traceme: bl dummy" : : "x" (a));
+#endif
+
   end ();
   return 0;
 }
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
similarity index 65%
rename from gdb/testsuite/gdb.trace/tfile-avx.exp
rename to gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
index 4c52c64..12a2740 100644
--- a/gdb/testsuite/gdb.trace/tfile-avx.exp
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
@@ -12,8 +12,8 @@
 # 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 { ! [is_amd64_regs_target] } {
-    verbose "Skipping tfile AVX test (target is not x86_64)."
+if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
+    verbose "Skipping tracefile pseudo register tests, target is not supported."
     return
 }
 
@@ -21,8 +21,14 @@ load_lib "trace-support.exp"
 
 standard_testfile
 
+if { [is_amd64_regs_target] } {
+ set add_flags "-mavx"
+} elseif { [istarget "arm*-*-*"] } {
+ set add_flags "-mfpu=neon"
+}
+
 if {[prepare_for_testing $testfile.exp $testfile $srcfile \
-     [list debug additional_flags=-mavx]]} {
+     [list debug additional_flags=$add_flags]]} {
     return -1
 }
 
@@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
     return -1
 }
 
-gdb_test_multiple "print \$ymm15" "check for AVX support" {
+if { [is_amd64_regs_target] } {
+    set reg "\$ymm15"
+    set reg_message "check for AVX support"
+} elseif { [istarget "arm*-*-*"] } {
+    set reg "\$s5"
+    set reg_message "check for Neon support"
+}
+
+gdb_test_multiple "print $reg" $reg_message {
     -re " = void.*$gdb_prompt $" {
-	verbose "Skipping tfile AVX test (target doesn't support AVX)."
+	verbose "Skipping tracefile pseudo register tests, target is not supported."
 	return
     }
     -re " = \\{.*}.*$gdb_prompt $" {
 	# All is well.
     }
+    -re " = 0.*$gdb_prompt $" {
+	# All is well.
+    }
 }
 
 gdb_test "trace traceme" ".*"
 
 gdb_trace_setactions "set actions for tracepoint" "" \
-	"collect \$ymm15" "^$"
+	"collect $reg" "^$"
 
 gdb_breakpoint "end"
 
@@ -70,4 +87,8 @@ gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
 
 gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
 
-gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+if { [is_amd64_regs_target] } {
+    gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+} elseif { [istarget "arm*-*-*"] } {
+    gdb_test "print \$s5" "2.80259693e-45"
+}
-- 
2.6.4

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/19/2016 07:28 PM, Antoine Tremblay wrote:

> +/* Map the pseudo register number REG to the proper register number.  */
> +
> +static int
> +arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
> +{

> +  /* Get the remote/tdesc register number.  */
> +  double_regnum = gdbarch_remote_register_number (gdbarch, double_regnum);

Hmm, I don't think it should be the responsibility of this function to
map gdb to remote numbers though.  Here I think we should just map
gdb pseudo to gdb raw.

> +
> +  return double_regnum;
> +}
> +
> +/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
> +
> +static int
> +arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
> +				struct agent_expr *ax, int reg)
> +{
> +  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
> +
> +  /* Error.  */
> +  if (rawnum < 0)
> +    return 1;
> +
> +  ax_reg_mask (ax, rawnum);

Hmm, seems to me that gdb raw -> target raw mapping should be
either here, or perhaps even in ax_reg / ax_reg_mask?

Consider the case of an expression requiring the collection of
a _raw_ register, thus not even reaching here.  Looking at
ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
is computing actually wrong for the target, and things just happen
to work because gdbserver ignores them and always collects all registers?

Thanks,
Pedro Alves

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

> On 02/19/2016 07:28 PM, Antoine Tremblay wrote:
>
>> +/* Map the pseudo register number REG to the proper register number.  */
>> +
>> +static int
>> +arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
>> +{
>
>> +  /* Get the remote/tdesc register number.  */
>> +  double_regnum = gdbarch_remote_register_number (gdbarch, double_regnum);
>
> Hmm, I don't think it should be the responsibility of this function to
> map gdb to remote numbers though.  Here I think we should just map
> gdb pseudo to gdb raw.

Yes I had created that function for arm_ax_pseudo_register_* functions
but yes maybe it would be better at a lower level and allow this
function to be used by something else.
>
>> +
>> +  return double_regnum;
>> +}
>> +
>> +/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
>> +
>> +static int
>> +arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
>> +				struct agent_expr *ax, int reg)
>> +{
>> +  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
>> +
>> +  /* Error.  */
>> +  if (rawnum < 0)
>> +    return 1;
>> +
>> +  ax_reg_mask (ax, rawnum);
>
> Hmm, seems to me that gdb raw -> target raw mapping should be
> either here, or perhaps even in ax_reg / ax_reg_mask?
>

Yes now that you mention it it would make sense in ax_reg/reg_mask.

> Consider the case of an expression requiring the collection of
> a _raw_ register, thus not even reaching here.  Looking at
> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
> is computing actually wrong for the target, and things just happen
> to work because gdbserver ignores them and always collects all registers?
>
I would assume so indeed!

I'll make this a small series send another patch to apply prior to this
one with the change to ax_reg, ax_reg_mask.

Thanks,
Antoine

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Hmm, I don't think it should be the responsibility of this function to
> map gdb to remote numbers though.  Here I think we should just map
> gdb pseudo to gdb raw.

Yes, I agree.  Each backend should map pseudo to gdb raw, and the common
code should map the gdb raw to target raw number.

-- 
Yao (齐尧)

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

> Hmm, seems to me that gdb raw -> target raw mapping should be
> either here, or perhaps even in ax_reg / ax_reg_mask?
>
> Consider the case of an expression requiring the collection of
> a _raw_ register, thus not even reaching here.  Looking at
> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
> is computing actually wrong for the target, and things just happen
> to work because gdbserver ignores them and always collects all registers?
>

Is there a good reason gdbserver actually ignores that ?

It seems all the code is there for it to consider it on gdb's
side. encode_actions, stringify_collection_list etc... The only thing
missing seems to be gdbserver interpretation of the R action.

While looking at fixing this for all the archs involved it would be
much simpler to test if gdbserver would make use of it.

As it is now, I'm concerned that calling gdbarch_remote_register_number
in ax_reg, ax_mask_reg could break things if the arch already considers
the gdb raw -> target raw mapping like s390 and x86 do already (I'm not
100% sure the mapping is already ok)? And that it is set to use tdesc
registers (so that gdbarch_remote_register_number maps to
tdesc_remote_register).

Thanks,
Antoine

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

>> +
>> +  return double_regnum;
>> +}
>> +
>> +/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
>> +
>> +static int
>> +arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
>> +				struct agent_expr *ax, int reg)
>> +{
>> +  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
>> +
>> +  /* Error.  */
>> +  if (rawnum < 0)
>> +    return 1;
>> +
>> +  ax_reg_mask (ax, rawnum);
>
> Hmm, seems to me that gdb raw -> target raw mapping should be
> either here, or perhaps even in ax_reg / ax_reg_mask?
>

After more investigation, this can't be in ax_reg / ax_reg_mask for
pseudo registers as this function is solely reponsible to encode the
right number here.

> Consider the case of an expression requiring the collection of
> a _raw_ register, thus not even reaching here.  Looking at
> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
> is computing actually wrong for the target, and things just happen
> to work because gdbserver ignores them and always collects all registers?

However yes it should be in ax_reg/ax_reg_mask for non-pseudo registers,
but this is not the objective of this patch, I suggest that such a
change be the subject of another patch maybe coupled with better
gdbserver handling of the R action.

I will send a v5 with the ax_pseudo_register_collect inside the
arm_ax_pseudo_register_collect/arm_ax_pseudo_register_push stack function.

[PATCH v5] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

In this v5:
* Moved remote register fetch.
-
This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.

gdb/testsuite/ChangeLog:

	* gdb.trace/tfile-avx.c: Move to...
	* gdb.trace/tracefile-pseudo-reg.c: Here.
	* gdb.trace/tfile-avx.exp: Move to...
	* gdb.trace/tracefile-pseudo-reg.exp: Here.
---
 gdb/arm-tdep.c                                     | 74 ++++++++++++++++++++++
 .../{tfile-avx.c => tracefile-pseudo-reg.c}        | 12 ++++
 .../{tfile-avx.exp => tracefile-pseudo-reg.exp}    | 35 ++++++++--
 3 files changed, 114 insertions(+), 7 deletions(-)
 rename gdb/testsuite/gdb.trace/{tfile-avx.c => tracefile-pseudo-reg.c} (80%)
 rename gdb/testsuite/gdb.trace/{tfile-avx.exp => tracefile-pseudo-reg.exp} (65%)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ccfefa8..cca0812 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8718,6 +8718,76 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int double_regnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+  char name_buf[4];
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs - 32) * 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  return double_regnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  /* Get the remote/tdesc register number.  */
+  rawnum = gdbarch_remote_register_number (gdbarch, rawnum);
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  /* Get the remote/tdesc register number.  */
+  rawnum = gdbarch_remote_register_number (gdbarch, rawnum);
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9379,6 +9449,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
similarity index 80%
rename from gdb/testsuite/gdb.trace/tfile-avx.c
rename to gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
index 3cc3ec0..473d805 100644
--- a/gdb/testsuite/gdb.trace/tfile-avx.c
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
@@ -20,7 +20,11 @@
  * registers on x86_64.
  */
 
+#if (defined __x86_64__)
 #include <immintrin.h>
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+#include <arm_neon.h>
+#endif
 
 void
 dummy (void)
@@ -37,6 +41,7 @@ main (void)
 {
   /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
      than 4.9 doesn't recognize "ymm15" as a valid register name.  */
+#if (defined __x86_64__)
   register __v8si a asm("xmm15") = {
     0x12340001,
     0x12340002,
@@ -48,6 +53,13 @@ main (void)
     0x12340008,
   };
   asm volatile ("traceme: call dummy" : : "x" (a));
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+  register uint32_t a asm("s5") = {
+    0x2
+  };
+  asm volatile ("traceme: bl dummy" : : "x" (a));
+#endif
+
   end ();
   return 0;
 }
diff --git a/gdb/testsuite/gdb.trace/tfile-avx.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
similarity index 65%
rename from gdb/testsuite/gdb.trace/tfile-avx.exp
rename to gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
index 4c52c64..12a2740 100644
--- a/gdb/testsuite/gdb.trace/tfile-avx.exp
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
@@ -12,8 +12,8 @@
 # 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 { ! [is_amd64_regs_target] } {
-    verbose "Skipping tfile AVX test (target is not x86_64)."
+if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
+    verbose "Skipping tracefile pseudo register tests, target is not supported."
     return
 }
 
@@ -21,8 +21,14 @@ load_lib "trace-support.exp"
 
 standard_testfile
 
+if { [is_amd64_regs_target] } {
+ set add_flags "-mavx"
+} elseif { [istarget "arm*-*-*"] } {
+ set add_flags "-mfpu=neon"
+}
+
 if {[prepare_for_testing $testfile.exp $testfile $srcfile \
-     [list debug additional_flags=-mavx]]} {
+     [list debug additional_flags=$add_flags]]} {
     return -1
 }
 
@@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
     return -1
 }
 
-gdb_test_multiple "print \$ymm15" "check for AVX support" {
+if { [is_amd64_regs_target] } {
+    set reg "\$ymm15"
+    set reg_message "check for AVX support"
+} elseif { [istarget "arm*-*-*"] } {
+    set reg "\$s5"
+    set reg_message "check for Neon support"
+}
+
+gdb_test_multiple "print $reg" $reg_message {
     -re " = void.*$gdb_prompt $" {
-	verbose "Skipping tfile AVX test (target doesn't support AVX)."
+	verbose "Skipping tracefile pseudo register tests, target is not supported."
 	return
     }
     -re " = \\{.*}.*$gdb_prompt $" {
 	# All is well.
     }
+    -re " = 0.*$gdb_prompt $" {
+	# All is well.
+    }
 }
 
 gdb_test "trace traceme" ".*"
 
 gdb_trace_setactions "set actions for tracepoint" "" \
-	"collect \$ymm15" "^$"
+	"collect $reg" "^$"
 
 gdb_breakpoint "end"
 
@@ -70,4 +87,8 @@ gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
 
 gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
 
-gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+if { [is_amd64_regs_target] } {
+    gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+} elseif { [istarget "arm*-*-*"] } {
+    gdb_test "print \$s5" "2.80259693e-45"
+}
-- 
2.6.4

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> Hi Antoine,
>
>> The reason for this is that the target's stack pointer is unavailable
>> when examining the trace buffer.  What we are seeing is due to the
>> 'tfind' command creating a sentinel frame and unwinding it.  If an
>> exception is thrown, we are left with the sentinel frame being displayed
>> at level #-1.  The exception is thrown when the prologue unwinder tries
>> to read the stack pointer to construct an ID for the frame.
>>
>> This patch fixes this and similar issues by making all the arm unwinders
>> catch NOT_AVAILABLE_ERROR exceptions when either register or memory is
>> unreadable and report back to the frame core code with UNWIND_UNAVAILABLE.
>>
>> Note this commit log adapted from 7dfa3edc033c443036d9f2a3e01120f7fb54f498
>> which fixed a similar issue for aarch64.
>
> It is right to follow aarch64 patch, but I am wondering whether we can
> do it better.
>
> Nowadays, the unwind termination due to unavailable memory is handled in
> unwinders in each arch backend.  However, as we support more and more
> arch for tracepoint, can we handle the unwind termination in target
> independent code?
>
> The initial work of unwind termination due to unavailable memory was
> done by Pedro https://www.sourceware.org/ml/gdb-patches/2011-02/msg00611.html
> in a way that each unwinder was taught to terminate with
> UNWIND_UNAVAILABLE.  At that moment, only x86 supports tracepoint, so it
> was reasonable to handle UNWIND_UNAVAILABLE inside unwinders of one arch.  Now,
> the situation changes, because we have more and more arch need
> tracepoint support, if we can handle UNWIND_UNAVAILABLE in the callers
> of each unwinder, each unwinder doesn't have to worry about the
> unavailable at all.  In fact, GDB has done that way when calling unwinder->sniffer,
> in frame_unwind_try_unwinder
>
>   TRY
>     {
>       res = unwinder->sniffer (unwinder, this_frame, this_cache);
>     }
>   CATCH (ex, RETURN_MASK_ERROR)
>     {
>       if (ex.error == NOT_AVAILABLE_ERROR)
> 	{
> 	  /* This usually means that not even the PC is available,
> 	     thus most unwinders aren't able to determine if they're
> 	     the best fit.  Keep trying.  Fallback prologue unwinders
> 	     should always accept the frame.  */
> 	  do_cleanups (old_cleanup);
> 	  return 0;
> 	}
>       throw_exception (ex);
>     }
>   END_CATCH
>
> we can wrap methods of 'struct frame_unwind' with try/catch, and handle
> NOT_AVAILABLE_ERROR properly.  In this way, each unwinder doesn't have
> to worry about unavailable memory at all.
>
> Pedro, what do you think?  Did you try this approach in the rest of 9
> different ways :) (you said you "implemented this differently in about
> 10 different ways" in your email) ?

Ping, Pedro ?

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/22/2016 04:51 PM, Antoine Tremblay wrote:
> 
> Pedro Alves writes:
> 
>> Hmm, seems to me that gdb raw -> target raw mapping should be
>> either here, or perhaps even in ax_reg / ax_reg_mask?
>>
>> Consider the case of an expression requiring the collection of
>> a _raw_ register, thus not even reaching here.  Looking at
>> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
>> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
>> is computing actually wrong for the target, and things just happen
>> to work because gdbserver ignores them and always collects all registers?
>>
> 
> Is there a good reason gdbserver actually ignores that ?

I don't recall any, other than collecting everything is expedient
and good enough...

> 
> It seems all the code is there for it to consider it on gdb's
> side. encode_actions, stringify_collection_list etc... The only thing
> missing seems to be gdbserver interpretation of the R action.

Right.  Obviously you'd need to consider how to represent the
partial register set in the trace frame as well.  Just marking
some registers as unavailable while still crafting a whole register
block in the trace buffer is pointless, obviously.

> 
> While looking at fixing this for all the archs involved it would be
> much simpler to test if gdbserver would make use of it.
> 
> As it is now, I'm concerned that calling gdbarch_remote_register_number
> in ax_reg, ax_mask_reg could break things if the arch already considers
> the gdb raw -> target raw mapping like s390 and x86 do already (I'm not
> 100% sure the mapping is already ok)?

WDTM?  Where do they do this already?


 And that it is set to use tdesc
> registers (so that gdbarch_remote_register_number maps to
> tdesc_remote_register).

Thanks,
Pedro Alves

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/23/2016 07:34 PM, Antoine Tremblay wrote:
> 
> Pedro Alves writes:
> 
>>> +
>>> +  return double_regnum;
>>> +}
>>> +
>>> +/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
>>> +
>>> +static int
>>> +arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
>>> +				struct agent_expr *ax, int reg)
>>> +{
>>> +  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
>>> +
>>> +  /* Error.  */
>>> +  if (rawnum < 0)
>>> +    return 1;
>>> +
>>> +  ax_reg_mask (ax, rawnum);
>>
>> Hmm, seems to me that gdb raw -> target raw mapping should be
>> either here, or perhaps even in ax_reg / ax_reg_mask?
>>
> 
> After more investigation, this can't be in ax_reg / ax_reg_mask for
> pseudo registers as this function is solely reponsible to encode the
> right number here.

I don't follow.

ax_reg / ax_reg_mask today obviously work with gdb numbers:

/* Add register REG to the register mask for expression AX.  */
void
ax_reg_mask (struct agent_expr *ax, int reg)
{
  if (reg >= gdbarch_num_regs (ax->gdbarch))
    {
      /* This is a pseudo-register.  */
      if (!gdbarch_ax_pseudo_register_collect_p (ax->gdbarch))
	error (_("'%s' is a pseudo-register; "
		 "GDB cannot yet trace its contents."),
	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
      if (gdbarch_ax_pseudo_register_collect (ax->gdbarch, ax, reg))
	error (_("Trace '%s' failed."),
	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
    }
  else
    ...


This is comparing gdb-side num_regs, and calling
gdbarch_ax_pseudo_register_collect, whose implementations expect
gdb register numbers.  And it calls user_reg_map_regnum_to_name,
which works with gdb register numbers.  Etc.

So it seems to me that we need to make ax_reg and ax_reg_mask
convert gdb -> remote numbers in their else branches.

> 
>> Consider the case of an expression requiring the collection of
>> a _raw_ register, thus not even reaching here.  Looking at
>> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
>> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
>> is computing actually wrong for the target, and things just happen
>> to work because gdbserver ignores them and always collects all registers?
> 
> However yes it should be in ax_reg/ax_reg_mask for non-pseudo registers,
> but this is not the objective of this patch, I suggest that such a
> change be the subject of another patch

Sure, but in that case, drop the gdb -> remote conversion entirely.
If with that things don't work for arm, let's fix ax_reg/ax_reg_mask
_first_.

> maybe coupled with better gdbserver handling of the R action.

I think this coupling would be a mistake.  This can be handled
independently, if at all.

> 
> I will send a v5 with the ax_pseudo_register_collect inside the
> arm_ax_pseudo_register_collect/arm_ax_pseudo_register_push stack function.

Thanks,
Pedro Alves

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Marcin Kościelnicki]

On 24/02/16 19:11, Pedro Alves wrote:
> On 02/22/2016 04:51 PM, Antoine Tremblay wrote:
>>
>> Pedro Alves writes:
>>
>>> Hmm, seems to me that gdb raw -> target raw mapping should be
>>> either here, or perhaps even in ax_reg / ax_reg_mask?
>>>
>>> Consider the case of an expression requiring the collection of
>>> a _raw_ register, thus not even reaching here.  Looking at
>>> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
>>> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
>>> is computing actually wrong for the target, and things just happen
>>> to work because gdbserver ignores them and always collects all registers?
>>>
>>
>> Is there a good reason gdbserver actually ignores that ?
>
> I don't recall any, other than collecting everything is expedient
> and good enough...
>
>>
>> It seems all the code is there for it to consider it on gdb's
>> side. encode_actions, stringify_collection_list etc... The only thing
>> missing seems to be gdbserver interpretation of the R action.
>
> Right.  Obviously you'd need to consider how to represent the
> partial register set in the trace frame as well.  Just marking
> some registers as unavailable while still crafting a whole register
> block in the trace buffer is pointless, obviously.
>
>>
>> While looking at fixing this for all the archs involved it would be
>> much simpler to test if gdbserver would make use of it.
>>
>> As it is now, I'm concerned that calling gdbarch_remote_register_number
>> in ax_reg, ax_mask_reg could break things if the arch already considers
>> the gdb raw -> target raw mapping like s390 and x86 do already (I'm not
>> 100% sure the mapping is already ok)?
>
> WDTM?  Where do they do this already?

FWIW, I failed to look at the numbering used when I wrote the x86 and 
s390 ax functions, so they're most likely wrong (I just copied the 
regnum computation logic from pseudo_read/write, which uses gdb 
numbers).  s390 hasn't landed yet, so it's only x86 that you'd have to 
fix now (and mips, I think, but that doesn't support tracepoints yet...).

Testing this is possible if you write some conditions that involve 
reading pseudo-registers (since ax_pseudo_register_push_stack will be 
called), the problem is that I only implemented 
ax_pseudo_register_collect for x86...

Are you going to make some higher-level patch that will magically fix it 
for my s390 patch, or do I have to fix that on my own?
>
>
>   And that it is set to use tdesc
>> registers (so that gdbarch_remote_register_number maps to
>> tdesc_remote_register).
>
> Thanks,
> Pedro Alves
>

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/24/2016 06:20 PM, Marcin Kościelnicki wrote:
> On 24/02/16 19:11, Pedro Alves wrote:
>> On 02/22/2016 04:51 PM, Antoine Tremblay wrote:
>>>

>>> While looking at fixing this for all the archs involved it would be
>>> much simpler to test if gdbserver would make use of it.
>>>
>>> As it is now, I'm concerned that calling gdbarch_remote_register_number
>>> in ax_reg, ax_mask_reg could break things if the arch already considers
>>> the gdb raw -> target raw mapping like s390 and x86 do already (I'm not
>>> 100% sure the mapping is already ok)?
>>
>> WDTM?  Where do they do this already?
> 
> FWIW, I failed to look at the numbering used when I wrote the x86 and 
> s390 ax functions, so they're most likely wrong (I just copied the 
> regnum computation logic from pseudo_read/write, which uses gdb 
> numbers).  s390 hasn't landed yet, so it's only x86 that you'd have to 
> fix now (and mips, I think, but that doesn't support tracepoints yet...).

I don't think there's anything that needs fixing in the i386 implementation.

The x86 implementation maps gdb pseudo register numbers to whatever
raw gdb registers back the former up, like:

      ax_reg_mask (ax, I387_FSTAT_REGNUM (tdep));

That OK.

The trouble is that in the end we send gdb numbers to the target in the
ax, instead of tdesc/remote numbers.

We never noticed because gdbserver always collects all raw registers
anyway.

Seems to me that the fix is to make ax_reg / ax_reg_mask take gdb raw
numbers as input (as it does today), and then make it map those to
tdesc/remote number just before it puts the reg number in the agent
expression bytecode / reg mask.  And that covers all archs.

> 
> Testing this is possible if you write some conditions that involve 
> reading pseudo-registers (since ax_pseudo_register_push_stack will be 
> called), the problem is that I only implemented 
> ax_pseudo_register_collect for x86...
> 
> Are you going to make some higher-level patch that will magically fix it 
> for my s390 patch, or do I have to fix that on my own?

I haven't memorized your s390 patch :-) but there's probably nothing to
do on the s390-specific bits.

Thanks,
Pedro Alves

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

> On 02/23/2016 07:34 PM, Antoine Tremblay wrote:
>> 
>> Pedro Alves writes:
>> 
>>>> +
>>>> +  return double_regnum;
>>>> +}
>>>> +
>>>> +/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
>>>> +
>>>> +static int
>>>> +arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
>>>> +				struct agent_expr *ax, int reg)
>>>> +{
>>>> +  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
>>>> +
>>>> +  /* Error.  */
>>>> +  if (rawnum < 0)
>>>> +    return 1;
>>>> +
>>>> +  ax_reg_mask (ax, rawnum);
>>>
>>> Hmm, seems to me that gdb raw -> target raw mapping should be
>>> either here, or perhaps even in ax_reg / ax_reg_mask?
>>>
>> 
>> After more investigation, this can't be in ax_reg / ax_reg_mask for
>> pseudo registers as this function is solely reponsible to encode the
>> right number here.
>
> I don't follow.
>
Nervermind that seems like I got confused.

> So it seems to me that we need to make ax_reg and ax_reg_mask
> convert gdb -> remote numbers in their else branches.
>
>> 
>>> Consider the case of an expression requiring the collection of
>>> a _raw_ register, thus not even reaching here.  Looking at
>>> ax-gdb.c/ax-general.c I don't see where is anything mapping gdb raw numbers
>>> to remote/tdesc numbers?  So how does _that_ work?  Are the register masks that gdb
>>> is computing actually wrong for the target, and things just happen
>>> to work because gdbserver ignores them and always collects all registers?
>> 
>> However yes it should be in ax_reg/ax_reg_mask for non-pseudo registers,
>> but this is not the objective of this patch, I suggest that such a
>> change be the subject of another patch
>
> Sure, but in that case, drop the gdb -> remote conversion entirely.
> If with that things don't work for arm, let's fix ax_reg/ax_reg_mask
> _first_.
>

OK.

>> maybe coupled with better gdbserver handling of the R action.
>
> I think this coupling would be a mistake.  This can be handled
> independently, if at all.
>
>>
OK.

Thanks,
Antoine

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

> On 02/24/2016 06:20 PM, Marcin Kościelnicki wrote:
>> On 24/02/16 19:11, Pedro Alves wrote:
>>> On 02/22/2016 04:51 PM, Antoine Tremblay wrote:
>>>>
>
>>>> While looking at fixing this for all the archs involved it would be
>>>> much simpler to test if gdbserver would make use of it.
>>>>
>>>> As it is now, I'm concerned that calling gdbarch_remote_register_number
>>>> in ax_reg, ax_mask_reg could break things if the arch already considers
>>>> the gdb raw -> target raw mapping like s390 and x86 do already (I'm not
>>>> 100% sure the mapping is already ok)?
>>>
>>> WDTM?  Where do they do this already?
>> 
>> FWIW, I failed to look at the numbering used when I wrote the x86 and 
>> s390 ax functions, so they're most likely wrong (I just copied the 
>> regnum computation logic from pseudo_read/write, which uses gdb 
>> numbers).  s390 hasn't landed yet, so it's only x86 that you'd have to 
>> fix now (and mips, I think, but that doesn't support tracepoints yet...).
>
> I don't think there's anything that needs fixing in the i386 implementation.
>
> The x86 implementation maps gdb pseudo register numbers to whatever
> raw gdb registers back the former up, like:
>
>       ax_reg_mask (ax, I387_FSTAT_REGNUM (tdep));
>
> That OK.
>
> The trouble is that in the end we send gdb numbers to the target in the
> ax, instead of tdesc/remote numbers.
>
> We never noticed because gdbserver always collects all raw registers
> anyway.
>
> Seems to me that the fix is to make ax_reg / ax_reg_mask take gdb raw
> numbers as input (as it does today), and then make it map those to
> tdesc/remote number just before it puts the reg number in the agent
> expression bytecode / reg mask.  And that covers all archs.
>
>> 
>> Testing this is possible if you write some conditions that involve 
>> reading pseudo-registers (since ax_pseudo_register_push_stack will be 
>> called), the problem is that I only implemented 
>> ax_pseudo_register_collect for x86...
>> 
>> Are you going to make some higher-level patch that will magically fix it 
>> for my s390 patch, or do I have to fix that on my own?
>
> I haven't memorized your s390 patch :-) but there's probably nothing to
> do on the s390-specific bits.
>

The only requirement for this to work properly is that the arch uses
tdesc_use_registers, otherwise the default mapping function to tdesc is
identity to GDB numbers.

s390 uses that so it should be fine.

Thanks,
Antoine

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

> On 02/24/2016 06:20 PM, Marcin Kościelnicki wrote:
>> On 24/02/16 19:11, Pedro Alves wrote:
>>> On 02/22/2016 04:51 PM, Antoine Tremblay wrote:
>>>>
>
>>>> While looking at fixing this for all the archs involved it would be
>>>> much simpler to test if gdbserver would make use of it.
>>>>
>>>> As it is now, I'm concerned that calling gdbarch_remote_register_number
>>>> in ax_reg, ax_mask_reg could break things if the arch already considers
>>>> the gdb raw -> target raw mapping like s390 and x86 do already (I'm not
>>>> 100% sure the mapping is already ok)?
>>>
>>> WDTM?  Where do they do this already?

I meant that the pseudo register code could have considered this already
and use tdesc numbers, thus adding a mapping would cause problems if it
tried to map tdesc to tdesc rather then gdb to tdesc.

But looking more into it, and you confirmed below, it does not, and s390
does not either so it should be straight forward to fix. In fact x86
sems to be in sync with tdesc AFAICT.

>> 
>> FWIW, I failed to look at the numbering used when I wrote the x86 and 
>> s390 ax functions, so they're most likely wrong (I just copied the 
>> regnum computation logic from pseudo_read/write, which uses gdb 
>> numbers).  s390 hasn't landed yet, so it's only x86 that you'd have to 
>> fix now (and mips, I think, but that doesn't support tracepoints yet...).
>
> I don't think there's anything that needs fixing in the i386 implementation.
>
> The x86 implementation maps gdb pseudo register numbers to whatever
> raw gdb registers back the former up, like:
>
>       ax_reg_mask (ax, I387_FSTAT_REGNUM (tdep));
>
> That OK.
>
> The trouble is that in the end we send gdb numbers to the target in the
> ax, instead of tdesc/remote numbers.
>
> We never noticed because gdbserver always collects all raw registers
> anyway.
>
> Seems to me that the fix is to make ax_reg / ax_reg_mask take gdb raw
> numbers as input (as it does today), and then make it map those to
> tdesc/remote number just before it puts the reg number in the agent
> expression bytecode / reg mask.  And that covers all archs.
>
>> 
>> Testing this is possible if you write some conditions that involve 
>> reading pseudo-registers (since ax_pseudo_register_push_stack will be 
>> called), the problem is that I only implemented 
>> ax_pseudo_register_collect for x86...
>> 
>> Are you going to make some higher-level patch that will magically fix it 
>> for my s390 patch, or do I have to fix that on my own?
>
> I haven't memorized your s390 patch :-) but there's probably nothing to
> do on the s390-specific bits.
>
> Thanks,
> Pedro Alves

Re: [PATCH v3] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/24/2016 06:55 PM, Antoine Tremblay wrote:

> The only requirement for this to work properly is that the arch uses
> tdesc_use_registers, otherwise the default mapping function to tdesc is
> identity to GDB numbers.

Even then, if the target doesn't report a tdesc, register numbers
on the target side must match gdb's.  So it still works.  The reason
the current code doesn't consider tdesc numbers is that AX predates
xml target descriptions, and back then gdb numbers was all you had.

> s390 uses that so it should be fine.

Thanks,
Pedro Alves

Re: [PATCH v5] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/23/2016 07:41 PM, Antoine Tremblay wrote:

> gdb/testsuite/ChangeLog:
> 
> 	* gdb.trace/tfile-avx.c: Move to...
> 	* gdb.trace/tracefile-pseudo-reg.c: Here.
> 	* gdb.trace/tfile-avx.exp: Move to...
> 	* gdb.trace/tracefile-pseudo-reg.exp: Here.

...

>  rename gdb/testsuite/gdb.trace/{tfile-avx.c => tracefile-pseudo-reg.c} (80%)
>  rename gdb/testsuite/gdb.trace/{tfile-avx.exp => tracefile-pseudo-reg.exp} (65%)

...

Thanks.  There's clearly more bits there than a move.

Please do the rename (only) as a preliminary patch.  That's pre-approved,
go ahead and push it.

Thanks,
Pedro Alves

Re: [PATCH v5] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

> On 02/23/2016 07:41 PM, Antoine Tremblay wrote:
>
>> gdb/testsuite/ChangeLog:
>> 
>> 	* gdb.trace/tfile-avx.c: Move to...
>> 	* gdb.trace/tracefile-pseudo-reg.c: Here.
>> 	* gdb.trace/tfile-avx.exp: Move to...
>> 	* gdb.trace/tracefile-pseudo-reg.exp: Here.
>
> ...
>
>>  rename gdb/testsuite/gdb.trace/{tfile-avx.c => tracefile-pseudo-reg.c} (80%)
>>  rename gdb/testsuite/gdb.trace/{tfile-avx.exp => tracefile-pseudo-reg.exp} (65%)
>
> ...
>
> Thanks.  There's clearly more bits there than a move.
>
> Please do the rename (only) as a preliminary patch.  That's pre-approved,
> go ahead and push it.

Done, thanks.

Re: [PATCH v5] Enable tracing of pseudo-registers on ARM

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> +/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
> +
> +static int
> +arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
> +				struct agent_expr *ax, int reg)
> +{
> +  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
> +
> +  /* Error.  */
> +  if (rawnum < 0)
> +    return 1;
> +
> +  /* Get the remote/tdesc register number.  */
> +  rawnum = gdbarch_remote_register_number (gdbarch, rawnum);
> +

I am expecting your patch v6 which moves gdbarch_remote_register_number
to the else branch of ax_reg/ax_reg_mask, if I correctly follow the discussion.

> +  ax_reg_mask (ax, rawnum);
> +
> +  return 0;
> +}

-- 
Yao (齐尧)

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Pedro Alves]

On 02/12/2016 02:46 PM, Yao Qi wrote:

> we can wrap methods of 'struct frame_unwind' with try/catch, and handle
> NOT_AVAILABLE_ERROR properly.  In this way, each unwinder doesn't have
> to worry about unavailable memory at all.
> 
> Pedro, what do you think?  Did you try this approach in the rest of 9
> different ways :) (you said you "implemented this differently in about
> 10 different ways" in your email) ?

I no longer recall exactly what I tried.  :-)

I think it may be a good idea.

There are a few constraints that we need to keep in mind:

- Frames that only have the PC available should have distinct frame ids,
  and it should be distinct from outer_frame_id.  (See frame_id_build_unavailable_stack calls).

  This makes e.g., the frame_id_eq check in tfind_1 work as intended, see:
   https://sourceware.org/ml/gdb-patches/2013-12/msg00535.html

- When an unwind sniffer throws, it'll destroy its
  struct frame_unwind_cache.  So if we don't catch the error, the
  frame's this_id method can't return something more detailed than
  outer_frame_id.

I don't see this done by wrapping methods of 'struct frame_unwind'.

I think it'd work to have an ultimate-fallback unwinder that
frame_unwind_find_by_frame returns instead of the internal error at
the end.  This would return UNWIND_UNAVAILABLE or UNWIND_MEMORY_ERROR
in the unwinder->stop_reason method, depending on the error the last registered
unwinder thrown.  (That last unwinder will always be the arch's heuristic unwinder.)
And it would return frame_id_build_unavailable_stack(PC) in the unwinder->this_id
method if the last error was UNWIND_UNAVAILABLE, outer_frame_id otherwise
(or we add a new frame_id_build_stackless function, to go along with
frame_id_build_unavailable_stack).

I think that would fix the cases where we end up internal erroring,
like in today's Andreas' patch:

 https://sourceware.org/ml/gdb-patches/2016-02/msg00773.html

And then the heuristic unwinders probably no longer need to care to
use the safe_read_memory_xxx functions.

And it'd fix the bogus cases where the sentinel frame level (-1)
shows through, due to:

 struct frame_info *
 get_current_frame (void)
 {
 ...
  if (current_frame == NULL)
    {
      struct frame_info *sentinel_frame =
	create_sentinel_frame (current_program_space, get_current_regcache ());
      if (catch_exceptions (current_uiout, unwind_to_current_frame,
			    sentinel_frame, RETURN_MASK_ERROR) != 0)
	{
	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
             of zero, for instance.  */
	  current_frame = sentinel_frame;
	}

See recent example here:
 https://sourceware.org/ml/gdb-patches/2016-01/msg00222.html

Thanks,
Pedro Alves

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Antoine Tremblay]

Pedro Alves writes:

> On 02/12/2016 02:46 PM, Yao Qi wrote:
>
>> we can wrap methods of 'struct frame_unwind' with try/catch, and handle
>> NOT_AVAILABLE_ERROR properly.  In this way, each unwinder doesn't have
>> to worry about unavailable memory at all.
>> 
>> Pedro, what do you think?  Did you try this approach in the rest of 9
>> different ways :) (you said you "implemented this differently in about
>> 10 different ways" in your email) ?
>
> I no longer recall exactly what I tried.  :-)
>
> I think it may be a good idea.
>
> There are a few constraints that we need to keep in mind:
>
> - Frames that only have the PC available should have distinct frame ids,
>   and it should be distinct from outer_frame_id.  (See frame_id_build_unavailable_stack calls).
>
>   This makes e.g., the frame_id_eq check in tfind_1 work as intended, see:
>    https://sourceware.org/ml/gdb-patches/2013-12/msg00535.html
>
> - When an unwind sniffer throws, it'll destroy its
>   struct frame_unwind_cache.  So if we don't catch the error, the
>   frame's this_id method can't return something more detailed than
>   outer_frame_id.
>
> I don't see this done by wrapping methods of 'struct frame_unwind'.
>
> I think it'd work to have an ultimate-fallback unwinder that
> frame_unwind_find_by_frame returns instead of the internal error at
> the end.  This would return UNWIND_UNAVAILABLE or UNWIND_MEMORY_ERROR
> in the unwinder->stop_reason method, depending on the error the last registered
> unwinder thrown.  (That last unwinder will always be the arch's heuristic unwinder.)
> And it would return frame_id_build_unavailable_stack(PC) in the unwinder->this_id
> method if the last error was UNWIND_UNAVAILABLE, outer_frame_id otherwise
> (or we add a new frame_id_build_stackless function, to go along with
> frame_id_build_unavailable_stack).
>
> I think that would fix the cases where we end up internal erroring,
> like in today's Andreas' patch:
>
>  https://sourceware.org/ml/gdb-patches/2016-02/msg00773.html
>
> And then the heuristic unwinders probably no longer need to care to
> use the safe_read_memory_xxx functions.
>
> And it'd fix the bogus cases where the sentinel frame level (-1)
> shows through, due to:
>
>  struct frame_info *
>  get_current_frame (void)
>  {
>  ...
>   if (current_frame == NULL)
>     {
>       struct frame_info *sentinel_frame =
> 	create_sentinel_frame (current_program_space, get_current_regcache ());
>       if (catch_exceptions (current_uiout, unwind_to_current_frame,
> 			    sentinel_frame, RETURN_MASK_ERROR) != 0)
> 	{
> 	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
>              of zero, for instance.  */
> 	  current_frame = sentinel_frame;
> 	}
>
> See recent example here:
>  https://sourceware.org/ml/gdb-patches/2016-01/msg00222.html
>

Reading Pedro's description I'm not against the refactoring but it's non
trivial to me at the moment at least.

I suggest we allow this patch to go in in order to make progress on the
arm tracepoint patchset and do that refactoring in a subsequent patch.

Would that be OK ?

Regards,
Antoine

[PATCH v6] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

In this v6:
 * use https://sourceware.org/ml/gdb-patches/2016-02/msg00786.html to map
 registers to remote registers. (This is already in master)
 * Fix test changelog
 * Test is already renamed, update patch.
-
This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.

gdb/testsuite/ChangeLog:

	* gdb.trace/tracefile-pseudo-reg.c: Include arm_neon.h if on arm.
	(main): Add a register variable and a tracepoint label.
	* gdb.trace/tracefile-pseudo-reg.exp): Add arm pseudo register
	tracing test with s5 pseudo register.
---
 gdb/arm-tdep.c                                   | 68 ++++++++++++++++++++++++
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c   | 12 +++++
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp | 35 +++++++++---
 3 files changed, 108 insertions(+), 7 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 2151ffa..6d50e9e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8716,6 +8716,70 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int double_regnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+  char name_buf[4];
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs - 32) * 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  return double_regnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9377,6 +9441,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
index 3cc3ec0..473d805 100644
--- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
@@ -20,7 +20,11 @@
  * registers on x86_64.
  */
 
+#if (defined __x86_64__)
 #include <immintrin.h>
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+#include <arm_neon.h>
+#endif
 
 void
 dummy (void)
@@ -37,6 +41,7 @@ main (void)
 {
   /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
      than 4.9 doesn't recognize "ymm15" as a valid register name.  */
+#if (defined __x86_64__)
   register __v8si a asm("xmm15") = {
     0x12340001,
     0x12340002,
@@ -48,6 +53,13 @@ main (void)
     0x12340008,
   };
   asm volatile ("traceme: call dummy" : : "x" (a));
+#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
+  register uint32_t a asm("s5") = {
+    0x2
+  };
+  asm volatile ("traceme: bl dummy" : : "x" (a));
+#endif
+
   end ();
   return 0;
 }
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
index 4c52c64..12a2740 100644
--- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
@@ -12,8 +12,8 @@
 # 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 { ! [is_amd64_regs_target] } {
-    verbose "Skipping tfile AVX test (target is not x86_64)."
+if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
+    verbose "Skipping tracefile pseudo register tests, target is not supported."
     return
 }
 
@@ -21,8 +21,14 @@ load_lib "trace-support.exp"
 
 standard_testfile
 
+if { [is_amd64_regs_target] } {
+ set add_flags "-mavx"
+} elseif { [istarget "arm*-*-*"] } {
+ set add_flags "-mfpu=neon"
+}
+
 if {[prepare_for_testing $testfile.exp $testfile $srcfile \
-     [list debug additional_flags=-mavx]]} {
+     [list debug additional_flags=$add_flags]]} {
     return -1
 }
 
@@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
     return -1
 }
 
-gdb_test_multiple "print \$ymm15" "check for AVX support" {
+if { [is_amd64_regs_target] } {
+    set reg "\$ymm15"
+    set reg_message "check for AVX support"
+} elseif { [istarget "arm*-*-*"] } {
+    set reg "\$s5"
+    set reg_message "check for Neon support"
+}
+
+gdb_test_multiple "print $reg" $reg_message {
     -re " = void.*$gdb_prompt $" {
-	verbose "Skipping tfile AVX test (target doesn't support AVX)."
+	verbose "Skipping tracefile pseudo register tests, target is not supported."
 	return
     }
     -re " = \\{.*}.*$gdb_prompt $" {
 	# All is well.
     }
+    -re " = 0.*$gdb_prompt $" {
+	# All is well.
+    }
 }
 
 gdb_test "trace traceme" ".*"
 
 gdb_trace_setactions "set actions for tracepoint" "" \
-	"collect \$ymm15" "^$"
+	"collect $reg" "^$"
 
 gdb_breakpoint "end"
 
@@ -70,4 +87,8 @@ gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
 
 gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
 
-gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+if { [is_amd64_regs_target] } {
+    gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+} elseif { [istarget "arm*-*-*"] } {
+    gdb_test "print \$s5" "2.80259693e-45"
+}
-- 
2.6.4

Re: [PATCH v6] Enable tracing of pseudo-registers on ARM

[Pedro Alves]

On 02/25/2016 03:32 PM, Antoine Tremblay wrote:
> In this v6:
>  * use https://sourceware.org/ml/gdb-patches/2016-02/msg00786.html to map
>  registers to remote registers. (This is already in master)
>  * Fix test changelog
>  * Test is already renamed, update patch.
> -
> This patch implements the ax_pseudo_register_push_stack and
> ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
> be traced.
> 

FAOD, I'm happy with this version, if Yao is happy.

>  dummy (void)
> @@ -37,6 +41,7 @@ main (void)
>  {
>    /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
>       than 4.9 doesn't recognize "ymm15" as a valid register name.  */
> +#if (defined __x86_64__)

The comment should be within the #if.

Thanks,
Pedro Alves

Re: [PATCH v6] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Pedro Alves writes:

> On 02/25/2016 03:32 PM, Antoine Tremblay wrote:
>> In this v6:
>>  * use https://sourceware.org/ml/gdb-patches/2016-02/msg00786.html to map
>>  registers to remote registers. (This is already in master)
>>  * Fix test changelog
>>  * Test is already renamed, update patch.
>> -
>> This patch implements the ax_pseudo_register_push_stack and
>> ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
>> be traced.
>> 
>
> FAOD, I'm happy with this version, if Yao is happy.
>
OK. I'll wait for Yao's review.

>>  dummy (void)
>> @@ -37,6 +41,7 @@ main (void)
>>  {
>>    /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
>>       than 4.9 doesn't recognize "ymm15" as a valid register name.  */
>> +#if (defined __x86_64__)
>
> The comment should be within the #if.
>
Fixed, thanks.

Antoine

Re: [PATCH v6] Enable tracing of pseudo-registers on ARM

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> @@ -20,7 +20,11 @@
>   * registers on x86_64.
>   */
>  

The comments above should be updated as well.

> +#if (defined __x86_64__)
>  #include <immintrin.h>
> +#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)

__arm__ is defined even in thumb mode, so only "defined __arm__" is enough.

> +#include <arm_neon.h>

Why do you include arm_neon.h?  I don't see anything NEON specific is used.

> +#endif
>  
>  void
>  dummy (void)
> @@ -37,6 +41,7 @@ main (void)
>  {
>    /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
>       than 4.9 doesn't recognize "ymm15" as a valid register name.  */
> +#if (defined __x86_64__)
>    register __v8si a asm("xmm15") = {
>      0x12340001,
>      0x12340002,
> @@ -48,6 +53,13 @@ main (void)
>      0x12340008,
>    };
>    asm volatile ("traceme: call dummy" : : "x" (a));
> +#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)

Only "defined __arm__" is needed.

> +  register uint32_t a asm("s5") = {
> +    0x2
> +  };

I'd like to write an inline asm to set s5 a value and the value can be shown as
an integer so that the test is more reliable (current test tests float
"2.80259693e-45").

> +  asm volatile ("traceme: bl dummy" : : "x" (a));
> +#endif
> +
>    end ();
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
> index 4c52c64..12a2740 100644
> --- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
> +++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
> @@ -12,8 +12,8 @@
>  # 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 { ! [is_amd64_regs_target] } {
> -    verbose "Skipping tfile AVX test (target is not x86_64)."
> +if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
> +    verbose "Skipping tracefile pseudo register tests, target is not supported."
>      return
>  }
>  
> @@ -21,8 +21,14 @@ load_lib "trace-support.exp"
>  
>  standard_testfile
>  
> +if { [is_amd64_regs_target] } {
> + set add_flags "-mavx"
> +} elseif { [istarget "arm*-*-*"] } {
> + set add_flags "-mfpu=neon"

Don't have to pass -mfpu=neon, because the case is also valid for vfp.

> +}
> +
>  if {[prepare_for_testing $testfile.exp $testfile $srcfile \
> -     [list debug additional_flags=-mavx]]} {
> +     [list debug additional_flags=$add_flags]]} {
>      return -1
>  }
>  
> @@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
>      return -1
>  }
>  
> -gdb_test_multiple "print \$ymm15" "check for AVX support" {
> +if { [is_amd64_regs_target] } {
> +    set reg "\$ymm15"
> +    set reg_message "check for AVX support"
> +} elseif { [istarget "arm*-*-*"] } {
> +    set reg "\$s5"
> +    set reg_message "check for Neon support"

$s5 exists on the processors which support NEON or VFP, so the
$reg_message isn't accurate.  We can change reg_message to "check
register $reg".

-- 
Yao (齐尧)

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> Reading Pedro's description I'm not against the refactoring but it's non
> trivial to me at the moment at least.

It is not a simple refacotring...

>
> I suggest we allow this patch to go in in order to make progress on the
> arm tracepoint patchset and do that refactoring in a subsequent patch.
>
> Would that be OK ?

I am afraid not.  We should try this approach, because this will benefit
all targets.  IMO, handling unavailable memory in general frame
unwinding is more important.

b.t.w, I am still not confident on the arm software single step in
GDBserver on some cases, such as branch-to-self (".L2: b .L2") and
single step with signal.  ARM tracepoint patches can go in after these
issues are resolved (I am working on these issues).

-- 
Yao (齐尧)

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> Reading Pedro's description I'm not against the refactoring but it's non
>> trivial to me at the moment at least.
>
> It is not a simple refacotring...
>
>>
>> I suggest we allow this patch to go in in order to make progress on the
>> arm tracepoint patchset and do that refactoring in a subsequent patch.
>>
>> Would that be OK ?
>
> I am afraid not.  We should try this approach, because this will benefit
> all targets.  IMO, handling unavailable memory in general frame
> unwinding is more important.

So you intend to work on this ?

>
> b.t.w, I am still not confident on the arm software single step in
> GDBserver on some cases, such as branch-to-self (".L2: b .L2") and
> single step with signal.  ARM tracepoint patches can go in after these
> issues are resolved (I am working on these issues).

Thanks for working on that, I'm just trying to progress whereever I can
meanwhile so that tracepoint patches are ready to go when single
stepping is OK.

Thanks,
Antoine

Re: [PATCH v6] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> @@ -20,7 +20,11 @@
>>   * registers on x86_64.
>>   */
>>  
>
> The comments above should be updated as well.

Done.

>
>> +#if (defined __x86_64__)
>>  #include <immintrin.h>
>> +#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
>
> __arm__ is defined even in thumb mode, so only "defined __arm__" is enough.
>
>> +#include <arm_neon.h>
>
> Why do you include arm_neon.h?  I don't see anything NEON specific is used.
>
>> +#endif

Indeed I was playing with neon types before, forgot to remove it. Fixed.

>>  
>>  void
>>  dummy (void)
>> @@ -37,6 +41,7 @@ main (void)
>>  {
>>    /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
>>       than 4.9 doesn't recognize "ymm15" as a valid register name.  */
>> +#if (defined __x86_64__)
>>    register __v8si a asm("xmm15") = {
>>      0x12340001,
>>      0x12340002,
>> @@ -48,6 +53,13 @@ main (void)
>>      0x12340008,
>>    };
>>    asm volatile ("traceme: call dummy" : : "x" (a));
>> +#elif (defined __arm__ || defined __thumb2__ || defined __thumb__)
>
> Only "defined __arm__" is needed.
>

Done.

>> +  register uint32_t a asm("s5") = {
>> +    0x2
>> +  };
>
> I'd like to write an inline asm to set s5 a value and the value can be shown as
> an integer so that the test is more reliable (current test tests float
> "2.80259693e-45").
>

Done.

>> +  asm volatile ("traceme: bl dummy" : : "x" (a));
>> +#endif
>> +
>>    end ();
>>    return 0;
>>  }
>> diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
>> index 4c52c64..12a2740 100644
>> --- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
>> +++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
>> @@ -12,8 +12,8 @@
>>  # 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 { ! [is_amd64_regs_target] } {
>> -    verbose "Skipping tfile AVX test (target is not x86_64)."
>> +if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
>> +    verbose "Skipping tracefile pseudo register tests, target is not supported."
>>      return
>>  }
>>  
>> @@ -21,8 +21,14 @@ load_lib "trace-support.exp"
>>  
>>  standard_testfile
>>  
>> +if { [is_amd64_regs_target] } {
>> + set add_flags "-mavx"
>> +} elseif { [istarget "arm*-*-*"] } {
>> + set add_flags "-mfpu=neon"
>
> Don't have to pass -mfpu=neon, because the case is also valid for vfp.
>

Right, Fixed.

>> +}
>> +
>>  if {[prepare_for_testing $testfile.exp $testfile $srcfile \
>> -     [list debug additional_flags=-mavx]]} {
>> +     [list debug additional_flags=$add_flags]]} {
>>      return -1
>>  }
>>  
>> @@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
>>      return -1
>>  }
>>  
>> -gdb_test_multiple "print \$ymm15" "check for AVX support" {
>> +if { [is_amd64_regs_target] } {
>> +    set reg "\$ymm15"
>> +    set reg_message "check for AVX support"
>> +} elseif { [istarget "arm*-*-*"] } {
>> +    set reg "\$s5"
>> +    set reg_message "check for Neon support"
>
> $s5 exists on the processors which support NEON or VFP, so the
> $reg_message isn't accurate.  We can change reg_message to "check
> register $reg".

Done.

A v7 patch follows.

[PATCH v7] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

In this v7:
* Fixed according to comments about the test case.
-
This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.

gdb/testsuite/ChangeLog:

	* gdb.trace/tracefile-pseudo-reg.c: Include arm_neon.h if on arm.
	(main): Add a register variable and a tracepoint label.
	* gdb.trace/tracefile-pseudo-reg.exp): Add arm pseudo register
	tracing test with s5 pseudo register.
---
 gdb/arm-tdep.c                                   | 68 ++++++++++++++++++++++++
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c   | 14 ++++-
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp | 35 +++++++++---
 3 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 2151ffa..6d50e9e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8716,6 +8716,70 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int double_regnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+  char name_buf[4];
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs - 32) * 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  return double_regnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9377,6 +9441,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
index 3cc3ec0..33761c1 100644
--- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
@@ -16,11 +16,15 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 /*
- * Test program for reading target description from tfile: collects AVX
- * registers on x86_64.
+ * Test program for reading target description from tfile: collects pseudo
+ * register on the target.
  */
 
+#if (defined __x86_64__)
 #include <immintrin.h>
+#elif (defined __arm__)
+#include <stdint.h>
+#endif
 
 void
 dummy (void)
@@ -35,6 +39,7 @@ end (void)
 int
 main (void)
 {
+#if (defined __x86_64__)
   /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
      than 4.9 doesn't recognize "ymm15" as a valid register name.  */
   register __v8si a asm("xmm15") = {
@@ -48,6 +53,11 @@ main (void)
     0x12340008,
   };
   asm volatile ("traceme: call dummy" : : "x" (a));
+#elif (defined __arm__)
+  register uint32_t a asm("s5") = 0x3f800000; /* 1. */
+  asm volatile ("traceme: bl dummy" : : "x" (a));
+#endif
+
   end ();
   return 0;
 }
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
index 4c52c64..33677a1 100644
--- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
@@ -12,8 +12,8 @@
 # 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 { ! [is_amd64_regs_target] } {
-    verbose "Skipping tfile AVX test (target is not x86_64)."
+if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
+    verbose "Skipping tracefile pseudo register tests, target is not supported."
     return
 }
 
@@ -21,8 +21,14 @@ load_lib "trace-support.exp"
 
 standard_testfile
 
+set add_flags ""
+
+if { [is_amd64_regs_target] } {
+ set add_flags "-mavx"
+}
+
 if {[prepare_for_testing $testfile.exp $testfile $srcfile \
-     [list debug additional_flags=-mavx]]} {
+     [list debug additional_flags=$add_flags]]} {
     return -1
 }
 
@@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
     return -1
 }
 
-gdb_test_multiple "print \$ymm15" "check for AVX support" {
+if { [is_amd64_regs_target] } {
+    set reg "\$ymm15"
+    set reg_message "check for AVX support"
+} elseif { [istarget "arm*-*-*"] } {
+    set reg "\$s5"
+    set reg_message "check for register $reg"
+}
+
+gdb_test_multiple "print $reg" $reg_message {
     -re " = void.*$gdb_prompt $" {
-	verbose "Skipping tfile AVX test (target doesn't support AVX)."
+	verbose "Skipping tracefile pseudo register tests, target is not supported."
 	return
     }
     -re " = \\{.*}.*$gdb_prompt $" {
 	# All is well.
     }
+    -re " = 0.*$gdb_prompt $" {
+	# All is well.
+    }
 }
 
 gdb_test "trace traceme" ".*"
 
 gdb_trace_setactions "set actions for tracepoint" "" \
-	"collect \$ymm15" "^$"
+	"collect $reg" "^$"
 
 gdb_breakpoint "end"
 
@@ -70,4 +87,8 @@ gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
 
 gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
 
-gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+if { [is_amd64_regs_target] } {
+    gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+} elseif { [istarget "arm*-*-*"] } {
+    gdb_test "print \$s5" "1"
+}
-- 
2.6.4

Re: [PATCH v7] Enable tracing of pseudo-registers on ARM

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> gdb/testsuite/ChangeLog:
>
> 	* gdb.trace/tracefile-pseudo-reg.c: Include arm_neon.h if on arm.

arm_neon.h is not included now, but stdint.h is included.

> 	(main): Add a register variable and a tracepoint label.
> 	* gdb.trace/tracefile-pseudo-reg.exp): Add arm pseudo register
> 	tracing test with s5 pseudo register.

> diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
> index 3cc3ec0..33761c1 100644
> --- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
> +++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
> @@ -16,11 +16,15 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  /*
> - * Test program for reading target description from tfile: collects AVX
> - * registers on x86_64.
> + * Test program for reading target description from tfile: collects pseudo
> + * register on the target.
>   */

The comment format doesn't comply to GNU coding standard.  It should be

  /* Test program for reading target description from tfile: collects pseudo
      registers on the target.  */

>  
> +#if (defined __x86_64__)
>  #include <immintrin.h>
> +#elif (defined __arm__)
> +#include <stdint.h>
> +#endif
>  
>  void
>  dummy (void)
> @@ -35,6 +39,7 @@ end (void)
>  int
>  main (void)
>  {
> +#if (defined __x86_64__)
>    /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
>       than 4.9 doesn't recognize "ymm15" as a valid register name.  */
>    register __v8si a asm("xmm15") = {
> @@ -48,6 +53,11 @@ main (void)
>      0x12340008,
>    };
>    asm volatile ("traceme: call dummy" : : "x" (a));
> +#elif (defined __arm__)
> +  register uint32_t a asm("s5") = 0x3f800000; /* 1. */
> +  asm volatile ("traceme: bl dummy" : : "x" (a));
> +#endif
> +
>    end ();
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
> index 4c52c64..33677a1 100644
> --- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
> +++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
> @@ -12,8 +12,8 @@
>  # 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 { ! [is_amd64_regs_target] } {
> -    verbose "Skipping tfile AVX test (target is not x86_64)."
> +if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
> +    verbose "Skipping tracefile pseudo register tests, target is not supported."
>      return
>  }
>  
> @@ -21,8 +21,14 @@ load_lib "trace-support.exp"
>  
>  standard_testfile
>  
> +set add_flags ""
> +
> +if { [is_amd64_regs_target] } {
> + set add_flags "-mavx"
> +}
> +
>  if {[prepare_for_testing $testfile.exp $testfile $srcfile \
> -     [list debug additional_flags=-mavx]]} {
> +     [list debug additional_flags=$add_flags]]} {
>      return -1
>  }
>  
> @@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
>      return -1
>  }
>  
> -gdb_test_multiple "print \$ymm15" "check for AVX support" {
> +if { [is_amd64_regs_target] } {
> +    set reg "\$ymm15"
> +    set reg_message "check for AVX support"
> +} elseif { [istarget "arm*-*-*"] } {
> +    set reg "\$s5"
> +    set reg_message "check for register $reg"
> +}

We can set reg_message out of the condition block,

if { [is_amd64_regs_target] } {
    set reg "\$ymm15"
} elseif { [istarget "arm*-*-*"] } {
    set reg "\$s5"
}

set reg_message "check for register $reg"

-- 
Yao (齐尧)

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

>> I am afraid not.  We should try this approach, because this will benefit
>> all targets.  IMO, handling unavailable memory in general frame
>> unwinding is more important.
>
> So you intend to work on this ?

I'd like to add it to my todo list, but I don't mind someone else picks
it up.

-- 
Yao (齐尧)

Re: [PATCH v7] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.trace/tracefile-pseudo-reg.c: Include arm_neon.h if on arm.
>
> arm_neon.h is not included now, but stdint.h is included.

Yes this is due to the usage of uint32_t.

>
>> 	(main): Add a register variable and a tracepoint label.
>> 	* gdb.trace/tracefile-pseudo-reg.exp): Add arm pseudo register
>> 	tracing test with s5 pseudo register.
>
>> diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
>> index 3cc3ec0..33761c1 100644
>> --- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
>> +++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
>> @@ -16,11 +16,15 @@
>>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>  
>>  /*
>> - * Test program for reading target description from tfile: collects AVX
>> - * registers on x86_64.
>> + * Test program for reading target description from tfile: collects pseudo
>> + * register on the target.
>>   */
>
> The comment format doesn't comply to GNU coding standard.  It should be
>
>   /* Test program for reading target description from tfile: collects pseudo
>       registers on the target.  */
>

Fixed.

>>  
>> +#if (defined __x86_64__)
>>  #include <immintrin.h>
>> +#elif (defined __arm__)
>> +#include <stdint.h>
>> +#endif
>>  
>>  void
>>  dummy (void)
>> @@ -35,6 +39,7 @@ end (void)
>>  int
>>  main (void)
>>  {
>> +#if (defined __x86_64__)
>>    /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
>>       than 4.9 doesn't recognize "ymm15" as a valid register name.  */
>>    register __v8si a asm("xmm15") = {
>> @@ -48,6 +53,11 @@ main (void)
>>      0x12340008,
>>    };
>>    asm volatile ("traceme: call dummy" : : "x" (a));
>> +#elif (defined __arm__)
>> +  register uint32_t a asm("s5") = 0x3f800000; /* 1. */
>> +  asm volatile ("traceme: bl dummy" : : "x" (a));
>> +#endif
>> +
>>    end ();
>>    return 0;
>>  }
>> diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
>> index 4c52c64..33677a1 100644
>> --- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
>> +++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
>> @@ -12,8 +12,8 @@
>>  # 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 { ! [is_amd64_regs_target] } {
>> -    verbose "Skipping tfile AVX test (target is not x86_64)."
>> +if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
>> +    verbose "Skipping tracefile pseudo register tests, target is not supported."
>>      return
>>  }
>>  
>> @@ -21,8 +21,14 @@ load_lib "trace-support.exp"
>>  
>>  standard_testfile
>>  
>> +set add_flags ""
>> +
>> +if { [is_amd64_regs_target] } {
>> + set add_flags "-mavx"
>> +}
>> +
>>  if {[prepare_for_testing $testfile.exp $testfile $srcfile \
>> -     [list debug additional_flags=-mavx]]} {
>> +     [list debug additional_flags=$add_flags]]} {
>>      return -1
>>  }
>>  
>> @@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
>>      return -1
>>  }
>>  
>> -gdb_test_multiple "print \$ymm15" "check for AVX support" {
>> +if { [is_amd64_regs_target] } {
>> +    set reg "\$ymm15"
>> +    set reg_message "check for AVX support"
>> +} elseif { [istarget "arm*-*-*"] } {
>> +    set reg "\$s5"
>> +    set reg_message "check for register $reg"
>> +}
>
> We can set reg_message out of the condition block,
>
> if { [is_amd64_regs_target] } {
>     set reg "\$ymm15"
> } elseif { [istarget "arm*-*-*"] } {
>     set reg "\$s5"
> }
>
> set reg_message "check for register $reg"

OK.

Patch v8 follows.

[PATCH v8] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

This patch implements the ax_pseudo_register_push_stack and
ax_pseudo_register_collect gdbarch functions so that a pseudo-register can
be traced.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-tdep.c (arm_pseudo_register_to_register): New function.
	(arm_ax_pseudo_register_collect): New function.
	(arm_ax_pseudo_register_push_stack): New function.
	(arm_gdbarch_init): Set
	gdbarch_ax_pseudo_register_{collect,push_stack} functions.

gdb/testsuite/ChangeLog:

	* gdb.trace/tracefile-pseudo-reg.c: Include arm_neon.h if on arm.
	(main): Add a register variable and a tracepoint label.
	* gdb.trace/tracefile-pseudo-reg.exp): Add arm pseudo register
	tracing test with s5 pseudo register.
---
 gdb/arm-tdep.c                                   | 68 ++++++++++++++++++++++++
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c   | 16 ++++--
 gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp | 35 +++++++++---
 3 files changed, 108 insertions(+), 11 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 2151ffa..6d50e9e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8716,6 +8716,70 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
     }
 }
 
+/* Map the pseudo register number REG to the proper register number.  */
+
+static int
+arm_pseudo_register_to_register (struct gdbarch *gdbarch, int reg)
+{
+  int double_regnum = 0;
+  int num_regs = gdbarch_num_regs (gdbarch);
+  char name_buf[4];
+
+  /* Single precision pseudo registers. s0-s31.  */
+  if (reg >= num_regs && reg < num_regs + 32)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs) / 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Quadruple precision pseudo regisers. q0-q15.  */
+  else if (reg >= num_regs + 32 && reg < num_regs + 32 + 16)
+    {
+      xsnprintf (name_buf, sizeof (name_buf), "d%d", (reg - num_regs - 32) * 2);
+      double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
+						   strlen (name_buf));
+    }
+  /* Error bad register number.  */
+  else
+    return -1;
+
+  return double_regnum;
+}
+
+/* Implementation of the ax_pseudo_register_collect gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+/* Implementation of the ax_pseudo_register_push_stack gdbarch function.  */
+
+static int
+arm_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				   struct agent_expr *ax, int reg)
+{
+  int rawnum = arm_pseudo_register_to_register (gdbarch, reg);
+
+  /* Error.  */
+  if (rawnum < 0)
+    return 1;
+
+  ax_reg (ax, rawnum);
+
+  return 0;
+}
+
 static struct value *
 value_of_arm_user_reg (struct frame_info *frame, const void *baton)
 {
@@ -9377,6 +9441,10 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
+      set_gdbarch_ax_pseudo_register_push_stack
+	(gdbarch, arm_ax_pseudo_register_push_stack);
+      set_gdbarch_ax_pseudo_register_collect
+	(gdbarch, arm_ax_pseudo_register_collect);
     }
 
   if (tdesc_data)
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
index 3cc3ec0..1a751ee 100644
--- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.c
@@ -15,12 +15,14 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-/*
- * Test program for reading target description from tfile: collects AVX
- * registers on x86_64.
- */
+/* Test program for reading target description from tfile: collects pseudo
+   registers on the target.  */
 
+#if (defined __x86_64__)
 #include <immintrin.h>
+#elif (defined __arm__)
+#include <stdint.h>
+#endif
 
 void
 dummy (void)
@@ -35,6 +37,7 @@ end (void)
 int
 main (void)
 {
+#if (defined __x86_64__)
   /* Strictly speaking, it should be ymm15 (xmm15 is 128-bit), but gcc older
      than 4.9 doesn't recognize "ymm15" as a valid register name.  */
   register __v8si a asm("xmm15") = {
@@ -48,6 +51,11 @@ main (void)
     0x12340008,
   };
   asm volatile ("traceme: call dummy" : : "x" (a));
+#elif (defined __arm__)
+  register uint32_t a asm("s5") = 0x3f800000; /* 1. */
+  asm volatile ("traceme: bl dummy" : : "x" (a));
+#endif
+
   end ();
   return 0;
 }
diff --git a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
index 4c52c64..6125c23 100644
--- a/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
+++ b/gdb/testsuite/gdb.trace/tracefile-pseudo-reg.exp
@@ -12,8 +12,8 @@
 # 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 { ! [is_amd64_regs_target] } {
-    verbose "Skipping tfile AVX test (target is not x86_64)."
+if { ! [is_amd64_regs_target] && ! [istarget "arm*-*-*"] } {
+    verbose "Skipping tracefile pseudo register tests, target is not supported."
     return
 }
 
@@ -21,8 +21,14 @@ load_lib "trace-support.exp"
 
 standard_testfile
 
+set add_flags ""
+
+if { [is_amd64_regs_target] } {
+ set add_flags "-mavx"
+}
+
 if {[prepare_for_testing $testfile.exp $testfile $srcfile \
-     [list debug additional_flags=-mavx]]} {
+     [list debug additional_flags=$add_flags]]} {
     return -1
 }
 
@@ -36,20 +42,31 @@ if ![gdb_target_supports_trace] {
     return -1
 }
 
-gdb_test_multiple "print \$ymm15" "check for AVX support" {
+if { [is_amd64_regs_target] } {
+    set reg "\$ymm15"
+} elseif { [istarget "arm*-*-*"] } {
+    set reg "\$s5"
+}
+
+set reg_message "check for register $reg"
+
+gdb_test_multiple "print $reg" $reg_message {
     -re " = void.*$gdb_prompt $" {
-	verbose "Skipping tfile AVX test (target doesn't support AVX)."
+	verbose "Skipping tracefile pseudo register tests, target is not supported."
 	return
     }
     -re " = \\{.*}.*$gdb_prompt $" {
 	# All is well.
     }
+    -re " = 0.*$gdb_prompt $" {
+	# All is well.
+    }
 }
 
 gdb_test "trace traceme" ".*"
 
 gdb_trace_setactions "set actions for tracepoint" "" \
-	"collect \$ymm15" "^$"
+	"collect $reg" "^$"
 
 gdb_breakpoint "end"
 
@@ -70,4 +87,8 @@ gdb_test "target tfile ${tracefile}.tf" "" "change to tfile target" \
 
 gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
 
-gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+if { [is_amd64_regs_target] } {
+    gdb_test "print/x \$ymm15.v8_int32" " = \\{0x12340001, .*, 0x12340008}"
+} elseif { [istarget "arm*-*-*"] } {
+    gdb_test "print \$s5" "1"
+}
-- 
2.6.4

Re: [PATCH v8] Enable tracing of pseudo-registers on ARM

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> gdb/testsuite/ChangeLog:
>
> 	* gdb.trace/tracefile-pseudo-reg.c: Include arm_neon.h if on arm.

        * gdb.trace/tracefile-pseudo-reg.c[__arm__]: Inlcude stdint.h.

> 	(main): Add a register variable and a tracepoint label.
> 	* gdb.trace/tracefile-pseudo-reg.exp): Add arm pseudo register

Redundant ")" before ":".

> 	tracing test with s5 pseudo register.

Patch is OK, but should go in together with other arm tracepoint patches.

-- 
Yao (齐尧)

Re: [PATCH v8] Enable tracing of pseudo-registers on ARM

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.trace/tracefile-pseudo-reg.c: Include arm_neon.h if on arm.
>
>         * gdb.trace/tracefile-pseudo-reg.c[__arm__]: Inlcude stdint.h.
>
>> 	(main): Add a register variable and a tracepoint label.
>> 	* gdb.trace/tracefile-pseudo-reg.exp): Add arm pseudo register
>
> Redundant ")" before ":".

Fixed.

>
>> 	tracing test with s5 pseudo register.
>
> Patch is OK, but should go in together with other arm tracepoint patches.

OK, thanks.

Antoine

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>>> I am afraid not.  We should try this approach, because this will benefit
>>> all targets.  IMO, handling unavailable memory in general frame
>>> unwinding is more important.
>>
>> So you intend to work on this ?
>
> I'd like to add it to my todo list, but I don't mind someone else picks
> it up.

Just so that there is no confusion, I do not indend to pick this up.

Regards,
Antoine

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> I think it'd work to have an ultimate-fallback unwinder that
> frame_unwind_find_by_frame returns instead of the internal error at
> the end.  This would return UNWIND_UNAVAILABLE or UNWIND_MEMORY_ERROR
> in the unwinder->stop_reason method, depending on the error the last registered
> unwinder thrown.  (That last unwinder will always be the arch's heuristic unwinder.)
> And it would return frame_id_build_unavailable_stack(PC) in the unwinder->this_id
> method if the last error was UNWIND_UNAVAILABLE, outer_frame_id otherwise
> (or we add a new frame_id_build_stackless function, to go along with
> frame_id_build_unavailable_stack).

I write some code to implement your suggestion here, and it looks OK
except that I can't get PC to pass to frame_id_build_unavailable_stack,
since PC is extracted from frame cache which varies on different archs
and unwinders.

I tried to define a super class frame_cache for various frame cache
(nowadays, it is defined as void *), frame_cache has one field PC, and
various frame caches are the sub class of frame_cache.  Many frame
unwinding APIs need update, and many places need update too, as a
result.  I stop here as I am not sure it is a right approach.

However, I think we can still do the change you suggested, but in a
smaller scope, so the change is less aggressive, and some progress can
be made, like this,

 - Add an unavailable frame unwinder for gdbarch A which supports
   tracepoint, as the ultimate-fallback.

 - For the unwinders in gdbarch A, move the code creating frame cache to
   the sniffer.  If the sniffer accepts the frame, creates the frame cache.

 - Exceptions are allowed to be thrown out in frame cache creation.
   The exception is caught in the caller of sniffer
   (frame_unwind_try_unwinder) today, so if exception is thrown, GDB
   will try the next unwinder,

 - In this way, only 'sniffer' in 'frame_unwind' may throw exception, so
   we don't have to worry about other 'frame_unwind' methods.  IOW, all
   unwinders in gdbarch A except unavailable frame unwinder don't worry
   about the unavailable memory/register.

 - the unavailable frame unwinder is the last unwinder for gdbarch A, it
   knows how/where to get PC, if PC is available, return
   frame_id_build_unavailable_stack (PC), otherwise return outer_frame_id.

In this way, the change will be smaller, and we can apply this change to
each gdbarch one by one, and in the future, it is possible to have a
single gdbarch-independent unavailable frame unwinder once we figure out
how to get PC from various frame caches.

-- 
Yao (齐尧)

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Pedro Alves]

On 04/06/2016 04:54 PM, Yao Qi wrote:
> I write some code to implement your suggestion here, and it looks OK
> except that I can't get PC to pass to frame_id_build_unavailable_stack,
> since PC is extracted from frame cache which varies on different archs
> and unwinders.

Hmm, I think I'm confused, since the PC is extracted/unwound from
the _next_ frame, not the one we're building the cache for?
See get_frame_pc / get_frame_pc_if_available.

Thanks,
Pedro Alves

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Hmm, I think I'm confused, since the PC is extracted/unwound from
> the _next_ frame, not the one we're building the cache for?
> See get_frame_pc / get_frame_pc_if_available.

Oh, right.  Thanks for the explanation.

-- 
Yao (齐尧)

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> This patch series enables GDBServer to trace an ARM target on linux.
>>
>> Patches 1-3: Fixes collection failures in certain cases.
>>
>> Patch 4: Enables tracepoints on ARM and introduces the new TracepointKinds
>> feature and 'K' parameter to the QTDP packet.
>
> Hi Antoine,
> First of all, thanks for patches.  I am afraid can't review them soon
> because 1) I find there are some places in arm software single step code
> can be improved, so I'd like to clean the room first before we move in
> new furniture, 2) I have to fix AArch64 and ARM test fails before 7.11
> branch/release.  I'll review them after I finish them above, if nobody
> reviews them yet.

Hi Yao,

  From what I can tell issue 1) is about done ?

  On my end we have fast tracepoints for arm almost ready with JIT
  conditions and pc relative instructions relocation.

  I would like to post that in the next few weeks, but it would be
  better if the normal tracepoints were in before that.

  Is it a good time to review these patches now?

Thanks,
Antoine

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

>   From what I can tell issue 1) is about done ?
>

There are still some things we need to do before arm tracepoint support,
and I am working on them,

 - really exercise the software single step in gdbserver,
   1. software single step over instruction branch to itself.  [DONE]
   2. force gdb to use vCont;s for gdbserver using software single step,
   IOW, let gdbserver handle single step requested by gdb,
   3. turn range stepping on on arm linux,

   I have some patches in my tree.  After these steps, we are confident
   that software single step in gdbserver is reliable.

>   On my end we have fast tracepoints for arm almost ready with JIT
>   conditions and pc relative instructions relocation.
>
>   I would like to post that in the next few weeks, but it would be
>   better if the normal tracepoints were in before that.
>
>   Is it a good time to review these patches now?

 - handle unavailable memory/register in frame unwinding in target
   independent part, so that we don't have to worry about the
   unavailable memory in arm backend.
   I am writing a prototype according to Pedro's thoughts,
   https://sourceware.org/ml/gdb-patches/2016-02/msg00778.html
   but it is blocked by a patch related PR 19947,
   https://sourceware.org/ml/gdb-patches/2016-04/msg00429.html
   we need an approach to test each unwinder, the discussion is still
   ongoing.

I won't review arm tracepoint patches until all of them above are
fixed.

-- 
Yao (齐尧)

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>>   From what I can tell issue 1) is about done ?
>>
>
> There are still some things we need to do before arm tracepoint support,
> and I am working on them,
>
>  - really exercise the software single step in gdbserver,
>    1. software single step over instruction branch to itself.  [DONE]
>    2. force gdb to use vCont;s for gdbserver using software single step,
>    IOW, let gdbserver handle single step requested by gdb,
>    3. turn range stepping on on arm linux,
>
>    I have some patches in my tree.  After these steps, we are confident
>    that software single step in gdbserver is reliable.

OK. Thanks for the update.

Is your tree public somewhere btw ? As we're (Simon and I) almost done
with the fast tracepoints if we can help with this (2. 3.) we would be
glad to.

>
>>   On my end we have fast tracepoints for arm almost ready with JIT
>>   conditions and pc relative instructions relocation.
>>
>>   I would like to post that in the next few weeks, but it would be
>>   better if the normal tracepoints were in before that.
>>
>>   Is it a good time to review these patches now?
>
>  - handle unavailable memory/register in frame unwinding in target
>    independent part, so that we don't have to worry about the
>    unavailable memory in arm backend.
>    I am writing a prototype according to Pedro's thoughts,
>    https://sourceware.org/ml/gdb-patches/2016-02/msg00778.html
>    but it is blocked by a patch related PR 19947,
>    https://sourceware.org/ml/gdb-patches/2016-04/msg00429.html
>    we need an approach to test each unwinder, the discussion is still
>    ongoing.
>

Thanks for working on that one!

Note however that this only affects the tracing of pseudo registers
iirc, maybe we can live without this at first and add it as an
improvement.

Moreover, the required code changes to fix this issue have
no impact on the tracepoint patches afaik, so I don't see it as a hard
prerequisite for tracepoints.

> I won't review arm tracepoint patches until all of them above are
> fixed.

I will still send the fast tracepoints patches when they are ready as
the code is quite independant from these issues in the hope that we can
start the review process asap.

Regards,
Antoine

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> Is your tree public somewhere btw ? As we're (Simon and I) almost done
> with the fast tracepoints if we can help with this (2. 3.) we would be
> glad to.

This tree https://github.com/qiyao/gdb/tree/arm-sw-single-step-2
includes two commits that
 1) forces GDB use vCont;s with arm-linux gdbserver,
 2) enable range stepping on arm-linux,

they'll cause some test failures, and you can start from them.  I am not
sure what is the best fix could be so far, so I don't publish my fixes.
My patches might be completely wrong.

I don't mind if you guys jump in the muddy puddles together with me.

>
>>
>>>   On my end we have fast tracepoints for arm almost ready with JIT
>>>   conditions and pc relative instructions relocation.
>>>
>>>   I would like to post that in the next few weeks, but it would be
>>>   better if the normal tracepoints were in before that.
>>>
>>>   Is it a good time to review these patches now?
>>
>>  - handle unavailable memory/register in frame unwinding in target
>>    independent part, so that we don't have to worry about the
>>    unavailable memory in arm backend.
>>    I am writing a prototype according to Pedro's thoughts,
>>    https://sourceware.org/ml/gdb-patches/2016-02/msg00778.html
>>    but it is blocked by a patch related PR 19947,
>>    https://sourceware.org/ml/gdb-patches/2016-04/msg00429.html
>>    we need an approach to test each unwinder, the discussion is still
>>    ongoing.
>>
>
> Thanks for working on that one!
>
> Note however that this only affects the tracing of pseudo registers
> iirc, maybe we can live without this at first and add it as an
> improvement.
>
> Moreover, the required code changes to fix this issue have
> no impact on the tracepoint patches afaik, so I don't see it as a hard
> prerequisite for tracepoints.

I don't think so.  If that is done, unwinders in each target don't have
to worry about the unavailable memory/register, your patch 1/4 in this
series is no longer needed.

-- 
Yao (齐尧)

Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> Is your tree public somewhere btw ? As we're (Simon and I) almost done
>> with the fast tracepoints if we can help with this (2. 3.) we would be
>> glad to.
>
> This tree https://github.com/qiyao/gdb/tree/arm-sw-single-step-2
> includes two commits that
>  1) forces GDB use vCont;s with arm-linux gdbserver,
>  2) enable range stepping on arm-linux,
>
> they'll cause some test failures, and you can start from them.  I am not
> sure what is the best fix could be so far, so I don't publish my fixes.
> My patches might be completely wrong.
>

That's completly fine, thanks for sharing your work in progress, it
should give us a good start.

> I don't mind if you guys jump in the muddy puddles together with me.
>

Thanks, we'll do our best.

>>
>>>
>>>>   On my end we have fast tracepoints for arm almost ready with JIT
>>>>   conditions and pc relative instructions relocation.
>>>>
>>>>   I would like to post that in the next few weeks, but it would be
>>>>   better if the normal tracepoints were in before that.
>>>>
>>>>   Is it a good time to review these patches now?
>>>
>>>  - handle unavailable memory/register in frame unwinding in target
>>>    independent part, so that we don't have to worry about the
>>>    unavailable memory in arm backend.
>>>    I am writing a prototype according to Pedro's thoughts,
>>>    https://sourceware.org/ml/gdb-patches/2016-02/msg00778.html
>>>    but it is blocked by a patch related PR 19947,
>>>    https://sourceware.org/ml/gdb-patches/2016-04/msg00429.html
>>>    we need an approach to test each unwinder, the discussion is still
>>>    ongoing.
>>>
>>
>> Thanks for working on that one!
>>
>> Note however that this only affects the tracing of pseudo registers
>> iirc, maybe we can live without this at first and add it as an
>> improvement.
>>
>> Moreover, the required code changes to fix this issue have
>> no impact on the tracepoint patches afaik, so I don't see it as a hard
>> prerequisite for tracepoints.
>
> I don't think so.  If that is done, unwinders in each target don't have
> to worry about the unavailable memory/register, your patch 1/4 in this
> series is no longer needed.

Yes the 1/4 patch is no longer needed, that's a given we can abandon it,
but I don't see it impacting anything else code wise.

We can try to coordonate a bit too if you want since for us the unwinder
patch seems harder than the single step fixing, if you want to focus on
the unwinder, we will focus on the vCont/range and hopefully we'll make faster
progress ?

Thanks,
Antoine

Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

After trying two different approaches of ultimate-fallback unwinder, and
think again about 'wrapping methods of struct frame_unwind', I think
'wrapping struct frame_unwind methods' still works, maybe.

> There are a few constraints that we need to keep in mind:
>
> - Frames that only have the PC available should have distinct frame ids,
>   and it should be distinct from outer_frame_id.  (See frame_id_build_unavailable_stack calls).
>

This can be done by wrapping fi->unwind->this_id with TRY/CATCH, call
get_frame_pc_if_available, if PC is available, call
frame_id_build_unavailable_stack otherwise, fall back to
outer_frame_id.  I did that in my patch below,

>   This makes e.g., the frame_id_eq check in tfind_1 work as intended, see:
>    https://sourceware.org/ml/gdb-patches/2013-12/msg00535.html
>
> - When an unwind sniffer throws, it'll destroy its
>   struct frame_unwind_cache.  So if we don't catch the error, the
>   frame's this_id method can't return something more detailed than
>   outer_frame_id.

unwinder->sniffer is wrapped by TRY/CATCH nowadays, so we don't have to
change anything.

>
> I don't see this done by wrapping methods of 'struct frame_unwind'.

>
> I think it'd work to have an ultimate-fallback unwinder that
> frame_unwind_find_by_frame returns instead of the internal error at
> the end.  This would return UNWIND_UNAVAILABLE or UNWIND_MEMORY_ERROR
> in the unwinder->stop_reason method, depending on the error the last registered
> unwinder thrown.  (That last unwinder will always be the arch's
> heuristic unwinder.)

In my patch, this_frame->unwind->stop_reason is wrapped by TRY/CATCH,
and the stop_reason is set to UNWIND_UNAVAILABLE if error is
NOT_AVAILABLE_ERROR.  I don't set stop_reason to UNWIND_MEMORY_ERROR as
you suggested, because I think it can be a follow-up improvement.  Let
us focus on unavailable things first.

> And it would return frame_id_build_unavailable_stack(PC) in the unwinder->this_id
> method if the last error was UNWIND_UNAVAILABLE, outer_frame_id otherwise
> (or we add a new frame_id_build_stackless function, to go along with
> frame_id_build_unavailable_stack).

I think my patch can do this.  The patch below is an RFC.  Run
gdb.trace/*.exp tests on both x86_64-linux and aarch64-linux.  What do
you think?  This patch
https://sourceware.org/ml/gdb-patches/2016-04/msg00429.html is applied
locally to make sure x86 prologue unwinders are selected by gdb.

-- 
Yao (齐尧)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6f2e38e..52d89b7f 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -559,8 +559,7 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -687,8 +686,7 @@ aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0065523..192d27b 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2510,8 +2510,7 @@ amd64_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2638,8 +2637,7 @@ amd64_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2819,8 +2817,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
diff --git a/gdb/frame.c b/gdb/frame.c
index d621dd7..60deca3 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -478,7 +478,26 @@ compute_frame_id (struct frame_info *fi)
   /* Find THIS frame's ID.  */
   /* Default to outermost if no ID is found.  */
   fi->this_id.value = outer_frame_id;
-  fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+
+  TRY
+    {
+      fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error == NOT_AVAILABLE_ERROR)
+	{
+	  CORE_ADDR pc;
+
+	  /* Fall back to outer_frame_id if PC isn't available.  */
+	  if (get_frame_pc_if_available (fi, &pc))
+	    fi->this_id.value = frame_id_build_unavailable_stack (pc);
+	}
+      else
+	throw_exception (ex);
+    }
+  END_CATCH
+
   gdb_assert (frame_id_p (fi->this_id.value));
   fi->this_id.p = 1;
   if (frame_debug)
@@ -1882,9 +1901,20 @@ get_prev_frame_always_1 (struct frame_info *this_frame)
 
   /* Check that this frame is unwindable.  If it isn't, don't try to
      unwind to the prev frame.  */
-  this_frame->stop_reason
-    = this_frame->unwind->stop_reason (this_frame,
-				       &this_frame->prologue_cache);
+  TRY
+    {
+      this_frame->stop_reason
+	= this_frame->unwind->stop_reason (this_frame,
+					   &this_frame->prologue_cache);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error == NOT_AVAILABLE_ERROR)
+	this_frame->stop_reason = UNWIND_UNAVAILABLE;
+      else
+	throw_exception (ex);
+    }
+  END_CATCH
 
   if (this_frame->stop_reason != UNWIND_NO_REASON)
     {
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83a4881..f52eb0f 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2074,8 +2074,7 @@ i386_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2254,8 +2253,7 @@ i386_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2450,8 +2448,7 @@ i386_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH