Forking disassembler and simulator for Thumb2 support;...

Forking disassembler and simulator for Thumb2 support;
Limited experimental thumb2 support for AND and ADD

[Erik Corry, 2010-02-11 14:36:46]

I'm not done yet.  Here's some feedback to be getting on with.

http://codereview.chromium.org/601028/diff/1/9
File src/arm/assembler-thumb2-inl.h (right):

http://codereview.chromium.org/601028/diff/1/9#newcode151
src/arm/assembler-thumb2-inl.h:151: && ((Assembler::instrArm_at(pc_ +
Assembler::kInstrArmSize) & kLdrPCPattern)
This line is too long.

You need to install cpplint.py and run tools/presubmit.py

http://codereview.chromium.org/601028/diff/1/9#newcode223
src/arm/assembler-thumb2-inl.h:223: void Assembler::CheckArmMode() {
These should be called EnsureXxx instead of CheckXxx to be consistent with the
naming elsewhere in the VM.

http://codereview.chromium.org/601028/diff/1/9#newcode226
src/arm/assembler-thumb2-inl.h:226: if (pc_offset() & 2) {
Please do not use implicit conversions to bool (ie add != 0)

http://codereview.chromium.org/601028/diff/1/9#newcode229
src/arm/assembler-thumb2-inl.h:229: //      emitThumb(16*B12| 8 * B5 |
pc.code());
We don't allow commented out code in V8.

http://codereview.chromium.org/601028/diff/1/9#newcode231
src/arm/assembler-thumb2-inl.h:231: pc_ += 2;  // Padding
Can we emit a T2 nop here instead of leaving uninitialized memory?

http://codereview.chromium.org/601028/diff/1/9#newcode274
src/arm/assembler-thumb2-inl.h:274: Address target_pc = pc;
Here you are bringing the t2 file up to date wrt the ARM file.  I would have
preferred to see this in a different change.

http://codereview.chromium.org/601028/diff/1/10
File src/arm/assembler-thumb2.cc (right):

http://codereview.chromium.org/601028/diff/1/10#newcode34
src/arm/assembler-thumb2.cc:34: // modified significantly by Google Inc.
Accidental edit.  Removing the double 'above' would be better.

http://codereview.chromium.org/601028/diff/1/10#newcode597
src/arm/assembler-thumb2.cc:597: //    dataProcessingImm(op, s, rn, rd,
x.imm32_);
Commented out code.

http://codereview.chromium.org/601028/diff/1/7
File src/arm/assembler-thumb2.h (right):

http://codereview.chromium.org/601028/diff/1/7#newcode34
src/arm/assembler-thumb2.h:34: // modified significantly by Google Inc.
Please revert this accidental change (above).

http://codereview.chromium.org/601028/diff/1/7#newcode50
src/arm/assembler-thumb2.h:50: using namespace assembler::arm;
You can't use this here according to the style guide
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

http://codereview.chromium.org/601028/diff/1/7#newcode54
src/arm/assembler-thumb2.h:54: H   = 1 << 5,   // halfword (or byte)
Please use correct punctuation in comments (initial capital and terminal full
stop).

http://codereview.chromium.org/601028/diff/1/7#newcode55
src/arm/assembler-thumb2.h:55: S6  = 1 << 6,   // signed (or unsigned)
There seems to be a space too many in these lines (In the line above it's after
S6).

http://codereview.chromium.org/601028/diff/1/7#newcode67
src/arm/assembler-thumb2.h:67: RdMask     = 15 << 12,  // in str instruction
Extra spaces (I might find it OK if it allowed the lines to line up, but it
doesn't).

http://codereview.chromium.org/601028/diff/1/7#newcode943
src/arm/assembler-thumb2.h:943: static InstrArm instrArm_at(byte* pc) { return
*reinterpret_cast<InstrArm*>(pc); }
We should be consistent with the existing style here.  Since the existing style
in this originally non-Google file uses lower_case_with_underscores then that is
what the new code should use.  If we departed from that then the style guide
should be followed, which insists on UpperCamelCase for method names.

http://codereview.chromium.org/601028/diff/1/7#newcode1043
src/arm/assembler-thumb2.h:1043: bool thumbMode_;
lower_case_with_underscores_

http://codereview.chromium.org/601028/diff/1/7#newcode1051
src/arm/assembler-thumb2.h:1051: inline void emitThumb(InstrThumb x);
lower_case_with_underscores to be consistent with the preexisting code in this
class.  See also other examples below.

http://codereview.chromium.org/601028/diff/1/7#newcode1084
src/arm/assembler-thumb2.h:1084: 
Please remove accidental edit.

[iposva, 2010-02-16 06:51:35]

I guess I do not understand the motivation to add a complete duplicate copy of
the simulator to the system. Can you please elaborate your plans in that
respect?

Thanks,
-Ivan

[haustein, 2010-02-23 14:01:32]

Changed as discussed. Created a new CL due to some SVN issues: 
http://codereview.chromium.org/651029/show