520712 - nanojit: print assembly code for X64 backend with TMFLAGS=assembly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

With TMFLAGS=assembly the X64 backend only prints the instruction bytes, not the assembly code.  This probably keeps the debug output code simple, but is incredibly unhelpful for anyone trying to decipher/debug/improve the X64 backend.

The backend should be changed to print the assembly code, just like the other backends.  (After the NJ merge is complete, though, as usual.)

Comment 1

[Edwin Smith]

I'm to blame for that.  All development was done within gdb whose disassm command worked fine, but yeah, no good for analysis.  At the time there was consensus to actually embed a small simple disassembler (maybe udis86?), but other priorities intervened unfortunately.

I actually had a simple disassembler written in about a 1/2 day, with only another 1/2 day or day to finish it.  either way, the one nice thing about using a real disassembler is that we'll see what the emitter really emitted, and not what we think it emitted.  

just posting thoughts here in case we want to do this.  explicitly printing the assembly code is okay too, its what the other backends do, although it might be slightly more complicated due to the x64 backend's macro-free style of manipulating opcodes as values.

Comment 2

[Nicholas Nethercote [inactive]]

Re the disassembler, would it allow the register and stack state to be interleaved with the assembly code?  This is crucial IMO.

I'm happy to give the explicit printing approach a go, see if it's not too difficult.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch changes the X64 back-end to print generated code as asm instead
of instruction bytes.  It has the disadvantage of printing what we think we
generate rather than what we actually generated, as comment 1 mentions, but
this (a) matches all the other back-ends, and (b) you can disassemble in GDB
with some effort if you really need to.  And it interleaves the generated
code with the original LIR and the regalloc info which I think is crucial.

The patch introduces a layer of macros (ADDRR, etc) which makes it more like
the other backends.  Each macro matches the name of its X64_opcode (eg.
ADDRR --> X64_addrr).  This makes the code less neat than it was,
particularly in the JMPcc and CMOVcc cases which now require switches
instead of opcode tables, but I chose to provide important functionality
over optional elegance.  Macros might not be the best way to do it, but it
matches other backends and it kept things relatively compact, albeit with
some long lines.  I'm open to other suggestions.

The patch also cleans up the layering a bit, eg. we have the "emit" layer
which gets passed X64_opcodes and generates the code bytes, and the "asm"
layer which makes higher level decisions, but they were mixed up a bit.
(And the new macro layer now sits between them.)  Example cleanups:
- underrunProtect is now only called from "emit" functions
- emit_quad() was renamed as asm_quad();  asm_quad() is now overloaded
- emit_int() was replaced with MOVI
- some new "emit" functions were created/modified (emit_target8,
  emit_target32, emitr_imm64, emitxm_abs, emitxm_rel

And it renames some of the cmov X64_opcodes so they're all X64_cmovn*.  And
a couple of other X64_opcodes could be removed.

It passes trace-tests and the unit tests.

Comment 4

[Andreas Gal :gal]

I liked that the x64 backend used mostly functions instead of macros. That seems like the way to go. Could we convert the macros to functions? I am ok with all caps function names and very long lines if you want to keep it compact.

Comment 5

[Nicholas Nethercote [inactive]]

Ok, but I won't get to it until Monday Oct 26th.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch v2, with macros replaced by functions

Comment 7

[Andreas Gal :gal]

Comment on attachment 411358 [details] [diff] [review]
patch v2, with macros replaced by functions

Whitespace formating seems to be all over the place. Otherwise looks really great.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> (From update of attachment 411358 [details] [diff] [review])
> Whitespace formating seems to be all over the place. 

I could clean up the declarations in NativeX64.h up.  Anywhere else you are referring to?

Comment 9

[Andreas Gal :gal]

Yeah its the declarations mostly. I don't think we do those kinds of alignment tricks usually. Otoh its nanojit and not SM/TM, so whatever you get past Ed is fine by me :)

Comment 10

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/bc99b0483e32

Comment 11

[Edwin Smith]

Attachment: Must do underrunProtect before testing offset size (fixes tamarin-redux test failure on x64 in debug mode)

Comment 12

[Nicholas Nethercote [inactive]]

Sorry for the breakage, I promise I did test on TR.

I don't like the fix much.  One of the things the patch did was create stronger layering of the code, in particular, underrunProtect() was only being called from within emit() functions.  I think it would be better to encapsulate the short vs long jump test within JNO et al.  I'll file a follow-up bug.

I'm also now worried about other uses of _nIns functions, whether they have the same problem.  Dangerous-looking ones are JMP(), asm_call(), asm_quad(), asm_fneg().  (Some of those were dangerous-looking even before this patch was applied.)

And for the record, Ed's commit was:
http://hg.mozilla.org/projects/nanojit-central/rev/9b249328236f

Comment 13

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/b50c31eb3edd
http://hg.mozilla.org/tracemonkey/rev/7604d91b400c

It's also been landed in tamarin-redux.

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b50c31eb3edd