581553 - Display bytes values associated with an instruction

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Rick Reitmaier]

Attachment: v1

Posting to see if anyone shows interest in this patch.  It adds byte values to the instruciton output; e.g.

000072bf86   83c410         add esp,16
    immi13 = immi 0
    eqi1 = eqi doubleToBool1, immi13/*0*/
    jf eqi1 -> B62
000072bf89   85c0           test eax,eax
000072bf8b   7512           jne   0x72bf9f
    immi15 = immi 5
000072bf8d   b805000000     mov eax,5
    ldi6 = ldi.o methodFrame[0]
000072bf92   8b4dbc         mov ecx,-68(ebp)
    sti.o immi1/*6381608*/[256] = ldi6
000072bf95   890d28616100   mov 6381864(0),ecx
    reti immi15/*5*/
000072bf9b   8be5           mov esp,ebp
000072bf9d   5d             pop ebp
000072bf9e   c3             ret

Comment 1

[Rick Reitmaier]

Attachment: v2

Rebased.

I'm finding this useful enough to land;  Also fixed an error in PEDANTIC ifdef.

Comment 2

[Rick Reitmaier]

Attachment: v3

removed xcode settings that leaked into v2 of patch.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 481708 [details] [diff] [review]
v3

I like this idea a lot.  I had to use GDB to get these values just
yesterday, printing them is great.


Here's how objdump does it:

 804b81a:       55                      push   %ebp
 804b81b:       89 e5                   mov    %esp,%ebp
 804b81d:       53                      push   %ebx
 804b81e:       83 ec 14                sub    $0x14,%esp
 804b821:       e8 b6 ca 00 00          call   80582dc <__i686.get_pc_thunk.bx>
 804b826:       81 c3 ce d7 33 00       add    $0x33d7ce,%ebx

I like having spaces between each byte, it distinguishes it nicely from the
addresses.  Can you do that?


You forgot to update the SH4 back-end, but you might not need to...


>         #ifdef NJ_VERBOSE
>+            NIns*       _nInsNext;              // next instruction (ascending) in current normal/exit code chunk (for verbose output)

It's annoying how many places you need to update _nInsNext, esp. that it's
per back-end.  It looks like you have to update _nInsNext every time
codeAlloc() is called -- could you just update it within codeAlloc()?


>     #ifdef NJ_NO_VARIADIC_MACROS
>         static void asm_output(const char *f, ...) {}
>         #define gpn(r)                    regNames[(r)]
>     #elif defined(NJ_VERBOSE)
>+        inline unsigned char cvaltoa(unsigned char c) {
>+            return c<10 ? c+'0' : c+'a'-10;
>+        }
>+
>+        inline char* outputHexVals(char* str, char* strEnd, char* valFrom, char* valTo) {
>+            AvmAssert(valFrom<=valTo);
>+            for(unsigned char* ch=(unsigned char*)valFrom; ch<(unsigned char*)valTo; ch++) {
>+                *str++ = cvaltoa(*ch>>4);
>+                *str++ = cvaltoa(*ch&0xf);
>+            }
>+            NanoAssertMsg(str<=strEnd, "Increase NJ_MAX_BYTES_OF_INSTRUCTION by strEnd-str");
>+            while(str<strEnd)
>+                *str++ = ' ';
>+            *strEnd = '\0';
>+            return str;
>+        }

Ow, my eyes!  Can we please have some whitespace after 'for' and 'while', and
around all binary operators? :)


>         // Used for printing native instructions.  Like Assembler::outputf(),
>         // but only outputs if LC_Native is set.  Also prepends the output
>         // with the address of the current native instruction.
>         #define asm_output(...) do { \
>             if (_logc->lcbits & LC_Native) { \
>                 outline[0]='\0'; \
>-                VMPI_sprintf(outline, "%p   ", _nIns);  \
>+                VMPI_sprintf(outline, "%p  ", _nIns);   \
>+                uint32_t sz = VMPI_strlen(outline);     \
>+                outputHexVals(&outline[sz], &outline[sz+2*NJ_MAX_BYTES_OF_INSTRUCTION], (char*)_nIns, (char*)_nInsNext); \
>+                _nInsNext = _nIns;                      \
>                 VMPI_sprintf(outline+VMPI_strlen(outline), ##__VA_ARGS__);   \
>                 output();                               \
>             } \
>         } while (0) /* no semi */

I would get rid of the per-platform NJ_MAX_BYTES_OF_INSTRUCTION value.
x86/x86_64 has a max length of 15, AIUI;  I'd just define that here.  Then
you wouldn't need to pass in 'strEnd'.  (But I'm sure we never generate
anything like that long, so you could reduce it to 10 safely, esp. with that
assertion in outputHexVals().)

I think you need to add a small constant to the
[sz+2*NJ_MAX_BYTES_OF_INSTRUCTION] value so that in the worst case there is
still some space after the byte values.  (And it'll need to be 3 instead of
2 if you put spaces between the byte values.)

I'm giving an r- but this is definitely on the right track -- I just want to
see an updated patch and try it out for myself before approving :)

Comment 4

[Edwin Smith]

(In reply to comment #3)
> I like having spaces between each byte, it distinguishes it nicely from the
> addresses.  Can you do that?

+1, I was going to suggest that too.

> It's annoying how many places you need to update _nInsNext, esp. that it's
> per back-end.  It looks like you have to update _nInsNext every time
> codeAlloc() is called -- could you just update it within codeAlloc()?

Back in the dark ages, The X64 backend didn't print asm, and only printed the instruction bytes.  Here's a link to the old code; that old approach wouldn't work on other backends, but since I bothered to go look up the code, I'll drop the link here anyway.  (no action needed).

http://hg.mozilla.org/tamarin-redux/file/1416/nanojit/NativeX64.cpp#l165

Comment 5

[Rick Reitmaier]

Attachment: update to include comments from prior review

- byte output is optional.  If we always enabled it, then it's difficult to compare runs (not impossible but just a pain, since we'd need to strip the hex values).  That along with the difficulty of reading the output when other verbose switches are enabled, such as LC_RegAlloc prompted this change.
- No longer compact bytes together in output (spaces added)
- whitespace in for/while loops
- NJ_MAX_... removed, fixed at 15.
- codeAlloc now houses _nInsNext change which reduces # of back-end changes.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 497932 [details] [diff] [review]
update to include comments from prior review

Lovely!  The output looks really good.  Thanks for doing this.

One nit:  you use AvmAssert() in nanojit.h which gave me a compile error.  Please change to NanoAssert().

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: TM patch (against TM 58597:dab87d48d3dd)

Simple TM patch.

Comment 8

[Nicholas Nethercote [inactive]]

Actually, you should be able to factor out the |_nInsAfter = _nIns| assignments, so that the back-ends don't need to be changed at all... that'd be nice.

Comment 9

[Rick Reitmaier]

(In reply to comment #8)
> Actually, you should be able to factor out the |_nInsAfter = _nIns|
> assignments, so that the back-ends don't need to be changed at all... that'd be
> nice.

I can't see a simple way to do this with all the SWAP() macros in there.  Is there something I'm missing?

re: avmassert in comment 6; fixed.

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> 
> I can't see a simple way to do this with all the SWAP() macros in there.  Is
> there something I'm missing?

The assignment always happens near the end of swapCodeChunks(), so you could do it in Assembler.cpp just after each call to swapCodeChunks()?

Why does the assignment happen in two places in the MIPS back-end but only once in all the other back-ends?

Comment 11

[Rick Reitmaier]

Attachment: consolidate back end changes

Remove most back end changes (re: nick's suggestion) and fixed nanoassert.  

MIPS required the extra _nInsAfter = _nIns since codeAlloc is called twice, with the 2nd call setting up the _nExitIns.  But avoided the whole issue by moving the assignment into Assembler.beginAssembly.

Comment 12

[Rick Reitmaier]

rreitmai http://hg.mozilla.org/projects/nanojit-central/rev/8e80b4cfa30d

Comment 13

[Tamarin Bot]

changeset: 5729:89faf93d0c24
user:      Rick Reitmaier <[e-mail]>
summary:   Bug 581553 - Display bytes values associated with an instruction (r+nnethercote)

http://hg.mozilla.org/tamarin-redux/rev/89faf93d0c24

Comment 14

[Tamarin Bot]

changeset: 5738:cf2980327db3
user:      Rick Reitmaier <[e-mail]>
summary:   Bug 581553 - Verbose switch to display byte values associated with an instruction (r+nnethercote)

The 'regs' and 'opt' switch semantics have changed slightly, combining either
of them with 'jit' will result in previous behaviour.

http://hg.mozilla.org/tamarin-redux/rev/cf2980327db3

Comment 15

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/d7e0e1cf9334

Comment 16

[Nicholas Nethercote [inactive]]

TM-specific part:
http://hg.mozilla.org/tracemonkey/rev/5b6c54fcec35

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/5b6c54fcec35
http://hg.mozilla.org/mozilla-central/rev/d7e0e1cf9334