Skip to content

Commit

Permalink
[compiler-v2] Do not pop non-droppables on stack flush
Browse files Browse the repository at this point in the history
Fixes #14233

On stack flush we must not pop values which cannot be dropped. Rather they should be stored in temps. This faciltates scenarios where an `abort` happens before the end of a block. Without this, we pop the value before the abort, leading to a bytecode verification error.

There is still an issue with a missing warning, added #14320 for that.
  • Loading branch information
wrwg committed Aug 19, 2024
1 parent fa28b93 commit f828325
Show file tree
Hide file tree
Showing 8 changed files with 77 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1025,8 +1025,10 @@ impl<'a> FunctionGenerator<'a> {
if before && ctx.is_alive_before(temp)
|| !before && ctx.is_alive_after(temp, &[], false)
|| self.pinned.contains(&temp)
|| !ctx.fun_ctx.is_droppable(temp)
{
// Only need to save to a local if the temp is still used afterwards
// Only need to save to a local if the temp is still used afterwards,
// or if it is not droppable
let local = self.temp_to_local(fun_ctx, Some(ctx.attr_id), temp);
self.emit(FF::Bytecode::StLoc(local));
} else {
Expand Down Expand Up @@ -1147,6 +1149,14 @@ impl<'env> FunctionContext<'env> {
.type_abilities(self.temp_type(temp), &self.type_parameters)
.has_ability(FF::Ability::Copy)
}

/// Returns true of the given temporary can/should be dropped when flushing the stack.
pub fn is_droppable(&self, temp: TempIndex) -> bool {
self.module
.env
.type_abilities(self.temp_type(temp), &self.type_parameters)
.has_ability(FF::Ability::Drop)
}
}

impl<'env> BytecodeContext<'env> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module 0xCAFE::Module0 {
struct S {}

public fun f() {
let _x = S {};
abort 1;
let S {} = _x;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

Diagnostics:
error: local `_x` of type `Module0::S` does not have the `drop` ability
┌─ tests/ability-check/bug_14233_unused_non_droppable_no_abort.move:5:18
5 │ let _x = S {};
│ ^^^^ implicitly dropped here since it is no longer used
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module 0xCAFE::Module0 {
struct S {}

public fun f() {
let _x = S {};
//abort 1;
//let S {} = _x;
}
}
10 changes: 5 additions & 5 deletions third_party/move/move-compiler-v2/tests/testsuite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,11 +283,11 @@ const TEST_CONFIGS: Lazy<BTreeMap<&str, TestConfig>> = Lazy::new(|| {
dump_bytecode: DumpLevel::None,
dump_bytecode_filter:
// For debugging (dump_bytecode set DumpLevel::AllStages)
Some(vec![
INITIAL_BYTECODE_STAGE,
"ReferenceSafetyProcessor",
"DeadStoreElimination",
FILE_FORMAT_STAGE,
Some(vec![
INITIAL_BYTECODE_STAGE,
"ReferenceSafetyProcessor",
"DeadStoreElimination",
FILE_FORMAT_STAGE,
]),
},
// Reference safety tests (with optimizations on)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
comparison between v1 and v2 failed:
- processed 2 tasks
+ processed 2 tasks
=
- task 0 'publish'. lines 1-10:
- warning[W09005]: dead or unreachable code
- ┌─ TEMPFILE:7:9
- │
- 7 │ let S {} = _x;
- │ ^^^^^^^^^^^^^ Unreachable code. This statement (and any following statements) will not be executed.
-
-
-
= task 1 'run'. lines 12-12:
= Error: Function execution failed with VMError: {
= major_status: ABORTED,
= sub_status: Some(1),
= location: 0xcafe::m,
= indices: redacted,
= offsets: redacted,
= }
=
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//# publish
module 0xCAFE::m {
struct S {}

public fun f() {
let _x = S {};
abort 1;
let S {} = _x;
}
}

//# run 0xCAFE::m::f

0 comments on commit f828325

Please sign in to comment.