Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Implement instruction flowgraph generator #2422

Closed
wants to merge 25 commits into from

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Nov 8, 2022

This PR is a WIP implementation of a vm instruction flowgraph generator

This aims to make the vm easier to debug and understand for both newcomers and experienced devs.

For example if we have the following code:

let i = 0;
while (i < 10) {
    if (i == 3) {
        break;
    }
    i++;
}

It generates the following instructions (which is hard to read, especially jumps):

----------------------Compiled Output: '<main>'-----------------------
Location  Count   Opcode                     Operands

000000    0000    PushZero
000001    0001    DefInitLet                 0000: 'i'
000006    0002    LoopStart
000007    0003    LoopContinue
000008    0004    GetName                    0000: 'i'
000013    0005    PushInt8                   10
000015    0006    LessThan
000016    0007    JumpIfFalse                78
000021    0008    PushDeclarativeEnvironment 0, 1
000030    0009    GetName                    0000: 'i'
000035    0010    PushInt8                   3
000037    0011    Eq
000038    0012    JumpIfFalse                58
000043    0013    PushDeclarativeEnvironment 0, 0
000052    0014    Jump                       78
000057    0015    PopEnvironment
000058    0016    GetName                    0000: 'i'
000063    0017    IncPost
000064    0018    RotateRight                2
000066    0019    SetName                    0000: 'i'
000071    0020    Pop
000072    0021    PopEnvironment
000073    0022    Jump                       7
000078    0023    LoopEnd

Literals:
    <empty>

Bindings:
    0000: i

Functions:
    <empty>

And the flow graph is generated:
flowgraph

The beginning of the function is marked by the start node (in green) and end (in red). In branching the "yes" branch is marked in green and "no" in red.

This only generates in graphviz format (a widely used format) but it would be nice to also generate to a format that mermaid.js can understand and that could be put in articles boa-dev/boa-dev.github.io#26

TODO:

  • Generate graphviz format
  • Generate mermaid format
  • Programmatically generate colors push and pop env instructions
  • Display nested functions in sub-sub-graphs.
  • Put under a feature ("flowgraph")
  • Handle try/catch, switch instructions
  • CLI option for configuring direction of flow (by default it is top down)
  • Handle Throw instruction (requires keeping track of try blocks)
  • Documentation
  • Prevent node name collisions (functions with the same name)

@HalidOdat HalidOdat added enhancement New feature or request vm Issues and PRs related to the Boa Virtual Machine. labels Nov 8, 2022
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Test262 conformance changes

Test result main count PR count difference
Total 93,831 93,831 0
Passed 69,620 69,620 0
Ignored 18,472 18,472 0
Failed 5,739 5,739 0
Panics 0 0 0
Conformance 74.20% 74.20% 0.00%

@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #2422 (c6e956f) into main (20db887) will decrease coverage by 0.94%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2422      +/-   ##
==========================================
- Coverage   52.58%   51.63%   -0.95%     
==========================================
  Files         329      336       +7     
  Lines       34905    35541     +636     
==========================================
- Hits        18354    18353       -1     
- Misses      16551    17188     +637     
Impacted Files Coverage Δ
boa_cli/src/main.rs 0.00% <ø> (ø)
boa_engine/src/vm/flowgraph/color.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/flowgraph/edge.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/flowgraph/graph.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/flowgraph/mod.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/flowgraph/node.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/mod.rs 47.97% <ø> (ø)
boa_engine/src/vm/opcode/mod.rs 57.14% <ø> (ø)
boa_ast/src/visitor.rs 10.58% <0.00%> (-0.59%) ⬇️
boa_engine/src/object/mod.rs 45.98% <0.00%> (-0.11%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@HalidOdat HalidOdat changed the title Implement instruction to flowgraph visualizer Implement instruction flowgraph generator Nov 8, 2022
@jedel1043
Copy link
Member

Very nice work! This is a huge improvement for debugging. I have just a small nitpick though. Can it be generated vertically instead of horizontally? I think it will read better that way.

@HalidOdat
Copy link
Member Author

HalidOdat commented Nov 8, 2022

Very nice work! This is a huge improvement for debugging. I have just a small nitpick though. Can it be generated vertically instead of horizontally? I think it will read better that way.

Yes, that is configurable (I did it horizontaly because it looked better on github comments) I plan to add an option for that

And there are other generation modes, where you can better see the loop:
index

@HalidOdat
Copy link
Member Author

HalidOdat commented Nov 8, 2022

Hmmm.. looking at the flowgraph it looks like there is a PopEnvironment which will never be executed (since nothing "flows" into it). I'm not sure if it is a bug on my part or in the bytecompiler, if it is on the bytecompiler this is already proving to be useful 😆

@jedel1043
Copy link
Member

Hmmm.. looking at the flowgraph it looks like there is a PopEnvironment which will never be executed (since nothing "flows" into it). I'm not sure if it is a bug on my part or in the bytecompiler, if it is on the bytecompiler this is already proving to be useful 😆

That's definitely a bug! JumpIfFalse is jumping to instruction 0x58, skipping the PopEnvironment, and the plain Jump also skips it by jumping to the end of the function.

@HalidOdat
Copy link
Member Author

I guess, I'll create an issue about it :)

@jedel1043
Copy link
Member

jedel1043 commented Nov 8, 2022

What if we coloured pairs of PushEnvironmnent and PopEnvironment with the same colour, and changed the colour for each pair? That way we can identify if environments are being handled correctly.

@HalidOdat
Copy link
Member Author

HalidOdat commented Nov 8, 2022

Here is a implementation of that, it assumes that the generated push and pop instructions always are in that order., which should be the case generaly (if not always), unless there are some wierd jumping back and forth.
Otherwise we need to hook into the bytecompiler, because we lose context after compilation.

Also included the opcode location prefix on all nodes.

Here are the graphs generated:

This is all temporary code, will try to create nice options for generating dot and mermaid.js formats tomorrow :)

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is extremely good. Just seeing the graphs I'm starting to understand the VM, which is already a huge step for me 😅

And it's already showing how this can spot problems!

The code needs a bit of documentation, polishing and maybe the color constant could be reviewed to be more configurable, but it's already in pretty good shape.

Thank you!

@jasonwilliams
Copy link
Member

jasonwilliams commented Nov 9, 2022

This is great! And definitely helps with seeing how things work with the VM a lot.

I’m very interested in mermaid integration as that’s supported on GitHub and VSCode now so should make collaboration easier. It should save on needing to install an additional viewer. The syntax looks similar but I don’t know if there’s any limitations.

@HalidOdat HalidOdat force-pushed the feature/instruction-flowgraph branch from 8182cc6 to 467f143 Compare November 9, 2022 14:09
@HalidOdat
Copy link
Member Author

I still need to add many things but we can generate graphviz and mermaid diagrams

Since mermaid diagrams are supported by github comments (flipped it horizontally because its to big):

graph LR
    __main___i_0[PushZero]
    __main___i_1[DefInitLet 'i']
    __main___i_6[LoopStart]
    __main___i_7[LoopContinue]
    __main___i_8[GetName 'i']
    __main___i_13[PushInt8 10]
    __main___i_15[LessThan]
    __main___i_16{JumpIfFalse}
    __main___i_21[PushDeclarativeEnvironment 0, 1]
    __main___i_30[GetName 'i']
    __main___i_35[PushInt8 3]
    __main___i_37[Eq]
    __main___i_38{JumpIfFalse}
    __main___i_43[PushDeclarativeEnvironment 0, 0]
    __main___i_52{Jump}
    __main___i_57[PopEnvironment]
    __main___i_58[GetName 'i']
    __main___i_63[IncPost]
    __main___i_64[RotateRight 2]
    __main___i_66[SetName 'i']
    __main___i_71[Pop]
    __main___i_72[PopEnvironment]
    __main___i_73{Jump}
    __main___i_78[LoopEnd]
    __main___i_79{End}
    __main___i_80{Start}
    __main___i_0 -->| | __main___i_1
    __main___i_1 -->| | __main___i_6
    __main___i_6 -->| | __main___i_7
    __main___i_7 -->| | __main___i_8
    __main___i_8 -->| | __main___i_13
    __main___i_13 -->| | __main___i_15
    __main___i_15 -->| | __main___i_16
    __main___i_16 -->| YES| __main___i_78
    __main___i_16 -->| NO| __main___i_21
    __main___i_21 -->| | __main___i_30
    __main___i_30 -->| | __main___i_35
    __main___i_35 -->| | __main___i_37
    __main___i_37 -->| | __main___i_38
    __main___i_38 -->| YES| __main___i_58
    __main___i_38 -->| NO| __main___i_43
    __main___i_43 -->| | __main___i_52
    __main___i_52 -->| | __main___i_78
    __main___i_57 -->| | __main___i_58
    __main___i_58 -->| | __main___i_63
    __main___i_63 -->| | __main___i_64
    __main___i_64 -->| | __main___i_66
    __main___i_66 -->| | __main___i_71
    __main___i_71 -->| | __main___i_72
    __main___i_72 -->| | __main___i_73
    __main___i_73 -->| | __main___i_7
    __main___i_78 -->| | __main___i_79
    __main___i_80 -->| | __main___i_0

Loading

@Razican Razican added this to the v0.17.0 milestone Nov 11, 2022
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a lot to review, but it looks great :) I added some comments related to documentation and Clone implementations.

boa_cli/src/main.rs Show resolved Hide resolved
boa_cli/src/main.rs Show resolved Hide resolved
boa_cli/src/main.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/graph.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/graph.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/graph.rs Outdated Show resolved Hide resolved
@HalidOdat
Copy link
Member Author

Added documentation in debugging.md file, you can view how it looks here: https://github.com/boa-dev/boa/blob/60d33337d322b7cf328b9237cf67a172a24d2d20/docs/debugging.md#instruction-flowgraph

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@Razican
Copy link
Member

Razican commented Nov 22, 2022

bors r+

@bors
Copy link

bors bot commented Nov 22, 2022

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@bors
Copy link

bors bot commented Nov 22, 2022

👎 Rejected by PR status

@Razican
Copy link
Member

Razican commented Nov 22, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 22, 2022
This PR is a WIP implementation of a vm instruction flowgraph generator

This aims to make the vm easier to debug and understand for both newcomers and experienced devs.

For example if we have the following code:
```js
let i = 0;
while (i < 10) {
    if (i == 3) {
        break;
    }
    i++;
}
```
It generates the following instructions (which is hard to read, especially jumps):
<details>

```
----------------------Compiled Output: '<main>'-----------------------
Location  Count   Opcode                     Operands

000000    0000    PushZero
000001    0001    DefInitLet                 0000: 'i'
000006    0002    LoopStart
000007    0003    LoopContinue
000008    0004    GetName                    0000: 'i'
000013    0005    PushInt8                   10
000015    0006    LessThan
000016    0007    JumpIfFalse                78
000021    0008    PushDeclarativeEnvironment 0, 1
000030    0009    GetName                    0000: 'i'
000035    0010    PushInt8                   3
000037    0011    Eq
000038    0012    JumpIfFalse                58
000043    0013    PushDeclarativeEnvironment 0, 0
000052    0014    Jump                       78
000057    0015    PopEnvironment
000058    0016    GetName                    0000: 'i'
000063    0017    IncPost
000064    0018    RotateRight                2
000066    0019    SetName                    0000: 'i'
000071    0020    Pop
000072    0021    PopEnvironment
000073    0022    Jump                       7
000078    0023    LoopEnd

Literals:
    <empty>

Bindings:
    0000: i

Functions:
    <empty>
```

</details>

And the flow graph is generated:
![flowgraph](https://user-images.githubusercontent.com/8566042/200589387-40b36ad7-d2f2-4918-a3e4-5a8fa5eee89b.png)

The beginning of the function is  marked by the `start` node (in green) and end (in red). In branching the "yes" branch is marked  in green and "no" in red.

~~This only generates in [graphviz format](https://en.wikipedia.org/wiki/DOT_(graph_description_language)) (a widely used format) but it would be nice to also generate to a format that `mermaid.js` can understand and that could be put in articles boa-dev/boa-dev.github.io#26

TODO:
  - [x] Generate graphviz format
  - [x] Generate mermaid format
  - [x] Programmatically generate colors push and pop env instructions
  - [x] Display nested functions in sub-sub-graphs.
  - [x] Put under a feature (`"flowgraph"`)
  - [x] Handle try/catch, switch instructions
  - [x] CLI option for configuring direction of flow (by default it is top down)
  - [x] Handle `Throw` instruction (requires keeping track of try blocks)
  - [x] Documentation
  - [x] Prevent node name collisions (functions with the same name)
@bors
Copy link

bors bot commented Nov 22, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Implement instruction flowgraph generator [Merged by Bors] - Implement instruction flowgraph generator Nov 22, 2022
@bors bors bot closed this Nov 22, 2022
@bors bors bot deleted the feature/instruction-flowgraph branch November 22, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants