Generalize ARM disassembler and implement for ARM64

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: lth, Assigned: lth)

Tracking

unspecified
mozilla60
Other
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The ARM disassembler is fairly general and only requires that the back-end supply a simple "disassemble one instruction" method; it then takes care of label management, and has utilities for handling pc-relative literal loads and similar gunk.

It turns out to be reasonably painless to factor out the ARM disassembler and then implement it for ARM64, see the commit message for technical information.
Attachment #8942234 - Flags: review?(nicolas.b.pierron)
This update fixes some bugs that were caused by the disassembler always being enabled for simulator builds, even Release builds.  As a consequence a number of the ifdefs were not quite right, and some (no-op) methods had to be provided for Release builds.

To be more resilient, this version disables the disassemblers in Simulator release builds; this requires a slight adjustment to the ARM simulator, which had a dependency on the disassembler for some instruction decoding (probably code I wrote).
Attachment #8942234 - Attachment is obsolete: true
Attachment #8942234 - Flags: review?(nicolas.b.pierron)
Attachment #8942959 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8942959 [details] [diff] [review]
bug1430161-arm-disasm-generalization-v2.patch

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

::: js/src/jit/arm64/vixl/MozBaseAssembler-vixl.h
@@ +127,5 @@
> +#ifdef JS_DISASM_ARM64
> +  virtual void spew(const vixl::Instruction* instr) = 0;
> +  virtual void spewBranch(const vixl::Instruction* instr, const LabelDoc& l) = 0;
> +  virtual void spewLiteralLoad(const vixl::Instruction* instr, const LiteralDoc& l) = 0;
> +  virtual LabelDoc refLabel(js::jit::Label* label) = 0;

Q: Could we remove the virtual / override, by moving the spew_ field to the MozBaseAssembler?
Why is the MozBaseAssembler under the vixl directory anyway?

::: js/src/jit/shared/Disassembler-shared.h
@@ +103,5 @@
> +            float    f32;
> +            double   f64;
> +        } value;
> +        LiteralDoc() : type(Type::Patchable) {}
> +        LiteralDoc(int32_t v) : type(Type::I32) { value.i32 = v; }

nit: explicit keyword.
Attachment #8942959 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 8942959 [details] [diff] [review]
> bug1430161-arm-disasm-generalization-v2.patch
> 
> Review of attachment 8942959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm64/vixl/MozBaseAssembler-vixl.h
> @@ +127,5 @@
> > +#ifdef JS_DISASM_ARM64
> > +  virtual void spew(const vixl::Instruction* instr) = 0;
> > +  virtual void spewBranch(const vixl::Instruction* instr, const LabelDoc& l) = 0;
> > +  virtual void spewLiteralLoad(const vixl::Instruction* instr, const LiteralDoc& l) = 0;
> > +  virtual LabelDoc refLabel(js::jit::Label* label) = 0;
> 
> Q: Could we remove the virtual / override, by moving the spew_ field to the
> MozBaseAssembler?

That worked very well, thank you.

> Why is the MozBaseAssembler under the vixl directory anyway?

Oh, I have a lot of questions about why things are the way they are in the ARM64 backend, but I fear we'll have to ask Sean or Marty about that.
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/045ded11d3f8
Factor ARM disassembler, implement for ARM64.  r=nbp
https://hg.mozilla.org/mozilla-central/rev/045ded11d3f8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1455702
Depends on: 1491337
You need to log in before you can comment on or make changes to this bug.