Re: [Simulavr-devel] [PATCH] Add BREAK-instruction, causes simulavr to halt.

[Petr Hluzín]

On 4 January 2012 16:09, Stan Behrens
<address@hidden> wrote:
> TODO: Check if execution is continuable on a real MCU.
> ---
>  src/avrdevice.cpp     |   11 ++++++++++-
>  src/avrdevice.h       |    1 +
>  src/decoder.cpp       |   10 ++++++++++
>  src/decoder.h         |   20 ++++++++++++++++++++
>  src/decoder_trace.cpp |    6 ++++++
>  5 files changed, 47 insertions(+), 1 deletions(-)
>
> diff --git a/src/avrdevice.cpp b/src/avrdevice.cpp
> index 0d2b38e..4a4e2af 100644
> --- a/src/avrdevice.cpp
> +++ b/src/avrdevice.cpp
> @@ -534,7 +534,16 @@ int AvrDevice::Step(bool &untilCoreStepFinished, 
> SystemClockOffset *nextStepIn_n
>
>     untilCoreStepFinished = !((cpuCycles > 0) || hwWait);
>     dump_manager->cycle();
> -    return (cpuCycles < 0) ? cpuCycles : 0;
> +
> +       if(instructionBREAKJustExecuted) {
> +               instructionBREAKJustExecuted = false;
> +
> +               if(trace_on)
> +                       traceOut << "Breakpoint-Instruction executed at 0x" 
> << hex << PC << dec << endl;
> +
> +               return BREAK_POINT;
> +       } else
> +               return (cpuCycles < 0) ? cpuCycles : 0;
>  }
>
>  void AvrDevice::Reset() {
> diff --git a/src/avrdevice.h b/src/avrdevice.h
> index 9141e17..61975da 100644
> --- a/src/avrdevice.h
> +++ b/src/avrdevice.h
> @@ -89,6 +89,7 @@ class AvrDevice: public SimulationMember, public 
> TraceValueRegister {
>         bool abortOnInvalidAccess; //!< Flag, that simulation abort if an 
> invalid access occured, default is false
>         TraceValueCoreRegister coreTraceGroup;
>         bool instructionSEIJustEnabledInterrupts;  ///< Almost always false.
> +        bool instructionBREAKJustExecuted;  ///< Almost always false.
>
>         bool flagIWInstructions; //!< ADIW and SBIW instructions are 
> available (not on most tiny's!)
>         bool flagJMPInstructions; //!< CALL and JMP instructions are 
> available (only on devices with bigger flash)
> diff --git a/src/decoder.cpp b/src/decoder.cpp
> index f7cf3cb..16af091 100644
> --- a/src/decoder.cpp
> +++ b/src/decoder.cpp
> @@ -1767,6 +1767,15 @@ int avr_op_WDR::operator()() {
>     return 1;
>  }
>
> +avr_op_BREAK::avr_op_BREAK(word opcode, AvrDevice *c):
> +    DecodedInstruction(c) {}
> +
> +int avr_op_BREAK::operator()() {
> +       core->instructionBREAKJustExecuted = true;
> +
> +    return 1;
> +}
> +
>  avr_op_ILLEGAL::avr_op_ILLEGAL(word opcode, AvrDevice *c):
>     DecodedInstruction(c) {}
>
> @@ -1996,6 +2005,7 @@ DecodedInstruction* lookup_opcode( word opcode, 
> AvrDevice *core )
>             else
>                 return new avr_op_ILLEGAL(opcode, core);
>         case 0x95A8: return new  avr_op_WDR(opcode, core);                 /* 
> 1001 0101 1010 1000 | WDR */
> +        case 0x9598: return new  avr_op_BREAK(opcode, core);               
> /* 1001 0101 1001 1000 | BREAK */
>         default:
>                      {
>                          /* opcodes with two 5-bit register (Rd and Rr) 
> operands */
> diff --git a/src/decoder.h b/src/decoder.h
> index 4e5d13f..b810fef 100644
> --- a/src/decoder.h
> +++ b/src/decoder.h
> @@ -2055,6 +2055,26 @@ class avr_op_WDR: public DecodedInstruction
>         int Trace();
>  };
>
> +class avr_op_BREAK: public DecodedInstruction
> +{
> +    /*
> +     * Halts execution
> +     *
> +     * TODO Check if execution is continuable on a real MCU
> +     *
> +     * Opcode     : 1001 0101 1001 1000
> +     * Usage      : BREAK
> +     * Operation  : (see specific hardware specification for BREAK)
> +     * Flags      : None
> +     * Num Clocks : N/A
> +     */
> +
> +    public:
> +        avr_op_BREAK(word opcode, AvrDevice *c);
> +        int operator()();
> +        int Trace();
> +};
> +
>
>
>  class avr_op_ILLEGAL: public DecodedInstruction
> diff --git a/src/decoder_trace.cpp b/src/decoder_trace.cpp
> index 4e2e5eb..3bfc30f 100644
> --- a/src/decoder_trace.cpp
> +++ b/src/decoder_trace.cpp
> @@ -750,6 +750,12 @@ int avr_op_WDR::Trace() {
>     return ret;
>  }
>
> +int avr_op_BREAK::Trace() {
> +    traceOut << "BREAK ";
> +    int ret = this->operator()();
> +    return ret;
> +}
> +
>  int avr_op_ILLEGAL::Trace() {
>     traceOut << "Invalid Instruction! ";
>     int ret = this->operator()();
> --
> 1.7.5.4

Why does your patch add instructionBREAKJustExecuted to AvrDevice and
then have the if() condition in AvrDevice::Step()?
It would be easier to do the tracing in avr_op_BREAK::operator()() and
return BREAK_POINT. Function AvrDevice::Step() and class AvrDevice
would be left as is.

That way we would avoid the conditional jump in function
AvrDevice::Step() - the function is performance sensitive.

I have the alternate implementation ready to commit.

-- 
Petr Hluzin