[PATCH 0/8] [AArch64] Add support for tracepoints

[PATCH 0/8] [AArch64] Add support for tracepoints

[Pierre Langlois]

Hi all,

These patches enable tracepoints for AArch64.  Although tracepoints are
enabled in GDBServer with the last patch, most of the changes are in GDB.
The most important changes teach AArch64's frame unwinders to report when
the inferior is unavailable.

The first three patches refactor the frame caches.  The idea is to keep
accesses to the inferior's registers in aarch64_make_prologue_cache and
aarch64_make_stub_cache.  This way the following patches can easily catch
exceptions when the inferior is unavailable.

The following two patches teach AArch64's unwinders to terminate
gracefully, in a similar way as it was done for x86 here:

https://sourceware.org/ml/gdb-patches/2011-02/msg00611.html

It fixes cases where we do not have debugging information and AArch64's
unwinders need to be used when examining a trace buffer.  In this context
we cannot assume that the inferior's memory and registers are available.

Thanks,
Pierre

Pierre Langlois (8):
  [AArch64] Refactor aarch64_make_prologue_cache
  [AArch64] Refactor aarch64_make_stub_cache
  [AArch64] Only access inferior registers when creating a frame cache
  [AArch64] Teach prologue unwinder to terminate gracefully
  [AArch64] Teach stub unwinder to terminate gracefully
  [AArch64] Implement gdbarch_gen_return_address gdbarch method
  [testsuite][AArch64] Port gdb.trace
  [GDBServer][AArch64] Enable support for tracepoints

 gdb/aarch64-tdep.c                                 | 183 +++++++++++++++------
 gdb/gdbserver/linux-aarch64-low.c                  |  10 ++
 gdb/testsuite/gdb.trace/backtrace.exp              |   3 +
 gdb/testsuite/gdb.trace/collection.exp             |   4 +
 .../gdb.trace/mi-trace-frame-collected.exp         |   2 +
 gdb/testsuite/gdb.trace/mi-trace-unavailable.exp   |   2 +
 gdb/testsuite/gdb.trace/report.exp                 |   4 +
 gdb/testsuite/gdb.trace/trace-break.exp            |   2 +
 gdb/testsuite/gdb.trace/unavailable.exp            |   4 +
 gdb/testsuite/gdb.trace/while-dyn.exp              |   2 +
 10 files changed, 166 insertions(+), 50 deletions(-)

-- 
2.1.0

[PATCH 7/8] [testsuite][AArch64] Port gdb.trace

[Pierre Langlois]

This patch adds support for AArch64 to the gdb.trace testsuite.

Note that it does not add support for testing fast tracepoint as it
isn't supported.  Therefore the test cases with inline assembly are not
ported in this patch, as we do not know what the conditions for
inserting a fast tracepoint on AArch64 would be.

gdb/testsuite/ChangeLog:

	* gdb.trace/backtrace.exp: Set registers for aarch64*.
	* gdb.trace/collection.exp: Likewise.
	* gdb.trace/mi-trace-frame-collected.exp: Likewise.
	* gdb.trace/mi-trace-unavailable.exp: Likewise.
	* gdb.trace/report.exp: Likewise.
	* gdb.trace/trace-break.exp: Likewise.
	* gdb.trace/unavailable.exp: Likewise.
	* gdb.trace/while-dyn.exp: Likewise.
---
 gdb/testsuite/gdb.trace/backtrace.exp                | 3 +++
 gdb/testsuite/gdb.trace/collection.exp               | 4 ++++
 gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp | 2 ++
 gdb/testsuite/gdb.trace/mi-trace-unavailable.exp     | 2 ++
 gdb/testsuite/gdb.trace/report.exp                   | 4 ++++
 gdb/testsuite/gdb.trace/trace-break.exp              | 2 ++
 gdb/testsuite/gdb.trace/unavailable.exp              | 4 ++++
 gdb/testsuite/gdb.trace/while-dyn.exp                | 2 ++
 8 files changed, 23 insertions(+)

diff --git a/gdb/testsuite/gdb.trace/backtrace.exp b/gdb/testsuite/gdb.trace/backtrace.exp
index 045778e..cf8b0ef 100644
--- a/gdb/testsuite/gdb.trace/backtrace.exp
+++ b/gdb/testsuite/gdb.trace/backtrace.exp
@@ -146,6 +146,9 @@ if [is_amd64_regs_target] {
 } elseif [is_x86_like_target] {
     set fpreg "\$ebp"
     set spreg "\$esp"
+} elseif [istarget "aarch64*"] {
+    set fpreg "\$x29"
+    set spreg "\$sp"
 } else {
     set fpreg "\$fp"
     set spreg "\$sp"
diff --git a/gdb/testsuite/gdb.trace/collection.exp b/gdb/testsuite/gdb.trace/collection.exp
index bd42cfa..de04ae2 100644
--- a/gdb/testsuite/gdb.trace/collection.exp
+++ b/gdb/testsuite/gdb.trace/collection.exp
@@ -44,6 +44,10 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [istarget "aarch64*"] {
+    set fpreg "x29"
+    set spreg "sp"
+    set pcreg "pc"
 } else {
     set fpreg "fp"
     set spreg "sp"
diff --git a/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp b/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
index 51ed479..b4991c9 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
@@ -56,6 +56,8 @@ if [is_amd64_regs_target] {
     set pcreg "rip"
 } elseif [is_x86_like_target] {
     set pcreg "eip"
+} elseif [istarget "aarch64*"] {
+    set pcreg "pc"
 } else {
     # Other ports that support tracepoints should set the name of pc
     # register here.
diff --git a/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp b/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
index 6b97d9d..eb9f745 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
@@ -135,6 +135,8 @@ proc test_trace_unavailable { data_source } {
 	    set pcnum 16
 	} elseif [is_x86_like_target] {
 	    set pcnum 8
+	} elseif [istarget "aarch64*"] {
+	    set pcnum 32
 	} else {
 	    # Other ports support tracepoint should define the number
 	    # of its own pc register.
diff --git a/gdb/testsuite/gdb.trace/report.exp b/gdb/testsuite/gdb.trace/report.exp
index 2fa676b..cd02123 100644
--- a/gdb/testsuite/gdb.trace/report.exp
+++ b/gdb/testsuite/gdb.trace/report.exp
@@ -158,6 +158,10 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [istarget "aarch64*"] {
+    set fpreg "x29"
+    set spreg "sp"
+    set pcreg "pc"
 } else {
     set fpreg "fp"
     set spreg "sp"
diff --git a/gdb/testsuite/gdb.trace/trace-break.exp b/gdb/testsuite/gdb.trace/trace-break.exp
index 4283ca6..5044c6f 100644
--- a/gdb/testsuite/gdb.trace/trace-break.exp
+++ b/gdb/testsuite/gdb.trace/trace-break.exp
@@ -49,6 +49,8 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [istarget "aarch64*"] {
+    set fpreg "x29"
 }
 
 # Set breakpoint and tracepoint at the same address.
diff --git a/gdb/testsuite/gdb.trace/unavailable.exp b/gdb/testsuite/gdb.trace/unavailable.exp
index 5be9704..0fe11fe 100644
--- a/gdb/testsuite/gdb.trace/unavailable.exp
+++ b/gdb/testsuite/gdb.trace/unavailable.exp
@@ -34,6 +34,10 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [istarget "aarch64*"] {
+    set fpreg "x29"
+    set spreg "sp"
+    set pcreg "pc"
 } else {
     set fpreg "fp"
     set spreg "sp"
diff --git a/gdb/testsuite/gdb.trace/while-dyn.exp b/gdb/testsuite/gdb.trace/while-dyn.exp
index 198421e..21c03e2 100644
--- a/gdb/testsuite/gdb.trace/while-dyn.exp
+++ b/gdb/testsuite/gdb.trace/while-dyn.exp
@@ -47,6 +47,8 @@ if [is_amd64_regs_target] {
     set fpreg "\$rbp"
 } elseif [is_x86_like_target] {
     set fpreg "\$ebp"
+} elseif [istarget "aarch64*"] {
+    set fpreg "\$x29"
 } else {
     set fpreg "\$fp"
 }
-- 
2.1.0

Re: [PATCH 7/8] [testsuite][AArch64] Port gdb.trace

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> +} elseif [istarget "aarch64*"] {
> +    set fpreg "\$x29"
> +    set spreg "\$sp"

We need to check is_aarch64_target instead of istarget "aarch64*",
considering the WIP aarch64 multi-arch debugging work.  Please replace
istarget "aarch64*" with is_aarch64_target in your patch.

OK with the change.

-- 
Yao (齐尧)

[PATCH v2 7/8] [testsuite][AArch64] Port gdb.trace

[Pierre Langlois]

Here is the updated patch.

Thanks,
Pierre

---

This patch adds support for AArch64 to the gdb.trace testsuite.

Note that it does not add support for testing fast tracepoint as it
isn't supported.  Therefore the test cases with inline assembly are not
ported in this patch, as we do not know what the conditions for
inserting a fast tracepoint on AArch64 would be.

gdb/testsuite/ChangeLog:

	* gdb.trace/backtrace.exp: Set registers for aarch64 target.
	* gdb.trace/collection.exp: Likewise.
	* gdb.trace/mi-trace-frame-collected.exp: Likewise.
	* gdb.trace/mi-trace-unavailable.exp: Likewise.
	* gdb.trace/report.exp: Likewise.
	* gdb.trace/trace-break.exp: Likewise.
	* gdb.trace/unavailable.exp: Likewise.
	* gdb.trace/while-dyn.exp: Likewise.
---
 gdb/testsuite/gdb.trace/backtrace.exp                | 3 +++
 gdb/testsuite/gdb.trace/collection.exp               | 4 ++++
 gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp | 2 ++
 gdb/testsuite/gdb.trace/mi-trace-unavailable.exp     | 2 ++
 gdb/testsuite/gdb.trace/report.exp                   | 4 ++++
 gdb/testsuite/gdb.trace/trace-break.exp              | 2 ++
 gdb/testsuite/gdb.trace/unavailable.exp              | 4 ++++
 gdb/testsuite/gdb.trace/while-dyn.exp                | 2 ++
 8 files changed, 23 insertions(+)

diff --git a/gdb/testsuite/gdb.trace/backtrace.exp b/gdb/testsuite/gdb.trace/backtrace.exp
index 045778e..f69089b 100644
--- a/gdb/testsuite/gdb.trace/backtrace.exp
+++ b/gdb/testsuite/gdb.trace/backtrace.exp
@@ -146,6 +146,9 @@ if [is_amd64_regs_target] {
 } elseif [is_x86_like_target] {
     set fpreg "\$ebp"
     set spreg "\$esp"
+} elseif [is_aarch64_target] {
+    set fpreg "\$x29"
+    set spreg "\$sp"
 } else {
     set fpreg "\$fp"
     set spreg "\$sp"
diff --git a/gdb/testsuite/gdb.trace/collection.exp b/gdb/testsuite/gdb.trace/collection.exp
index bd42cfa..69ad6ee 100644
--- a/gdb/testsuite/gdb.trace/collection.exp
+++ b/gdb/testsuite/gdb.trace/collection.exp
@@ -44,6 +44,10 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [is_aarch64_target] {
+    set fpreg "x29"
+    set spreg "sp"
+    set pcreg "pc"
 } else {
     set fpreg "fp"
     set spreg "sp"
diff --git a/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp b/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
index 51ed479..a7bed0b 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
@@ -56,6 +56,8 @@ if [is_amd64_regs_target] {
     set pcreg "rip"
 } elseif [is_x86_like_target] {
     set pcreg "eip"
+} elseif [is_aarch64_target] {
+    set pcreg "pc"
 } else {
     # Other ports that support tracepoints should set the name of pc
     # register here.
diff --git a/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp b/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
index 6b97d9d..ea9cddd 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
@@ -135,6 +135,8 @@ proc test_trace_unavailable { data_source } {
 	    set pcnum 16
 	} elseif [is_x86_like_target] {
 	    set pcnum 8
+	} elseif [is_aarch64_target] {
+	    set pcnum 32
 	} else {
 	    # Other ports support tracepoint should define the number
 	    # of its own pc register.
diff --git a/gdb/testsuite/gdb.trace/report.exp b/gdb/testsuite/gdb.trace/report.exp
index 2fa676b..b3e9000 100644
--- a/gdb/testsuite/gdb.trace/report.exp
+++ b/gdb/testsuite/gdb.trace/report.exp
@@ -158,6 +158,10 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [is_aarch64_target] {
+    set fpreg "x29"
+    set spreg "sp"
+    set pcreg "pc"
 } else {
     set fpreg "fp"
     set spreg "sp"
diff --git a/gdb/testsuite/gdb.trace/trace-break.exp b/gdb/testsuite/gdb.trace/trace-break.exp
index 4283ca6..1f57601 100644
--- a/gdb/testsuite/gdb.trace/trace-break.exp
+++ b/gdb/testsuite/gdb.trace/trace-break.exp
@@ -49,6 +49,8 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [is_aarch64_target] {
+    set fpreg "x29"
 }
 
 # Set breakpoint and tracepoint at the same address.
diff --git a/gdb/testsuite/gdb.trace/unavailable.exp b/gdb/testsuite/gdb.trace/unavailable.exp
index 5be9704..910c1dd 100644
--- a/gdb/testsuite/gdb.trace/unavailable.exp
+++ b/gdb/testsuite/gdb.trace/unavailable.exp
@@ -34,6 +34,10 @@ if [is_amd64_regs_target] {
     set fpreg "ebp"
     set spreg "esp"
     set pcreg "eip"
+} elseif [is_aarch64_target] {
+    set fpreg "x29"
+    set spreg "sp"
+    set pcreg "pc"
 } else {
     set fpreg "fp"
     set spreg "sp"
diff --git a/gdb/testsuite/gdb.trace/while-dyn.exp b/gdb/testsuite/gdb.trace/while-dyn.exp
index 198421e..fe4535e 100644
--- a/gdb/testsuite/gdb.trace/while-dyn.exp
+++ b/gdb/testsuite/gdb.trace/while-dyn.exp
@@ -47,6 +47,8 @@ if [is_amd64_regs_target] {
     set fpreg "\$rbp"
 } elseif [is_x86_like_target] {
     set fpreg "\$ebp"
+} elseif [is_aarch64_target] {
+    set fpreg "\$x29"
 } else {
     set fpreg "\$fp"
 }
-- 
2.1.0

Re: [PATCH v2 7/8] [testsuite][AArch64] Port gdb.trace

[Pierre Langlois]

Fixed and pushed.

Thanks,
Pierre

[PATCH 3/8] [AArch64] Only access inferior registers when creating a frame cache

[Pierre Langlois]

This patch moves the address of the start of a function (func) and the
address from which it was called (prev_pc) into aarch64_prologue_cache.
The idea is to keep accesses to the inferior's registers into
aarch64_make_prologue_cache and aarch64_make_stub_cache.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_prologue_cache) <func, prev_pc>: New
	fields.
	(aarch64_scan_prologue): Set prev_pc.
	(aarch64_make_prologue_cache): Set func.
	(aarch64_make_stub_cache): Set prev_pc.

	(aarch64_prologue_this_id): Remove local variables id, pc and
	func.  Read prev_pc and func from cache.
	(aarch64_stub_this_id): Read prev_pc from cache.
---
 gdb/aarch64-tdep.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 90a63e9..6a38f18 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -148,6 +148,15 @@ static const char *const aarch64_v_register_names[] =
 /* AArch64 prologue cache structure.  */
 struct aarch64_prologue_cache
 {
+  /* The program counter at the start of the function.  It is used to
+     identify this frame as a prologue frame.  */
+  CORE_ADDR func;
+
+  /* The program counter at the time this frame was created; i.e. where
+     this function was called from.  It is used to identify this frame as a
+     stub frame.  */
+  CORE_ADDR prev_pc;
+
   /* The stack pointer at the time this frame was created; i.e. the
      caller's stack pointer when this function was called.  It is used
      to identify this frame.  */
@@ -889,6 +898,8 @@ aarch64_scan_prologue (struct frame_info *this_frame,
   CORE_ADDR prev_pc = get_frame_pc (this_frame);
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
 
+  cache->prev_pc = prev_pc;
+
   /* Assume we do not find a frame.  */
   cache->framereg = -1;
   cache->framesize = 0;
@@ -964,6 +975,8 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
     if (trad_frame_addr_p (cache->saved_regs, reg))
       cache->saved_regs[reg].addr += cache->prev_sp;
 
+  cache->func = get_frame_func (this_frame);
+
   return cache;
 }
 
@@ -976,21 +989,16 @@ aarch64_prologue_this_id (struct frame_info *this_frame,
 {
   struct aarch64_prologue_cache *cache
     = aarch64_make_prologue_cache (this_frame, this_cache);
-  struct frame_id id;
-  CORE_ADDR pc, func;
 
   /* 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)
+  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
     return;
 
   /* If we've hit a wall, stop.  */
   if (cache->prev_sp == 0)
     return;
 
-  func = get_frame_func (this_frame);
-  id = frame_id_build (cache->prev_sp, func);
-  *this_id = id;
+  *this_id = frame_id_build (cache->prev_sp, cache->func);
 }
 
 /* Implement the "prev_register" frame_unwind method.  */
@@ -1065,6 +1073,7 @@ aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
 
   cache->prev_sp
     = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
+  cache->prev_pc = get_frame_pc (this_frame);
 
   return cache;
 }
@@ -1078,7 +1087,7 @@ aarch64_stub_this_id (struct frame_info *this_frame,
   struct aarch64_prologue_cache *cache
     = aarch64_make_stub_cache (this_frame, this_cache);
 
-  *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
+  *this_id = frame_id_build (cache->prev_sp, cache->prev_pc);
 }
 
 /* Implement the "sniffer" frame_unwind method.  */
-- 
2.1.0

Re: [PATCH 3/8] [AArch64] Only access inferior registers when creating a frame cache

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> gdb/ChangeLog:
>
> 	* aarch64-tdep.c (aarch64_prologue_cache) <func, prev_pc>: New
> 	fields.
> 	(aarch64_scan_prologue): Set prev_pc.
> 	(aarch64_make_prologue_cache): Set func.
> 	(aarch64_make_stub_cache): Set prev_pc.
>
> 	(aarch64_prologue_this_id): Remove local variables id, pc and
> 	func.  Read prev_pc and func from cache.
> 	(aarch64_stub_this_id): Read prev_pc from cache.

It is OK to me.

-- 
Yao (齐尧)

Re: [PATCH 3/8] [AArch64] Only access inferior registers when creating a frame cache

[Pierre Langlois]

On 08/07/15 17:15, Yao Qi wrote:
> Pierre Langlois <pierre.langlois@arm.com> writes:
> 
>> gdb/ChangeLog:
>>
>> 	* aarch64-tdep.c (aarch64_prologue_cache) <func, prev_pc>: New
>> 	fields.
>> 	(aarch64_scan_prologue): Set prev_pc.
>> 	(aarch64_make_prologue_cache): Set func.
>> 	(aarch64_make_stub_cache): Set prev_pc.
>>
>> 	(aarch64_prologue_this_id): Remove local variables id, pc and
>> 	func.  Read prev_pc and func from cache.
>> 	(aarch64_stub_this_id): Read prev_pc from cache.
> 
> It is OK to me.
> 

Thanks, I've removed the empty line in the ChangeLog and pushed this.

[PATCH 2/8] [AArch64] Refactor aarch64_make_stub_cache

[Pierre Langlois]

We would previously have to make sure the frame cache was not already
created before calling aarch64_make_stub_cache.  This patch makes this
function check it so the caller does not need to do so.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_make_stub_cache): New argument
	this_cache.  Remove unused local variables reg and unwound_fp.
	Return early if this_cache is already set.  Set this_cache.
	(aarch64_stub_this_id): Update call to aarch64_make_stub_cache.
---
 gdb/aarch64-tdep.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index c4b7fe8..90a63e9 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1052,14 +1052,16 @@ struct frame_unwind aarch64_prologue_unwind =
    about the prologue of *THIS_FRAME.  */
 
 static struct aarch64_prologue_cache *
-aarch64_make_stub_cache (struct frame_info *this_frame)
+aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
 {
-  int reg;
   struct aarch64_prologue_cache *cache;
-  CORE_ADDR unwound_fp;
+
+  if (*this_cache)
+    return *this_cache;
 
   cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  *this_cache = cache;
 
   cache->prev_sp
     = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
@@ -1073,11 +1075,8 @@ static void
 aarch64_stub_this_id (struct frame_info *this_frame,
 		      void **this_cache, struct frame_id *this_id)
 {
-  struct aarch64_prologue_cache *cache;
-
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_stub_cache (this_frame);
-  cache = *this_cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_stub_cache (this_frame, this_cache);
 
   *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
 }
-- 
2.1.0

Re: [PATCH 2/8] [AArch64] Refactor aarch64_make_stub_cache

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

>  static struct aarch64_prologue_cache *
> -aarch64_make_stub_cache (struct frame_info *this_frame)
> +aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
>  {

We need comments on this function, about its arguments and return value.

> -  int reg;
>    struct aarch64_prologue_cache *cache;
> -  CORE_ADDR unwound_fp;
> +
> +  if (*this_cache)

     if (*this_cache != NULL)

> +    return *this_cache;

OK with these changes.

-- 
Yao (齐尧)

[PATCH v2 2/8] [AArch64] Refactor aarch64_make_stub_cache

[Pierre Langlois]

Here is the updated patch that I'll push.

Thanks,
Pierre

---

We would previously have to make sure the frame cache was not already
created before calling aarch64_make_stub_cache.  This patch makes this
function check it so the caller does not need to do so.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_make_stub_cache): Update comment.  New
	argument this_cache.  Remove unused local variables reg and
	unwound_fp.  Return early if this_cache is already set.  Set
	this_cache.
	(aarch64_stub_this_id): Update call to aarch64_make_stub_cache.
---
 gdb/aarch64-tdep.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 0496536..51dda92 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1050,18 +1050,22 @@ struct frame_unwind aarch64_prologue_unwind =
   default_frame_sniffer
 };
 
-/* Allocate an aarch64_prologue_cache and fill it with information
-   about the prologue of *THIS_FRAME.  */
+/* Allocate and fill in *THIS_CACHE with information about the prologue of
+   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
+   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
+   */
 
 static struct aarch64_prologue_cache *
-aarch64_make_stub_cache (struct frame_info *this_frame)
+aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
 {
-  int reg;
   struct aarch64_prologue_cache *cache;
-  CORE_ADDR unwound_fp;
+
+  if (*this_cache != NULL)
+    return *this_cache;
 
   cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  *this_cache = cache;
 
   cache->prev_sp
     = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
@@ -1075,11 +1079,8 @@ static void
 aarch64_stub_this_id (struct frame_info *this_frame,
 		      void **this_cache, struct frame_id *this_id)
 {
-  struct aarch64_prologue_cache *cache;
-
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_stub_cache (this_frame);
-  cache = *this_cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_stub_cache (this_frame, this_cache);
 
   *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
 }
-- 
2.1.0

Re: [PATCH v2 2/8] [AArch64] Refactor aarch64_make_stub_cache

[Pierre Langlois]

> -/* Allocate an aarch64_prologue_cache and fill it with information
> -   about the prologue of *THIS_FRAME.  */
> +/* Allocate and fill in *THIS_CACHE with information about the prologue of
> +   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
> +   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
> +   */

Pushed with this comment's formatting fixed.

Thanks,
Pierre

[PATCH 5/8] [AArch64] Teach stub unwinder to terminate gracefully

[Pierre Langlois]

The stub unwinder is used on AArch64 if the target's memory is not
readable at the current PC.  If we purposely kill the inferior before
examining the trace then we get the following issue:

~~~
...
(gdb) trace f
Tracepoint 3 at 0x7fb7fc28c0
(gdb) tstart
(gdb) continue
...
(gdb) tstop
(gdb) tsave /tmp/trace
(gdb) kill
...
(gdb) target tfile /tmp/trace
...
(gdb) tfind
Register 31 is not available.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Found trace frame 0, tracepoint 3
#-1 0x0000007fb7fc28c0 in f () ...
^^^
~~~

This patch teaches the stub unwinder to report to the core frame code
with UNWIND_UNAVAILABLE when either the stack pointer of the return
address are unavailable to read from the target.

The following test cases now pass when enabling tracepoints:

PASS: gdb.trace/pending.exp: trace works: tfind test frame 0
PASS: gdb.trace/pending.exp: trace resolved_in_trace: tfind test frame 0
PASS: gdb.trace/pending.exp: trace installed_in_trace: tfind test frame 0
PASS: gdb.trace/mi-tracepoint-changed.exp: pending resolved: -trace-find frame-number 0

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_make_stub_cache): Set available_p and
	swallow NOT_AVAILABLE_ERROR.
	(aarch64_stub_this_id): Call frame_id_build_unavailable_stack if
	available_p is not set.
	(aarch64_stub_frame_unwind_stop_reason): New function.
	(aarch64_stub_unwind): Install it.
---
 gdb/aarch64-tdep.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 87a6d61..57976b7 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1109,13 +1109,36 @@ aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
   *this_cache = cache;
 
-  cache->prev_sp
-    = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
-  cache->prev_pc = get_frame_pc (this_frame);
+  TRY
+    {
+      cache->prev_sp
+	= get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
+      cache->prev_pc = get_frame_pc (this_frame);
+      cache->available_p = 1;
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
 
   return cache;
 }
 
+static enum unwind_stop_reason
+aarch64_stub_frame_unwind_stop_reason (struct frame_info *this_frame,
+				       void **this_cache)
+{
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_stub_cache (this_frame, this_cache);
+
+  if (!cache->available_p)
+    return UNWIND_UNAVAILABLE;
+
+  return UNWIND_NO_REASON;
+}
+
 /* Our frame ID for a stub frame is the current SP and LR.  */
 
 static void
@@ -1125,7 +1148,10 @@ aarch64_stub_this_id (struct frame_info *this_frame,
   struct aarch64_prologue_cache *cache
     = aarch64_make_stub_cache (this_frame, this_cache);
 
-  *this_id = frame_id_build (cache->prev_sp, cache->prev_pc);
+  if (cache->available_p)
+    *this_id = frame_id_build (cache->prev_sp, cache->prev_pc);
+  else
+    *this_id = frame_id_build_unavailable_stack (cache->prev_pc);
 }
 
 /* Implement the "sniffer" frame_unwind method.  */
@@ -1152,7 +1178,7 @@ aarch64_stub_unwind_sniffer (const struct frame_unwind *self,
 struct frame_unwind aarch64_stub_unwind =
 {
   NORMAL_FRAME,
-  default_frame_unwind_stop_reason,
+  aarch64_stub_frame_unwind_stop_reason,
   aarch64_stub_this_id,
   aarch64_prologue_prev_register,
   NULL,
-- 
2.1.0

Re: [PATCH 5/8] [AArch64] Teach stub unwinder to terminate gracefully

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> The stub unwinder is used on AArch64 if the target's memory is not
> readable at the current PC.  If we purposely kill the inferior before

The stub unwinder is used if the target memory is not readable, for
example, PC is 0x0.  Many GDB ports use stub unwinder to handle this
case.  This is not aarch64 specific.  Please update your commit log to
reflect this.

> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 87a6d61..57976b7 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1109,13 +1109,36 @@ aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
>    cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
>    *this_cache = cache;
>  
> -  cache->prev_sp
> -    = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
> -  cache->prev_pc = get_frame_pc (this_frame);
> +  TRY
> +    {
> +      cache->prev_sp
> +	= get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);

I feel this is a better way to indent the code,

         cache->prev_sp	= get_frame_register_unsigned (this_frame,
                                                      AARCH64_SP_REGNUM);

> +      cache->prev_pc = get_frame_pc (this_frame);
> +      cache->available_p = 1;
> +    }
> +  CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      if (ex.error != NOT_AVAILABLE_ERROR)
> +	throw_exception (ex);
> +    }
> +  END_CATCH
>  
>    return cache;
>  }
>  
> +static enum unwind_stop_reason
> +aarch64_stub_frame_unwind_stop_reason (struct frame_info *this_frame,
> +				       void **this_cache)
> +{

We need comments to this function.

> +  struct aarch64_prologue_cache *cache
> +    = aarch64_make_stub_cache (this_frame, this_cache);

I realise that prologue cache is used for stub unwinder.  If stub
unwinder doesn't use all the fields of prologue cache, probably, we can
create a stub cache.  However, it can be a follow-up patch.

This patch is good to me with the change.

-- 
Yao (齐尧)

[PATCH v2 5/8] [AArch64] Teach stub unwinder to terminate gracefully

[Pierre Langlois]

Here is the updated patch.

Thanks,
Pierre

---

The stub unwinder is used on AArch64 if the target's memory is not
readable at the current PC.  For example, the user could try to call at
an invalid address such as 0x0, as covered in the gdb.base/signull.exp
test case.  Many GDB ports use a similar unwinder to handle this case
too.

If we purposely kill the inferior before examining the trace then we get
the following issue:

~~~
...
(gdb) trace f
Tracepoint 3 at 0x7fb7fc28c0
(gdb) tstart
(gdb) continue
...
(gdb) tstop
(gdb) tsave /tmp/trace
(gdb) kill
...
(gdb) target tfile /tmp/trace
...
(gdb) tfind
Register 31 is not available.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Found trace frame 0, tracepoint 3
#-1 0x0000007fb7fc28c0 in f () ...
^^^
~~~

This patch teaches the stub unwinder to report to the core frame code
with UNWIND_UNAVAILABLE when either the stack pointer of the return
address are unavailable to read from the target.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_make_stub_cache): Set available_p and
	swallow NOT_AVAILABLE_ERROR.
	(aarch64_stub_this_id): Call frame_id_build_unavailable_stack if
	available_p is not set.
	(aarch64_stub_frame_unwind_stop_reason): New function.
	(aarch64_stub_unwind): Install it.
---
 gdb/aarch64-tdep.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 4a6df25..e791fcf 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1119,13 +1119,38 @@ aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
   *this_cache = cache;
 
-  cache->prev_sp
-    = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
-  cache->prev_pc = get_frame_pc (this_frame);
+  TRY
+    {
+      cache->prev_sp = get_frame_register_unsigned (this_frame,
+						    AARCH64_SP_REGNUM);
+      cache->prev_pc = get_frame_pc (this_frame);
+      cache->available_p = 1;
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
 
   return cache;
 }
 
+/* Implement the "stop_reason" frame_unwind method.  */
+
+static enum unwind_stop_reason
+aarch64_stub_frame_unwind_stop_reason (struct frame_info *this_frame,
+				       void **this_cache)
+{
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_stub_cache (this_frame, this_cache);
+
+  if (!cache->available_p)
+    return UNWIND_UNAVAILABLE;
+
+  return UNWIND_NO_REASON;
+}
+
 /* Our frame ID for a stub frame is the current SP and LR.  */
 
 static void
@@ -1135,7 +1160,10 @@ aarch64_stub_this_id (struct frame_info *this_frame,
   struct aarch64_prologue_cache *cache
     = aarch64_make_stub_cache (this_frame, this_cache);
 
-  *this_id = frame_id_build (cache->prev_sp, cache->prev_pc);
+  if (cache->available_p)
+    *this_id = frame_id_build (cache->prev_sp, cache->prev_pc);
+  else
+    *this_id = frame_id_build_unavailable_stack (cache->prev_pc);
 }
 
 /* Implement the "sniffer" frame_unwind method.  */
@@ -1162,7 +1190,7 @@ aarch64_stub_unwind_sniffer (const struct frame_unwind *self,
 struct frame_unwind aarch64_stub_unwind =
 {
   NORMAL_FRAME,
-  default_frame_unwind_stop_reason,
+  aarch64_stub_frame_unwind_stop_reason,
   aarch64_stub_this_id,
   aarch64_prologue_prev_register,
   NULL,
-- 
2.1.0

Re: [PATCH v2 5/8] [AArch64] Teach stub unwinder to terminate gracefully

[Pierre Langlois]

I've pushed this.

Thanks,
Pierre

[PATCH 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Pierre Langlois]

Without debugging information, we have the following issue when
examining a trace buffer:

~~~
...
(gdb) trace f
Tracepoint 3 at 0x7fb7fc28c0
(gdb) tstart
(gdb) continue
...
(gdb) tstop
(gdb) tfind start
Register 31 is not available.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Found trace frame 0, tracepoint 3
#-1 0x0000007fb7fc28c0 in f () ...
^^^
~~~

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 by making the prologue unwinder catch
NOT_AVAILABLE_ERROR exceptions when either registers or memory is
unreadable and report back to the frame core code with
UNWIND_UNAVAILABLE.

The following test cases now pass when enabling tracepoints:

PASS: gdb.trace/report.exp: live: 9.1: tdump, args collected
PASS: gdb.trace/report.exp: live: 9.1: tdump, locals collected
PASS: gdb.trace/report.exp: live: 12.3: trace report #3
PASS: gdb.trace/report.exp: tfile: 9.1: tdump, args collected
PASS: gdb.trace/report.exp: tfile: 9.1: tdump, locals collected
PASS: gdb.trace/report.exp: tfile: 12.3: trace report #3
PASS: gdb.trace/collection.exp: collect local string: collected local string
PASS: gdb.trace/collection.exp: collect long local string: collected local string
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing foo: tfind 0
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing foo: p/d x
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing foo: p/d y
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing foo: p/d z
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing foo: tfind none
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: trace bar
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: tstart
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: continue
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: tstop
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: tfind 0
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: p/d x
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: p/d y
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: p/d z
PASS: gdb.trace/unavailable-dwarf-piece.exp: tracing bar: tfind none

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_prologue_cache) <available_p>: New
	field.
	(aarch64_make_prologue_cache_1): New function, factored out from
	aarch64_make_prologue_cache.  Do not allocate cache.  Set
	available_p.
	(aarch64_make_prologue_cache): Reimplement wrapping
	aarch64_make_prologue_cache_1, and swallowing
	NOT_AVAILABLE_ERROR.

	(aarch64_prologue_frame_unwind_stop_reason): New function.
	Return UNWIND_UNAVAILABLE if available_p is not set.
	(aarch64_prologue_unwind): Install it.
	(aarch64_prologue_this_id): Move prev_pc and prev_sp limit
	checks into aarch64_prologue_frame_unwind_stop_reason.  Call
	frame_id_build_unavailable_stack if available_p is not set.
---
 gdb/aarch64-tdep.c | 82 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 22 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6a38f18..87a6d61 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -162,6 +162,9 @@ struct aarch64_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.  */
@@ -944,28 +947,21 @@ aarch64_scan_prologue (struct frame_info *this_frame,
 /* Allocate an aarch64_prologue_cache and fill it with information
    about the prologue of *THIS_FRAME.  */
 
-static struct aarch64_prologue_cache *
-aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
+static void
+aarch64_make_prologue_cache_1 (struct frame_info *this_frame,
+			       struct aarch64_prologue_cache *cache)
 {
-  struct aarch64_prologue_cache *cache;
   CORE_ADDR unwound_fp;
   int reg;
 
-  if (*this_cache)
-    return *this_cache;
-
-  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
-  *this_cache = cache;
-
   aarch64_scan_prologue (this_frame, cache);
 
   if (cache->framereg == -1)
-    return cache;
+    return;
 
   unwound_fp = get_frame_register_unsigned (this_frame, cache->framereg);
   if (unwound_fp == 0)
-    return cache;
+    return;
 
   cache->prev_sp = unwound_fp + cache->framesize;
 
@@ -977,9 +973,56 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
 
   cache->func = get_frame_func (this_frame);
 
+  cache->available_p = 1;
+}
+
+static struct aarch64_prologue_cache *
+aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
+{
+  struct aarch64_prologue_cache *cache;
+
+  if (*this_cache)
+    return *this_cache;
+
+  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
+  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  *this_cache = cache;
+
+  TRY
+    {
+      aarch64_make_prologue_cache_1 (this_frame, cache);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
+
   return cache;
 }
 
+static enum unwind_stop_reason
+aarch64_prologue_frame_unwind_stop_reason (struct frame_info *this_frame,
+					   void **this_cache)
+{
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
+
+  if (!cache->available_p)
+    return UNWIND_UNAVAILABLE;
+
+  /* Halt the backtrace at "_start".  */
+  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
+    return UNWIND_OUTERMOST;
+
+  /* We've hit a wall, stop.  */
+  if (cache->prev_sp == 0)
+      return UNWIND_OUTERMOST;
+
+  return UNWIND_NO_REASON;
+}
+
 /* Our frame ID for a normal frame is the current function's starting
    PC and the caller's SP when we were called.  */
 
@@ -990,15 +1033,10 @@ aarch64_prologue_this_id (struct frame_info *this_frame,
   struct aarch64_prologue_cache *cache
     = aarch64_make_prologue_cache (this_frame, this_cache);
 
-  /* This is meant to halt the backtrace at "_start".  */
-  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
-    return;
-
-  /* If we've hit a wall, stop.  */
-  if (cache->prev_sp == 0)
-    return;
-
-  *this_id = frame_id_build (cache->prev_sp, cache->func);
+  if (!cache->available_p)
+    *this_id = frame_id_build_unavailable_stack (cache->func);
+  else
+    *this_id = frame_id_build (cache->prev_sp, cache->func);
 }
 
 /* Implement the "prev_register" frame_unwind method.  */
@@ -1049,7 +1087,7 @@ aarch64_prologue_prev_register (struct frame_info *this_frame,
 struct frame_unwind aarch64_prologue_unwind =
 {
   NORMAL_FRAME,
-  default_frame_unwind_stop_reason,
+  aarch64_prologue_frame_unwind_stop_reason,
   aarch64_prologue_this_id,
   aarch64_prologue_prev_register,
   NULL,
-- 
2.1.0

Re: [PATCH 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> @@ -944,28 +947,21 @@ aarch64_scan_prologue (struct frame_info *this_frame,
>  /* Allocate an aarch64_prologue_cache and fill it with information
>     about the prologue of *THIS_FRAME.  */
>  

This comment should be moved to aarch64_make_prologue_cache below, and
we need add something for argument this_cache.  Secondly, we need
comments for this function too.

> -static struct aarch64_prologue_cache *
> -aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
> +static void
> +aarch64_make_prologue_cache_1 (struct frame_info *this_frame,
> +			       struct aarch64_prologue_cache *cache)



> +
> +static struct aarch64_prologue_cache *
> +aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
> +{
> +  struct aarch64_prologue_cache *cache;
> +
> +  if (*this_cache)

     if (*this_cache != NULL)

> +    return *this_cache;
> +
> +  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
> +  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
> +  *this_cache = cache;
> +
> +  TRY
> +    {
> +      aarch64_make_prologue_cache_1 (this_frame, cache);
> +    }
> +  CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      if (ex.error != NOT_AVAILABLE_ERROR)
> +	throw_exception (ex);
> +    }
> +  END_CATCH
> +
>    return cache;
>  }
>  
> +static enum unwind_stop_reason
> +aarch64_prologue_frame_unwind_stop_reason (struct frame_info *this_frame,
> +					   void **this_cache)

The indentation looks odd.

> +{
> +  struct aarch64_prologue_cache *cache
> +    = aarch64_make_prologue_cache (this_frame, this_cache);
> +
> +  if (!cache->available_p)
> +    return UNWIND_UNAVAILABLE;
> +
> +  /* Halt the backtrace at "_start".  */
> +  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
> +    return UNWIND_OUTERMOST;
> +
> +  /* We've hit a wall, stop.  */
> +  if (cache->prev_sp == 0)
> +      return UNWIND_OUTERMOST;

As Pedro pointed out, the indentation is wrong.

OK with changes.

-- 
Yao (齐尧)

[PATCH v2 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Pierre Langlois]

Here is the updated patch that I'll push.

Thanks,
Pierre

---

Without debugging information, we have the following issue when
examining a trace buffer:

~~~
...
(gdb) trace f
Tracepoint 3 at 0x7fb7fc28c0
(gdb) tstart
(gdb) continue
...
(gdb) tstop
(gdb) tfind start
Register 31 is not available.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Found trace frame 0, tracepoint 3
#-1 0x0000007fb7fc28c0 in f () ...
^^^
~~~

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 by making the prologue unwinder catch
NOT_AVAILABLE_ERROR exceptions when either registers or memory is
unreadable and report back to the frame core code with
UNWIND_UNAVAILABLE.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_prologue_cache) <available_p>: New
	field.
	(aarch64_make_prologue_cache_1): New function, factored out from
	aarch64_make_prologue_cache.  Do not allocate cache.  Set
	available_p.
	(aarch64_make_prologue_cache): Reimplement wrapping
	aarch64_make_prologue_cache_1, and swallowing
	NOT_AVAILABLE_ERROR.
	(aarch64_prologue_frame_unwind_stop_reason): New function.
	Return UNWIND_UNAVAILABLE if available_p is not set.
	(aarch64_prologue_unwind): Install it.
	(aarch64_prologue_this_id): Move prev_pc and prev_sp limit
	checks into aarch64_prologue_frame_unwind_stop_reason.  Call
	frame_id_build_unavailable_stack if available_p is not set.
---
 gdb/aarch64-tdep.c | 96 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 26 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 0a5d01b..6e42ad1 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -162,6 +162,9 @@ struct aarch64_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.  */
@@ -941,33 +944,25 @@ aarch64_scan_prologue (struct frame_info *this_frame,
     }
 }
 
-/* Allocate and fill in *THIS_CACHE with information about the prologue of
-   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
-   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
-   */
+/* Fill in *CACHE with information about the prologue of *THIS_FRAME.  This
+   function may throw an exception if the inferior's registers or memory is
+   not available.  */
 
-static struct aarch64_prologue_cache *
-aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
+static void
+aarch64_make_prologue_cache_1 (struct frame_info *this_frame,
+			       struct aarch64_prologue_cache *cache)
 {
-  struct aarch64_prologue_cache *cache;
   CORE_ADDR unwound_fp;
   int reg;
 
-  if (*this_cache != NULL)
-    return *this_cache;
-
-  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
-  *this_cache = cache;
-
   aarch64_scan_prologue (this_frame, cache);
 
   if (cache->framereg == -1)
-    return cache;
+    return;
 
   unwound_fp = get_frame_register_unsigned (this_frame, cache->framereg);
   if (unwound_fp == 0)
-    return cache;
+    return;
 
   cache->prev_sp = unwound_fp + cache->framesize;
 
@@ -979,9 +974,63 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
 
   cache->func = get_frame_func (this_frame);
 
+  cache->available_p = 1;
+}
+
+/* Allocate and fill in *THIS_CACHE with information about the prologue of
+   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
+   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
+   */
+
+static struct aarch64_prologue_cache *
+aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
+{
+  struct aarch64_prologue_cache *cache;
+
+  if (*this_cache)
+    return *this_cache;
+
+  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
+  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  *this_cache = cache;
+
+  TRY
+    {
+      aarch64_make_prologue_cache_1 (this_frame, cache);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
+
   return cache;
 }
 
+/* Implement the "stop_reason" frame_unwind method.  */
+
+static enum unwind_stop_reason
+aarch64_prologue_frame_unwind_stop_reason (struct frame_info *this_frame,
+					   void **this_cache)
+{
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
+
+  if (!cache->available_p)
+    return UNWIND_UNAVAILABLE;
+
+  /* Halt the backtrace at "_start".  */
+  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
+    return UNWIND_OUTERMOST;
+
+  /* We've hit a wall, stop.  */
+  if (cache->prev_sp == 0)
+    return UNWIND_OUTERMOST;
+
+  return UNWIND_NO_REASON;
+}
+
 /* Our frame ID for a normal frame is the current function's starting
    PC and the caller's SP when we were called.  */
 
@@ -992,15 +1041,10 @@ aarch64_prologue_this_id (struct frame_info *this_frame,
   struct aarch64_prologue_cache *cache
     = aarch64_make_prologue_cache (this_frame, this_cache);
 
-  /* This is meant to halt the backtrace at "_start".  */
-  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
-    return;
-
-  /* If we've hit a wall, stop.  */
-  if (cache->prev_sp == 0)
-    return;
-
-  *this_id = frame_id_build (cache->prev_sp, cache->func);
+  if (!cache->available_p)
+    *this_id = frame_id_build_unavailable_stack (cache->func);
+  else
+    *this_id = frame_id_build (cache->prev_sp, cache->func);
 }
 
 /* Implement the "prev_register" frame_unwind method.  */
@@ -1051,7 +1095,7 @@ aarch64_prologue_prev_register (struct frame_info *this_frame,
 struct frame_unwind aarch64_prologue_unwind =
 {
   NORMAL_FRAME,
-  default_frame_unwind_stop_reason,
+  aarch64_prologue_frame_unwind_stop_reason,
   aarch64_prologue_this_id,
   aarch64_prologue_prev_register,
   NULL,
-- 
2.1.0

Re: [PATCH v2 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Pierre Langlois]

On 09/07/15 11:49, Pierre Langlois wrote:
> +/* Allocate and fill in *THIS_CACHE with information about the prologue of
> +   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
> +   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
> +   */
> +
> +static struct aarch64_prologue_cache *
> +aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
> +{
> +  struct aarch64_prologue_cache *cache;
> +
> +  if (*this_cache)
> +    return *this_cache;

Oops,  I forgot to update this when rebasing.

> +
> +  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
> +  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
> +  *this_cache = cache;
> +
> +  TRY

[PATCH v3 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Pierre Langlois]

Without debugging information, we have the following issue when
examining a trace buffer:

~~~
...
(gdb) trace f
Tracepoint 3 at 0x7fb7fc28c0
(gdb) tstart
(gdb) continue
...
(gdb) tstop
(gdb) tfind start
Register 31 is not available.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Found trace frame 0, tracepoint 3
#-1 0x0000007fb7fc28c0 in f () ...
^^^
~~~

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 by making the prologue unwinder catch
NOT_AVAILABLE_ERROR exceptions when either registers or memory is
unreadable and report back to the frame core code with
UNWIND_UNAVAILABLE.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_prologue_cache) <available_p>: New
	field.
	(aarch64_make_prologue_cache_1): New function, factored out from
	aarch64_make_prologue_cache.  Do not allocate cache.  Set
	available_p.
	(aarch64_make_prologue_cache): Reimplement wrapping
	aarch64_make_prologue_cache_1, and swallowing
	NOT_AVAILABLE_ERROR.
	(aarch64_prologue_frame_unwind_stop_reason): New function.
	Return UNWIND_UNAVAILABLE if available_p is not set.
	(aarch64_prologue_unwind): Install it.
	(aarch64_prologue_this_id): Move prev_pc and prev_sp limit
	checks into aarch64_prologue_frame_unwind_stop_reason.  Call
	frame_id_build_unavailable_stack if available_p is not set.
---
 gdb/aarch64-tdep.c | 96 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 26 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 0a5d01b..4a6df25 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -162,6 +162,9 @@ struct aarch64_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.  */
@@ -941,33 +944,25 @@ aarch64_scan_prologue (struct frame_info *this_frame,
     }
 }
 
-/* Allocate and fill in *THIS_CACHE with information about the prologue of
-   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
-   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
-   */
+/* Fill in *CACHE with information about the prologue of *THIS_FRAME.  This
+   function may throw an exception if the inferior's registers or memory is
+   not available.  */
 
-static struct aarch64_prologue_cache *
-aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
+static void
+aarch64_make_prologue_cache_1 (struct frame_info *this_frame,
+			       struct aarch64_prologue_cache *cache)
 {
-  struct aarch64_prologue_cache *cache;
   CORE_ADDR unwound_fp;
   int reg;
 
-  if (*this_cache != NULL)
-    return *this_cache;
-
-  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
-  *this_cache = cache;
-
   aarch64_scan_prologue (this_frame, cache);
 
   if (cache->framereg == -1)
-    return cache;
+    return;
 
   unwound_fp = get_frame_register_unsigned (this_frame, cache->framereg);
   if (unwound_fp == 0)
-    return cache;
+    return;
 
   cache->prev_sp = unwound_fp + cache->framesize;
 
@@ -979,9 +974,63 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
 
   cache->func = get_frame_func (this_frame);
 
+  cache->available_p = 1;
+}
+
+/* Allocate and fill in *THIS_CACHE with information about the prologue of
+   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
+   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
+   */
+
+static struct aarch64_prologue_cache *
+aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
+{
+  struct aarch64_prologue_cache *cache;
+
+  if (*this_cache != NULL)
+    return *this_cache;
+
+  cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
+  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  *this_cache = cache;
+
+  TRY
+    {
+      aarch64_make_prologue_cache_1 (this_frame, cache);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error != NOT_AVAILABLE_ERROR)
+	throw_exception (ex);
+    }
+  END_CATCH
+
   return cache;
 }
 
+/* Implement the "stop_reason" frame_unwind method.  */
+
+static enum unwind_stop_reason
+aarch64_prologue_frame_unwind_stop_reason (struct frame_info *this_frame,
+					   void **this_cache)
+{
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
+
+  if (!cache->available_p)
+    return UNWIND_UNAVAILABLE;
+
+  /* Halt the backtrace at "_start".  */
+  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
+    return UNWIND_OUTERMOST;
+
+  /* We've hit a wall, stop.  */
+  if (cache->prev_sp == 0)
+    return UNWIND_OUTERMOST;
+
+  return UNWIND_NO_REASON;
+}
+
 /* Our frame ID for a normal frame is the current function's starting
    PC and the caller's SP when we were called.  */
 
@@ -992,15 +1041,10 @@ aarch64_prologue_this_id (struct frame_info *this_frame,
   struct aarch64_prologue_cache *cache
     = aarch64_make_prologue_cache (this_frame, this_cache);
 
-  /* This is meant to halt the backtrace at "_start".  */
-  if (cache->prev_pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
-    return;
-
-  /* If we've hit a wall, stop.  */
-  if (cache->prev_sp == 0)
-    return;
-
-  *this_id = frame_id_build (cache->prev_sp, cache->func);
+  if (!cache->available_p)
+    *this_id = frame_id_build_unavailable_stack (cache->func);
+  else
+    *this_id = frame_id_build (cache->prev_sp, cache->func);
 }
 
 /* Implement the "prev_register" frame_unwind method.  */
@@ -1051,7 +1095,7 @@ aarch64_prologue_prev_register (struct frame_info *this_frame,
 struct frame_unwind aarch64_prologue_unwind =
 {
   NORMAL_FRAME,
-  default_frame_unwind_stop_reason,
+  aarch64_prologue_frame_unwind_stop_reason,
   aarch64_prologue_this_id,
   aarch64_prologue_prev_register,
   NULL,
-- 
2.1.0

Re: [PATCH v3 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Pierre Langlois]

> @@ -979,9 +974,63 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
>  
>    cache->func = get_frame_func (this_frame);
>  
> +  cache->available_p = 1;
> +}
> +
> +/* Allocate and fill in *THIS_CACHE with information about the prologue of
> +   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
> +   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
> +   */

Pushed with this comment's formatting fixed.

Thanks,
Pierre

Re: [PATCH v2 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> +/* Allocate and fill in *THIS_CACHE with information about the prologue of
> +   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
> +   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
> +   */

We don't write */-only line in the comment.  See "Block Comment Formatting"
in https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

You can change it to
      ...
      *THIS_CACHE.  */

-- 
Yao (齐尧)

Re: [PATCH v2 4/8] [AArch64] Teach prologue unwinder to terminate gracefully

[Pierre Langlois]

On 09/07/15 13:46, Yao Qi wrote:
> Pierre Langlois <pierre.langlois@arm.com> writes:
> 
>> +/* Allocate and fill in *THIS_CACHE with information about the prologue of
>> +   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
>> +   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
>> +   */
> 
> We don't write */-only line in the comment.  See "Block Comment Formatting"
> in https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
> 
> You can change it to
>       ...
>       *THIS_CACHE.  */
> 

Oh I see, sorry I missed this.  I'll update this comment on the other
patches too.

[PATCH 1/8] [AArch64] Refactor aarch64_make_prologue_cache

[Pierre Langlois]

We would previously have to make sure the frame cache was not already
created before calling aarch64_make_prologue_cache.  This patch makes
this function check it so that the caller does not need to do so.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_prologue_cache): New argument
	this_cache.  Return early if this_cache is already set.  Set
	this_cache.
	(aarch64_prologue_this_id): Update call to
	aarch64_prologue_cache.
	(aarch64_prologue_prev_register): Likewise.
	(aarch64_normal_frame_base): Likewise.
---
 gdb/aarch64-tdep.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 9650a7a..c4b7fe8 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -934,14 +934,18 @@ aarch64_scan_prologue (struct frame_info *this_frame,
    about the prologue of *THIS_FRAME.  */
 
 static struct aarch64_prologue_cache *
-aarch64_make_prologue_cache (struct frame_info *this_frame)
+aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
 {
   struct aarch64_prologue_cache *cache;
   CORE_ADDR unwound_fp;
   int reg;
 
+  if (*this_cache)
+    return *this_cache;
+
   cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  *this_cache = cache;
 
   aarch64_scan_prologue (this_frame, cache);
 
@@ -970,14 +974,11 @@ static void
 aarch64_prologue_this_id (struct frame_info *this_frame,
 			  void **this_cache, struct frame_id *this_id)
 {
-  struct aarch64_prologue_cache *cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
   struct frame_id id;
   CORE_ADDR pc, func;
 
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_prologue_cache (this_frame);
-  cache = *this_cache;
-
   /* 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)
@@ -999,11 +1000,8 @@ aarch64_prologue_prev_register (struct frame_info *this_frame,
 				void **this_cache, int prev_regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
-  struct aarch64_prologue_cache *cache;
-
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_prologue_cache (this_frame);
-  cache = *this_cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
 
   /* If we are asked to unwind the PC, then we need to return the LR
      instead.  The prologue may save PC, but it will point into this
@@ -1120,11 +1118,8 @@ struct frame_unwind aarch64_stub_unwind =
 static CORE_ADDR
 aarch64_normal_frame_base (struct frame_info *this_frame, void **this_cache)
 {
-  struct aarch64_prologue_cache *cache;
-
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_prologue_cache (this_frame);
-  cache = *this_cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
 
   return cache->prev_sp - cache->framesize;
 }
-- 
2.1.0

Re: [PATCH 1/8] [AArch64] Refactor aarch64_make_prologue_cache

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

>  static struct aarch64_prologue_cache *
> -aarch64_make_prologue_cache (struct frame_info *this_frame)
> +aarch64_make_prologue_cache (struct frame_info *this_frame, void
> **this_cache)

Could you document this function?  add comments about it, its
arguments, and return value.

>  {
>    struct aarch64_prologue_cache *cache;
>    CORE_ADDR unwound_fp;
>    int reg;
>  
> +  if (*this_cache)
> +    return *this_cache;
> +

Please check NULL explicitly, like if (*this_cache != NULL),  see
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

OK with these changes.

-- 
Yao (齐尧)

[PATCH] [AArch64] Refactor aarch64_make_prologue_cache

[Pierre Langlois]

Hi Yao,

I'll push this patch if you're happy with the comment.

Thanks for the reviews,
Pierre

---

We would previously have to make sure the frame cache was not already
created before calling aarch64_make_prologue_cache.  This patch makes
this function check it so that the caller does not need to do so.

gdb/ChangeLog:

	* aarch64-tdep.c (aarch64_prologue_cache): Update comment.  New
	argument this_cache.  Return early if this_cache is already set.
	Set this_cache.
	(aarch64_prologue_this_id): Update call to
	aarch64_prologue_cache.
	(aarch64_prologue_prev_register): Likewise.
	(aarch64_normal_frame_base): Likewise.
---
 gdb/aarch64-tdep.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 9650a7a..0496536 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -930,18 +930,24 @@ aarch64_scan_prologue (struct frame_info *this_frame,
     }
 }
 
-/* Allocate an aarch64_prologue_cache and fill it with information
-   about the prologue of *THIS_FRAME.  */
+/* Allocate and fill in *THIS_CACHE with information about the prologue of
+   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
+   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
+   */
 
 static struct aarch64_prologue_cache *
-aarch64_make_prologue_cache (struct frame_info *this_frame)
+aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
 {
   struct aarch64_prologue_cache *cache;
   CORE_ADDR unwound_fp;
   int reg;
 
+  if (*this_cache != NULL)
+    return *this_cache;
+
   cache = FRAME_OBSTACK_ZALLOC (struct aarch64_prologue_cache);
   cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  *this_cache = cache;
 
   aarch64_scan_prologue (this_frame, cache);
 
@@ -970,14 +976,11 @@ static void
 aarch64_prologue_this_id (struct frame_info *this_frame,
 			  void **this_cache, struct frame_id *this_id)
 {
-  struct aarch64_prologue_cache *cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
   struct frame_id id;
   CORE_ADDR pc, func;
 
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_prologue_cache (this_frame);
-  cache = *this_cache;
-
   /* 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)
@@ -999,11 +1002,8 @@ aarch64_prologue_prev_register (struct frame_info *this_frame,
 				void **this_cache, int prev_regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
-  struct aarch64_prologue_cache *cache;
-
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_prologue_cache (this_frame);
-  cache = *this_cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
 
   /* If we are asked to unwind the PC, then we need to return the LR
      instead.  The prologue may save PC, but it will point into this
@@ -1120,11 +1120,8 @@ struct frame_unwind aarch64_stub_unwind =
 static CORE_ADDR
 aarch64_normal_frame_base (struct frame_info *this_frame, void **this_cache)
 {
-  struct aarch64_prologue_cache *cache;
-
-  if (*this_cache == NULL)
-    *this_cache = aarch64_make_prologue_cache (this_frame);
-  cache = *this_cache;
+  struct aarch64_prologue_cache *cache
+    = aarch64_make_prologue_cache (this_frame, this_cache);
 
   return cache->prev_sp - cache->framesize;
 }
-- 
2.1.0

Re: [PATCH] [AArch64] Refactor aarch64_make_prologue_cache

[Pierre Langlois]

> -/* Allocate an aarch64_prologue_cache and fill it with information
> -   about the prologue of *THIS_FRAME.  */
> +/* Allocate and fill in *THIS_CACHE with information about the prologue of
> +   *THIS_FRAME.  Do not do this is if *THIS_CACHE was already allocated.
> +   Return a pointer to the current aarch64_prologue_cache in *THIS_CACHE.
> +   */

Pushed with this comment's formatting fixed.

Thanks,
Pierre

[PATCH 6/8] [AArch64] Implement gdbarch_gen_return_address gdbarch method

[Pierre Langlois]

This patch implements the 'collect $_ret' command to collect the return
address of a function in a tracepoint.  It marks the LR register for
collection.

gdb/ChangeLog:

	* aarch64-tdep.c: Add ax.h and ax-gdb.h includes.
	(aarch64_gen_return_address): New function.
	(aarch64_gdbarch_init): Hook it.
---
 gdb/aarch64-tdep.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 57976b7..c7ccbb5 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -42,6 +42,8 @@
 #include "user-regs.h"
 #include "language.h"
 #include "infcall.h"
+#include "ax.h"
+#include "ax-gdb.h"
 
 #include "aarch64-tdep.h"
 
@@ -2261,6 +2263,18 @@ aarch64_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
   *pc = extract_unsigned_integer (buf, X_REGISTER_SIZE, byte_order);
   return 1;
 }
+
+/* Implement the "gen_return_address" gdbarch method.  */
+
+static void
+aarch64_gen_return_address (struct gdbarch *gdbarch,
+			    struct agent_expr *ax, struct axs_value *value,
+			    CORE_ADDR scope)
+{
+  value->type = register_type (gdbarch, AARCH64_LR_REGNUM);
+  value->kind = axs_lvalue_register;
+  value->u.reg = AARCH64_LR_REGNUM;
+}
 \f
 
 /* Return the pseudo register name corresponding to register regnum.  */
@@ -2830,6 +2844,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   if (tdep->jb_pc >= 0)
     set_gdbarch_get_longjmp_target (gdbarch, aarch64_get_longjmp_target);
 
+  set_gdbarch_gen_return_address (gdbarch, aarch64_gen_return_address);
+
   tdesc_use_registers (gdbarch, tdesc, tdesc_data);
 
   /* Add standard register aliases.  */
-- 
2.1.0

Re: [PATCH 6/8] [AArch64] Implement gdbarch_gen_return_address gdbarch method

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> gdb/ChangeLog:
>
> 	* aarch64-tdep.c: Add ax.h and ax-gdb.h includes.
> 	(aarch64_gen_return_address): Ne

Looks good to me.

-- 
Yao (齐尧)

Re: [PATCH 6/8] [AArch64] Implement gdbarch_gen_return_address gdbarch method

[Pierre Langlois]

On 08/07/15 17:35, Yao Qi wrote:
> Pierre Langlois <pierre.langlois@arm.com> writes:
> 
>> gdb/ChangeLog:
>>
>> 	* aarch64-tdep.c: Add ax.h and ax-gdb.h includes.
>> 	(aarch64_gen_return_address): Ne
> 
> Looks good to me.
> 

Thanks, pushed.

[PATCH 8/8] [GDBServer][AArch64] Enable support for tracepoints

[Pierre Langlois]

gdb/gdbserver/ChangeLog:

	* linux-aarch64-low.c (aarch64_supports_tracepoints): New
	function.  Return 1.
	(the_low_target): Install it.
---
 gdb/gdbserver/linux-aarch64-low.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 8a30b00..c4082e1 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -1304,6 +1304,14 @@ aarch64_regs_info (void)
   return &regs_info;
 }
 
+/* Implementation of linux_target_ops method "supports_tracepoints".  */
+
+static int
+aarch64_supports_tracepoints (void)
+{
+  return 1;
+}
+
 struct linux_target_ops the_low_target =
 {
   aarch64_arch_setup,
@@ -1330,6 +1338,8 @@ struct linux_target_ops the_low_target =
   aarch64_linux_new_thread,
   aarch64_linux_new_fork,
   aarch64_linux_prepare_to_resume,
+  NULL,
+  aarch64_supports_tracepoints,
 };
 
 void
-- 
2.1.0

Re: [PATCH 8/8] [GDBServer][AArch64] Enable support for tracepoints

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> 	* linux-aarch64-low.c (aarch64_supports_tracepoints): New
> 	function.  Return 1.
> 	(the_low_target): Install it.

Looks good to me.

-- 
Yao (齐尧)

Re: [PATCH 8/8] [GDBServer][AArch64] Enable support for tracepoints

[Pierre Langlois]

On 08/07/15 17:40, Yao Qi wrote:
> Pierre Langlois <pierre.langlois@arm.com> writes:
> 
>> 	* linux-aarch64-low.c (aarch64_supports_tracepoints): New
>> 	function.  Return 1.
>> 	(the_low_target): Install it.
> 
> Looks good to me.
> 
Thanks, pushed.

Re: [PATCH 0/8] [AArch64] Add support for tracepoints

[Pedro Alves]

On 07/07/2015 01:51 PM, Pierre Langlois wrote:
> Hi all,
> 
> These patches enable tracepoints for AArch64.  Although tracepoints are
> enabled in GDBServer with the last patch, most of the changes are in GDB.
> The most important changes teach AArch64's frame unwinders to report when
> the inferior is unavailable.
> 
> The first three patches refactor the frame caches.  The idea is to keep
> accesses to the inferior's registers in aarch64_make_prologue_cache and
> aarch64_make_stub_cache.  This way the following patches can easily catch
> exceptions when the inferior is unavailable.
> 
> The following two patches teach AArch64's unwinders to terminate
> gracefully, in a similar way as it was done for x86 here:
> 
> https://sourceware.org/ml/gdb-patches/2011-02/msg00611.html
> 
> It fixes cases where we do not have debugging information and AArch64's
> unwinders need to be used when examining a trace buffer.  In this context
> we cannot assume that the inferior's memory and registers are available.
> 

This all looks like what I'd expect to see.  LGTM.
Yao may want to take a look too.

Just a few nits:

- In a few patches, you had a spurious empty line in the middle
  of the ChangeLog entry.  Remove it please.

- Several new functions are missing an intro comment.

- aarch64_prologue_frame_unwind_stop_reason in patch 4 has
  odd indentation in the "We've hit a wall" case.

Thanks,
Pedro Alves

Re: [PATCH 0/8] [AArch64] Add support for tracepoints

[Yao Qi]

Pierre Langlois <pierre.langlois@arm.com> writes:

> These patches enable tracepoints for AArch64.  Although tracepoints are
> enabled in GDBServer with the last patch, most of the changes are in GDB.
> The most important changes teach AArch64's frame unwinders to report when
> the inferior is unavailable.
>
> The first three patches refactor the frame caches.  The idea is to keep
> accesses to the inferior's registers in aarch64_make_prologue_cache and
> aarch64_make_stub_cache.  This way the following patches can easily catch
> exceptions when the inferior is unavailable.
>
> The following two patches teach AArch64's unwinders to terminate
> gracefully, in a similar way as it was done for x86 here:
>
> https://sourceware.org/ml/gdb-patches/2011-02/msg00611.html
>
> It fixes cases where we do not have debugging information and AArch64's
> unwinders need to be used when examining a trace buffer.  In this context
> we cannot assume that the inferior's memory and registers are
> available.

Beside Pedro's comments, we need a NEWS entry.  Could you please write
one?

-- 
Yao (齐尧)

[PATCH] Add NEWS entry for tracepoints support on aarch64-linux

[Pierre Langlois]

Here is a NEWS entry patch, is this OK?

Thanks,
Pierre

---

Add a NEWS entry mentionning tracepoints support on aarch64-linux in
GDBserver.

gdb/ChangeLog:

	* NEWS: Mention support for tracepoints on aarch64-linux.
---
 gdb/NEWS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index bbeeb40..7ce9758 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 7.10
 
+* Support for tracepoints on aarch64-linux was added in GDBserver.
+
 *** Changes in GDB 7.10
 
 * Support for process record-replay and reverse debugging on aarch64*-linux*
-- 
2.1.0

Re: [PATCH] Add NEWS entry for tracepoints support on aarch64-linux

[Eli Zaretskii]

> From: Pierre Langlois <pierre.langlois@arm.com>
> Cc: Pierre Langlois <pierre.langlois@arm.com>,	gdb-patches@sourceware.org
> Date: Thu,  9 Jul 2015 14:16:36 +0100
> 
> Here is a NEWS entry patch, is this OK?

Yes, thanks.

Re: [PATCH] Add NEWS entry for tracepoints support on aarch64-linux

[Pierre Langlois]

On 09/07/15 15:44, Eli Zaretskii wrote:
>> From: Pierre Langlois <pierre.langlois@arm.com>
>> Cc: Pierre Langlois <pierre.langlois@arm.com>,	gdb-patches@sourceware.org
>> Date: Thu,  9 Jul 2015 14:16:36 +0100
>>
>> Here is a NEWS entry patch, is this OK?
> 
> Yes, thanks.
> 
I've pushed this now.

Thanks,
Pierre