| Message ID | 1361886653-10886-1-git-send-email-anna_neiman@mentor.com |
|---|---|
| State | New |
| Headers | show |
On 26 February 2013 13:50, Anna Neiman <anna_neiman@mentor.com> wrote: > > Signed-off-by: Anna Neiman <anna_neiman@mentor.com> The formatting of your patches is still a mess (huge long subject, not properly threaded with a cover letter, etc) and you're still doing too much in one patch. Hint: "add missing include guards to header files" is probably a patch of its own. -- PMM
Relating to size of patch - I divided it maximally, Relating to subject - thank for your note - I'll take it in account next time. -----Original Message----- From: Peter Maydell [mailto:peter.maydell@linaro.org] Sent: Tuesday, February 26, 2013 3:54 PM To: Neiman, Anna Cc: qemu-devel@nongnu.org; Rozenman, Alex; Schreiber, Amir; Anthony Liguori; Brook, Paul Subject: Re: [PATCH] PATCH1 - infrastructure for Evaluate breakpoint condition on target.OPC_MAX_SIZE - size of translation buffer - was increased because condition translated bytecode should be executed in one block On 26 February 2013 13:50, Anna Neiman <anna_neiman@mentor.com> wrote: > > Signed-off-by: Anna Neiman <anna_neiman@mentor.com> The formatting of your patches is still a mess (huge long subject, not properly threaded with a cover letter, etc) and you're still doing too much in one patch. Hint: "add missing include guards to header files" is probably a patch of its own. -- PMM
Please study http://wiki.qemu.org/Contribute/SubmitAPatch carefully. But lets assume this patch only adds the infrastructure to parse and store the condition expression in a breakpoint. On 2013-02-26 14:50, Anna Neiman wrote: > Signed-off-by: Anna Neiman <anna_neiman@mentor.com> > --- > exec.c | 16 ++++++++++++- > gdbstub.c | 57 ++++++++++++++++++++++++++++++++++++++++++----- > include/exec/cpu-all.h | 1 + > include/exec/cpu-defs.h | 20 +++++++++++++++++ > include/exec/exec-all.h | 2 +- > target-i386/helper.c | 2 +- > tcg/tcg-op.h | 10 +++++++++ > tcg/tcg.h | 6 +++++ > 8 files changed, 106 insertions(+), 8 deletions(-) > > diff --git a/exec.c b/exec.c > index a41bcb8..4289c87 100644 > --- a/exec.c > +++ b/exec.c > @@ -398,6 +398,7 @@ void cpu_watchpoint_remove_all(CPUArchState *env, int mask) > > /* Add a breakpoint. */ > int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, > + long cond_len, uint8_t *cond_exp, ^^^^ ^^^^^^^ size_t Is it a string or a blob? > CPUBreakpoint **breakpoint) > { > #if defined(TARGET_HAS_ICE) > @@ -407,6 +408,13 @@ int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, > > bp->pc = pc; > bp->flags = flags; > + bp->cond_len = cond_len; > + if (cond_exp == NULL || cond_len == 0) { > + bp->cond_exp = NULL; > + } else { > + bp->cond_exp = g_malloc(sizeof(uint8_t) * cond_len); Why do you replicate the condition here? Is there a use case where the caller wants to pass in something static? You can document that this argument is g_free'd on breakpoint release. > + memcpy(bp->cond_exp, cond_exp, sizeof(uint8_t) * cond_len); ^^^^^^^^^^^^^^^ well... > + } > > /* keep all GDB-injected breakpoints in front */ > if (flags & BP_GDB) > @@ -450,6 +458,11 @@ void cpu_breakpoint_remove_by_ref(CPUArchState *env, CPUBreakpoint *breakpoint) > > breakpoint_invalidate(env, breakpoint->pc); > > + if (breakpoint->cond_len != 0 && breakpoint->cond_exp != NULL) { Unneeded. g_free is a nop if breakpoint->cond_exp is NULL. > + g_free(breakpoint->cond_exp); > + } > + > + > g_free(breakpoint); > #endif > } > @@ -551,7 +564,8 @@ CPUArchState *cpu_copy(CPUArchState *env) > QTAILQ_INIT(&env->watchpoints); > #if defined(TARGET_HAS_ICE) > QTAILQ_FOREACH(bp, &env->breakpoints, entry) { > - cpu_breakpoint_insert(new_env, bp->pc, bp->flags, NULL); > + cpu_breakpoint_insert(new_env, bp->pc, bp->flags, > + bp->cond_len, bp->cond_exp, NULL); > } > QTAILQ_FOREACH(wp, &env->watchpoints, entry) { > cpu_watchpoint_insert(new_env, wp->vaddr, (~wp->len_mask) + 1, > diff --git a/gdbstub.c b/gdbstub.c > index 32dfea9..814f596 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -1941,7 +1941,8 @@ static const int xlat_gdb_type[] = { > }; > #endif > > -static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type) > +static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type, > + long cond_len, uint8_t *cond_exp) > { > CPUArchState *env; > int err = 0; > @@ -1953,7 +1954,8 @@ static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type) > case GDB_BREAKPOINT_SW: > case GDB_BREAKPOINT_HW: > for (env = first_cpu; env != NULL; env = env->next_cpu) { > - err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL); > + err = cpu_breakpoint_insert(env, addr, BP_GDB, > + cond_len, cond_exp, NULL); > if (err) > break; > } > @@ -2089,6 +2091,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > uint8_t *registers; > target_ulong addr, len; > > + uint8_t *bp_cond_expr = NULL; > + int bp_cond_len = 0; > + int i = 0 ; > + > #ifdef DEBUG_GDB > printf("command='%s'\n", line_buf); > #endif > @@ -2310,16 +2316,54 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > if (*p == ',') > p++; > len = strtoull(p, (char **)&p, 16); > - if (ch == 'Z') > - res = gdb_breakpoint_insert(addr, len, type); > - else > + while (isspace(*p)) { > + p++; > + } "We include spaces in some of the templates for clarity; these are not part of the packet's syntax. No gdb packet uses spaces to separate its components." > + if (ch == 'Z' && *p == ';') { > + p++; > + while (isspace(*p)) { > + p++; > + } > + if (*p == 'X') { > + p++; > + bp_cond_len = strtoul(p, (char **)&p, 16); > + if (*p == ',') { > + p++; > + } > + if (bp_cond_len > 0) { > + int bp_cond_size = sizeof(uint8_t) * bp_cond_len; > + bp_cond_expr = (uint8_t *)g_malloc(bp_cond_size); ^^^^^^^^^^ g_malloc returns void * - implicitly castable. > + memset(bp_cond_expr, 0, bp_cond_size); > + > + for (i = 0 ; i < bp_cond_len ; i++) { > + if (!isxdigit(*p) || !isxdigit(*(p + 1))) { > + bp_cond_len = 0 ; > + g_free(bp_cond_expr); > + bp_cond_expr = NULL; > + error_report("Error in breakpoint condition"); Shouldn't this be reported to the gdb frontend instead? And not breakpoint inserted? Or does the spec say something else? > + } else { > + hextomem(bp_cond_expr+i, p, 1); > + p += 2; > + } > + } > + } > + } > + } > + if (ch == 'Z') { But the logic above in a separate function and call it here. > + res = gdb_breakpoint_insert(addr, len, type, > + bp_cond_len, bp_cond_expr); > + } else { > res = gdb_breakpoint_remove(addr, len, type); > + } > if (res >= 0) > put_packet(s, "OK"); > else if (res == -ENOSYS) > put_packet(s, ""); > else > put_packet(s, "E22"); > + if (bp_cond_expr != NULL) { > + g_free(bp_cond_expr); > + } > break; > case 'H': > type = *p++; > @@ -2445,6 +2489,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > #endif /* !CONFIG_USER_ONLY */ > if (strncmp(p, "Supported", 9) == 0) { > snprintf(buf, sizeof(buf), "PacketSize=%x", MAX_PACKET_LENGTH); > + > + /* conditional breakpoint evaluation on target*/ > + pstrcat(buf, sizeof(buf), ";ConditionalBreakpoints+"); Feature activation should not happen before the feature is complete. So this should be part of a separate last patch of your series. > #ifdef GDB_CORE_XML > pstrcat(buf, sizeof(buf), ";qXfer:features:read+"); > #endif > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index 249e046..383c4c1 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -448,6 +448,7 @@ void cpu_exit(CPUArchState *s); > #define BP_CPU 0x20 > > int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, > + long cond_len, uint8_t *cond_exp, > CPUBreakpoint **breakpoint); > int cpu_breakpoint_remove(CPUArchState *env, target_ulong pc, int flags); > void cpu_breakpoint_remove_by_ref(CPUArchState *env, CPUBreakpoint *breakpoint); > diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h > index 3dc9656..0bf63c2 100644 > --- a/include/exec/cpu-defs.h > +++ b/include/exec/cpu-defs.h > @@ -136,9 +136,13 @@ typedef struct icount_decr_u16 { > typedef struct CPUBreakpoint { > target_ulong pc; > int flags; /* BP_* */ > + int cond_len; > + uint8_t *cond_exp; > QTAILQ_ENTRY(CPUBreakpoint) entry; > } CPUBreakpoint; > > +int bp_has_cond(CPUBreakpoint *bp); Not defined in this patch. > + > typedef struct CPUWatchpoint { > target_ulong vaddr; > target_ulong len_mask; > @@ -146,6 +150,18 @@ typedef struct CPUWatchpoint { > QTAILQ_ENTRY(CPUWatchpoint) entry; > } CPUWatchpoint; > > + > + > +typedef double target_double; > + > +typedef union BPAgentStackElementType { > + target_long l; > + target_double d; > +} BPAgentStackElementType; > + > + > +#define BP_AGENT_MAX_STACK_SIZE 1024 > + Also unrelated, likely everything below does not belong here. And a singe newline suffices as separator. > #define CPU_TEMP_BUF_NLONGS 128 > #define CPU_COMMON \ > /* soft mmu support */ \ > @@ -191,6 +207,10 @@ typedef struct CPUWatchpoint { > /* user data */ \ > void *opaque; \ > \ > + BPAgentStackElementType bp_agent_stack[BP_AGENT_MAX_STACK_SIZE]; \ > + BPAgentStackElementType *bp_agent_stack_current; \ > + /*bp_agent_stack_current is the current location in bp_agent_stack */ \ > + int bp_agent_error; /* error in evaluation - ex., divide by zero*/ \ > const char *cpu_model_str; > > #endif > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index e856191..db5a38c 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -59,7 +59,7 @@ typedef struct TranslationBlock TranslationBlock; > * and up to 4 + N parameters on 64-bit archs > * (N = number of input arguments + output arguments). */ > #define MAX_OPC_PARAM (4 + (MAX_OPC_PARAM_PER_ARG * MAX_OPC_PARAM_ARGS)) > -#define OPC_BUF_SIZE 640 > +#define OPC_BUF_SIZE 2560 > #define OPC_MAX_SIZE (OPC_BUF_SIZE - MAX_OP_PER_INSTR) > > /* Maximum size a TCG op can expand to. This is complicated because a > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 82a731c..a0d9e93 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -984,7 +984,7 @@ void hw_breakpoint_insert(CPUX86State *env, int index) > switch (hw_breakpoint_type(env->dr[7], index)) { > case DR7_TYPE_BP_INST: > if (hw_breakpoint_enabled(env->dr[7], index)) { > - err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU, > + err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU, 0, NULL, > &env->cpu_breakpoint[index]); > } > break; > diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h > index d70b2eb..6c3353c 100644 > --- a/tcg/tcg-op.h > +++ b/tcg/tcg-op.h > @@ -21,6 +21,10 @@ > * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > * THE SOFTWARE. > */ > + > + > +#ifndef __TCG_OP_H__ > +#define __TCG_OP_H__ > #include "tcg.h" > > int gen_new_label(void); > @@ -2946,6 +2950,8 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index) > TCGV_PTR_TO_NAT(B)) > #define tcg_gen_addi_ptr(R, A, B) tcg_gen_addi_i32(TCGV_PTR_TO_NAT(R), \ > TCGV_PTR_TO_NAT(A), (B)) > +#define tcg_gen_subi_ptr(R, A, B) tcg_gen_subi_i32(TCGV_PTR_TO_NAT(R), \ > + TCGV_PTR_TO_NAT(A), (B)) > #define tcg_gen_ext_i32_ptr(R, A) tcg_gen_mov_i32(TCGV_PTR_TO_NAT(R), (A)) > #else /* TCG_TARGET_REG_BITS == 32 */ > #define tcg_gen_add_ptr(R, A, B) tcg_gen_add_i64(TCGV_PTR_TO_NAT(R), \ > @@ -2953,5 +2959,9 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index) > TCGV_PTR_TO_NAT(B)) > #define tcg_gen_addi_ptr(R, A, B) tcg_gen_addi_i64(TCGV_PTR_TO_NAT(R), \ > TCGV_PTR_TO_NAT(A), (B)) > +#define tcg_gen_subi_ptr(R, A, B) tcg_gen_subi_i64(TCGV_PTR_TO_NAT(R), \ > + TCGV_PTR_TO_NAT(A), (B)) > #define tcg_gen_ext_i32_ptr(R, A) tcg_gen_ext_i32_i64(TCGV_PTR_TO_NAT(R), (A)) > #endif /* TCG_TARGET_REG_BITS != 32 */ > + > +#endif /* __TCG_OP_H__ */ > diff --git a/tcg/tcg.h b/tcg/tcg.h > index b195396..b367406 100644 > --- a/tcg/tcg.h > +++ b/tcg/tcg.h > @@ -21,6 +21,10 @@ > * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > * THE SOFTWARE. > */ > + > +#ifndef __TCG_H__ > +#define __TCG_H__ > + > #include "qemu-common.h" > > /* Target word size (must be identical to pointer size). */ > @@ -690,3 +694,5 @@ void tcg_register_jit(void *buf, size_t buf_size); > /* Generate TB finalization at the end of block */ > void tcg_out_tb_finalize(TCGContext *s); > #endif > + > +#endif /* __TCG_H__*/ > I'm looking forward to this feature, and I'm already dreaming of extending KVM to support this as well. Too often I had to add a condition at performance sensitive spots in the target kernel's source code. Jan
Hi Jan, Thank you for your review. I added 4 patches including whole implementation of this feature, not just infrastructure. Some explanations for your notes : Cond_exp is the array of bytecodes ( not string ), cond_len - size of this array . For more info ,please, look GDB agent documentation: http://sourceware.org/gdb/onlinedocs/gdb/Agent-Expressions.html#Agent-Expressions Cond_exp is removed on breakpoint release. In all cases when we have error in breakpoint condition, it just will be evaluated on host ( the old mode ). Relating checks for spaces - maybe it is not needed but it seems me more safe and it doesn't require huge resources. Please, let me know if you see critical issues , which prevent the confirmation of the patch. The issues listed below are not look those. Thanks, Anna -----Original Message----- From: Jan Kiszka [mailto:jan.kiszka@siemens.com] Sent: Tuesday, February 26, 2013 5:53 PM To: Neiman, Anna Cc: qemu-devel@nongnu.org; Peter Maydell; Anthony Liguori; Brook, Paul; Schreiber, Amir; Rozenman, Alex Subject: Re: [PATCH] PATCH1 - infrastructure for Evaluate breakpoint condition on target.OPC_MAX_SIZE - size of translation buffer - was increased because condition translated bytecode should be executed in one block Please study http://wiki.qemu.org/Contribute/SubmitAPatch carefully. But lets assume this patch only adds the infrastructure to parse and store the condition expression in a breakpoint. On 2013-02-26 14:50, Anna Neiman wrote: > Signed-off-by: Anna Neiman <anna_neiman@mentor.com> > --- > exec.c | 16 ++++++++++++- > gdbstub.c | 57 ++++++++++++++++++++++++++++++++++++++++++----- > include/exec/cpu-all.h | 1 + > include/exec/cpu-defs.h | 20 +++++++++++++++++ > include/exec/exec-all.h | 2 +- > target-i386/helper.c | 2 +- > tcg/tcg-op.h | 10 +++++++++ > tcg/tcg.h | 6 +++++ > 8 files changed, 106 insertions(+), 8 deletions(-) > > diff --git a/exec.c b/exec.c > index a41bcb8..4289c87 100644 > --- a/exec.c > +++ b/exec.c > @@ -398,6 +398,7 @@ void cpu_watchpoint_remove_all(CPUArchState *env, int mask) > > /* Add a breakpoint. */ > int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, > + long cond_len, uint8_t *cond_exp, ^^^^ ^^^^^^^ size_t Is it a string or a blob? > CPUBreakpoint **breakpoint) > { > #if defined(TARGET_HAS_ICE) > @@ -407,6 +408,13 @@ int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, > > bp->pc = pc; > bp->flags = flags; > + bp->cond_len = cond_len; > + if (cond_exp == NULL || cond_len == 0) { > + bp->cond_exp = NULL; > + } else { > + bp->cond_exp = g_malloc(sizeof(uint8_t) * cond_len); Why do you replicate the condition here? Is there a use case where the caller wants to pass in something static? You can document that this argument is g_free'd on breakpoint release. > + memcpy(bp->cond_exp, cond_exp, sizeof(uint8_t) * cond_len); ^^^^^^^^^^^^^^^ well... > + } > > /* keep all GDB-injected breakpoints in front */ > if (flags & BP_GDB) > @@ -450,6 +458,11 @@ void cpu_breakpoint_remove_by_ref(CPUArchState *env, CPUBreakpoint *breakpoint) > > breakpoint_invalidate(env, breakpoint->pc); > > + if (breakpoint->cond_len != 0 && breakpoint->cond_exp != NULL) { Unneeded. g_free is a nop if breakpoint->cond_exp is NULL. > + g_free(breakpoint->cond_exp); > + } > + > + > g_free(breakpoint); > #endif > } > @@ -551,7 +564,8 @@ CPUArchState *cpu_copy(CPUArchState *env) > QTAILQ_INIT(&env->watchpoints); > #if defined(TARGET_HAS_ICE) > QTAILQ_FOREACH(bp, &env->breakpoints, entry) { > - cpu_breakpoint_insert(new_env, bp->pc, bp->flags, NULL); > + cpu_breakpoint_insert(new_env, bp->pc, bp->flags, > + bp->cond_len, bp->cond_exp, NULL); > } > QTAILQ_FOREACH(wp, &env->watchpoints, entry) { > cpu_watchpoint_insert(new_env, wp->vaddr, (~wp->len_mask) + 1, > diff --git a/gdbstub.c b/gdbstub.c > index 32dfea9..814f596 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -1941,7 +1941,8 @@ static const int xlat_gdb_type[] = { > }; > #endif > > -static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type) > +static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type, > + long cond_len, uint8_t *cond_exp) > { > CPUArchState *env; > int err = 0; > @@ -1953,7 +1954,8 @@ static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type) > case GDB_BREAKPOINT_SW: > case GDB_BREAKPOINT_HW: > for (env = first_cpu; env != NULL; env = env->next_cpu) { > - err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL); > + err = cpu_breakpoint_insert(env, addr, BP_GDB, > + cond_len, cond_exp, NULL); > if (err) > break; > } > @@ -2089,6 +2091,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > uint8_t *registers; > target_ulong addr, len; > > + uint8_t *bp_cond_expr = NULL; > + int bp_cond_len = 0; > + int i = 0 ; > + > #ifdef DEBUG_GDB > printf("command='%s'\n", line_buf); > #endif > @@ -2310,16 +2316,54 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > if (*p == ',') > p++; > len = strtoull(p, (char **)&p, 16); > - if (ch == 'Z') > - res = gdb_breakpoint_insert(addr, len, type); > - else > + while (isspace(*p)) { > + p++; > + } "We include spaces in some of the templates for clarity; these are not part of the packet's syntax. No gdb packet uses spaces to separate its components." > + if (ch == 'Z' && *p == ';') { > + p++; > + while (isspace(*p)) { > + p++; > + } > + if (*p == 'X') { > + p++; > + bp_cond_len = strtoul(p, (char **)&p, 16); > + if (*p == ',') { > + p++; > + } > + if (bp_cond_len > 0) { > + int bp_cond_size = sizeof(uint8_t) * bp_cond_len; > + bp_cond_expr = (uint8_t *)g_malloc(bp_cond_size); ^^^^^^^^^^ g_malloc returns void * - implicitly castable. > + memset(bp_cond_expr, 0, bp_cond_size); > + > + for (i = 0 ; i < bp_cond_len ; i++) { > + if (!isxdigit(*p) || !isxdigit(*(p + 1))) { > + bp_cond_len = 0 ; > + g_free(bp_cond_expr); > + bp_cond_expr = NULL; > + error_report("Error in breakpoint condition"); Shouldn't this be reported to the gdb frontend instead? And not breakpoint inserted? Or does the spec say something else? > + } else { > + hextomem(bp_cond_expr+i, p, 1); > + p += 2; > + } > + } > + } > + } > + } > + if (ch == 'Z') { But the logic above in a separate function and call it here. > + res = gdb_breakpoint_insert(addr, len, type, > + bp_cond_len, bp_cond_expr); > + } else { > res = gdb_breakpoint_remove(addr, len, type); > + } > if (res >= 0) > put_packet(s, "OK"); > else if (res == -ENOSYS) > put_packet(s, ""); > else > put_packet(s, "E22"); > + if (bp_cond_expr != NULL) { > + g_free(bp_cond_expr); > + } > break; > case 'H': > type = *p++; > @@ -2445,6 +2489,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > #endif /* !CONFIG_USER_ONLY */ > if (strncmp(p, "Supported", 9) == 0) { > snprintf(buf, sizeof(buf), "PacketSize=%x", MAX_PACKET_LENGTH); > + > + /* conditional breakpoint evaluation on target*/ > + pstrcat(buf, sizeof(buf), ";ConditionalBreakpoints+"); Feature activation should not happen before the feature is complete. So this should be part of a separate last patch of your series. > #ifdef GDB_CORE_XML > pstrcat(buf, sizeof(buf), ";qXfer:features:read+"); > #endif > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index 249e046..383c4c1 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -448,6 +448,7 @@ void cpu_exit(CPUArchState *s); > #define BP_CPU 0x20 > > int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, > + long cond_len, uint8_t *cond_exp, > CPUBreakpoint **breakpoint); > int cpu_breakpoint_remove(CPUArchState *env, target_ulong pc, int flags); > void cpu_breakpoint_remove_by_ref(CPUArchState *env, CPUBreakpoint *breakpoint); > diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h > index 3dc9656..0bf63c2 100644 > --- a/include/exec/cpu-defs.h > +++ b/include/exec/cpu-defs.h > @@ -136,9 +136,13 @@ typedef struct icount_decr_u16 { > typedef struct CPUBreakpoint { > target_ulong pc; > int flags; /* BP_* */ > + int cond_len; > + uint8_t *cond_exp; > QTAILQ_ENTRY(CPUBreakpoint) entry; > } CPUBreakpoint; > > +int bp_has_cond(CPUBreakpoint *bp); Not defined in this patch. > + > typedef struct CPUWatchpoint { > target_ulong vaddr; > target_ulong len_mask; > @@ -146,6 +150,18 @@ typedef struct CPUWatchpoint { > QTAILQ_ENTRY(CPUWatchpoint) entry; > } CPUWatchpoint; > > + > + > +typedef double target_double; > + > +typedef union BPAgentStackElementType { > + target_long l; > + target_double d; > +} BPAgentStackElementType; > + > + > +#define BP_AGENT_MAX_STACK_SIZE 1024 > + Also unrelated, likely everything below does not belong here. And a singe newline suffices as separator. > #define CPU_TEMP_BUF_NLONGS 128 > #define CPU_COMMON \ > /* soft mmu support */ \ > @@ -191,6 +207,10 @@ typedef struct CPUWatchpoint { > /* user data */ \ > void *opaque; \ > \ > + BPAgentStackElementType bp_agent_stack[BP_AGENT_MAX_STACK_SIZE]; \ > + BPAgentStackElementType *bp_agent_stack_current; \ > + /*bp_agent_stack_current is the current location in bp_agent_stack */ \ > + int bp_agent_error; /* error in evaluation - ex., divide by zero*/ \ > const char *cpu_model_str; > > #endif > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index e856191..db5a38c 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -59,7 +59,7 @@ typedef struct TranslationBlock TranslationBlock; > * and up to 4 + N parameters on 64-bit archs > * (N = number of input arguments + output arguments). */ > #define MAX_OPC_PARAM (4 + (MAX_OPC_PARAM_PER_ARG * MAX_OPC_PARAM_ARGS)) > -#define OPC_BUF_SIZE 640 > +#define OPC_BUF_SIZE 2560 > #define OPC_MAX_SIZE (OPC_BUF_SIZE - MAX_OP_PER_INSTR) > > /* Maximum size a TCG op can expand to. This is complicated because a > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 82a731c..a0d9e93 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -984,7 +984,7 @@ void hw_breakpoint_insert(CPUX86State *env, int index) > switch (hw_breakpoint_type(env->dr[7], index)) { > case DR7_TYPE_BP_INST: > if (hw_breakpoint_enabled(env->dr[7], index)) { > - err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU, > + err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU, 0, NULL, > &env->cpu_breakpoint[index]); > } > break; > diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h > index d70b2eb..6c3353c 100644 > --- a/tcg/tcg-op.h > +++ b/tcg/tcg-op.h > @@ -21,6 +21,10 @@ > * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > * THE SOFTWARE. > */ > + > + > +#ifndef __TCG_OP_H__ > +#define __TCG_OP_H__ > #include "tcg.h" > > int gen_new_label(void); > @@ -2946,6 +2950,8 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index) > TCGV_PTR_TO_NAT(B)) > #define tcg_gen_addi_ptr(R, A, B) tcg_gen_addi_i32(TCGV_PTR_TO_NAT(R), \ > TCGV_PTR_TO_NAT(A), (B)) > +#define tcg_gen_subi_ptr(R, A, B) tcg_gen_subi_i32(TCGV_PTR_TO_NAT(R), \ > + TCGV_PTR_TO_NAT(A), (B)) > #define tcg_gen_ext_i32_ptr(R, A) tcg_gen_mov_i32(TCGV_PTR_TO_NAT(R), (A)) > #else /* TCG_TARGET_REG_BITS == 32 */ > #define tcg_gen_add_ptr(R, A, B) tcg_gen_add_i64(TCGV_PTR_TO_NAT(R), \ > @@ -2953,5 +2959,9 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index) > TCGV_PTR_TO_NAT(B)) > #define tcg_gen_addi_ptr(R, A, B) tcg_gen_addi_i64(TCGV_PTR_TO_NAT(R), \ > TCGV_PTR_TO_NAT(A), (B)) > +#define tcg_gen_subi_ptr(R, A, B) tcg_gen_subi_i64(TCGV_PTR_TO_NAT(R), \ > + TCGV_PTR_TO_NAT(A), (B)) > #define tcg_gen_ext_i32_ptr(R, A) tcg_gen_ext_i32_i64(TCGV_PTR_TO_NAT(R), (A)) > #endif /* TCG_TARGET_REG_BITS != 32 */ > + > +#endif /* __TCG_OP_H__ */ > diff --git a/tcg/tcg.h b/tcg/tcg.h > index b195396..b367406 100644 > --- a/tcg/tcg.h > +++ b/tcg/tcg.h > @@ -21,6 +21,10 @@ > * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > * THE SOFTWARE. > */ > + > +#ifndef __TCG_H__ > +#define __TCG_H__ > + > #include "qemu-common.h" > > /* Target word size (must be identical to pointer size). */ > @@ -690,3 +694,5 @@ void tcg_register_jit(void *buf, size_t buf_size); > /* Generate TB finalization at the end of block */ > void tcg_out_tb_finalize(TCGContext *s); > #endif > + > +#endif /* __TCG_H__*/ > I'm looking forward to this feature, and I'm already dreaming of extending KVM to support this as well. Too often I had to add a condition at performance sensitive spots in the target kernel's source code. Jan
On 2013-02-27 08:25, Neiman, Anna wrote: > Hi Jan, > Thank you for your review. > I added 4 patches including whole implementation of this feature, not just infrastructure. > > Some explanations for your notes : Please comment inline, do not top-post. > Cond_exp is the array of bytecodes ( not string ), cond_len - size of this array . For more info ,please, look GDB agent documentation: http://sourceware.org/gdb/onlinedocs/gdb/Agent-Expressions.html#Agent-Expressions OK, thanks for clarifying. > Cond_exp is removed on breakpoint release. > In all cases when we have error in breakpoint condition, it just will be evaluated on host ( the old mode ). Not sure what you mean with "host" here, but syntax errors in the remote gdb command (maybe not the condition bytecode) shall be evaluated before injecting the breakpoint and shall be reported back to the gdb frontend, not the local QEMU console. > Relating checks for spaces - maybe it is not needed but it seems me more safe and it doesn't require huge resources. Spaces are clearly against the spec and shall not be accepted by QEMU's gdbstub. > > Please, let me know if you see critical issues , which prevent the confirmation of the patch. > The issues listed below are not look those. All issues are important unless you can convince us that they aren't. You should try to adopt all the suggestions we make quickly and carefully or explain very well where you don't. Then repost new versions of the series. That will help making the patches acceptable for upstream. The current form is not yet. But that's normal, specifically when you start working in a new community. Jan
diff --git a/exec.c b/exec.c index a41bcb8..4289c87 100644 --- a/exec.c +++ b/exec.c @@ -398,6 +398,7 @@ void cpu_watchpoint_remove_all(CPUArchState *env, int mask) /* Add a breakpoint. */ int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, + long cond_len, uint8_t *cond_exp, CPUBreakpoint **breakpoint) { #if defined(TARGET_HAS_ICE) @@ -407,6 +408,13 @@ int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, bp->pc = pc; bp->flags = flags; + bp->cond_len = cond_len; + if (cond_exp == NULL || cond_len == 0) { + bp->cond_exp = NULL; + } else { + bp->cond_exp = g_malloc(sizeof(uint8_t) * cond_len); + memcpy(bp->cond_exp, cond_exp, sizeof(uint8_t) * cond_len); + } /* keep all GDB-injected breakpoints in front */ if (flags & BP_GDB) @@ -450,6 +458,11 @@ void cpu_breakpoint_remove_by_ref(CPUArchState *env, CPUBreakpoint *breakpoint) breakpoint_invalidate(env, breakpoint->pc); + if (breakpoint->cond_len != 0 && breakpoint->cond_exp != NULL) { + g_free(breakpoint->cond_exp); + } + + g_free(breakpoint); #endif } @@ -551,7 +564,8 @@ CPUArchState *cpu_copy(CPUArchState *env) QTAILQ_INIT(&env->watchpoints); #if defined(TARGET_HAS_ICE) QTAILQ_FOREACH(bp, &env->breakpoints, entry) { - cpu_breakpoint_insert(new_env, bp->pc, bp->flags, NULL); + cpu_breakpoint_insert(new_env, bp->pc, bp->flags, + bp->cond_len, bp->cond_exp, NULL); } QTAILQ_FOREACH(wp, &env->watchpoints, entry) { cpu_watchpoint_insert(new_env, wp->vaddr, (~wp->len_mask) + 1, diff --git a/gdbstub.c b/gdbstub.c index 32dfea9..814f596 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -1941,7 +1941,8 @@ static const int xlat_gdb_type[] = { }; #endif -static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type) +static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type, + long cond_len, uint8_t *cond_exp) { CPUArchState *env; int err = 0; @@ -1953,7 +1954,8 @@ static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type) case GDB_BREAKPOINT_SW: case GDB_BREAKPOINT_HW: for (env = first_cpu; env != NULL; env = env->next_cpu) { - err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL); + err = cpu_breakpoint_insert(env, addr, BP_GDB, + cond_len, cond_exp, NULL); if (err) break; } @@ -2089,6 +2091,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) uint8_t *registers; target_ulong addr, len; + uint8_t *bp_cond_expr = NULL; + int bp_cond_len = 0; + int i = 0 ; + #ifdef DEBUG_GDB printf("command='%s'\n", line_buf); #endif @@ -2310,16 +2316,54 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) if (*p == ',') p++; len = strtoull(p, (char **)&p, 16); - if (ch == 'Z') - res = gdb_breakpoint_insert(addr, len, type); - else + while (isspace(*p)) { + p++; + } + if (ch == 'Z' && *p == ';') { + p++; + while (isspace(*p)) { + p++; + } + if (*p == 'X') { + p++; + bp_cond_len = strtoul(p, (char **)&p, 16); + if (*p == ',') { + p++; + } + if (bp_cond_len > 0) { + int bp_cond_size = sizeof(uint8_t) * bp_cond_len; + bp_cond_expr = (uint8_t *)g_malloc(bp_cond_size); + memset(bp_cond_expr, 0, bp_cond_size); + + for (i = 0 ; i < bp_cond_len ; i++) { + if (!isxdigit(*p) || !isxdigit(*(p + 1))) { + bp_cond_len = 0 ; + g_free(bp_cond_expr); + bp_cond_expr = NULL; + error_report("Error in breakpoint condition"); + } else { + hextomem(bp_cond_expr+i, p, 1); + p += 2; + } + } + } + } + } + if (ch == 'Z') { + res = gdb_breakpoint_insert(addr, len, type, + bp_cond_len, bp_cond_expr); + } else { res = gdb_breakpoint_remove(addr, len, type); + } if (res >= 0) put_packet(s, "OK"); else if (res == -ENOSYS) put_packet(s, ""); else put_packet(s, "E22"); + if (bp_cond_expr != NULL) { + g_free(bp_cond_expr); + } break; case 'H': type = *p++; @@ -2445,6 +2489,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) #endif /* !CONFIG_USER_ONLY */ if (strncmp(p, "Supported", 9) == 0) { snprintf(buf, sizeof(buf), "PacketSize=%x", MAX_PACKET_LENGTH); + + /* conditional breakpoint evaluation on target*/ + pstrcat(buf, sizeof(buf), ";ConditionalBreakpoints+"); #ifdef GDB_CORE_XML pstrcat(buf, sizeof(buf), ";qXfer:features:read+"); #endif diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 249e046..383c4c1 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -448,6 +448,7 @@ void cpu_exit(CPUArchState *s); #define BP_CPU 0x20 int cpu_breakpoint_insert(CPUArchState *env, target_ulong pc, int flags, + long cond_len, uint8_t *cond_exp, CPUBreakpoint **breakpoint); int cpu_breakpoint_remove(CPUArchState *env, target_ulong pc, int flags); void cpu_breakpoint_remove_by_ref(CPUArchState *env, CPUBreakpoint *breakpoint); diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index 3dc9656..0bf63c2 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -136,9 +136,13 @@ typedef struct icount_decr_u16 { typedef struct CPUBreakpoint { target_ulong pc; int flags; /* BP_* */ + int cond_len; + uint8_t *cond_exp; QTAILQ_ENTRY(CPUBreakpoint) entry; } CPUBreakpoint; +int bp_has_cond(CPUBreakpoint *bp); + typedef struct CPUWatchpoint { target_ulong vaddr; target_ulong len_mask; @@ -146,6 +150,18 @@ typedef struct CPUWatchpoint { QTAILQ_ENTRY(CPUWatchpoint) entry; } CPUWatchpoint; + + +typedef double target_double; + +typedef union BPAgentStackElementType { + target_long l; + target_double d; +} BPAgentStackElementType; + + +#define BP_AGENT_MAX_STACK_SIZE 1024 + #define CPU_TEMP_BUF_NLONGS 128 #define CPU_COMMON \ /* soft mmu support */ \ @@ -191,6 +207,10 @@ typedef struct CPUWatchpoint { /* user data */ \ void *opaque; \ \ + BPAgentStackElementType bp_agent_stack[BP_AGENT_MAX_STACK_SIZE]; \ + BPAgentStackElementType *bp_agent_stack_current; \ + /*bp_agent_stack_current is the current location in bp_agent_stack */ \ + int bp_agent_error; /* error in evaluation - ex., divide by zero*/ \ const char *cpu_model_str; #endif diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index e856191..db5a38c 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -59,7 +59,7 @@ typedef struct TranslationBlock TranslationBlock; * and up to 4 + N parameters on 64-bit archs * (N = number of input arguments + output arguments). */ #define MAX_OPC_PARAM (4 + (MAX_OPC_PARAM_PER_ARG * MAX_OPC_PARAM_ARGS)) -#define OPC_BUF_SIZE 640 +#define OPC_BUF_SIZE 2560 #define OPC_MAX_SIZE (OPC_BUF_SIZE - MAX_OP_PER_INSTR) /* Maximum size a TCG op can expand to. This is complicated because a diff --git a/target-i386/helper.c b/target-i386/helper.c index 82a731c..a0d9e93 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -984,7 +984,7 @@ void hw_breakpoint_insert(CPUX86State *env, int index) switch (hw_breakpoint_type(env->dr[7], index)) { case DR7_TYPE_BP_INST: if (hw_breakpoint_enabled(env->dr[7], index)) { - err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU, + err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU, 0, NULL, &env->cpu_breakpoint[index]); } break; diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h index d70b2eb..6c3353c 100644 --- a/tcg/tcg-op.h +++ b/tcg/tcg-op.h @@ -21,6 +21,10 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN * THE SOFTWARE. */ + + +#ifndef __TCG_OP_H__ +#define __TCG_OP_H__ #include "tcg.h" int gen_new_label(void); @@ -2946,6 +2950,8 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index) TCGV_PTR_TO_NAT(B)) #define tcg_gen_addi_ptr(R, A, B) tcg_gen_addi_i32(TCGV_PTR_TO_NAT(R), \ TCGV_PTR_TO_NAT(A), (B)) +#define tcg_gen_subi_ptr(R, A, B) tcg_gen_subi_i32(TCGV_PTR_TO_NAT(R), \ + TCGV_PTR_TO_NAT(A), (B)) #define tcg_gen_ext_i32_ptr(R, A) tcg_gen_mov_i32(TCGV_PTR_TO_NAT(R), (A)) #else /* TCG_TARGET_REG_BITS == 32 */ #define tcg_gen_add_ptr(R, A, B) tcg_gen_add_i64(TCGV_PTR_TO_NAT(R), \ @@ -2953,5 +2959,9 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index) TCGV_PTR_TO_NAT(B)) #define tcg_gen_addi_ptr(R, A, B) tcg_gen_addi_i64(TCGV_PTR_TO_NAT(R), \ TCGV_PTR_TO_NAT(A), (B)) +#define tcg_gen_subi_ptr(R, A, B) tcg_gen_subi_i64(TCGV_PTR_TO_NAT(R), \ + TCGV_PTR_TO_NAT(A), (B)) #define tcg_gen_ext_i32_ptr(R, A) tcg_gen_ext_i32_i64(TCGV_PTR_TO_NAT(R), (A)) #endif /* TCG_TARGET_REG_BITS != 32 */ + +#endif /* __TCG_OP_H__ */ diff --git a/tcg/tcg.h b/tcg/tcg.h index b195396..b367406 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -21,6 +21,10 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN * THE SOFTWARE. */ + +#ifndef __TCG_H__ +#define __TCG_H__ + #include "qemu-common.h" /* Target word size (must be identical to pointer size). */ @@ -690,3 +694,5 @@ void tcg_register_jit(void *buf, size_t buf_size); /* Generate TB finalization at the end of block */ void tcg_out_tb_finalize(TCGContext *s); #endif + +#endif /* __TCG_H__*/
Signed-off-by: Anna Neiman <anna_neiman@mentor.com> --- exec.c | 16 ++++++++++++- gdbstub.c | 57 ++++++++++++++++++++++++++++++++++++++++++----- include/exec/cpu-all.h | 1 + include/exec/cpu-defs.h | 20 +++++++++++++++++ include/exec/exec-all.h | 2 +- target-i386/helper.c | 2 +- tcg/tcg-op.h | 10 +++++++++ tcg/tcg.h | 6 +++++ 8 files changed, 106 insertions(+), 8 deletions(-)