LGTM
http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h File src/x64/assembler-x64.h (right): http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode1378 src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) { I don't see us using get_opcode anywhere. Just remove it. http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode1378 src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) { The call site can make its own local functions by any name, which call the generic method on the assembler. The generic method is there now, and public, for anyone to use at any time. For that reason alone, I think it should be less bound to the current use-case. E.g., I think it should take a zero-based index, not a pc-based index. It's easy to convert between the two (using pc_offset()). Or, preferably, replace it with a macro like: PatchableNearJump(byte opcode, Label* label) { db(opcode); emit_near_disp(label); } but there is a visibility issue to work around with that, since MacroAssembler cant see emit_near_disp. Perhaps make a db_near_disp(Label*) method for that. http://codereview.chromium.org/7122003/diff/3010/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): http://codereview.chromium.org/7122003/diff/3010/src/x64/code-stubs-x64.cc#newcode5193 src/x64/code-stubs-x64.cc:5193: // branch when we start and stop incremental heap marking. LGTM http://codereview.chromium.org/7122003/diff/3010/src/x64/code-stubs-x64.cc#newcode5197 src/x64/code-stubs-x64.cc:5197: masm->set_opcode(-2, kTwoByteNopInstruction); Still think "set_opcode" is too specific for a byte-patcher. http://codereview.chromium.org/7122003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
