Copies Simulator Debugger from ARM to MIPS.

Copies Simulator Debugger from ARM to MIPS.

Also includes changes to keep it from aborting unexpectedly.
Among other things, I changed the Disassembler::Disassemble to return false
when the underlying decoder can't decode an instruction. Then,
the MIPS SimulatorDebugger will refuse to step or cont on an instruction
that the Disassembler can't decode.
Committed: https://code.google.com/p/dart/source/detail?r=19958

[regis, 2013-03-12 20:53:19]

I did not review the debugger code in detail, since it is copied from the ARM
simulator.

I feel like maintaining the 'decoded' local variable in the disassembler is
brittle. Also, it is not necessary on ia32 or x64. How about adding an instance
variable to ARMDecoder and MIPSDecoder instead? This would not require any
changes to the Intel disassembler, and the ARM and MIPS disassemblers would
still be able to print data present in the decoded stream without choking. The
simulator would consult the decoder instance variable to see if the last call
decoded a valid instruction. It would be set to true on entry in decoder's
InstructionDecode() and only set to false in decoder's Unknown().

What do you think?

https://codereview.chromium.org/12431016/diff/1/runtime/vm/constants_mips.h
File runtime/vm/constants_mips.h (right):

https://codereview.chromium.org/12431016/diff/1/runtime/vm/constants_mips.h#n...
runtime/vm/constants_mips.h:365: return (InstructionBits() ==
kBreakPointInstruction);
You should mask the code and not require the code to be zero.
But we have to be careful with a simulator redirection, which may use a specific
code. Unless you plan on using another instruction? SYSCALL maybe?

https://codereview.chromium.org/12431016/diff/1/runtime/vm/disassembler_arm.cc
File runtime/vm/disassembler_arm.cc (right):

https://codereview.chromium.org/12431016/diff/1/runtime/vm/disassembler_arm.c...
runtime/vm/disassembler_arm.cc:1325: return -Instr::kInstrSize;
Did you mean to return 0? I am worried about returning a negative value or even
0, as will get stuck in a never ending decoding loop.

https://codereview.chromium.org/12431016/diff/1/runtime/vm/disassembler_arm.c...
runtime/vm/disassembler_arm.cc:1364: pc += (-instruction_length);
I see. You encode success/failure in the returned size. It would be cleaner to
change DecodeInstruction to also return a bool and add a decoded_size reference
parameter.

[zra, 2013-03-12 21:54:30]

On 2013/03/12 20:53:19, regis wrote:
> I did not review the debugger code in detail, since it is copied from the ARM
> simulator.
> 
> I feel like maintaining the 'decoded' local variable in the disassembler is
> brittle. Also, it is not necessary on ia32 or x64. How about adding an
instance
> variable to ARMDecoder and MIPSDecoder instead? This would not require any
> changes to the Intel disassembler, and the ARM and MIPS disassemblers would
> still be able to print data present in the decoded stream without choking. The
> simulator would consult the decoder instance variable to see if the last call
> decoded a valid instruction. It would be set to true on entry in decoder's
> InstructionDecode() and only set to false in decoder's Unknown().
> 
> What do you think?

The problem is that the ARMDecoder and MIPSDecoder instances are local to
Disassembler::DecodeInstruction. What I could do, though, is have
Disassembler::Disassemble take the Decoder as an argument. We've got one version
for each of the architectures already, anyway. Then the Decoder would have to be
passed in by the Simulator. How does that sound?

[zra, 2013-03-12 22:09:03]

On 2013/03/12 21:54:30, zra wrote:
> On 2013/03/12 20:53:19, regis wrote:
> > I did not review the debugger code in detail, since it is copied from the
ARM
> > simulator.
> > 
> > I feel like maintaining the 'decoded' local variable in the disassembler is
> > brittle. Also, it is not necessary on ia32 or x64. How about adding an
> instance
> > variable to ARMDecoder and MIPSDecoder instead? This would not require any
> > changes to the Intel disassembler, and the ARM and MIPS disassemblers would
> > still be able to print data present in the decoded stream without choking.
The
> > simulator would consult the decoder instance variable to see if the last
call
> > decoded a valid instruction. It would be set to true on entry in decoder's
> > InstructionDecode() and only set to false in decoder's Unknown().
> > 
> > What do you think?
> 
> The problem is that the ARMDecoder and MIPSDecoder instances are local to
> Disassembler::DecodeInstruction. What I could do, though, is have
> Disassembler::Disassemble take the Decoder as an argument. We've got one
version
> for each of the architectures already, anyway. Then the Decoder would have to
be
> passed in by the Simulator. How does that sound?

Ahh. Actually, I can't do that easily either since the Disassembler is not
currently architecture specific.

[regis, 2013-03-12 22:23:40]

On 2013/03/12 22:09:03, zra wrote:
> On 2013/03/12 21:54:30, zra wrote:
> > On 2013/03/12 20:53:19, regis wrote:
> > > I did not review the debugger code in detail, since it is copied from the
> ARM
> > > simulator.
> > > 
> > > I feel like maintaining the 'decoded' local variable in the disassembler
is
> > > brittle. Also, it is not necessary on ia32 or x64. How about adding an
> > instance
> > > variable to ARMDecoder and MIPSDecoder instead? This would not require any
> > > changes to the Intel disassembler, and the ARM and MIPS disassemblers
would
> > > still be able to print data present in the decoded stream without choking.
> The
> > > simulator would consult the decoder instance variable to see if the last
> call
> > > decoded a valid instruction. It would be set to true on entry in decoder's
> > > InstructionDecode() and only set to false in decoder's Unknown().
> > > 
> > > What do you think?
> > 
> > The problem is that the ARMDecoder and MIPSDecoder instances are local to
> > Disassembler::DecodeInstruction. What I could do, though, is have
> > Disassembler::Disassemble take the Decoder as an argument. We've got one
> version
> > for each of the architectures already, anyway. Then the Decoder would have
to
> be
> > passed in by the Simulator. How does that sound?
> 
> Ahh. Actually, I can't do that easily either since the Disassembler is not
> currently architecture specific.

Can you add the instance variable to both decoders as suggested and change
Disassembler::Disassemble to return it as a bool?

[zra, 2013-03-12 22:32:09]

On 2013/03/12 22:23:40, regis wrote:
> On 2013/03/12 22:09:03, zra wrote:
> > On 2013/03/12 21:54:30, zra wrote:
> > > On 2013/03/12 20:53:19, regis wrote:
> > > > I did not review the debugger code in detail, since it is copied from
the
> > ARM
> > > > simulator.
> > > > 
> > > > I feel like maintaining the 'decoded' local variable in the disassembler
> is
> > > > brittle. Also, it is not necessary on ia32 or x64. How about adding an
> > > instance
> > > > variable to ARMDecoder and MIPSDecoder instead? This would not require
any
> > > > changes to the Intel disassembler, and the ARM and MIPS disassemblers
> would
> > > > still be able to print data present in the decoded stream without
choking.
> > The
> > > > simulator would consult the decoder instance variable to see if the last
> > call
> > > > decoded a valid instruction. It would be set to true on entry in
decoder's
> > > > InstructionDecode() and only set to false in decoder's Unknown().
> > > > 
> > > > What do you think?
> > > 
> > > The problem is that the ARMDecoder and MIPSDecoder instances are local to
> > > Disassembler::DecodeInstruction. What I could do, though, is have
> > > Disassembler::Disassemble take the Decoder as an argument. We've got one
> > version
> > > for each of the architectures already, anyway. Then the Decoder would have
> to
> > be
> > > passed in by the Simulator. How does that sound?
> > 
> > Ahh. Actually, I can't do that easily either since the Disassembler is not
> > currently architecture specific.
> 
> Can you add the instance variable to both decoders as suggested and change
> Disassembler::Disassemble to return it as a bool?

I think so. I'll give that a shot.

[zra, 2013-03-13 18:09:14]

Moved "decoded" variable into ARMDecoder and MIPSDecoder, which is set inside
their Unknown methods. Then Disassembler::Disassemble returns false if an
instruction couldn't be decoded.

https://codereview.chromium.org/12431016/diff/1/runtime/vm/constants_mips.h
File runtime/vm/constants_mips.h (right):

https://codereview.chromium.org/12431016/diff/1/runtime/vm/constants_mips.h#n...
runtime/vm/constants_mips.h:365: return (InstructionBits() ==
kBreakPointInstruction);
On 2013/03/12 20:53:19, regis wrote:
> You should mask the code and not require the code to be zero.
> But we have to be careful with a simulator redirection, which may use a
specific
> code. Unless you plan on using another instruction? SYSCALL maybe?

Done.

[regis, 2013-03-13 18:29:59]

LGTM

https://codereview.chromium.org/12431016/diff/18003/runtime/vm/disassembler_m...
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/12431016/diff/18003/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:30: bool GetDecodeFailure() const { return
decode_failure_; }
We have the convention to use lower case getters and setters when reading and
writing instance variables.
In this case, you would write:
bool decode_failure() const { return decode_failure_; }
void set_decode_failure(bool value) ...

[zra, 2013-03-13 18:43:03]

https://codereview.chromium.org/12431016/diff/18003/runtime/vm/disassembler_m...
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/12431016/diff/18003/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:30: bool GetDecodeFailure() const { return
decode_failure_; }
On 2013/03/13 18:29:59, regis wrote:
> We have the convention to use lower case getters and setters when reading and
> writing instance variables.
> In this case, you would write:
> bool decode_failure() const { return decode_failure_; }
> void set_decode_failure(bool value) ...
>  

Done.

[zra, 2013-03-13 18:43:23]

Committed patchset #4 manually as r19958 (presubmit successful).

[Ivan Posva, 2013-03-13 21:50:34]

DBC -ip

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/constants_mips.h
File runtime/vm/constants_mips.h (right):

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/constants_mips...
runtime/vm/constants_mips.h:76: S7 = R23,
Thanks!

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_arm.cc
File runtime/vm/simulator_arm.cc (right):

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_arm....
runtime/vm/simulator_arm.cc:356: if (decoded) {
I am still not quite sure what we are benefitting from this extra information.

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_mips.cc
File runtime/vm/simulator_mips.cc (right):

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_mips...
runtime/vm/simulator_mips.cc:802: UNIMPLEMENTED();
What are you planning to implement here?

[zra, 2013-03-14 15:48:14]

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_arm.cc
File runtime/vm/simulator_arm.cc (right):

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_arm....
runtime/vm/simulator_arm.cc:356: if (decoded) {
On 2013/03/13 21:50:34, Ivan Posva wrote:
> I am still not quite sure what we are benefitting from this extra information.

If you step onto an unknown instruction, or set a break point on an unknown
instruction, you now won't lose your debug session if you then try to do a stepi
or cont.

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_mips.cc
File runtime/vm/simulator_mips.cc (right):

https://codereview.chromium.org/12431016/diff/16003/runtime/vm/simulator_mips...
runtime/vm/simulator_mips.cc:802: UNIMPLEMENTED();
On 2013/03/13 21:50:34, Ivan Posva wrote:
> What are you planning to implement here?

There are several Special instructions remaining to be implemented.