Re: [Qemu-devel] [PATCH 24028/24028] Evaluate breakpoint condition on target.

[Paul Brook]

In addition to the comments others made about patch formatting, etc:

> +            /* conditional breakpoint evaluation on target*/
> +            pstrcat(buf, sizeof(buf), ";ConditionalBreakpoints+");

I'm pretty sure this is a lie for most targets, given later on we have:

> +#if defined(TARGET_ARM)
> +    cpu_get_reg_var_func = cpu_get_reg_var_arm;
> +#else
> +    cpu_get_reg_var_func = 0;
> +#endif

> +                        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;
> +                                perror("Error in breakpoint condition");

perror is the wrong way to report a malformed gdb command.

> +#if TARGET_LONG_SIZE == 4
> +typedef float target_double;
> +#else /* TARGET_LONG_SIZE == 8 */
> +typedef double target_double;
> +#endif

This clearly has nothing to do with the target double precision floating point 
type.

> +    int qemu_rw_debug_flag;

This appears to be a write-only variable.

> +#define BP_AGENT_MAX_COND_SIZE 1024

By my reading this isn't the maximim size, it's the maximum stack depth.

> +void cpu_get_reg_var_arm(TCGv var, int reg)
> +{
> +    tcg_gen_mov_i32(var, cpu_R[reg]);
> +}

Looks like it will break horribly when the user requests anything other than 
r0-r15.  And r15 is probably also wrong.

> +            bswap16(val);

Clearly wrong.

> +            fprintf(stderr,
> +                    "GDB agent: const 64 is not supported for 32 bit

This is not a good way to report user errors.  Several other occurances.

> +static target_long bp_agent_get_arg(const uint8_t *cond_exp,
>...
> +    case 4:
> +    default:

I'd be amazed if this default case is correct.

> +    /*for case error , ex.buffer overloading -
> +      need to set labels anyway in order to avoid segmentation fault  */

Sounds like you're failing to check for errors somewhere else.