1287485 - Display comments inside dumped assembly code (-D flag)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sander Mathijs van Veen]

Attachment: add-comments-to-dumped-assembly.patch

I've made a small, WIP patch that displays comments inside dumped assembly code (-D flag). The idea is that the comments are visible for humans and not for machines.

Consider the example of the getting a property that is polymorphic: 

[GetPropertyPolymorphicV:a0]
# GuardReceiver
movabsq    $0x7f3a3a8f0d90, %r11
cmpq       %r11, 0x0(%rax)
jne        .Lfrom1117
# loadUnboxedProperty
movq       0x10(%rax), %rcx
testq      %rcx, %rcx
jne        .Lfrom1130
movabsq    $0xfffc000000000000, %rcx
jmp        .Lfrom1145
.set .Llabel1145, .
.set .Lfrom1130, .Llabel1145
movabsq    $0xfffe000000000000, %r11
orq        %r11, %rcx
.set .Llabel1158, .
.set .Lfrom1145, .Llabel1158
jmp        .Lfrom1163
.set .Llabel1163, .
.set .Lfrom1117, .Llabel1163
# GuardReceiver
movabsq    $0x7f3a3fac17e0, %r11
cmpq       %r11, 0x8(%rax)
jne        .Lfrom1183
# loadTypedOrValue
movq       0x20(%rax), %rcx
jmp        .Lfrom1192
.set .Llabel1192, .
.set .Lfrom1183, .Llabel1192
# GuardReceiver
movabsq    $0x7f3a3fac1d58, %r11
cmpq       %r11, 0x8(%rax)
jne        .Lfrom1212
# loadTypedOrValue
movq       0x20(%rax), %rcx
.set .Llabel1216, .
.set .Lfrom1192, .Llabel1216
.set .Lfrom1163, .Llabel1216

Without the patch, it is harder to see which code is generated using where and what the code does.
Vim adds nice syntax highlighting when setting |ft=asm| on a dumped assembly log file.

What do you think of this idea?

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8772010 [details] [diff] [review]
add-comments-to-dumped-assembly.patch

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

Move the comment function into the MacroAssembler.h file.
Comments in assembly languages are semi-colons (;), and not hash signs (#).

Move the comment symbol into the spew of the comment function.

Comment 2

[Sander Mathijs van Veen]

Attachment: add-comments-to-dumped-assembly.patch

Changed patch using your suggestions. I'm not sure if this should be implemented cross architecture or only on x86-shared. What do you think?

Comment 3

[Lars T Hansen [:lth]]

I, for one, would appreciate propagating this to other architectures, it seems ARM is always playing catch-up with x86 in regard to debugging functionality like this.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8772359 [details] [diff] [review]
add-comments-to-dumped-assembly.patch

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

This patch looks good, apart from the comments below.

::: js/src/jit/MacroAssembler.h
@@ +1647,5 @@
>      Label* failureLabel() {
>          return &failureLabel_;
>      }
>  
> +    void comment(const char* msg) {

nit: New MacroAssembler functions should be added in the  check_macroassembler_style  sections.  I will suggest the first one, namely “MacroAssembler high-level usage”.

Also, we no longer allow definitions inside the MacroAssembler.h file, so you should follow the rules defined on [1], to decide where to put the implementation of it.

I agree, with Lars comment than this would be nice to have on ARM, but I would still accept patches as long as they are compiling & running fine on all platforms.

[1] http://searchfox.org/mozilla-central/source/js/src/jit/MacroAssembler.h#40-65

@@ +1648,5 @@
>          return &failureLabel_;
>      }
>  
> +    void comment(const char* msg) {
> +        masm.comment(msg);

This function seems to be only implemented in the x86-shared backend.  This will fail to compile on ARM.
Compiling with the ARM simulator should raise an error.

Comment 5

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=708649531f75

Comment 6

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc0955062e0a

Comment 7

[Sander Mathijs van Veen]

Attachment: add-comments-to-dumped-assembly.patch

Change the patch according to nbp's suggestions.

This patch passes try builds for at least arm, arm64, x86, and x86-64.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8774819 [details] [diff] [review]
add-comments-to-dumped-assembly.patch

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

Thanks! :)

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +1449,5 @@
>  
> +void
> +MacroAssembler::comment(const char* msg)
> +{
> +    asMasm().comment(msg);

nit: asMasm() is used for making down cast to the MacroAssembler, thus, this is an infinite loop.
I suggest you do the same as arm64.

Comment 9

[Sander Mathijs van Veen]

Attachment: add-comments-to-dumped-assembly.patch

Fixed comment about mips-shared

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30da5718207a
Display comments inside dumped assembly code (-D flag). r=nbp

Comment 11

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32803998&repo=mozilla-inbound

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b6929d3c0b
Backed out changeset 30da5718207a for bustage

Comment 13

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34998e283d5e

Comment 14

[Sander Mathijs van Veen]

Attachment: add-comments-to-dumped-assembly.patch

Verified that it builds correctly on all platforms (including android). See try job results above this comment.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69237267b62
Display comments inside dumped assembly code (-D flag) r=nbp

Comment 16

[Carsten Book [:Tomcat]]

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