* [PATCH 0/4] Support tracepoints for ARM linux in GDBServer
@ 2016-01-07 17:45 Antoine Tremblay
2016-01-07 17:45 ` [PATCH 4/4] " Antoine Tremblay
` (4 more replies)
0 siblings, 5 replies; 65+ messages in thread
From: Antoine Tremblay @ 2016-01-07 17:45 UTC (permalink / raw)
To: gdb-patches
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.
^ permalink raw reply [flat|nested] 65+ messages in thread* [PATCH 4/4] Support tracepoints for ARM linux in GDBServer 2016-01-07 17:45 [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Antoine Tremblay @ 2016-01-07 17:45 ` Antoine Tremblay 2016-02-08 14:45 ` Antoine Tremblay 2016-01-07 17:45 ` [PATCH 3/4] Enable tracing of pseudo-registers on ARM Antoine Tremblay ` (3 subsequent siblings) 4 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-01-07 17:45 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ®s_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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 4/4] Support tracepoints for ARM linux in GDBServer 2016-01-07 17:45 ` [PATCH 4/4] " Antoine Tremblay @ 2016-02-08 14:45 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-08 14:45 UTC (permalink / raw) To: gdb-patches; +Cc: Marcin Kościelnicki 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 ®s_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" > ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH 3/4] Enable tracing of pseudo-registers on ARM 2016-01-07 17:45 [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Antoine Tremblay 2016-01-07 17:45 ` [PATCH 4/4] " Antoine Tremblay @ 2016-01-07 17:45 ` Antoine Tremblay 2016-02-12 15:14 ` Yao Qi 2016-01-07 17:45 ` [PATCH 2/4] Use the target architecture when encoding tracepoint actions Antoine Tremblay ` (2 subsequent siblings) 4 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-01-07 17:45 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM 2016-01-07 17:45 ` [PATCH 3/4] Enable tracing of pseudo-registers on ARM Antoine Tremblay @ 2016-02-12 15:14 ` Yao Qi 2016-02-12 15:54 ` Marcin Kościelnicki 2016-02-15 14:46 ` [PATCH v2] " Antoine Tremblay 0 siblings, 2 replies; 65+ messages in thread From: Yao Qi @ 2016-02-12 15:14 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM 2016-02-12 15:14 ` Yao Qi @ 2016-02-12 15:54 ` Marcin Kościelnicki 2016-02-15 10:27 ` Yao Qi 2016-02-15 14:46 ` [PATCH v2] " Antoine Tremblay 1 sibling, 1 reply; 65+ messages in thread From: Marcin Kościelnicki @ 2016-02-12 15:54 UTC (permalink / raw) To: Yao Qi, Antoine Tremblay; +Cc: gdb-patches 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... ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM 2016-02-12 15:54 ` Marcin Kościelnicki @ 2016-02-15 10:27 ` Yao Qi 2016-02-15 10:57 ` Pedro Alves 0 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-02-15 10:27 UTC (permalink / raw) To: Marcin Kościelnicki; +Cc: Yao Qi, Antoine Tremblay, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 3/4] Enable tracing of pseudo-registers on ARM 2016-02-15 10:27 ` Yao Qi @ 2016-02-15 10:57 ` Pedro Alves 0 siblings, 0 replies; 65+ messages in thread From: Pedro Alves @ 2016-02-15 10:57 UTC (permalink / raw) To: Yao Qi, Marcin Kościelnicki Cc: Antoine Tremblay, gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH v2] Enable tracing of pseudo-registers on ARM 2016-02-12 15:14 ` Yao Qi 2016-02-12 15:54 ` Marcin Kościelnicki @ 2016-02-15 14:46 ` Antoine Tremblay 2016-02-19 16:33 ` Antoine Tremblay 1 sibling, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-15 14:46 UTC (permalink / raw) To: gdb-patches, qiyaoltc; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2] Enable tracing of pseudo-registers on ARM 2016-02-15 14:46 ` [PATCH v2] " Antoine Tremblay @ 2016-02-19 16:33 ` Antoine Tremblay 2016-02-19 19:29 ` [PATCH v3] " Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-19 16:33 UTC (permalink / raw) To: Antoine Tremblay, qiyaoltc; +Cc: gdb-patches 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? ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-19 16:33 ` Antoine Tremblay @ 2016-02-19 19:29 ` Antoine Tremblay 2016-02-19 20:06 ` [PATCH v4] " Antoine Tremblay 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves 0 siblings, 2 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-19 19:29 UTC (permalink / raw) To: gdb-patches, qiyaoltc; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH v4] Enable tracing of pseudo-registers on ARM 2016-02-19 19:29 ` [PATCH v3] " Antoine Tremblay @ 2016-02-19 20:06 ` Antoine Tremblay 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves 1 sibling, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-19 20:06 UTC (permalink / raw) To: gdb-patches, qiyaoltc; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-19 19:29 ` [PATCH v3] " Antoine Tremblay 2016-02-19 20:06 ` [PATCH v4] " Antoine Tremblay @ 2016-02-19 20:22 ` Pedro Alves 2016-02-19 20:32 ` Antoine Tremblay ` (4 more replies) 1 sibling, 5 replies; 65+ messages in thread From: Pedro Alves @ 2016-02-19 20:22 UTC (permalink / raw) To: Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves @ 2016-02-19 20:32 ` Antoine Tremblay 2016-02-22 11:51 ` Yao Qi ` (3 subsequent siblings) 4 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-19 20:32 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves 2016-02-19 20:32 ` Antoine Tremblay @ 2016-02-22 11:51 ` Yao Qi 2016-02-22 16:51 ` Antoine Tremblay ` (2 subsequent siblings) 4 siblings, 0 replies; 65+ messages in thread From: Yao Qi @ 2016-02-22 11:51 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches, qiyaoltc 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves 2016-02-19 20:32 ` Antoine Tremblay 2016-02-22 11:51 ` Yao Qi @ 2016-02-22 16:51 ` Antoine Tremblay 2016-02-24 18:11 ` Pedro Alves 2016-02-23 19:34 ` Antoine Tremblay 2016-02-23 19:41 ` [PATCH v5] " Antoine Tremblay 4 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-22 16:51 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-22 16:51 ` Antoine Tremblay @ 2016-02-24 18:11 ` Pedro Alves 2016-02-24 18:21 ` Marcin Kościelnicki 0 siblings, 1 reply; 65+ messages in thread From: Pedro Alves @ 2016-02-24 18:11 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-24 18:11 ` Pedro Alves @ 2016-02-24 18:21 ` Marcin Kościelnicki 2016-02-24 18:33 ` Pedro Alves 0 siblings, 1 reply; 65+ messages in thread From: Marcin Kościelnicki @ 2016-02-24 18:21 UTC (permalink / raw) To: Pedro Alves, Antoine Tremblay; +Cc: gdb-patches, qiyaoltc 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 > ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-24 18:21 ` Marcin Kościelnicki @ 2016-02-24 18:33 ` Pedro Alves 2016-02-24 18:55 ` Antoine Tremblay 2016-02-24 19:02 ` Antoine Tremblay 0 siblings, 2 replies; 65+ messages in thread From: Pedro Alves @ 2016-02-24 18:33 UTC (permalink / raw) To: Marcin Kościelnicki, Antoine Tremblay; +Cc: gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-24 18:33 ` Pedro Alves @ 2016-02-24 18:55 ` Antoine Tremblay 2016-02-24 19:02 ` Pedro Alves 2016-02-24 19:02 ` Antoine Tremblay 1 sibling, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-24 18:55 UTC (permalink / raw) To: Pedro Alves Cc: Marcin Kościelnicki, Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-24 18:55 ` Antoine Tremblay @ 2016-02-24 19:02 ` Pedro Alves 0 siblings, 0 replies; 65+ messages in thread From: Pedro Alves @ 2016-02-24 19:02 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Marcin Kościelnicki, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-24 18:33 ` Pedro Alves 2016-02-24 18:55 ` Antoine Tremblay @ 2016-02-24 19:02 ` Antoine Tremblay 1 sibling, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-24 19:02 UTC (permalink / raw) To: Pedro Alves Cc: Marcin Kościelnicki, Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves ` (2 preceding siblings ...) 2016-02-22 16:51 ` Antoine Tremblay @ 2016-02-23 19:34 ` Antoine Tremblay 2016-02-24 18:20 ` Pedro Alves 2016-02-23 19:41 ` [PATCH v5] " Antoine Tremblay 4 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-23 19:34 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches, qiyaoltc 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. ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-23 19:34 ` Antoine Tremblay @ 2016-02-24 18:20 ` Pedro Alves 2016-02-24 18:47 ` Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Pedro Alves @ 2016-02-24 18:20 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v3] Enable tracing of pseudo-registers on ARM 2016-02-24 18:20 ` Pedro Alves @ 2016-02-24 18:47 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-24 18:47 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH v5] Enable tracing of pseudo-registers on ARM 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves ` (3 preceding siblings ...) 2016-02-23 19:34 ` Antoine Tremblay @ 2016-02-23 19:41 ` Antoine Tremblay 2016-02-24 19:12 ` Pedro Alves 2016-02-25 10:35 ` Yao Qi 4 siblings, 2 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-23 19:41 UTC (permalink / raw) To: gdb-patches, palves; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v5] Enable tracing of pseudo-registers on ARM 2016-02-23 19:41 ` [PATCH v5] " Antoine Tremblay @ 2016-02-24 19:12 ` Pedro Alves 2016-02-24 19:25 ` Antoine Tremblay 2016-02-25 10:35 ` Yao Qi 1 sibling, 1 reply; 65+ messages in thread From: Pedro Alves @ 2016-02-24 19:12 UTC (permalink / raw) To: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v5] Enable tracing of pseudo-registers on ARM 2016-02-24 19:12 ` Pedro Alves @ 2016-02-24 19:25 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-24 19:25 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches 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. ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v5] Enable tracing of pseudo-registers on ARM 2016-02-23 19:41 ` [PATCH v5] " Antoine Tremblay 2016-02-24 19:12 ` Pedro Alves @ 2016-02-25 10:35 ` Yao Qi 2016-02-25 15:33 ` [PATCH v6] " Antoine Tremblay 1 sibling, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-02-25 10:35 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches, palves 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH v6] Enable tracing of pseudo-registers on ARM 2016-02-25 10:35 ` Yao Qi @ 2016-02-25 15:33 ` Antoine Tremblay 2016-02-25 17:59 ` Pedro Alves 2016-02-26 8:34 ` Yao Qi 0 siblings, 2 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-25 15:33 UTC (permalink / raw) To: gdb-patches, qiyaoltc; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v6] Enable tracing of pseudo-registers on ARM 2016-02-25 15:33 ` [PATCH v6] " Antoine Tremblay @ 2016-02-25 17:59 ` Pedro Alves 2016-02-25 18:19 ` Antoine Tremblay 2016-02-26 8:34 ` Yao Qi 1 sibling, 1 reply; 65+ messages in thread From: Pedro Alves @ 2016-02-25 17:59 UTC (permalink / raw) To: Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v6] Enable tracing of pseudo-registers on ARM 2016-02-25 17:59 ` Pedro Alves @ 2016-02-25 18:19 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-25 18:19 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches, qiyaoltc 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v6] Enable tracing of pseudo-registers on ARM 2016-02-25 15:33 ` [PATCH v6] " Antoine Tremblay 2016-02-25 17:59 ` Pedro Alves @ 2016-02-26 8:34 ` Yao Qi 2016-02-26 13:00 ` Antoine Tremblay 1 sibling, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-02-26 8:34 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches, qiyaoltc 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v6] Enable tracing of pseudo-registers on ARM 2016-02-26 8:34 ` Yao Qi @ 2016-02-26 13:00 ` Antoine Tremblay 2016-02-26 13:03 ` [PATCH v7] " Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-26 13:00 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches 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. ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH v7] Enable tracing of pseudo-registers on ARM 2016-02-26 13:00 ` Antoine Tremblay @ 2016-02-26 13:03 ` Antoine Tremblay 2016-02-26 14:14 ` Yao Qi 0 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-26 13:03 UTC (permalink / raw) To: gdb-patches, qiyaoltc; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v7] Enable tracing of pseudo-registers on ARM 2016-02-26 13:03 ` [PATCH v7] " Antoine Tremblay @ 2016-02-26 14:14 ` Yao Qi 2016-02-26 14:57 ` Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-02-26 14:14 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches, qiyaoltc 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v7] Enable tracing of pseudo-registers on ARM 2016-02-26 14:14 ` Yao Qi @ 2016-02-26 14:57 ` Antoine Tremblay 2016-02-26 14:59 ` [PATCH v8] " Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-26 14:57 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches 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. ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH v8] Enable tracing of pseudo-registers on ARM 2016-02-26 14:57 ` Antoine Tremblay @ 2016-02-26 14:59 ` Antoine Tremblay 2016-02-26 15:57 ` Yao Qi 0 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-26 14:59 UTC (permalink / raw) To: gdb-patches, qiyaoltc; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v8] Enable tracing of pseudo-registers on ARM 2016-02-26 14:59 ` [PATCH v8] " Antoine Tremblay @ 2016-02-26 15:57 ` Yao Qi 2016-02-26 17:45 ` Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-02-26 15:57 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches, qiyaoltc 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v8] Enable tracing of pseudo-registers on ARM 2016-02-26 15:57 ` Yao Qi @ 2016-02-26 17:45 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-26 17:45 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH 2/4] Use the target architecture when encoding tracepoint actions 2016-01-07 17:45 [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Antoine Tremblay 2016-01-07 17:45 ` [PATCH 4/4] " Antoine Tremblay 2016-01-07 17:45 ` [PATCH 3/4] Enable tracing of pseudo-registers on ARM Antoine Tremblay @ 2016-01-07 17:45 ` Antoine Tremblay 2016-02-06 20:58 ` Marcin Kościelnicki 2016-02-11 13:02 ` Pedro Alves 2016-01-07 17:45 ` [PATCH 1/4] Teach arm unwinders to terminate gracefully Antoine Tremblay 2016-01-11 12:17 ` [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Yao Qi 4 siblings, 2 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-01-07 17:45 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 2/4] Use the target architecture when encoding tracepoint actions 2016-01-07 17:45 ` [PATCH 2/4] Use the target architecture when encoding tracepoint actions Antoine Tremblay @ 2016-02-06 20:58 ` Marcin Kościelnicki 2016-02-11 13:02 ` Pedro Alves 1 sibling, 0 replies; 65+ messages in thread From: Marcin Kościelnicki @ 2016-02-06 20:58 UTC (permalink / raw) To: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 2/4] Use the target architecture when encoding tracepoint actions 2016-01-07 17:45 ` [PATCH 2/4] Use the target architecture when encoding tracepoint actions Antoine Tremblay 2016-02-06 20:58 ` Marcin Kościelnicki @ 2016-02-11 13:02 ` Pedro Alves 2016-02-11 13:21 ` Antoine Tremblay 1 sibling, 1 reply; 65+ messages in thread From: Pedro Alves @ 2016-02-11 13:02 UTC (permalink / raw) To: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 2/4] Use the target architecture when encoding tracepoint actions 2016-02-11 13:02 ` Pedro Alves @ 2016-02-11 13:21 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-11 13:21 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-01-07 17:45 [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Antoine Tremblay ` (2 preceding siblings ...) 2016-01-07 17:45 ` [PATCH 2/4] Use the target architecture when encoding tracepoint actions Antoine Tremblay @ 2016-01-07 17:45 ` Antoine Tremblay 2016-02-12 14:46 ` Yao Qi 2016-01-11 12:17 ` [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Yao Qi 4 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-01-07 17:45 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-01-07 17:45 ` [PATCH 1/4] Teach arm unwinders to terminate gracefully Antoine Tremblay @ 2016-02-12 14:46 ` Yao Qi 2016-02-24 17:57 ` Antoine Tremblay 2016-02-25 11:44 ` Pedro Alves 0 siblings, 2 replies; 65+ messages in thread From: Yao Qi @ 2016-02-12 14:46 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches, palves 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-12 14:46 ` Yao Qi @ 2016-02-24 17:57 ` Antoine Tremblay 2016-02-25 11:44 ` Pedro Alves 1 sibling, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-24 17:57 UTC (permalink / raw) To: palves; +Cc: Antoine Tremblay, gdb-patches, Yao Qi 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 ? ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-12 14:46 ` Yao Qi 2016-02-24 17:57 ` Antoine Tremblay @ 2016-02-25 11:44 ` Pedro Alves 2016-02-25 13:15 ` Antoine Tremblay ` (2 more replies) 1 sibling, 3 replies; 65+ messages in thread From: Pedro Alves @ 2016-02-25 11:44 UTC (permalink / raw) To: Yao Qi, Antoine Tremblay; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-25 11:44 ` Pedro Alves @ 2016-02-25 13:15 ` Antoine Tremblay 2016-02-26 9:12 ` Yao Qi 2016-04-06 15:54 ` Yao Qi 2016-05-04 16:24 ` Yao Qi 2 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-25 13:15 UTC (permalink / raw) To: Yao Qi; +Cc: Pedro Alves, Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-25 13:15 ` Antoine Tremblay @ 2016-02-26 9:12 ` Yao Qi 2016-02-26 12:26 ` Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-02-26 9:12 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Yao Qi, Pedro Alves, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-26 9:12 ` Yao Qi @ 2016-02-26 12:26 ` Antoine Tremblay 2016-02-26 14:25 ` Yao Qi 0 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-02-26 12:26 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-26 12:26 ` Antoine Tremblay @ 2016-02-26 14:25 ` Yao Qi 2016-02-26 20:10 ` Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-02-26 14:25 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Yao Qi, Pedro Alves, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-26 14:25 ` Yao Qi @ 2016-02-26 20:10 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-02-26 20:10 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-25 11:44 ` Pedro Alves 2016-02-25 13:15 ` Antoine Tremblay @ 2016-04-06 15:54 ` Yao Qi 2016-04-06 16:30 ` Pedro Alves 2016-05-04 16:24 ` Yao Qi 2 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-04-06 15:54 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, Antoine Tremblay, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-04-06 15:54 ` Yao Qi @ 2016-04-06 16:30 ` Pedro Alves 2016-04-07 16:33 ` Yao Qi 0 siblings, 1 reply; 65+ messages in thread From: Pedro Alves @ 2016-04-06 16:30 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-04-06 16:30 ` Pedro Alves @ 2016-04-07 16:33 ` Yao Qi 0 siblings, 0 replies; 65+ messages in thread From: Yao Qi @ 2016-04-07 16:33 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, Antoine Tremblay, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully 2016-02-25 11:44 ` Pedro Alves 2016-02-25 13:15 ` Antoine Tremblay 2016-04-06 15:54 ` Yao Qi @ 2016-05-04 16:24 ` Yao Qi 2 siblings, 0 replies; 65+ messages in thread From: Yao Qi @ 2016-05-04 16:24 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-01-07 17:45 [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Antoine Tremblay ` (3 preceding siblings ...) 2016-01-07 17:45 ` [PATCH 1/4] Teach arm unwinders to terminate gracefully Antoine Tremblay @ 2016-01-11 12:17 ` Yao Qi 2016-01-11 12:56 ` Antoine Tremblay 2016-04-26 19:11 ` Antoine Tremblay 4 siblings, 2 replies; 65+ messages in thread From: Yao Qi @ 2016-01-11 12:17 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-01-11 12:17 ` [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Yao Qi @ 2016-01-11 12:56 ` Antoine Tremblay 2016-01-11 13:41 ` Yao Qi 2016-04-26 19:11 ` Antoine Tremblay 1 sibling, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-01-11 12:56 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-01-11 12:56 ` Antoine Tremblay @ 2016-01-11 13:41 ` Yao Qi 0 siblings, 0 replies; 65+ messages in thread From: Yao Qi @ 2016-01-11 13:41 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Yao Qi, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-01-11 12:17 ` [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Yao Qi 2016-01-11 12:56 ` Antoine Tremblay @ 2016-04-26 19:11 ` Antoine Tremblay 2016-04-27 8:00 ` Yao Qi 1 sibling, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-04-26 19:11 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-04-26 19:11 ` Antoine Tremblay @ 2016-04-27 8:00 ` Yao Qi 2016-04-27 12:07 ` Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-04-27 8:00 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Yao Qi, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-04-27 8:00 ` Yao Qi @ 2016-04-27 12:07 ` Antoine Tremblay 2016-04-27 13:57 ` Yao Qi 0 siblings, 1 reply; 65+ messages in thread From: Antoine Tremblay @ 2016-04-27 12:07 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-04-27 12:07 ` Antoine Tremblay @ 2016-04-27 13:57 ` Yao Qi 2016-04-27 14:41 ` Antoine Tremblay 0 siblings, 1 reply; 65+ messages in thread From: Yao Qi @ 2016-04-27 13:57 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Yao Qi, gdb-patches 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 (齐尧) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 0/4] Support tracepoints for ARM linux in GDBServer 2016-04-27 13:57 ` Yao Qi @ 2016-04-27 14:41 ` Antoine Tremblay 0 siblings, 0 replies; 65+ messages in thread From: Antoine Tremblay @ 2016-04-27 14:41 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches 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 ^ permalink raw reply [flat|nested] 65+ messages in thread
end of thread, other threads:[~2016-05-04 16:24 UTC | newest] Thread overview: 65+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-07 17:45 [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Antoine Tremblay 2016-01-07 17:45 ` [PATCH 4/4] " Antoine Tremblay 2016-02-08 14:45 ` Antoine Tremblay 2016-01-07 17:45 ` [PATCH 3/4] Enable tracing of pseudo-registers on ARM Antoine Tremblay 2016-02-12 15:14 ` Yao Qi 2016-02-12 15:54 ` Marcin Kościelnicki 2016-02-15 10:27 ` Yao Qi 2016-02-15 10:57 ` Pedro Alves 2016-02-15 14:46 ` [PATCH v2] " Antoine Tremblay 2016-02-19 16:33 ` Antoine Tremblay 2016-02-19 19:29 ` [PATCH v3] " Antoine Tremblay 2016-02-19 20:06 ` [PATCH v4] " Antoine Tremblay 2016-02-19 20:22 ` [PATCH v3] " Pedro Alves 2016-02-19 20:32 ` Antoine Tremblay 2016-02-22 11:51 ` Yao Qi 2016-02-22 16:51 ` Antoine Tremblay 2016-02-24 18:11 ` Pedro Alves 2016-02-24 18:21 ` Marcin Kościelnicki 2016-02-24 18:33 ` Pedro Alves 2016-02-24 18:55 ` Antoine Tremblay 2016-02-24 19:02 ` Pedro Alves 2016-02-24 19:02 ` Antoine Tremblay 2016-02-23 19:34 ` Antoine Tremblay 2016-02-24 18:20 ` Pedro Alves 2016-02-24 18:47 ` Antoine Tremblay 2016-02-23 19:41 ` [PATCH v5] " Antoine Tremblay 2016-02-24 19:12 ` Pedro Alves 2016-02-24 19:25 ` Antoine Tremblay 2016-02-25 10:35 ` Yao Qi 2016-02-25 15:33 ` [PATCH v6] " Antoine Tremblay 2016-02-25 17:59 ` Pedro Alves 2016-02-25 18:19 ` Antoine Tremblay 2016-02-26 8:34 ` Yao Qi 2016-02-26 13:00 ` Antoine Tremblay 2016-02-26 13:03 ` [PATCH v7] " Antoine Tremblay 2016-02-26 14:14 ` Yao Qi 2016-02-26 14:57 ` Antoine Tremblay 2016-02-26 14:59 ` [PATCH v8] " Antoine Tremblay 2016-02-26 15:57 ` Yao Qi 2016-02-26 17:45 ` Antoine Tremblay 2016-01-07 17:45 ` [PATCH 2/4] Use the target architecture when encoding tracepoint actions Antoine Tremblay 2016-02-06 20:58 ` Marcin Kościelnicki 2016-02-11 13:02 ` Pedro Alves 2016-02-11 13:21 ` Antoine Tremblay 2016-01-07 17:45 ` [PATCH 1/4] Teach arm unwinders to terminate gracefully Antoine Tremblay 2016-02-12 14:46 ` Yao Qi 2016-02-24 17:57 ` Antoine Tremblay 2016-02-25 11:44 ` Pedro Alves 2016-02-25 13:15 ` Antoine Tremblay 2016-02-26 9:12 ` Yao Qi 2016-02-26 12:26 ` Antoine Tremblay 2016-02-26 14:25 ` Yao Qi 2016-02-26 20:10 ` Antoine Tremblay 2016-04-06 15:54 ` Yao Qi 2016-04-06 16:30 ` Pedro Alves 2016-04-07 16:33 ` Yao Qi 2016-05-04 16:24 ` Yao Qi 2016-01-11 12:17 ` [PATCH 0/4] Support tracepoints for ARM linux in GDBServer Yao Qi 2016-01-11 12:56 ` Antoine Tremblay 2016-01-11 13:41 ` Yao Qi 2016-04-26 19:11 ` Antoine Tremblay 2016-04-27 8:00 ` Yao Qi 2016-04-27 12:07 ` Antoine Tremblay 2016-04-27 13:57 ` Yao Qi 2016-04-27 14:41 ` Antoine Tremblay
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).