You should ensure the disassembler prints something sensible for the switches.
http://codereview.chromium.org/500095/diff/9/15 File src/arm/assembler-thumb2.cc (right): http://codereview.chromium.org/500095/diff/9/15#newcode563 src/arm/assembler-thumb2.cc:563: if (thumb_mode_) I think we should assert here that we are not in a bit of code that should have a predictable size. Also we prefer to use {} on multiline ifs in V8. http://codereview.chromium.org/500095/diff/9/15#newcode622 src/arm/assembler-thumb2.cc:622: } Double blank lines between functions. http://codereview.chromium.org/500095/diff/9/15#newcode624 src/arm/assembler-thumb2.cc:624: void Assembler::forceThumbMode() { Google style is to use CaptializedCamelCase for functions. This file (which predates V8) also has lower_case_with_underscores. This new function uses neither convention. I suggest you choose whichever convention makes it blend in best with the rest of the file. I would prefer 'Ensure' to 'Force' since we are not overriding anything here. http://codereview.chromium.org/500095/diff/9/15#newcode626 src/arm/assembler-thumb2.cc:626: return; Multiline if. http://codereview.chromium.org/500095/diff/9/15#newcode629 src/arm/assembler-thumb2.cc:629: // thumb mode. We encoder this by hand, because the sub() encoder -> encode sub() -> sub http://codereview.chromium.org/500095/diff/9/15#newcode630 src/arm/assembler-thumb2.cc:630: // instruction should thumb its nose? http://codereview.chromium.org/500095/diff/9/15#newcode634 src/arm/assembler-thumb2.cc:634: // The thumb instructions follow at once with no padding Please use full stops at the end of comments. http://codereview.chromium.org/500095/diff/9/15#newcode638 src/arm/assembler-thumb2.cc:638: void Assembler::forceArmMode() { CamelCase. http://codereview.chromium.org/500095/diff/9/15#newcode639 src/arm/assembler-thumb2.cc:639: if (!thumb_mode_) Multiline if. http://codereview.chromium.org/500095/diff/9/15#newcode641 src/arm/assembler-thumb2.cc:641: // PC is 4 bytes ahead of the current instruction, and the instruction 4 or 8? http://codereview.chromium.org/500095/diff/9/15#newcode674 src/arm/assembler-thumb2.cc:674: forceArmMode(); Why do we need this here. It deserves a comment explaining. http://codereview.chromium.org/500095/diff/9/15#newcode904 src/arm/assembler-thumb2.cc:904: // For testing purposes, a gratuitous diversion into thumb This isn't for testing, this is for real! (But you might want to point out that the addrmod1 call switches it back at the moment). http://codereview.chromium.org/500095/diff/9/13 File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/500095/diff/9/13#newcode2078 src/arm/simulator-arm.cc:2078: // Temporary special-casing of our particular ARM/THUMB switch I think this can be moved to where it fits (in the sub instruction somewhere). http://codereview.chromium.org/500095/diff/9/13#newcode2129 src/arm/simulator-arm.cc:2129: // For now, assume all thumb instructions are an aligned switch back to ARM Please assert this is true. http://codereview.chromium.org/500095 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
