Re: [Qemu-devel] [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

[Neiman, Anna]

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:address@hidden 
Sent: Tuesday, February 26, 2013 5:53 PM
To: Neiman, Anna
Cc: address@hidden; 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 <address@hidden>
> ---
>  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