Show function names in explore tool instead of only function indices #8639

lpereira · 2024-05-16T21:23:57Z

Another one in the "let's make debugging a bit easier": show function names in the "explore" tool. (There's a bit of code repetition here to clean up the names, but it's slightly different when we're considering HTML output, so I think that's OK.)

Here's how it works on my machine:

jameysharp · 2024-05-16T22:04:25Z

Very nice!

This can be further improved by demangling the function names first. For some example code that does that, see:

wasmtime/crates/wasmtime/src/runtime/profiling.rs

Lines 194 to 200 in 54e53cc

    
           let mut name = String::new(); 
        
           demangle_function_name_or_index( 
        
               &mut name, 
        
               compiled.func_name(func_idx), 
        
               defined_idx.as_u32() as usize, 
        
           ) 
        
           .unwrap();

I don't think we need to be nearly so careful about sanitizing function names in this setting. These strings are first serialized to JSON, and that should ensure that any string we provide is valid JSON. The JSON string is then used in a <script> tag; we might need to escape the JSON for embedding in HTML, but <script> tags are implicitly treated as CDATA in HTML5, right? Finally, the strings embedded in the JSON are spliced into the DOM using textContent, so they don't need to be escaped at that point.

As a result I think we should just give the user whatever they provided, preferably after demangling.

fitzgen

Thanks! One nitpick below before we merge this.

crates/wasmtime/src/runtime/module.rs

fitzgen · 2024-05-16T22:25:18Z

Agreed demangling would be great -- also fine with deferring that to a follow up PR.

lpereira · 2024-05-16T23:13:47Z

Pushed the requested changes. Current output here is now this one (notice the pop-up showing the mangled name):

jameysharp · 2024-05-17T00:10:11Z

Ooh, the pop-up is a nice touch. There's a lot more that could be done with wasmtime explore eventually.

I'd be happy to review this next week, or just as happy if you want to approve it @fitzgen.

fitzgen

Thanks! Really excited to get this, and your other PR, in wasmtime explore.

I have a few comments and suggestions below that we should address before merging this however. I'll also hold off on reviewing the other PR until after this one merges.

Let me know if anything I said isn't clear or something like that. Thanks again!

fitzgen · 2024-05-20T20:53:04Z

crates/explorer/src/index.js

@@ -7,27 +7,12 @@ class State {
  }
 }

-const state = window.STATE = new State(window.WAT, window.ASM);
+const state = (window.STATE = new State(window.WAT, window.ASM));


Let's revert this change; it just introduces unnecessary parens.

fitzgen · 2024-05-20T20:54:25Z

crates/explorer/src/index.js

@@ -36,7 +21,7 @@ const nextHue = (function () {
  return () => {
    return hues[++i % hues.length];
  };
-}());
+})();


Ditto. Let's not change code style when making unrelated changes.

fitzgen · 2024-05-20T20:54:40Z

crates/explorer/src/index.js

@@ -45,7 +30,7 @@ const offsetToHue = new Map();

 // Get the hue for the given offset, or assign it a new one if it doesn't have
 // one already.
-const hueForOffset = offset => {
+const hueForOffset = (offset) => {


fitzgen · 2024-05-20T20:54:49Z

crates/explorer/src/index.js

@@ -57,7 +42,7 @@ const hueForOffset = offset => {

 // Get the hue for the given offset, only if the offset has already been
 // assigned a hue.
-const existingHueForOffset = offset => {
+const existingHueForOffset = (offset) => {


crates/explorer/src/index.js

fitzgen · 2024-05-20T20:59:47Z

crates/explorer/src/index.js

+  funcHeader.textContent = `Disassembly of function <${func.demangled_name}>:`;
+  funcHeader.title = func.name;


AFAICT, this is the only non-stylistic change to the whole JS file? Can we remove all the other changes to this file?

Sure. It was the auto-formatting tool that my editor uses. I'll have to force-push to fix this easily.

fitzgen · 2024-05-20T21:04:42Z

crates/explorer/src/lib.rs

+            let demangled_name = match name.splitn(2, "::").nth(1) {
+                Some(name) => {
+                    let mut demangled = String::new();
+                    demangle_function_name(&mut demangled, name)
+                        .map_or_else(|_| format!("demangle-error::{}", name), |_| demangled)
+                }
+                None => name.to_string(),
+            };


I think it would be better to let demangled_name be an Option<String> rather than making a clone of name and then have the JS choose the demangled name when available and fall back to the plain name.

crates/wasmtime/src/runtime/module.rs

Map the iterator returned by Module::function_locations() to another one that returns a 3-tuple containing the function name, the offset, and the length of each function defined in this particular module.

lpereira · 2024-05-20T22:24:21Z

Alright, force-pushed (sorry!) due to the formatting changes, but it should be a whole lot clearer now. I'll rebase the other PR once this one lands.

fitzgen

Thanks! Looks just about ready to merge, couple tiny things still to go first.

crates/explorer/src/index.js

crates/wasmtime/src/runtime/module.rs

lpereira · 2024-05-21T17:06:33Z

Alright, @fitzgen, pushed a new revision that should address your comments.

fitzgen

Looks great, thanks! And thanks for your patience with review :)

lpereira requested a review from a team as a code owner May 16, 2024 21:23

lpereira requested review from pchickey and removed request for a team May 16, 2024 21:23

lpereira force-pushed the show-func-names-in-explore branch from 9ef98c3 to deccfe3 Compare May 16, 2024 21:32

fitzgen reviewed May 16, 2024

View reviewed changes

crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved

lpereira requested a review from a team as a code owner May 16, 2024 23:10

github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label May 17, 2024

lpereira mentioned this pull request May 17, 2024

Miscellaneous Explore tool improvements #8649

Open

fitzgen reviewed May 20, 2024

View reviewed changes

lpereira added 3 commits May 20, 2024 15:17

Implement runtime::Module::function_locations_with_names()

7660c8f

Map the iterator returned by Module::function_locations() to another one that returns a 3-tuple containing the function name, the offset, and the length of each function defined in this particular module.

Show function names in "explore" instead of just the indices

f6b435b

Address review: Change iterator format

c069922

lpereira force-pushed the show-func-names-in-explore branch from 4304bb3 to c18e755 Compare May 20, 2024 22:21

Address review: use the new iterator struct

277f57c

lpereira force-pushed the show-func-names-in-explore branch from c18e755 to 277f57c Compare May 20, 2024 23:20

fitzgen reviewed May 21, 2024

View reviewed changes

crates/explorer/src/index.js Outdated Show resolved Hide resolved

crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved

Address review comments

ebab1fc

fitzgen approved these changes May 21, 2024

View reviewed changes

fitzgen added this pull request to the merge queue May 21, 2024

Merged via the queue into bytecodealliance:main with commit f994647 May 21, 2024
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show function names in explore tool instead of only function indices #8639

Show function names in explore tool instead of only function indices #8639

lpereira commented May 16, 2024

jameysharp commented May 16, 2024

fitzgen left a comment

fitzgen commented May 16, 2024

lpereira commented May 16, 2024 •

edited

Loading

jameysharp commented May 17, 2024

fitzgen left a comment

fitzgen May 20, 2024

fitzgen May 20, 2024

fitzgen May 20, 2024

fitzgen May 20, 2024

fitzgen May 20, 2024

lpereira May 20, 2024

fitzgen May 20, 2024

lpereira May 20, 2024

lpereira commented May 20, 2024

fitzgen left a comment

lpereira commented May 21, 2024

fitzgen left a comment

		funcHeader.textContent = `Disassembly of function <${func.demangled_name}>:`;
		funcHeader.title = func.name;

Show function names in explore tool instead of only function indices #8639

Show function names in explore tool instead of only function indices #8639

Conversation

lpereira commented May 16, 2024

jameysharp commented May 16, 2024

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented May 16, 2024

lpereira commented May 16, 2024 • edited Loading

jameysharp commented May 17, 2024

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira commented May 20, 2024

fitzgen left a comment

Choose a reason for hiding this comment

lpereira commented May 21, 2024

fitzgen left a comment

Choose a reason for hiding this comment

lpereira commented May 16, 2024 •

edited

Loading