Re: [Qemu-devel] [PATCH RFC v20 3/8] target/avr: Add mechanism to check for active debugger connection

[Michael Rolnik]

Hi Richard.

I am still struggling with this one.

The spec says.
The BREAK instruction is used by the On-chip Debug system, and is normally
not used in the application software.
When the BREAK instruction is executed, the AVR CPU is set in the Stopped
Mode.
This gives the On-chip Debugger access to internal resources.
If any Lock bits are set, or either the JTAGEN or OCDEN Fuses are
unprogrammed, the CPU will treat the BREAK instruction as a NOP and will
not enter the Stopped mode.

I read is as follows
- If user has an intention of using debugger, BREAK should be translated to
QEMU debug breakpoint
- If user has no intention of using debugger, BREAK should be translated
into NOP.

however it seems that rising EXCP_DEBUG crashes QEMU when no debugger is
present or *-s* was not supplied.

This lead me to the following.
- checking for active GDB connection is not good, because it might change
but translated BREAK will not
- if *-s* is supplied BREAK should always raise EXCP_DEBUG exception
- if *-s* is not supplied BREAK should be translated into NOP

What do you think? How to check existence of *-s* option?

Regards,
Michael



On Mon, Jun 3, 2019 at 8:04 PM Michael Rolnik <address@hidden> wrote:

> Got it.
>
> Sent from my cell phone, please ignore typos
>
> On Mon, Jun 3, 2019, 7:37 PM Richard Henderson <
> address@hidden> wrote:
>
>> On 6/3/19 11:29 AM, Michael Rolnik wrote:
>> > 1. There's a break
>> > instruction
>> https://www.microchip.com/webdoc/avrassembler/avrassembler.wb_BREAK.html
>> > 2. There's a set of tests that use break.
>> >
>> > So I assume I have to implement this instruction as described in the
>> spec.
>>
>> The spec talks about fuses, not gdb.  A valid implementation of this
>> instruction is a no-op -- it say so right there in the spec.
>>
>> What does it mean to "test" break?  AFAIK, you can't test this at all from
>> within the cpu itself, since it does not generate a cpu-level exception.
>>
>> If gdb is setting a breakpoint via -S, it should be done via
>> cpu_breakpoint_test.
>>
>>
>>
>> > On Mon, Jun 3, 2019, 6:44 PM Richard Henderson <
>> address@hidden
>> > <mailto:address@hidden>> wrote:
>> >
>> >     On 6/1/19 4:12 PM, Michael Rolnik wrote:
>> >     > Hi Richard.
>> >     >
>> >     > If I implement it this way
>> >     >
>> >     > ```
>> >     >  static bool trans_BREAK(DisasContext *ctx, arg_BREAK *a)
>> >     >  {
>> >     >      if (avr_feature(ctx->env, AVR_FEATURE_BREAK) == false) {
>> >     >          gen_helper_unsupported(cpu_env);
>> >     >      } else {
>> >     >          tcg_gen_movi_tl(cpu_pc, ctx->inst[0].npc);
>> >     >          gen_helper_debug(cpu_env);
>> >     >      }
>> >     >
>> >     >      ctx->bstate = BS_EXCP;
>> >     >
>> >     >      return true;
>> >     >  }
>> >     > ```
>> >     >
>> >     > qemu (without -s -S flags) crashes when debugger is not connected
>> >
>> >     I was not suggesting using the internal qemu EXCP_DEBUG, but
>> another AVR
>> >     specific exception, much the same way as every other cpu has a
>> cpu-specific
>> >     debug exception.
>> >
>> >     Or perhaps always do nothing.  Why is gdb insertting BREAK in the
>> first place?
>> >      It should be using the "hardware breakpoint" support that qemu
>> advertises as
>> >     part of the gdbstub protocol, and that you support here:
>> >
>> >     > +        if (unlikely(cpu_breakpoint_test(cs, OFFSET_CODE + cpc *
>> 2, BP_ANY))
>> >     > +                 || cpu_breakpoint_test(cs, OFFSET_DATA + cpc *
>> 2,
>> >     BP_ANY)) {
>> >     > +            tcg_gen_movi_i32(cpu_pc, cpc);
>> >     > +            gen_helper_debug(cpu_env);
>> >     > +            ctx.bstate = BS_EXCP;
>> >     > +            goto done_generating;
>> >     > +        }
>>
>>

-- 
Best Regards,
Michael Rolnik