Closed Bug 1287485 Opened 5 years ago Closed 5 years ago

Display comments inside dumped assembly code (-D flag)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: sandervv, Assigned: sandervv)

Details

Attachments

(1 file, 4 obsolete files)

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 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.
Attachment #8772010 - Flags: feedback+
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?
Assignee: nobody → sandervv
Attachment #8772010 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8772359 - Flags: review?(nicolas.b.pierron)
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 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.
Attachment #8772359 - Flags: review?(nicolas.b.pierron) → feedback+
Change the patch according to nbp's suggestions.

This patch passes try builds for at least arm, arm64, x86, and x86-64.
Attachment #8772359 - Attachment is obsolete: true
Attachment #8774819 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8774819 - Flags: review?(nicolas.b.pierron) → review+
Fixed comment about mips-shared
Attachment #8774819 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30da5718207a
Display comments inside dumped assembly code (-D flag). r=nbp
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b6929d3c0b
Backed out changeset 30da5718207a for bustage
Verified that it builds correctly on all platforms (including android). See try job results above this comment.
Attachment #8775479 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69237267b62
Display comments inside dumped assembly code (-D flag) r=nbp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a69237267b62
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.