Re: [PATCH for-6.1 v6 12/17] target/avr: Implement gdb_adjust_breakpoint

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 7/20/21 9:54 PM, Richard Henderson wrote:
> Ensure at registration that all breakpoints are in
> code space, not data space.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/cpu.h       |  1 +
>  target/avr/cpu.c       |  1 +
>  target/avr/gdbstub.c   | 13 +++++++++++++
>  target/avr/translate.c | 14 --------------
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
> index d148e8c75a..93e3faa0a9 100644
> --- a/target/avr/cpu.h
> +++ b/target/avr/cpu.h
> @@ -162,6 +162,7 @@ hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int avr_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>  int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  int avr_print_insn(bfd_vma addr, disassemble_info *info);
> +vaddr avr_cpu_gdb_adjust_breakpoint(CPUState *cpu, vaddr addr);
>  
>  static inline int avr_feature(CPUAVRState *env, AVRFeature feature)
>  {
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 57e3fab4a0..ea14175ca5 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -223,6 +223,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
>      cc->disas_set_info = avr_cpu_disas_set_info;
>      cc->gdb_read_register = avr_cpu_gdb_read_register;
>      cc->gdb_write_register = avr_cpu_gdb_write_register;
> +    cc->gdb_adjust_breakpoint = avr_cpu_gdb_adjust_breakpoint;
>      cc->gdb_num_core_regs = 35;
>      cc->gdb_core_xml_file = "avr-cpu.xml";
>      cc->tcg_ops = &avr_tcg_ops;
> diff --git a/target/avr/gdbstub.c b/target/avr/gdbstub.c
> index c28ed67efe..1c1b908c92 100644
> --- a/target/avr/gdbstub.c
> +++ b/target/avr/gdbstub.c
> @@ -82,3 +82,16 @@ int avr_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>  
>      return 0;
>  }
> +
> +vaddr avr_cpu_gdb_adjust_breakpoint(CPUState *cpu, vaddr addr)
> +{
> +    /*
> +     * This is due to some strange GDB behavior
> +     * Let's assume main has address 0x100:
> +     * b main   - sets breakpoint at address 0x00000100 (code)

I'd say hardware breakpoint is used here (using the Breakpoint
Unit via JTAG),

> +     * b *0x100 - sets breakpoint at address 0x00800100 (data)

while software breakpoint is used here (insert a BREAK instruction
at that address).

> +     *
> +     * Force all breakpoints into code space.
> +     */
> +    return addr % OFFSET_DATA;
> +}

From ATmega640 DS:

The Break Point Unit implements Break on Change of Program Flow,
Single Step Break, two Program Memory Break Points, and two combined
Break Points. Together, the four Break Points can be configured as
either:
  • 4 single Program Memory Break Points

  • 3 Single Program Memory Break Points
    + 1 single Data Memory Break Point

  • 2 single Program Memory Break Points
    + 2 single Data Memory Break Points

  • 2 single Program Memory Break Points
    + 1 Program Memory Break Point with mask (“range Break Point”)

  • 2 single Program Memory Break Points
    + 1 Data Memory Break Point with mask (“range Break Point”)

A debugger, like the AVR Studio, may however use one or more of
these resources for its internal purpose, leaving less flexibility
to the end-user.

[...]

All necessary execution commands are available in AVR Studio, both
on source level and on disassembly level.
The user can execute the program, single step through the code either
by tracing into or stepping over functions, step out of functions,
place the cursor on a statement and execute until the statement is
reached, stop the execution, and reset the execution target.
In addition, the user can have an unlimited number of code Break
Points (using the BREAK instruction) and up to two data memory Break
Points, alternatively combined as a mask (range) Break Point.

I wish we didn't have to add gdb_adjust_breakpoint() but we can
remove it later, so for this patch:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>