[move] Added bytecode map generation during bytecode disassembly #20797

awelc · 2025-01-06T22:05:35Z

Description

This PR adds support for generating "bytecode maps" (source maps for disassembled bytecode)

Test plan

A new test has been added and all other test have to pass

vercel · 2025-01-06T22:05:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 9, 2025 10:44pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Jan 9, 2025 10:44pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Jan 9, 2025 10:44pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Jan 9, 2025 10:44pm

tzakian · 2025-01-08T17:32:52Z

crates/sui-graphql-rpc/src/types/move_module.rs

+        )
+        .map_err(|e| Error::Internal(format!("Error creating disassembler: {e}")))
+        .extend()?
+        .disassemble()


I'm not sure how I feel about this. I'd probably prefer something like adding a new function, e.g., disasssemble_with_source_map (or something like that) that would also generate the source map during disassembly, and then keeping this function the same.

tzakian · 2025-01-08T17:37:39Z

external-crates/move/crates/move-cli/src/base/disassemble.rs

+                    if bytecode_map {
+                        d.generate_bytecode_map();
+                    }
+                    let (disassemble_string, bcode_map) = d.disassemble()?;
+                    if bytecode_map {
+                        println!("{}", serialize_to_json_string(&bcode_map)?);
+                    }
+                    println!("{}", disassemble_string);


I think places like this will be nicer with the two function approach as this will also allow removing the generate_bytecode_map method as well. E.g., something like this

Suggested change

if bytecode_map {

d.generate_bytecode_map();

}

let (disassemble_string, bcode_map) = d.disassemble()?;

if bytecode_map {

println!("{}", serialize_to_json_string(&bcode_map)?);

}

println!("{}", disassemble_string);

let diss = if bytecode_map {

let (disassemble_string, bcode_map) = d.disassemble_with_source_map()?;

println!("{}", serialize_to_json_string(&bcode_map);

disassemble_string

} else {

d.disassemble()?;

};

println!("{}", disassemble_string);

Indeed, it allowed me to remove this function but it also means that having a global flag is no longer necessary (and somewhat awkward even)

tzakian · 2025-01-08T17:44:48Z

external-crates/move/crates/move-cli/src/base/test.rs

@@ -253,9 +272,59 @@ pub fn run_move_unit_tests<W: Write + Send>(
        let coverage_map = CoverageMap::from_trace_file(trace_path);
        output_map_to_file(coverage_map_path, &coverage_map).unwrap();
    }
+    if save_disassembly {


I get why you put it here, since for the immediate use case it's only needed during testing, but producing disassembly here feels wrong to me.

IMO it feels like an option that should be able to be fed into the build process, and should be done when saving a compiled package out on that side.

I originally followed my principle to avoid modifications to other crates but it certainly makes sense in this case. I think it also made the code cleaner as I didn't have to repeat some of the boilerplate.

tzakian · 2025-01-08T18:17:22Z

external-crates/move/crates/move-disassembler/src/disassembler.rs

@@ -360,7 +448,20 @@ impl<'a> Disassembler<'a> {
                    "",
                    buffer,
                    |buffer, (name, ty)| {
-                        any_write!(buffer, "\t{name}: ")?;
+                        any_write!(buffer, "\t")?;


I think a lot of these places could be simplified if any_write/any_writeln returned back start and end offsets. e.g., you'd then have something like

let (fname_start, fname_end) = any_write!(buffer, "{name}")?;

I'd also see about abstracting the source map generation and location generation as well. I think just a method would be fine for this. e.g.,

fn map_location(&self, start: u32, end: u32, action: FnMut(Loc) -> ()) { if self.bytecode_map { let loc = Loc::new(FileHash::empty(), start, end); action(loc) } }

With that this part could look something like

let (start, end) = any_write!(buffer, "{name}")?; self.map_location(start, end, |loc| bcode_map.add_struct_field_mapping(struct_def_idx, loc))

Alternately, if map_location took a tuple this could be

self.map_location(any_write!(buffer, "{name}")?, |loc| bcode_map.add_struct_field_mapping(struct_def_idx, loc));

I did something similar, but I actually pulled the Loc generation into macros. I think it's probably simpler and thus possibly more readable than the callback solution

tzakian · 2025-01-08T18:19:05Z

external-crates/move/crates/move-disassembler/src/disassembler.rs

-                any_write!(buffer, " }}")
+                any_write!(buffer, " }}")?;
+                // perhaps surprisingly, but the location is of the whole variant
+                // and not just of its name


This is indeed surprising. Maybe we should make it just be of its name?

Didn't want to sneak it in here, but can certainly do it in a separate PR if need be

tzakian · 2025-01-09T17:33:16Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+        unit: &CompiledUnitWithSource,
+    ) -> Result<()> {
+        let root_package = self.package.compiled_package_info.package_name;
+        let bytecode_modules_dir = CompiledPackageLayout::CompiledModules.path();


I'm of two minds on this. I think I'm fine with this, but part of me thinks this might be nice to be in its own separate top-level dir under the build/<pkg_name> (i.e., a sibling of bytecode_modules/sources (Etc)).

Thoughts? I'm pretty open to either option I think

Actually, it's a great point. Putting them in a separate directory for trace-debugger to pick up will make it easier down the line - we can have other methods of generating disassembly (e.g., from the replay tool) to keep the same directory structure. I am putting them into build/disassembly directory then

tzakian · 2025-01-09T17:41:04Z

external-crates/move/crates/move-disassembler/src/disassembler.rs

+        bcode_map_gen: bool,
+        bcode_map: &mut SourceMap,


Sorry to be so nitty, but I only mention it as I think it will clean up the logic...

What about passing bcode_map: &mut Option<SourceMap> here. Then below, instead of things like

if bcode_map_gen { bcode_map.add_top_level_struct_mapping(...) }

This could be rewritten as

bcode_map.map(|m| m.add_top_level_struct_mapping(..));

or even as

if let Some(m) = bcode_map { m.add_top_level_struct_mapping(...); }

Good idea - will also let me shave off a parameter in a few places

tzakian · 2025-01-09T17:42:21Z

external-crates/move/crates/move-disassembler/src/disassembler.rs

        // NB: The order in which these are called is important as each function is effectful.
-        self.print_header(buffer)?;
+        let mut bcode_map = self.print_header(buffer)?;


See comment below but I think turning this into an option may make things even cleaner.

tzakian · 2025-01-09T17:46:39Z

external-crates/move/crates/move-disassembler/src/disassembler.rs

-            SignatureToken::U256 => any_write!(buffer, "u256"),
-            SignatureToken::Address => any_write!(buffer, "address"),
-            SignatureToken::Signer => any_write!(buffer, "signer"),
+            SignatureToken::Bool => any_write!(buffer, "bool").map(|_| ()),


IMO would be cleaner to do something like

match sig_tok { .... }.map(|_| ())

or to ? all of these and then have an Ok(()) at the bottom of the function.

I tried that some match arms return Result<()> instead of Result<Loc>, and I had to unify them which made this look uglier...

tzakian

One small nit to consider, but otherwise LGTM!

tzakian · 2025-01-09T23:01:20Z

external-crates/move/crates/move-disassembler/src/disassembler.rs

@@ -279,13 +279,14 @@ impl<'a> Disassembler<'a> {
        bcode_map_gen: bool,
    ) -> Result<SourceMap> {
        // NB: The order in which these are called is important as each function is effectful.
-        let mut bcode_map = self.print_header(buffer)?;
+        let mut res = Some(self.print_header(buffer)?);


Nit: consider if this needs to make the source map at all, but I think this is fine as-is if you'd prefer to keep it like that.

I was thinking about it decided to let it simply return largely empty SourceMap rather than None is that on the client side we then don't have to do another check (and possibly user-facing message) when unwrapping (e.g. for printing)

[move] Added bytecode map generation during bytecode disassembly

1ec008b

awelc requested a review from tzakian January 6, 2025 22:05

awelc self-assigned this Jan 6, 2025

awelc requested review from amnn, wlmyng, stefan-mysten, emmazzz and suiwombat as code owners January 6, 2025 22:05

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 6, 2025 22:05 — with GitHub Actions Inactive

Test fixes

a6fe15a

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 6, 2025 23:06 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 6, 2025 23:07 View deployment

Added a flag to prevent byteceode map generation

e339c34

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 8, 2025 01:42 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 8, 2025 01:43 View deployment

Made clippy happy

6f6cccf

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 8, 2025 01:45 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 8, 2025 01:47 View deployment

Made clippy happy

dbdb6a3

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 8, 2025 17:56 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 8, 2025 17:57 View deployment

tzakian reviewed Jan 8, 2025

View reviewed changes

awelc added 4 commits January 8, 2025 15:29

Added a separate function to deserialize with source maps

dad8d93

Streamlined location generation

27fe3be

Moved on-disk disassembly generation to package

8111262

Lowerd function visibility to previous level

8dce534

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 9, 2025 02:16 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 9, 2025 02:18 View deployment

awelc requested a review from tzakian January 9, 2025 02:20

tzakian reviewed Jan 9, 2025

View reviewed changes

awelc added 3 commits January 9, 2025 13:57

Eliminated unnecessary parameter

04679b2

Moved disassembly/maps to top-level dir

b619864

Regenerated tests

dd15c7c

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 9, 2025 22:18 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 9, 2025 22:19 View deployment

awelc requested a review from tzakian January 9, 2025 22:23

Regenerated test result

9ae1dd0

awelc temporarily deployed to sui-typescript-aws-kms-test-env January 9, 2025 22:43 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 9, 2025 22:44 View deployment

tzakian approved these changes Jan 9, 2025

View reviewed changes

awelc merged commit 2b655b2 into main Jan 10, 2025
51 checks passed

awelc deleted the aw/bytecode-map branch January 10, 2025 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move] Added bytecode map generation during bytecode disassembly #20797

[move] Added bytecode map generation during bytecode disassembly #20797

awelc commented Jan 6, 2025

vercel bot commented Jan 6, 2025 •

edited

Loading

tzakian Jan 8, 2025

awelc Jan 9, 2025

tzakian Jan 8, 2025

awelc Jan 9, 2025

tzakian Jan 8, 2025

awelc Jan 9, 2025

tzakian Jan 8, 2025

awelc Jan 9, 2025

tzakian Jan 8, 2025

awelc Jan 9, 2025

tzakian Jan 9, 2025

awelc Jan 9, 2025 •

edited

Loading

tzakian Jan 9, 2025

awelc Jan 9, 2025

tzakian Jan 9, 2025

tzakian Jan 9, 2025

awelc Jan 9, 2025

tzakian left a comment

tzakian Jan 9, 2025

awelc Jan 9, 2025

[move] Added bytecode map generation during bytecode disassembly #20797

[move] Added bytecode map generation during bytecode disassembly #20797

Conversation

awelc commented Jan 6, 2025

Description

Test plan

vercel bot commented Jan 6, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc Jan 9, 2025 • edited Loading

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

tzakian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jan 6, 2025 •

edited

Loading

awelc Jan 9, 2025 •

edited

Loading