1157934 - Import V8's ARM Disassembler

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 0001-Import-v8-s-ARM-disassembler.patch

We don't have a disassembler on ARM. This makes simulator crashes very difficult to debug. The current code uses the following ultra-exciting helper function to attempt disassembly:

> // Observe that llvm-mc may have a different name on your system.  Make a symlink.
> static void
> DisassembleInstruction(uint32_t pc)
> {
>     uint8_t* bytes = reinterpret_cast<uint8_t*>(pc);
>     char hexbytes[256];
>     sprintf(hexbytes, "0x%x 0x%x 0x%x 0x%x", bytes[0], bytes[1], bytes[2], bytes[3]);
>     char llvmcmd[1024];
>     sprintf(llvmcmd, "bash -c \"echo -n '%p'; echo '%s' | "
>             "llvm-mc -disassemble -arch=arm -mcpu=cortex-a9 | "
>             "grep -v pure_instructions | grep -v .text\"",
>             reinterpret_cast<void*>(pc), hexbytes);
>     system(llvmcmd);
> }

Beyond being hacky, this is pretty damn slow, especially since to give useful introspection it should disassemble every instruction executed, as on ARM64.

Luckily, v8 already solved this issue by writing a disassembler (Probably ARM actually wrote it, then contributed it to the v8 project. It looks very similar to VIXL). This patch imports the disassembler. Because of namespace conflicts in jit/arm/Assembler-arm.cpp, all of the newly-imported stuff is put into a "disasm" namespace.

Jan: Is this something you want to import? Do you have any preference for how to toggle the spew on/off? On ARM64 we used an envvar USE_DEBUGGER. Other options include a shell arg and a new flavor of JitSpew.

Comment 1

[Nicolas B. Pierron [:nbp]]

Wouldn't the debugger (gdb/lldb) be able to do the same thing in a faster way?
In which case this would be something worth to document in the HackingTips page.

Comment 2

[Lars T Hansen [:lth]]

nbp, can you clarify?  The likely scenario is that i've hit a breakpoint in arm code in the simulator (often I set that breakpoint when I generate code) and I'd like to disassemble the ARM code around that breakpoint.

The GDB that comes with my Linux install also doesn't come preconfigured with ARM support, I don't know how much extra work it is to set that up and maintain it, or install an additional package.  Guidance welcome.

Comment 3

[Nicolas B. Pierron [:nbp]]

Ok, I was expecting gdb to be capable to do so knowing that is use objdump to disassemble the code, but apparently the commands are only used to change the flavor of the assembly, and not the architecture.

If you have the binutils packages compiled to enable all targets, or if you have an ARM toolchain, then you can use objdump to decode a buffer of assembly instructions.

 $ objdump -D -b binary -marm /tmp/memory.dump

I guess we can even redefine disassemble in command in gdb.

http://stackoverflow.com/questions/3859453/help-using-objdump-disassembling-to-arm

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8596862 [details] [diff] [review]
0001-Import-v8-s-ARM-disassembler.patch

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

Looks very self-contained, seems OK. Maybe put it in jit/arm/disasm/ to separate it more? Or jit/arm/simulator/?

Maybe we should build the disassembler only as part of simulator builds, to avoid code bloat and keep other ARM code from depending on these constants etc.

Comment 5

[Lars T Hansen [:lth]]

IWBNI it would not be too tricky to enable it on ARM hardware and make it work with -D, which I find I rely on quite a bit for looking at generated code.  (I'm fine with it not being on by default.)

Comment 6

[Lars T Hansen [:lth]]

Attachment: V8's ARM disassembler, v2

(Poaching because I need this and I sense Sean is busy elsewhere.)

Updated patch for current m-i plus include ordering etc.  Files moved to jit/arm/disasm, and disassembler is now only enabled if the ARM simulator is enabled or if it's a DEBUG build.

I have another patch coming that hooks this into the IONFLAGS=codegen machinery.  (This is why I want the disassembler enabled in DEBUG builds.)

Jan/Sean, the copyright notices in the disasm code mention a file LICENSE that supposedly contains the v8 license language.  The file is absent in this patch.  Should I track it down and insert it, or do we have some other type of machinery for these kinds of situations, and if so, what is that?

Comment 7

[Sean Stangl [:sstangl]]

(In reply to Lars T Hansen [:lth] from comment #6)
> Jan/Sean, the copyright notices in the disasm code mention a file LICENSE
> that supposedly contains the v8 license language.  The file is absent in
> this patch.  Should I track it down and insert it, or do we have some other
> type of machinery for these kinds of situations, and if so, what is that?

The v8 LICENSE file has already been inserted as part of browser/base/content/overrides/app-license.html.

The license we have there looks about 2 years out of date, and should probably be updated to read "2014" instead of "2006-2012".

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 8645610 [details] [diff] [review]
V8's ARM disassembler, v2

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

::: js/src/jit/arm/Simulator-arm.cpp
@@ +632,5 @@
> +#ifdef JS_DISASM_ARM
> +            disasm::NameConverter converter;
> +            disasm::Disassembler dasm(converter);
> +            // Use a reasonably large buffer.
> +            disasm::EmbeddedVector<char, 256> buffer;

Would strongly prefer a name instead of "256", to be shared with the similar code below.

@@ +799,3 @@
>                  }
> +#else
> +                printf("  No disassembler.");

This could be a little more helpful.

::: js/src/jit/arm/Simulator-arm.h
@@ +415,5 @@
>      ICacheMap icache_;
>  
> +#ifdef JS_DISASM_ARM
> +    // String output buffer for the Disassembler.
> +    disasm::EmbeddedVector<char, 256> decodeBuffer_;

256 again! Can share a name with the others.

::: js/src/jit/arm/disasm/Constants-arm.h
@@ +2,5 @@
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +#ifndef V8_ARM_CONSTANTS_ARM_H_
> +#define V8_ARM_CONSTANTS_ARM_H_

This (and at the bottom of the file) should probably be changed to fit our standard. Also it would be nice to put the vim modeline at the top of the file.

@@ +5,5 @@
> +#ifndef V8_ARM_CONSTANTS_ARM_H_
> +#define V8_ARM_CONSTANTS_ARM_H_
> +
> +#if defined(JS_CODEGEN_ARM) && defined(DEBUG) && !defined(JS_DISASM_ARM)
> +  #define JS_DISASM_ARM

This logic belongs in configure.in.

::: js/src/jit/arm/disasm/Disasm-arm.cpp
@@ +1,1 @@
> +// Copyright 2011 the V8 project authors. All rights reserved.

Maybe a modeline?

::: js/src/jit/arm/disasm/Disasm-arm.h
@@ +2,5 @@
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +#ifndef V8_DISASM_H_
> +#define V8_DISASM_H_

Ideally update.

@@ +5,5 @@
> +#ifndef V8_DISASM_H_
> +#define V8_DISASM_H_
> +
> +#if defined(JS_CODEGEN_ARM) && defined(DEBUG) && !defined(JS_DISASM_ARM)
> +  #define JS_DISASM_ARM

This logic *really* does not belong both here *and* and in Constants-arm.h.

Most likely, it belongs in configure.in, and these files really want to |#include "js-config.h"|.

Comment 9

[Lars T Hansen [:lth]]

(In reply to Sean Stangl [:sstangl] from comment #8)
> Comment on attachment 8645610 [details] [diff] [review]
> V8's ARM disassembler, v2
> 
> Review of attachment 8645610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

(All good suggestions, will address.)

What about indentation and other style matters?  I meant to ask earlier but forgot.  My normal inclination is to keep that the way it is, to simplify comparisons with future versions of this code if that becomes desirable.

Comment 10

[Lars T Hansen [:lth]]

(In reply to Sean Stangl [:sstangl] from comment #7)
> (In reply to Lars T Hansen [:lth] from comment #6)
> > Jan/Sean, the copyright notices in the disasm code mention a file LICENSE
> > that supposedly contains the v8 license language.  The file is absent in
> > this patch.  Should I track it down and insert it, or do we have some other
> > type of machinery for these kinds of situations, and if so, what is that?
> 
> The v8 LICENSE file has already been inserted as part of
> browser/base/content/overrides/app-license.html.
> 
> The license we have there looks about 2 years out of date, and should
> probably be updated to read "2014" instead of "2006-2012".

Strictly for the record, that should probably be toolkit/content/license.html (about:license in Firefox, I did not know this).  The copyright notices on the disassembler files have years in the range 2007-2011 so I'll probably leave well enough alone on the v8 license.

Comment 11

[Lars T Hansen [:lth]]

Attachment: V8's ARM disassembler, v3

Addresses all of Sean's concerns and more: adds js::jit namespaces, and reindents the code.  The reindentation is "roughly right" - some inline methods in classes are properly indented but one could argue that there should be more line breaks.  I hope this is OK for now.

Also, arguably there should be some configure option to enable the disassembler when it is not currently enabled (release builds on device), but I'd like to make a separate bug for that.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8646319 [details] [diff] [review]
V8's ARM disassembler, v3

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

Sorry for the delay! Looks good.

Comment 13

[Lars T Hansen [:lth]]

Try run with rebased patch, just in case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bee43dd045ae

Comment 14

[Pulsebot]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cdf1e714b330