Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1803373002:
[interpreter] Print bytecode offsets when dumping bytecode. (Closed)
|
Created:
4 years, 9 months ago by Michael Starzinger Modified:
4 years, 8 months ago Reviewers:
rmcilroy CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[interpreter] Print bytecode offsets when dumping bytecode.
R=rmcilroy@chromium.org
Committed: https://crrev.com/8aa6f8716bf09da4415a3665a989b8e164c33c43
Cr-Commit-Position: refs/heads/master@{#35363}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebased. #Patch Set 3 : Addressed comments. #
Messages
Total messages: 11 (2 generated)
LGTM, thanks. Could you also update Runtime_InterpreterTraceBytecodeEntry to have the same format - also, I think that code treats the bytecode offset as including the Bytecode Header, whereas this code doesn't - could you update Runtime_InterpreterTraceBytecodeEntry to be the same as here? https://codereview.chromium.org/1803373002/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1803373002/diff/1/src/objects.cc#newcode14996 src/objects.cc:14996: << iterator.current_offset() << " : "; It's difficult to tell what this looks like when run, but would it be clearer to make the offset clearly differentated from the address, e.g., putting it in square brackets both here and below? Feel free to push back or choose a different option if it looks better to you.
https://codereview.chromium.org/1803373002/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1803373002/diff/1/src/objects.cc#newcode14996 src/objects.cc:14996: << iterator.current_offset() << " : "; On 2016/03/17 11:29:37, rmcilroy wrote: > It's difficult to tell what this looks like when run, but would it be clearer to > make the offset clearly differentated from the address, e.g., putting it in > square brackets both here and below? Feel free to push back or choose a > different option if it looks better to you. I went with this formatting because it is the same that you get for disassembled machine code (i.e. 4 spaces between address and offset). One alternative proposal I was thinking about while implementing this was to prefix all offsets with a certain character everywhere (e.g. the "@" character) so that it is clear throughout the printout when an offset is printed. Your suggestion with using square brackets sound like aiming in the same direction and would of course work for me too. But I am happy change it to any other format, anything is fine with me. Just let me know on the example below how you'd like the output to look like. Here is how the dump looks like at the moment: $ ./out/ia32.debug/d8 --ignition -e "function f(a) { try { return a ? 1 : 2 } finally { return 3 } } f(4)" --print-bytecode [generating bytecode for function: f] Parameter count 2 Frame size 12 0x42484708 0 : 81 StackCheck 0x42484709 1 : 21 05 fe Mov <context>, r2 22 S> 0x4248470c 4 : 1f 08 Ldar a0 0x4248470e 6 : 6f 06 JumpIfToBooleanFalse #6 (0x42484714 / 12) 0x42484710 8 : 01 01 LdaSmi8 #1 0x42484712 10 : 63 04 Jump #4 (0x42484716 / 14) 0x42484714 12 : 01 02 LdaSmi8 #2 0x42484716 14 : 20 ff Star r1 0x42484718 16 : 00 LdaZero 0x42484719 17 : 20 00 Star r0 0x4248471b 19 : 63 0e Jump #14 (0x42484729 / 33) 0x4248471d 21 : 01 ff LdaSmi8 #-1 0x4248471f 23 : 20 00 Star r0 0x42484721 25 : 63 08 Jump #8 (0x42484729 / 33) 0x42484723 27 : 20 ff Star r1 0x42484725 29 : 01 01 LdaSmi8 #1 0x42484727 31 : 20 00 Star r0 0x42484729 33 : 44 ab 00 00 00 CallRuntime [171], r0, #0 0x4248472e 38 : 20 fe Star r2 51 S> 0x42484730 40 : 01 03 LdaSmi8 #3 62 S> 0x42484732 42 : 84 Return 0x42484733 43 : 1f ff Ldar r1 62 S> 0x42484735 45 : 84 Return 0x42484736 46 : 1f ff Ldar r1 0x42484738 48 : 83 ReThrow 0x42484739 49 : 02 LdaUndefined 62 S> 0x4248473a 50 : 84 Return Handler Table (size = 24) from to hdlr ( 4, 21) -> 27 (prediction=0, data=2)
https://codereview.chromium.org/1803373002/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1803373002/diff/1/src/objects.cc#newcode14996 src/objects.cc:14996: << iterator.current_offset() << " : "; On 2016/03/17 11:51:14, Michael Starzinger wrote: > On 2016/03/17 11:29:37, rmcilroy wrote: > > It's difficult to tell what this looks like when run, but would it be clearer > to > > make the offset clearly differentated from the address, e.g., putting it in > > square brackets both here and below? Feel free to push back or choose a > > different option if it looks better to you. > > I went with this formatting because it is the same that you get for disassembled > machine code (i.e. 4 spaces between address and offset). One alternative > proposal I was thinking about while implementing this was to prefix all offsets > with a certain character everywhere (e.g. the "@" character) so that it is clear > throughout the printout when an offset is printed. Your suggestion with using > square brackets sound like aiming in the same direction and would of course work > for me too. > > But I am happy change it to any other format, anything is fine with me. Just let > me know on the example below how you'd like the output to look like. Here is how > the dump looks like at the moment: > > $ ./out/ia32.debug/d8 --ignition -e "function f(a) { try { return a ? 1 : 2 } > finally { return 3 } } f(4)" --print-bytecode > > [generating bytecode for function: f] > Parameter count 2 > Frame size 12 > 0x42484708 0 : 81 StackCheck > 0x42484709 1 : 21 05 fe Mov <context>, r2 > 22 S> 0x4248470c 4 : 1f 08 Ldar a0 > 0x4248470e 6 : 6f 06 JumpIfToBooleanFalse #6 (0x42484714 > / 12) > 0x42484710 8 : 01 01 LdaSmi8 #1 > 0x42484712 10 : 63 04 Jump #4 (0x42484716 / 14) > 0x42484714 12 : 01 02 LdaSmi8 #2 > 0x42484716 14 : 20 ff Star r1 > 0x42484718 16 : 00 LdaZero > 0x42484719 17 : 20 00 Star r0 > 0x4248471b 19 : 63 0e Jump #14 (0x42484729 / 33) > 0x4248471d 21 : 01 ff LdaSmi8 #-1 > 0x4248471f 23 : 20 00 Star r0 > 0x42484721 25 : 63 08 Jump #8 (0x42484729 / 33) > 0x42484723 27 : 20 ff Star r1 > 0x42484725 29 : 01 01 LdaSmi8 #1 > 0x42484727 31 : 20 00 Star r0 > 0x42484729 33 : 44 ab 00 00 00 CallRuntime [171], r0, #0 > 0x4248472e 38 : 20 fe Star r2 > 51 S> 0x42484730 40 : 01 03 LdaSmi8 #3 > 62 S> 0x42484732 42 : 84 Return > 0x42484733 43 : 1f ff Ldar r1 > 62 S> 0x42484735 45 : 84 Return > 0x42484736 46 : 1f ff Ldar r1 > 0x42484738 48 : 83 ReThrow > 0x42484739 49 : 02 LdaUndefined > 62 S> 0x4248473a 50 : 84 Return > Handler Table (size = 24) > from to hdlr > ( 4, 21) -> 27 (prediction=0, data=2) @ symbols sound good to me, let's go with that.
Reviving this ancient CL of mine. Addressed comments. This is how the output
looks like now. Please confirm that this is what you had in mind:
Output for --print-bytecode:
[generating bytecode for function: f]
Parameter count 2
Frame size 12
0x4e786b28 @ 0 : 5e StackCheck
0x4e786b29 @ 1 : 18 05 fe Mov <context>, r2
22 S> 0x4e786b2c @ 4 : 16 08 Ldar a0
0x4e786b2e @ 6 : 52 06 JumpIfToBooleanFalse [6]
(0x4e786b34 @ 12)
0x4e786b30 @ 8 : 03 01 LdaSmi [1]
0x4e786b32 @ 10 : 4a 04 Jump [4] (0x4e786b36 @ 14)
0x4e786b34 @ 12 : 03 02 LdaSmi [2]
0x4e786b36 @ 14 : 17 ff Star r1
0x4e786b38 @ 16 : 02 LdaZero
0x4e786b39 @ 17 : 17 00 Star r0
0x4e786b3b @ 19 : 4a 0e Jump [14] (0x4e786b49 @ 33)
0x4e786b3d @ 21 : 03 ff LdaSmi [-1]
0x4e786b3f @ 23 : 17 00 Star r0
0x4e786b41 @ 25 : 4a 08 Jump [8] (0x4e786b49 @ 33)
0x4e786b43 @ 27 : 17 ff Star r1
0x4e786b45 @ 29 : 03 01 LdaSmi [1]
0x4e786b47 @ 31 : 17 00 Star r0
0x4e786b49 @ 33 : 32 ac 00 00 00 CallRuntime [172], r0, #0
0x4e786b4e @ 38 : 17 fe Star r2
51 S> 0x4e786b50 @ 40 : 03 03 LdaSmi [3]
62 S> 0x4e786b52 @ 42 : 61 Return
0x4e786b53 @ 43 : 16 ff Ldar r1
62 S> 0x4e786b55 @ 45 : 61 Return
0x4e786b56 @ 46 : 16 ff Ldar r1
0x4e786b58 @ 48 : 60 ReThrow
0x4e786b59 @ 49 : 04 LdaUndefined
62 S> 0x4e786b5a @ 50 : 61 Return
Handler Table (size = 24)
from to hdlr
( 4, 21) -> 27 (prediction=0, data=2)
Output snippet for --trace-ignition on same code:
-> 0x4e786b28 @ 0 : 5e StackCheck
-> 0x4e786b29 @ 1 : 18 05 fe Mov <context>, r2
[ <context> -> 0x4e76ee89 <FixedArray[173]> ]
[ r2 <- 0x4e76ee89 <FixedArray[173]> ]
-> 0x4e786b2c @ 4 : 16 08 Ldar a0
[ a0 -> 4 ]
[ accumulator <- 4 ]
-> 0x4e786b2e @ 6 : 52 06 JumpIfToBooleanFalse [6]
[ accumulator -> 4 ]
-> 0x4e786b30 @ 8 : 03 01 LdaSmi [1]
[ accumulator <- 1 ]
-> 0x4e786b32 @ 10 : 4a 04 Jump [4]
-> 0x4e786b36 @ 14 : 17 ff Star r1
[ accumulator -> 1 ]
[ r1 <- 1 ]
-> 0x4e786b38 @ 16 : 02 LdaZero
[ accumulator <- 0 ]
-> 0x4e786b39 @ 17 : 17 00 Star r0
[ accumulator -> 0 ]
[ r0 <- 0 ]
-> 0x4e786b3b @ 19 : 4a 0e Jump [14]
-> 0x4e786b49 @ 33 : 32 ac 00 00 00 CallRuntime [172], r0, #0
[ accumulator <- 0x4e7081cd <the hole> ]
-> 0x4e786b4e @ 38 : 17 fe Star r2
[ accumulator -> 0x4e7081cd <the hole> ]
[ r2 <- 0x4e7081cd <the hole> ]
-> 0x4e786b50 @ 40 : 03 03 LdaSmi [3]
[ accumulator <- 3 ]
-> 0x4e786b52 @ 42 : 61 Return
[ accumulator <- 3 ]
-> 0x4e786bee @ 30 : 17 00 Star r0
[ accumulator -> 3 ]
[ r0 <- 3 ]
-> 0x4e786bf0 @ 32 : 61 Return
https://codereview.chromium.org/1803373002/diff/1/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1803373002/diff/1/src/objects.cc#newcode14996
src/objects.cc:14996: << iterator.current_offset() << " : ";
On 2016/03/17 13:17:08, rmcilroy (Slow) wrote:
> On 2016/03/17 11:51:14, Michael Starzinger wrote:
> > On 2016/03/17 11:29:37, rmcilroy wrote:
> > > It's difficult to tell what this looks like when run, but would it be
> clearer
> > to
> > > make the offset clearly differentated from the address, e.g., putting it
in
> > > square brackets both here and below? Feel free to push back or choose a
> > > different option if it looks better to you.
> >
> > I went with this formatting because it is the same that you get for
> disassembled
> > machine code (i.e. 4 spaces between address and offset). One alternative
> > proposal I was thinking about while implementing this was to prefix all
> offsets
> > with a certain character everywhere (e.g. the "@" character) so that it is
> clear
> > throughout the printout when an offset is printed. Your suggestion with
using
> > square brackets sound like aiming in the same direction and would of course
> work
> > for me too.
> >
> > But I am happy change it to any other format, anything is fine with me. Just
> let
> > me know on the example below how you'd like the output to look like. Here is
> how
> > the dump looks like at the moment:
> >
> > $ ./out/ia32.debug/d8 --ignition -e "function f(a) { try { return a ? 1 : 2
}
> > finally { return 3 } } f(4)" --print-bytecode
> >
> > [generating bytecode for function: f]
> > Parameter count 2
> > Frame size 12
> > 0x42484708 0 : 81 StackCheck
> > 0x42484709 1 : 21 05 fe Mov <context>, r2
> > 22 S> 0x4248470c 4 : 1f 08 Ldar a0
> > 0x4248470e 6 : 6f 06 JumpIfToBooleanFalse #6
> (0x42484714
> > / 12)
> > 0x42484710 8 : 01 01 LdaSmi8 #1
> > 0x42484712 10 : 63 04 Jump #4 (0x42484716 / 14)
> > 0x42484714 12 : 01 02 LdaSmi8 #2
> > 0x42484716 14 : 20 ff Star r1
> > 0x42484718 16 : 00 LdaZero
> > 0x42484719 17 : 20 00 Star r0
> > 0x4248471b 19 : 63 0e Jump #14 (0x42484729 / 33)
> > 0x4248471d 21 : 01 ff LdaSmi8 #-1
> > 0x4248471f 23 : 20 00 Star r0
> > 0x42484721 25 : 63 08 Jump #8 (0x42484729 / 33)
> > 0x42484723 27 : 20 ff Star r1
> > 0x42484725 29 : 01 01 LdaSmi8 #1
> > 0x42484727 31 : 20 00 Star r0
> > 0x42484729 33 : 44 ab 00 00 00 CallRuntime [171], r0, #0
> > 0x4248472e 38 : 20 fe Star r2
> > 51 S> 0x42484730 40 : 01 03 LdaSmi8 #3
> > 62 S> 0x42484732 42 : 84 Return
> > 0x42484733 43 : 1f ff Ldar r1
> > 62 S> 0x42484735 45 : 84 Return
> > 0x42484736 46 : 1f ff Ldar r1
> > 0x42484738 48 : 83 ReThrow
> > 0x42484739 49 : 02 LdaUndefined
> > 62 S> 0x4248473a 50 : 84 Return
> > Handler Table (size = 24)
> > from to hdlr
> > ( 4, 21) -> 27 (prediction=0, data=2)
>
> @ symbols sound good to me, let's go with that.
Done.
On 2016/04/08 13:49:29, Michael Starzinger wrote: > Reviving this ancient CL of mine. Addressed comments. This is how the output > looks like now. Please confirm that this is what you had in mind: > > Output for --print-bytecode: > > [generating bytecode for function: f] > Parameter count 2 > Frame size 12 > 0x4e786b28 @ 0 : 5e StackCheck > 0x4e786b29 @ 1 : 18 05 fe Mov <context>, r2 > 22 S> 0x4e786b2c @ 4 : 16 08 Ldar a0 > 0x4e786b2e @ 6 : 52 06 JumpIfToBooleanFalse [6] > (0x4e786b34 @ 12) > 0x4e786b30 @ 8 : 03 01 LdaSmi [1] > 0x4e786b32 @ 10 : 4a 04 Jump [4] (0x4e786b36 @ 14) > 0x4e786b34 @ 12 : 03 02 LdaSmi [2] > 0x4e786b36 @ 14 : 17 ff Star r1 > 0x4e786b38 @ 16 : 02 LdaZero > 0x4e786b39 @ 17 : 17 00 Star r0 > 0x4e786b3b @ 19 : 4a 0e Jump [14] (0x4e786b49 @ 33) > 0x4e786b3d @ 21 : 03 ff LdaSmi [-1] > 0x4e786b3f @ 23 : 17 00 Star r0 > 0x4e786b41 @ 25 : 4a 08 Jump [8] (0x4e786b49 @ 33) > 0x4e786b43 @ 27 : 17 ff Star r1 > 0x4e786b45 @ 29 : 03 01 LdaSmi [1] > 0x4e786b47 @ 31 : 17 00 Star r0 > 0x4e786b49 @ 33 : 32 ac 00 00 00 CallRuntime [172], r0, #0 > 0x4e786b4e @ 38 : 17 fe Star r2 > 51 S> 0x4e786b50 @ 40 : 03 03 LdaSmi [3] > 62 S> 0x4e786b52 @ 42 : 61 Return > 0x4e786b53 @ 43 : 16 ff Ldar r1 > 62 S> 0x4e786b55 @ 45 : 61 Return > 0x4e786b56 @ 46 : 16 ff Ldar r1 > 0x4e786b58 @ 48 : 60 ReThrow > 0x4e786b59 @ 49 : 04 LdaUndefined > 62 S> 0x4e786b5a @ 50 : 61 Return > Handler Table (size = 24) > from to hdlr > ( 4, 21) -> 27 (prediction=0, data=2) > > Output snippet for --trace-ignition on same code: > > -> 0x4e786b28 @ 0 : 5e StackCheck > -> 0x4e786b29 @ 1 : 18 05 fe Mov <context>, r2 > [ <context> -> 0x4e76ee89 <FixedArray[173]> ] > [ r2 <- 0x4e76ee89 <FixedArray[173]> ] > -> 0x4e786b2c @ 4 : 16 08 Ldar a0 > [ a0 -> 4 ] > [ accumulator <- 4 ] > -> 0x4e786b2e @ 6 : 52 06 JumpIfToBooleanFalse [6] > [ accumulator -> 4 ] > -> 0x4e786b30 @ 8 : 03 01 LdaSmi [1] > [ accumulator <- 1 ] > -> 0x4e786b32 @ 10 : 4a 04 Jump [4] > -> 0x4e786b36 @ 14 : 17 ff Star r1 > [ accumulator -> 1 ] > [ r1 <- 1 ] > -> 0x4e786b38 @ 16 : 02 LdaZero > [ accumulator <- 0 ] > -> 0x4e786b39 @ 17 : 17 00 Star r0 > [ accumulator -> 0 ] > [ r0 <- 0 ] > -> 0x4e786b3b @ 19 : 4a 0e Jump [14] > -> 0x4e786b49 @ 33 : 32 ac 00 00 00 CallRuntime [172], r0, #0 > [ accumulator <- 0x4e7081cd <the hole> ] > -> 0x4e786b4e @ 38 : 17 fe Star r2 > [ accumulator -> 0x4e7081cd <the hole> ] > [ r2 <- 0x4e7081cd <the hole> ] > -> 0x4e786b50 @ 40 : 03 03 LdaSmi [3] > [ accumulator <- 3 ] > -> 0x4e786b52 @ 42 : 61 Return > [ accumulator <- 3 ] > -> 0x4e786bee @ 30 : 17 00 Star r0 > [ accumulator -> 3 ] > [ r0 <- 3 ] > -> 0x4e786bf0 @ 32 : 61 Return > > https://codereview.chromium.org/1803373002/diff/1/src/objects.cc > File src/objects.cc (right): > > https://codereview.chromium.org/1803373002/diff/1/src/objects.cc#newcode14996 > src/objects.cc:14996: << iterator.current_offset() << " : "; > On 2016/03/17 13:17:08, rmcilroy (Slow) wrote: > > On 2016/03/17 11:51:14, Michael Starzinger wrote: > > > On 2016/03/17 11:29:37, rmcilroy wrote: > > > > It's difficult to tell what this looks like when run, but would it be > > clearer > > > to > > > > make the offset clearly differentated from the address, e.g., putting it > in > > > > square brackets both here and below? Feel free to push back or choose a > > > > different option if it looks better to you. > > > > > > I went with this formatting because it is the same that you get for > > disassembled > > > machine code (i.e. 4 spaces between address and offset). One alternative > > > proposal I was thinking about while implementing this was to prefix all > > offsets > > > with a certain character everywhere (e.g. the "@" character) so that it is > > clear > > > throughout the printout when an offset is printed. Your suggestion with > using > > > square brackets sound like aiming in the same direction and would of course > > work > > > for me too. > > > > > > But I am happy change it to any other format, anything is fine with me. Just > > let > > > me know on the example below how you'd like the output to look like. Here is > > how > > > the dump looks like at the moment: > > > > > > $ ./out/ia32.debug/d8 --ignition -e "function f(a) { try { return a ? 1 : 2 > } > > > finally { return 3 } } f(4)" --print-bytecode > > > > > > [generating bytecode for function: f] > > > Parameter count 2 > > > Frame size 12 > > > 0x42484708 0 : 81 StackCheck > > > 0x42484709 1 : 21 05 fe Mov <context>, r2 > > > 22 S> 0x4248470c 4 : 1f 08 Ldar a0 > > > 0x4248470e 6 : 6f 06 JumpIfToBooleanFalse #6 > > (0x42484714 > > > / 12) > > > 0x42484710 8 : 01 01 LdaSmi8 #1 > > > 0x42484712 10 : 63 04 Jump #4 (0x42484716 / 14) > > > 0x42484714 12 : 01 02 LdaSmi8 #2 > > > 0x42484716 14 : 20 ff Star r1 > > > 0x42484718 16 : 00 LdaZero > > > 0x42484719 17 : 20 00 Star r0 > > > 0x4248471b 19 : 63 0e Jump #14 (0x42484729 / 33) > > > 0x4248471d 21 : 01 ff LdaSmi8 #-1 > > > 0x4248471f 23 : 20 00 Star r0 > > > 0x42484721 25 : 63 08 Jump #8 (0x42484729 / 33) > > > 0x42484723 27 : 20 ff Star r1 > > > 0x42484725 29 : 01 01 LdaSmi8 #1 > > > 0x42484727 31 : 20 00 Star r0 > > > 0x42484729 33 : 44 ab 00 00 00 CallRuntime [171], r0, #0 > > > 0x4248472e 38 : 20 fe Star r2 > > > 51 S> 0x42484730 40 : 01 03 LdaSmi8 #3 > > > 62 S> 0x42484732 42 : 84 Return > > > 0x42484733 43 : 1f ff Ldar r1 > > > 62 S> 0x42484735 45 : 84 Return > > > 0x42484736 46 : 1f ff Ldar r1 > > > 0x42484738 48 : 83 ReThrow > > > 0x42484739 49 : 02 LdaUndefined > > > 62 S> 0x4248473a 50 : 84 Return > > > Handler Table (size = 24) > > > from to hdlr > > > ( 4, 21) -> 27 (prediction=0, data=2) > > > > @ symbols sound good to me, let's go with that. > > Done. Looks great, thanks. LGTM
The CQ bit was checked by mstarzinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803373002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803373002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [interpreter] Print bytecode offsets when dumping bytecode. R=rmcilroy@chromium.org ========== to ========== [interpreter] Print bytecode offsets when dumping bytecode. R=rmcilroy@chromium.org Committed: https://crrev.com/8aa6f8716bf09da4415a3665a989b8e164c33c43 Cr-Commit-Position: refs/heads/master@{#35363} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8aa6f8716bf09da4415a3665a989b8e164c33c43 Cr-Commit-Position: refs/heads/master@{#35363} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
