1192786 - Make ARM disassembler do something useful for IONFLAGS=codegen

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

The disassembler in bug 1157934 can be used with IONFLAGS=codegen to examine generated code.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1192786.patch

Completely rudimentary, but works pretty well all the same.  Branches and labels need more work, to be sure.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8645677 [details] [diff] [review]
bug1192786.patch

Review of attachment 8645677 [details] [diff] [review]:
-----------------------------------------------------------------

That would be great :)

::: js/src/jit/arm/Assembler-arm.cpp
@@ +12,5 @@
>  #include "jscompartment.h"
>  #include "jsutil.h"
>  
>  #include "gc/Marking.h"
> +#include "jit/arm/disasm/Disasm-arm.h"

You forgot to include this header in your patch.

Comment 3

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> >  #include "gc/Marking.h"
> > +#include "jit/arm/disasm/Disasm-arm.h"
> 
> You forgot to include this header in your patch.

It should be present in the patch on bug 1157934.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1192786-ionflags-codegen-arm.patch

This is much improved (though not ready for landing).

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1192786-ionflags-codegen-arm.patch

This seems useful enough to review.  A couple of notes / questions:

- There's a little more complexity here than I wanted, but it has appeared
  in order to make the dump more useful (notably, to name jump targets
  properly).  Removing it means back-edges are going to be jumps to
  unknown locations, probably.

- Printing the memory addresses for the disassembly as i've done here 
  makes the output noisy and has marginal value, given that (in my testing)
  labels are generally emitted properly.  Opinions?

- I'm pretty sure some parts of Ion do not properly tag misc emitted stubs,
  so in some programs there will be a run of instructions at the end of the
  dump with duplicated label names; these are really separate assemblies.
  I figure fixing that can be done in a followup, but I've fixed a few here.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1192786-ionflags-codegen-arm.patch, v2

This fixes a bug - a missing guard - and moves guards on JitSpew_Codegen into the API functions generally, thus reducing clutter elsewhere.

Comment 7

[Lars T Hansen [:lth]]

Memo to self: I would like to support disassembly with -D and it's possible that should be rolled into this patch, since it means infiltrating a printer into all the spew machinery I set up here.  Investigating.

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1192786-printer.patch

This adds support for -D by hooking in a printer object.  The output is ugly (with undefined and unreferenced labels) but in truth the output on x86 is no better, so this may be close to right.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8648843 [details] [diff] [review]
bug1192786-ionflags-codegen-arm.patch, v2

Review of attachment 8648843 [details] [diff] [review]:
-----------------------------------------------------------------

This sounds good.
Thanks a lot for doing this works :)

::: js/src/jit/arm/Assembler-arm.cpp
@@ +1414,5 @@
> +// Labels are named as they are encountered by adding names to a
> +// table, using the Label address as the key.  This is made tricky by
> +// the (memory for) Label objects being reused, but reused label
> +// objects are recognizable from being marked as not used or not
> +// bound.  See spewResolve().

Clever, this makes me think … that I will have to open a bug after this review.

@@ +1481,5 @@
> +                ;
> +            buffer[i] = 0;
> +            if (target) {
> +                JS_snprintf(labelBuf, sizeof(labelBuf), "  -> %d%s", spewResolve(target),
> +                            !target->bound() ? "f" : "");

Could labelBuf default to something like "  -> (link-time target)", when the target is null ?

@@ +1619,5 @@
>  {
> +    BufferOffset offs = m_buffer.putInt(x, /* markAsBranch = */ true);
> +#ifdef JS_DISASM_ARM
> +    if (x != (Always | InstNOP::NopInst))
> +        spewBranch(m_buffer.getInst(offs), documentation);

At first sight, the only case where this condition matches, is when we are running Assembler::as_b(Label* l, Condition c) and Assembler::as_bl(Label* l, Condition c).  Except that these functions are calling the spewer directly, because (BOffImm off, Condition c, BufferOffset inst) variant has too many uses.

Do we still need this condition?

@@ +2086,5 @@
> +    return offs;
> +}
> +
> +// This is also used for instructions that might be resolved into branches,
> +// or might not.  If dest==pc then it is effectively a branch.

Move this comment above the previous function, that "loadToPC" is a boolean flag used to indicate if the value is used as a branch target.

Also, I wonder how something which load data into the PC cannot be considered as a branch on ARM ?!  I seems that markAsBranch is redundant with loadToPC.

@@ +2094,5 @@
>  {
>      PoolHintPun php;
>      php.phd.init(0, c, PoolHintData::PoolDTR, dest);
> +    BufferOffset offs = allocEntry(1, 1, (uint8_t*)&php.raw, (uint8_t*)&value, nullptr, false,
> +                                   dest == pc);

Can we assert that "dest != pc" ?

::: js/src/jit/arm/Assembler-arm.h
@@ +1291,5 @@
> +            uint32_t value;
> +            Node* next;
> +        };
> +
> +        Node* nodes;

It seems sad, that we have to re-implement a common container.
Would it make sense to use an InlineForwardList<Node> here?

@@ +1316,5 @@
>      // For the nopFill use a branch to the next instruction: 0xeaffffff.
>      Assembler()
>        : m_buffer(1, 1, 8, GetPoolMaxOffset(), 8, 0xe320f000, 0xeaffffff, GetNopFill()),
> +#ifdef JS_DISASM_ARM
> +        spewNext_(1000),

any reason to start at 1000?

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8649220 [details] [diff] [review]
bug1192786-printer.patch

Review of attachment 8649220 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/Assembler-arm.cpp
@@ +1546,5 @@
> +
> +    if (i > -1) {
> +        if (printer_)
> +            printer_->printf("%s\n", buf);
> +        js::jit::JitSpew(js::jit::JitSpew_Codegen, "%s", buf);

These functions are unlikely to be used at the same time, and if so the performance is not going to be a problem.
I suggest you do:

  if (printer_)
      printer_->vprintf(fmt, va);
  js::jit::JitSpewVA(js::jit::JitSpew_Codegen, fmt, va);

@@ +1556,5 @@
>  {
> +    // Note, spewResolve will sometimes return 0 when it is triggered
> +    // by the profiler and not by a full disassembly, since in that
> +    // case a label can be used or bound but not previously have been
> +    // defined.

Sounds that this modification should be part of the previous patch?

Comment 11

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 8648843 [details] [diff] [review]
> bug1192786-ionflags-codegen-arm.patch, v2
> 
> Review of attachment 8648843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clever, this makes me think … that I will have to open a bug after this
> review.
> 
> @@ +1481,5 @@
> > +                ;
> > +            buffer[i] = 0;
> > +            if (target) {
> > +                JS_snprintf(labelBuf, sizeof(labelBuf), "  -> %d%s", spewResolve(target),
> > +                            !target->bound() ? "f" : "");
> 
> Could labelBuf default to something like "  -> (link-time target)", when the
> target is null ?

In principle that would be misleading since the offset encoded in the instruction might be valid and the disassembly would actually give us the correct target address.  But whether that happens in practice I'm not sure, I'm inclined to think probably not but it's hard to judge from the control flow.  So a default like you suggest is probably reasonable.

> @@ +1619,5 @@
> >  {
> > +    BufferOffset offs = m_buffer.putInt(x, /* markAsBranch = */ true);
> > +#ifdef JS_DISASM_ARM
> > +    if (x != (Always | InstNOP::NopInst))
> > +        spewBranch(m_buffer.getInst(offs), documentation);
> 
> At first sight, the only case where this condition matches, is when we are
> running Assembler::as_b(Label* l, Condition c) and Assembler::as_bl(Label*
> l, Condition c).  Except that these functions are calling the spewer
> directly, because (BOffImm off, Condition c, BufferOffset inst) variant has
> too many uses.
> 
> Do we still need this condition?

Sadly yes.  The purpose is to avoid a spurious nop instruction being decoded when writeBranchInst is being called.  What happens is that as_b and as_bl use writeBranchInst to encode a nop instruction which is then overwritten immediately, and we want to decode the second representation not the first.

One fix is to avoid the hack here by rewriting the code that calls writeBranchInst.  A better fix is probably to add a second writeBranchInst that is about allocating storage for the branch, since that's what's going on, and then this non-local confusion disappears.  I think I'll do that.

> @@ +2086,5 @@
> > +    return offs;
> > +}
> > +
> > +// This is also used for instructions that might be resolved into branches,
> > +// or might not.  If dest==pc then it is effectively a branch.
> 
> Move this comment above the previous function, that "loadToPC" is a boolean
> flag used to indicate if the value is used as a branch target.
> 
> Also, I wonder how something which load data into the PC cannot be
> considered as a branch on ARM ?!  I seems that markAsBranch is redundant
> with loadToPC.

I'll investigate again.  loadToPC is a vestige of older, more elaborate code, maybe I don't need it anymore.

> @@ +2094,5 @@
> >  {
> >      PoolHintPun php;
> >      php.phd.init(0, c, PoolHintData::PoolDTR, dest);
> > +    BufferOffset offs = allocEntry(1, 1, (uint8_t*)&php.raw, (uint8_t*)&value, nullptr, false,
> > +                                   dest == pc);
> 
> Can we assert that "dest != pc" ?

No.  In fact, frequently that would fail.  This function is used to encode bailout branches.  Look at MacroAssemblerARM::ma_b(); since b_type() there now returns B_LDR we end up in the case that uses as_Imm32Pool to encode the branch.

> ::: js/src/jit/arm/Assembler-arm.h
> @@ +1291,5 @@
> > +            uint32_t value;
> > +            Node* next;
> > +        };
> > +
> > +        Node* nodes;
> 
> It seems sad, that we have to re-implement a common container.
> Would it make sense to use an InlineForwardList<Node> here?

I thought about that, but InlineForwardList seems to provide effectively no services that are of use to me, so I didn't do that.  The top contender was really mozilla::list but that seemed unreasonably heavyweight for what little I needed.  I'm happy to change this if you think it really makes a difference though (eg for sociological reasons, InlineForwardList sending a signal to the reader, say).  Let me know.

> @@ +1316,5 @@
> >      // For the nopFill use a branch to the next instruction: 0xeaffffff.
> >      Assembler()
> >        : m_buffer(1, 1, 8, GetPoolMaxOffset(), 8, 0xe320f000, 0xeaffffff, GetNopFill()),
> > +#ifdef JS_DISASM_ARM
> > +        spewNext_(1000),
> 
> any reason to start at 1000?

Small numbers tend to get lost among all the other small numbers in typical disassembly; once you're up to 4 digits the numbers tend to stand out and make reading easier (in my experience).

Comment 12

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Comment on attachment 8649220 [details] [diff] [review]
> bug1192786-printer.patch
> 
> Review of attachment 8649220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm/Assembler-arm.cpp
> @@ +1546,5 @@
> > +
> > +    if (i > -1) {
> > +        if (printer_)
> > +            printer_->printf("%s\n", buf);
> > +        js::jit::JitSpew(js::jit::JitSpew_Codegen, "%s", buf);
> 
> These functions are unlikely to be used at the same time, and if so the
> performance is not going to be a problem.
> I suggest you do:
> 
>   if (printer_)
>       printer_->vprintf(fmt, va);
>   js::jit::JitSpewVA(js::jit::JitSpew_Codegen, fmt, va);

Duh, of course.

> @@ +1556,5 @@
> >  {
> > +    // Note, spewResolve will sometimes return 0 when it is triggered
> > +    // by the profiler and not by a full disassembly, since in that
> > +    // case a label can be used or bound but not previously have been
> > +    // defined.
> 
> Sounds that this modification should be part of the previous patch?

Indeed.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #11)
> (In reply to Nicolas B. Pierron [:nbp] from comment #9)
> > ::: js/src/jit/arm/Assembler-arm.h
> > @@ +1291,5 @@
> > > +            uint32_t value;
> > > +            Node* next;
> > > +        };
> > > +
> > > +        Node* nodes;
> > 
> > It seems sad, that we have to re-implement a common container.
> > Would it make sense to use an InlineForwardList<Node> here?
> 
> I thought about that, but InlineForwardList seems to provide effectively no
> services that are of use to me, so I didn't do that.  The top contender was
> really mozilla::list but that seemed unreasonably heavyweight for what
> little I needed.  I'm happy to change this if you think it really makes a
> difference though (eg for sociological reasons, InlineForwardList sending a
> signal to the reader, say).  Let me know.

I admit that InlineForwardList is as verbose, or even more verbose than redoing the implementation.  The only argument which makes me think that we should use these, is that we should avoid reimplementation of more complex containers.

So I have no strong opinion.

I wonder what we could do to simplify this InlineForwardList implementation to make it appealing.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #11)
> (In reply to Nicolas B. Pierron [:nbp] from comment #9)
> > Comment on attachment 8648843 [details] [diff] [review]
> 
> > @@ +2086,5 @@
> > > +    return offs;
> > > +}
> > > +
> > > +// This is also used for instructions that might be resolved into branches,
> > > +// or might not.  If dest==pc then it is effectively a branch.
> > 
> > Move this comment above the previous function, that "loadToPC" is a boolean
> > flag used to indicate if the value is used as a branch target.
> > 
> > Also, I wonder how something which load data into the PC cannot be
> > considered as a branch on ARM ?!  I seems that markAsBranch is redundant
> > with loadToPC.
> 
> I'll investigate again.  loadToPC is a vestige of older, more elaborate
> code, maybe I don't need it anymore.

It turns out markAsBranch and loadToPC serve different purposes still.  Only actual branch instructions that need to be patched should be markedAsBranch.  Loads to the PC are branches (this again shows up in the bailout machinery, cf earlier comment) but should not be patched and so should not be marked.

I'll add a comment.

Comment 15

[Lars T Hansen [:lth]]

Attachment: bug1192786-printer.patch, v2

I moved a little of the obvious refactoring into the other patch, so this patch is now very simple and I feel foolish for even asking for review :)

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c9da5fe477
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e8e1595ee5

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a75ca46040fc

Comment 18

[Lars T Hansen [:lth]]

Fallout from this landing - OOM test failures due to an API issue - is covered by bug 1198145, as it seemed to be a possible bug all on its own.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7c9da5fe477
https://hg.mozilla.org/mozilla-central/rev/92e8e1595ee5
https://hg.mozilla.org/mozilla-central/rev/a75ca46040fc