[1/2,GDBserver] Fix compiling conditional expressions accessing registers
Commit Message
In order to compile conditions on fast tracepoints, we need to emit code
to read registers. This is done with the `reg' opcode, and when
generating native code for it we emit a call to `gdb_agent_get_raw_reg',
which is a shared symbol between GDBserver and the IPA.
However, compiled conditions accessing registers do not work at the
moment. For example, while debugging the ftrace test case on x86_64
using GDBserver:
~~~
(gdb) ftrace set_point if $rip == *set_point
Fast tracepoint 2 at 0x4006b9: ...
(gdb) tstart
(gdb) continue
Continuing.
Breakpoint 3, end () at ..
(gdb) tstatus
Trace is running on the target.
Collected 0 trace frames.
Trace buffer has 5242880 bytes of 5242880 bytes free (0% full).
Trace will stop if GDB disconnects.
Not looking at any trace frame.
Trace started at 1441727485.544642 secs.
~~~
The condition on the tracepoint should always evaluate to TRUE, $rip
being the program counter.
It turns out there is an error when emitting a function call to
`gdb_agent_get_raw_reg'. The call emitted by `amd64_emit_reg' should
pass a pointer to the raw registers saved on the stack as a first
argument, but instead it passes the fast tracepoint context object
(`struct fast_tracepoint_ctx *'). As a result, `gdb_agent_get_raw_reg'
always returns the wrong values.
We can see this by setting a breakpoint on both
`condition_true_at_tracepoint' and `gdb_agent_get_raw_reg'. We can see
the pointer being passed to the latter is the context:
~~~
(gdb) ftrace set_point if $rip == *set_point
Fast tracepoint 2 at 0x4006b9: ...
(gdb) b condition_true_at_tracepoint
Breakpoint 4 at ...
(gdb) b gdb_agent_get_raw_reg
Breakpoint 5 at ...
(gdb) tstart
(gdb) continue
Continuing.
Breakpoint 4, condition_true_at_tracepoint (ctx=0x7fffffffc038,
^^^^^^^^^^^^^^
tpoint=0x7ffff68e2010) at ...
(gdb) continue
Breakpoint 5, gdb_agent_get_raw_reg (raw_regs=0x7fffffffc038 "\001",
^^^^^^^^^^^^^^
regnum=16) at ...
(gdb)
~~~
This patch fixes this issue by replacing `gdb_agent_get_raw_reg' with a
`gdb_agent_get_reg' function which takes the tracepoint context object
as argument instead of a raw buffer. Additionally, this patch makes
this function architecture independent by initializing the context's
regcache early and making `gdb_agent_get_reg' use `collect_register'.
As a result, the fast tracepoint context object does not need to
contain the raw register buffer.
gdb/gdbserver/ChangeLog:
* linux-amd64-ipa.c (gdb_agent_get_raw_reg): Remove.
* linux-i386-ipa.c (gdb_agent_get_raw_reg): Remove.
* linux-x86-low.c (amd64_emit_reg): Call get_reg_func_addr
instead of get_raw_reg_func_addr.
(i386_emit_reg): Likewise.
* tracepoint.c (get_raw_reg): Rename to ...
(get_reg): ... this.
(struct ipa_sym_addresses) <addr_get_raw_reg>: Rename to ...
(struct ipa_sym_addresses) <add_get_reg>: ... this.
(symbol_list): Rename get_raw_reg to get_reg.
(struct fast_tracepoint_ctx) <regcache_initted>, <regs>: Remove.
(get_context_regcache): Do not initialize fctx->regcache.
(gdb_collect): Remove ctx.regs and ctx.regcache_initted.
Initialize ctx.regcache.
(gdb_agent_get_reg): New exported function.
(get_raw_reg_func_addr): Rename to ...
(get_reg_func_addr): ... this.
* tracepoint.h (gdb_agent_get_raw_reg): Remove.
(get_raw_reg_func_addr): Rename to ...
(get_reg_func_addr): ... this.
---
gdb/gdbserver/linux-amd64-ipa.c | 9 ---------
gdb/gdbserver/linux-i386-ipa.c | 15 ---------------
gdb/gdbserver/linux-x86-low.c | 4 ++--
gdb/gdbserver/tracepoint.c | 42 +++++++++++++++++++++++++----------------
gdb/gdbserver/tracepoint.h | 9 ++-------
5 files changed, 30 insertions(+), 49 deletions(-)
Comments
Pierre Langlois <pierre.langlois@arm.com> writes:
> This patch fixes this issue by replacing `gdb_agent_get_raw_reg' with a
> `gdb_agent_get_reg' function which takes the tracepoint context object
> as argument instead of a raw buffer. Additionally, this patch makes
> this function architecture independent by initializing the context's
> regcache early and making `gdb_agent_get_reg' use `collect_register'.
> As a result, the fast tracepoint context object does not need to
> contain the raw register buffer.
The fix looks reasonable to me as it makes no sense to keep two copies
of registers. I go through this patch, and it looks good to me.
This patch invalidates Wei-cheng's patch here
<https://sourceware.org/ml/gdb-patches/2015-06/msg00587.html> which is
already approved but not committed.
If Ulirch/Wei-cheng have no objections, this patch can go in.
@@ -68,15 +68,6 @@ supply_fast_tracepoint_registers (struct regcache *regcache,
((char *) buf) + x86_64_ft_collect_regmap[i]);
}
-IP_AGENT_EXPORT_FUNC ULONGEST
-gdb_agent_get_raw_reg (const unsigned char *raw_regs, int regnum)
-{
- if (regnum >= X86_64_NUM_FT_COLLECT_GREGS)
- return 0;
-
- return *(ULONGEST *) (raw_regs + x86_64_ft_collect_regmap[regnum]);
-}
-
#ifdef HAVE_UST
#include <ust/processor.h>
@@ -98,21 +98,6 @@ supply_fast_tracepoint_registers (struct regcache *regcache,
}
}
-IP_AGENT_EXPORT_FUNC ULONGEST
-gdb_agent_get_raw_reg (const unsigned char *raw_regs, int regnum)
-{
- /* This should maybe be allowed to return an error code, or perhaps
- better, have the emit_reg detect this, and emit a constant zero,
- or something. */
-
- if (regnum > i386_num_regs)
- return 0;
- else if (regnum >= I386_CS_REGNUM && regnum <= I386_GS_REGNUM)
- return *(short *) (raw_regs + i386_ft_collect_regmap[regnum]);
- else
- return *(int *) (raw_regs + i386_ft_collect_regmap[regnum]);
-}
-
#ifdef HAVE_UST
#include <ust/processor.h>
@@ -2287,7 +2287,7 @@ amd64_emit_reg (int reg)
i += 4;
append_insns (&buildaddr, i, buf);
current_insn_ptr = buildaddr;
- amd64_emit_call (get_raw_reg_func_addr ());
+ amd64_emit_call (get_reg_func_addr ());
}
static void
@@ -2896,7 +2896,7 @@ i386_emit_reg (int reg)
"mov %eax,4(%esp)\n\t"
"mov 8(%ebp),%eax\n\t"
"mov %eax,(%esp)");
- i386_emit_call (get_raw_reg_func_addr ());
+ i386_emit_call (get_reg_func_addr ());
EMIT_ASM32 (i386_reg_c,
"xor %ebx,%ebx\n\t"
"lea 0x8(%esp),%esp");
@@ -126,7 +126,7 @@ trace_vdebug (const char *fmt, ...)
# define traceframe_write_count IPA_SYM_EXPORTED_NAME (traceframe_write_count)
# define traceframes_created IPA_SYM_EXPORTED_NAME (traceframes_created)
# define trace_state_variables IPA_SYM_EXPORTED_NAME (trace_state_variables)
-# define get_raw_reg IPA_SYM_EXPORTED_NAME (get_raw_reg)
+# define get_reg IPA_SYM_EXPORTED_NAME (get_reg)
# define get_trace_state_variable_value \
IPA_SYM_EXPORTED_NAME (get_trace_state_variable_value)
# define set_trace_state_variable_value \
@@ -167,7 +167,7 @@ struct ipa_sym_addresses
CORE_ADDR addr_traceframe_write_count;
CORE_ADDR addr_traceframes_created;
CORE_ADDR addr_trace_state_variables;
- CORE_ADDR addr_get_raw_reg;
+ CORE_ADDR addr_get_reg;
CORE_ADDR addr_get_trace_state_variable_value;
CORE_ADDR addr_set_trace_state_variable_value;
CORE_ADDR addr_ust_loaded;
@@ -203,7 +203,7 @@ static struct
IPA_SYM(traceframe_write_count),
IPA_SYM(traceframes_created),
IPA_SYM(trace_state_variables),
- IPA_SYM(get_raw_reg),
+ IPA_SYM(get_reg),
IPA_SYM(get_trace_state_variable_value),
IPA_SYM(set_trace_state_variable_value),
IPA_SYM(ust_loaded),
@@ -1306,10 +1306,8 @@ struct fast_tracepoint_ctx
struct tracepoint_hit_ctx base;
struct regcache regcache;
- int regcache_initted;
unsigned char *regspace;
- unsigned char *regs;
struct tracepoint *tpoint;
};
@@ -4724,13 +4722,6 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
if (ctx->type == fast_tracepoint)
{
struct fast_tracepoint_ctx *fctx = (struct fast_tracepoint_ctx *) ctx;
- if (!fctx->regcache_initted)
- {
- fctx->regcache_initted = 1;
- init_register_cache (&fctx->regcache, ipa_tdesc, fctx->regspace);
- supply_regblock (&fctx->regcache, NULL);
- supply_fast_tracepoint_registers (&fctx->regcache, fctx->regs);
- }
regcache = &fctx->regcache;
}
#ifdef HAVE_UST
@@ -5796,11 +5787,16 @@ gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
return;
ctx.base.type = fast_tracepoint;
- ctx.regs = regs;
- ctx.regcache_initted = 0;
/* Wrap the regblock in a register cache (in the stack, we don't
want to malloc here). */
ctx.regspace = alloca (ipa_tdesc->registers_size);
+
+ /* Initialize the regcache with registers from the stack set up by jump
+ pad. */
+ init_register_cache (&ctx.regcache, ipa_tdesc, ctx.regspace);
+ supply_regblock (&ctx.regcache, NULL);
+ supply_fast_tracepoint_registers (&ctx.regcache, regs);
+
if (ctx.regspace == NULL)
{
trace_debug ("Trace buffer block allocation failed, skipping");
@@ -5853,14 +5849,28 @@ gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
}
}
+/* This function is called from native code generated for the emit_reg
+ agent expression bytecode. It returns the value of register number
+ REGNUM read from regcache contained into CTX. */
+
+IP_AGENT_EXPORT_FUNC ULONGEST
+gdb_agent_get_reg (struct fast_tracepoint_ctx *ctx, int regnum)
+{
+ ULONGEST reg;
+
+ collect_register (&ctx->regcache, regnum, ®);
+
+ return reg;
+}
+
#endif
#ifndef IN_PROCESS_AGENT
CORE_ADDR
-get_raw_reg_func_addr (void)
+get_reg_func_addr (void)
{
- return ipa_sym_addrs.addr_get_raw_reg;
+ return ipa_sym_addrs.addr_get_reg;
}
CORE_ADDR
@@ -163,13 +163,8 @@ int agent_mem_read_string (struct eval_agent_expr_context *ctx,
CORE_ADDR from,
ULONGEST len);
-/* The prototype the get_raw_reg function in the IPA. Each arch's
- bytecode compiler emits calls to this function. */
-IP_AGENT_EXPORT_FUNC ULONGEST gdb_agent_get_raw_reg
- (const unsigned char *raw_regs, int regnum);
-
-/* Returns the address of the get_raw_reg function in the IPA. */
-CORE_ADDR get_raw_reg_func_addr (void);
+/* Returns the address of the get_reg function in the IPA. */
+CORE_ADDR get_reg_func_addr (void);
/* Returns the address of the get_trace_state_variable_value
function in the IPA. */
CORE_ADDR get_get_tsv_func_addr (void);