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.

Sign up for GitHub

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

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

Merged
merged 13 commits into from
Jan 10, 2025
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1ec008b
[move] Added bytecode map generation during bytecode disassembly
awelc Jan 6, 2025
a6fe15a
Test fixes
awelc Jan 6, 2025
e339c34
Added a flag to prevent byteceode map generation
awelc Jan 8, 2025
6f6cccf
Made clippy happy
awelc Jan 8, 2025
dbdb6a3
Made clippy happy
awelc Jan 8, 2025
dad8d93
Added a separate function to deserialize with source maps
awelc Jan 8, 2025
27fe3be
Streamlined location generation
awelc Jan 9, 2025
8111262
Moved on-disk disassembly generation to package
awelc Jan 9, 2025
8dce534
Lowerd function visibility to previous level
awelc Jan 9, 2025
04679b2
Eliminated unnecessary parameter
awelc Jan 9, 2025
b619864
Moved disassembly/maps to top-level dir
awelc Jan 9, 2025
dd15c7c
Regenerated tests
awelc Jan 9, 2025
9ae1dd0
Regenerated test result
awelc Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions external-crates/move/crates/move-cli/tests/build_tests/help/args.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ Options:
Compile in 'test' mode. The 'dev-addresses' and 'dev-dependencies' fields will be used along with any code in the 'tests' directory
--doc
Generate documentation for packages
--disassemble
Save disassembly for generated bytecode along with bytecode maps (source maps for disassembeld bytecode)
--install-dir <INSTALL_DIR>
Installation directory for compiled artifacts. Defaults to current directory
--force
Expand Down
60 changes: 24 additions & 36 deletions external-crates/move/crates/move-disassembler/src/disassembler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)?);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

let bcode_map_opt = if bcode_map_gen { &mut res } else { &mut None };
self.print_imports(buffer)?;
self.print_user_defined_types(buffer, bcode_map_gen, &mut bcode_map)?;
self.print_function_definitions(buffer, bcode_map_gen, &mut bcode_map)?;
self.print_constants(buffer, bcode_map_gen, &mut bcode_map)?;
self.print_user_defined_types(buffer, bcode_map_opt)?;
self.print_function_definitions(buffer, bcode_map_opt)?;
self.print_constants(buffer, bcode_map_opt)?;
self.print_footer(buffer)?;
Ok(bcode_map)
Ok(res.unwrap())
}

fn print_header(&self, buffer: &mut (impl Write + ByteLength)) -> Result<SourceMap> {
Expand Down Expand Up @@ -318,26 +319,19 @@ impl<'a> Disassembler<'a> {
fn print_user_defined_types(
&self,
buffer: &mut (impl Write + ByteLength),
bcode_map_gen: bool,
bcode_map: &mut SourceMap,
bcode_map_opt: &mut Option<SourceMap>,
) -> Result<()> {
for i in 0..self.source_mapper.bytecode.struct_defs().len() {
self.disassemble_struct_def(
buffer,
bcode_map_gen,
bcode_map,
bcode_map_opt,
StructDefinitionIndex(i as TableIndex),
)?;
any_writeln!(buffer)?;
}

for i in 0..self.source_mapper.bytecode.enum_defs().len() {
self.disassemble_enum_def(
buffer,
bcode_map_gen,
bcode_map,
EnumDefinitionIndex(i as TableIndex),
)?;
self.disassemble_enum_def(buffer, bcode_map_opt, EnumDefinitionIndex(i as TableIndex))?;
any_writeln!(buffer)?;
}

Expand All @@ -347,14 +341,12 @@ impl<'a> Disassembler<'a> {
fn print_function_definitions(
&self,
buffer: &mut (impl Write + ByteLength),
bcode_map_gen: bool,
bcode_map: &mut SourceMap,
bcode_map_opt: &mut Option<SourceMap>,
) -> Result<()> {
for i in 0..self.source_mapper.bytecode.function_defs().len() {
self.disassemble_function_definition(
buffer,
bcode_map_gen,
bcode_map,
bcode_map_opt,
FunctionDefinitionIndex(i as TableIndex),
)?;
any_writeln!(buffer)?;
Expand All @@ -366,8 +358,7 @@ impl<'a> Disassembler<'a> {
fn print_constants(
&self,
buffer: &mut (impl Write + ByteLength),
bcode_map_gen: bool,
bcode_map: &mut SourceMap,
bcode_map_opt: &mut Option<SourceMap>,
) -> Result<()> {
delimited_list(
self.source_mapper
Expand All @@ -380,7 +371,7 @@ impl<'a> Disassembler<'a> {
"]\n",
buffer,
|buffer, (idx, constant)| {
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
// no constant name in the disassembled bytecode - use index as a name
bcode_map.add_const_mapping(
ConstantPoolIndex(idx as TableIndex),
Expand All @@ -405,8 +396,7 @@ impl<'a> Disassembler<'a> {
fn disassemble_struct_def(
&self,
buffer: &mut (impl Write + ByteLength),
bcode_map_gen: bool,
bcode_map: &mut SourceMap,
bcode_map_opt: &mut Option<SourceMap>,
struct_def_idx: StructDefinitionIndex,
) -> Result<()> {
let struct_definition = self.source_mapper.bytecode.struct_def_at(struct_def_idx);
Expand Down Expand Up @@ -446,7 +436,7 @@ impl<'a> Disassembler<'a> {

any_write!(buffer, "{native}struct ")?;
let struct_name_loc = any_write!(buffer, "{name}")?;
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
bcode_map.add_top_level_struct_mapping(struct_def_idx, struct_name_loc)?;
}

Expand All @@ -455,7 +445,7 @@ impl<'a> Disassembler<'a> {
&struct_source_map.type_parameters,
&struct_handle.type_parameters,
)?;
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
for n in type_param_source_names {
bcode_map.add_struct_type_parameter_mapping(struct_def_idx, n)?;
}
Expand All @@ -477,7 +467,7 @@ impl<'a> Disassembler<'a> {
|buffer, (name, ty)| {
any_write!(buffer, "\t")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

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));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let field_name_loc = any_write!(buffer, "{name}")?;
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
bcode_map.add_struct_field_mapping(struct_def_idx, field_name_loc)?;
}
any_write!(buffer, ": ")?;
Expand All @@ -502,8 +492,7 @@ impl<'a> Disassembler<'a> {
fn disassemble_enum_def(
&self,
buffer: &mut (impl Write + ByteLength),
bcode_map_gen: bool,
bcode_map: &mut SourceMap,
bcode_map_opt: &mut Option<SourceMap>,
enum_def_idx: EnumDefinitionIndex,
) -> Result<()> {
let enum_definition = self.source_mapper.bytecode.enum_def_at(enum_def_idx);
Expand All @@ -524,7 +513,7 @@ impl<'a> Disassembler<'a> {

any_write!(buffer, "enum ")?;
let enum_name_loc = any_write!(buffer, "{name}")?;
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
bcode_map.add_top_level_enum_mapping(enum_def_idx, enum_name_loc)?;
}

Expand All @@ -533,7 +522,7 @@ impl<'a> Disassembler<'a> {
&enum_source_map.type_parameters,
&enum_handle.type_parameters,
)?;
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
for n in type_param_source_names {
bcode_map.add_enum_type_parameter_mapping(enum_def_idx, n)?;
}
Expand Down Expand Up @@ -587,7 +576,7 @@ impl<'a> Disassembler<'a> {
// perhaps surprisingly, but the location is of the whole variant
// and not just of its name
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let variant_end_offset = buffer.byte_len();
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
let variant_loc =
Loc::new(FileHash::empty(), variant_start_offset, variant_end_offset);
bcode_map.add_enum_variant_mapping(
Expand Down Expand Up @@ -643,8 +632,7 @@ impl<'a> Disassembler<'a> {
fn disassemble_function_definition(
&self,
buffer: &mut (impl Write + ByteLength),
bcode_map_gen: bool,
bcode_map: &mut SourceMap,
bcode_map_opt: &mut Option<SourceMap>,
function_definition_index: FunctionDefinitionIndex,
) -> Result<()> {
let function_definition = self
Expand Down Expand Up @@ -756,7 +744,7 @@ impl<'a> Disassembler<'a> {
let Some(code) = &function.code else {
any_writeln!(buffer, ";")?;
let fun_def_end_offset = buffer.byte_len();
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
let fun_def_loc =
Loc::new(FileHash::empty(), fun_def_start_offset, fun_def_end_offset);
self.add_function_bytecode_map(
Expand Down Expand Up @@ -786,7 +774,7 @@ impl<'a> Disassembler<'a> {
any_writeln!(buffer, "}}")?;

let fun_def_end_offset = buffer.byte_len();
if bcode_map_gen {
if let Some(bcode_map) = bcode_map_opt {
let fun_loc = Loc::new(FileHash::empty(), fun_def_start_offset, fun_def_end_offset);
self.add_function_bytecode_map(
bcode_map,
Expand Down
7 changes: 4 additions & 3 deletions external-crates/move/crates/move-package/src/compilation/compiled_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,8 @@ impl OnDiskCompiledPackage {
unit: &CompiledUnitWithSource,
) -> Result<()> {
let root_package = self.package.compiled_package_info.package_name;
let bytecode_modules_dir = CompiledPackageLayout::CompiledModules.path();
assert!(self.root_path.ends_with(root_package.as_str()));
let disassembly_dir = CompiledPackageLayout::Disassembly.path();
let file_path = if root_package == package_name {
PathBuf::new()
} else {
Expand All @@ -376,13 +377,13 @@ impl OnDiskCompiledPackage {
let d = Disassembler::from_unit(&unit.unit);
let (disassembled_string, bytecode_map) = d.disassemble_with_source_map()?;
self.save_under(
bytecode_modules_dir
disassembly_dir
.join(&file_path)
.with_extension(MOVE_BYTECODE_EXTENSION),
disassembled_string.as_bytes(),
)?;
self.save_under(
bytecode_modules_dir.join(&file_path).with_extension("json"),
disassembly_dir.join(&file_path).with_extension("json"),
serialize_to_json_string(&bytecode_map)?.as_bytes(),
)
}
Expand Down
2 changes: 2 additions & 0 deletions external-crates/move/crates/move-package/src/compilation/package_layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub enum CompiledPackageLayout {
LockFiles,
CompiledModules,
CompiledDocs,
Disassembly,
}

impl CompiledPackageLayout {
Expand All @@ -27,6 +28,7 @@ impl CompiledPackageLayout {
Self::LockFiles => "locks",
Self::CompiledModules => "bytecode_modules",
Self::CompiledDocs => "docs",
Self::Disassembly => "disassembly",
};
Path::new(path)
}
Expand Down
1 change: 1 addition & 0 deletions external-crates/move/crates/move-package/tests/test_sources/basic_no_deps/Move.compiled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ CompiledPackageInfo {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions external-crates/move/crates/move-package/tests/test_sources/basic_no_deps/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions .../move/crates/move-package/tests/test_sources/basic_no_deps_address_assigned/Move.compiled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ CompiledPackageInfo {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions .../move/crates/move-package/tests/test_sources/basic_no_deps_address_assigned/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...e/tests/test_sources/basic_no_deps_address_not_assigned_with_dev_assignment/Move.compiled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ CompiledPackageInfo {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...e/tests/test_sources/basic_no_deps_address_not_assigned_with_dev_assignment/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...-crates/move/crates/move-package/tests/test_sources/basic_no_deps_test_mode/Move.compiled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ CompiledPackageInfo {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...rnal-crates/move/crates/move-package/tests/test_sources/dep_dev_dep_diamond/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions external-crates/move/crates/move-package/tests/test_sources/dep_good_digest/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions .../crates/move-package/tests/test_sources/diamond_problem_backflow_resolution/Move.compiled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ CompiledPackageInfo {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions .../crates/move-package/tests/test_sources/diamond_problem_backflow_resolution/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...s/move-package/tests/test_sources/diamond_problem_dep_dev_override_with_reg/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...es/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...rates/move-package/tests/test_sources/diamond_problem_dep_external_override/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions .../move-package/tests/test_sources/diamond_problem_dep_external_override_root/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions .../crates/move-package/tests/test_sources/diamond_problem_dep_nested_override/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...es/move/crates/move-package/tests/test_sources/diamond_problem_dep_override/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...e-package/tests/test_sources/diamond_problem_dep_transitive_nested_override/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...es/move-package/tests/test_sources/diamond_problem_dep_two_nested_overrides/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
1 change: 1 addition & 0 deletions ...s/move/crates/move-package/tests/test_sources/diamond_problem_dep_with_deps/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ ResolvedGraph {
dev_mode: true,
test_mode: false,
generate_docs: false,
save_disassembly: false,
install_dir: Some(
"ELIDED_FOR_TEST",
),
Expand Down
Loading
Loading