[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 24028/24028] Evaluate breakpoint condition on ta
From: |
Paul Brook |
Subject: |
Re: [Qemu-devel] [PATCH 24028/24028] Evaluate breakpoint condition on target. |
Date: |
Thu, 21 Feb 2013 20:10:17 +0000 |
User-agent: |
KMail/1.13.7 (Linux/3.7-trunk-amd64; KDE/4.8.4; x86_64; ; ) |
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.