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_nei...@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
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
