X64: Disassembler updated to using REX, extended registers and some X64 opcodes.

X64: Disassembler updated to using REX, extended registers and some X64 opcodes.
Not all opcodes fixed yet (some should be invalid in 64-bit mode, others should be added).

[Lasse Reichstein, 2009-07-06 13:06:53]

Review of "working" disassembler.
(Needs more work, but it runs and does reasonable things on most opcodes).

[William Hesse, 2009-07-07 07:42:33]

LGTM, with comments.

http://codereview.chromium.org/155087/diff/1/2
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/155087/diff/1/2#newcode1078
Line 1078: emit(Immediate(static_cast<int32_t>(value)));
Perhaps a comment that no 8-bit version exists is worthwhile.

http://codereview.chromium.org/155087/diff/1/3
File src/x64/disasm-x64.cc (right):

http://codereview.chromium.org/155087/diff/1/3#newcode348
Line 348: case 0:
All cases of a switch statement are indented 2 spaces, and their bodies a total
of 4 spaces.

http://codereview.chromium.org/155087/diff/1/3#newcode400
Line 400: int disp = mod == 2 ? *reinterpret_cast<int32_t*>(modrmp + 1) :
*(modrmp
Add more parentheses, and break at a operator higher in the expression's parse.
int disp = (mod == 2) ?
           *reinterpret .... :
           *(modrmp + 1);

http://codereview.chromium.org/155087/diff/1/3#newcode403
Line 403: return mod == 2 ? 5 : 2;
I think parentheses help to spot the condition.

http://codereview.chromium.org/155087/diff/1/3#newcode1315
Line 1315: const char* NameConverter::NameOfConstant(byte* addr) const {
byte* is Address, isn't it?

http://codereview.chromium.org/155087/diff/1/3#newcode1354
Line 1354: DisassemblerX64 d(converter_, false /* do not crash if unimplemented
*/);
Remove the inline comment?  Or make it crash if unimplemented?

http://codereview.chromium.org/155087/diff/1/3#newcode1358
Line 1358: // The X64 assembler does not currently use constant pools.
Make this a TODO, or remove "currently"

http://codereview.chromium.org/155087/diff/1/3#newcode1363
Line 1363: /*static*/void Disassembler::Disassemble(FILE* f, byte* begin, byte*
end) {
Remove the /*static*/.  If it were really needed, you could write a comment: //
This is a static function.

[Lasse Reichstein, 2009-07-07 12:00:46]

Managed to add some more features to the same branch. Could you do an
incremental check?

http://codereview.chromium.org/155087/diff/1/3
File src/x64/disasm-x64.cc (right):

http://codereview.chromium.org/155087/diff/1/3#newcode348
Line 348: case 0:
Done.

http://codereview.chromium.org/155087/diff/1/3#newcode1315
Line 1315: const char* NameConverter::NameOfConstant(byte* addr) const {
It is. NameConverter is declared elsewhere, and the byte* type is used here to
be consistent with the declaration.

http://codereview.chromium.org/155087/diff/1/3#newcode1354
Line 1354: DisassemblerX64 d(converter_, false /* do not crash if unimplemented
*/);
The comment explains the meaning of the "false" parameter.
I'll convert it to an enum instead.

http://codereview.chromium.org/155087/diff/1/3#newcode1358
Line 1358: // The X64 assembler does not currently use constant pools.
Removed.

http://codereview.chromium.org/155087/diff/1/3#newcode1363
Line 1363: /*static*/void Disassembler::Disassemble(FILE* f, byte* begin, byte*
end) {
Removed.

[William Hesse, 2009-07-07 12:30:58]

LGTM.

http://codereview.chromium.org/155087/diff/8/1005
File src/x64/disasm-x64.cc (right):

http://codereview.chromium.org/155087/diff/8/1005#newcode996
Line 996: //----------------------------
This isn't really standard.  Couldn't you put an explanatory comment here
instead, that would also indicate a new section?