-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) #15387
base: main
Are you sure you want to change the base?
Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) #15387
Changes from all commits
e9a5231
22a0bdb
bfc73ac
d3cd457
fd9e1e9
e8107d7
be210b7
76b3042
4f10a35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -546,12 +546,12 @@ impl<'a> BoundsChecker<'a> { | |
)?; | ||
} | ||
}, | ||
Call(idx) => self.check_code_unit_bounds_impl( | ||
Call(idx) | LdFunction(idx) => self.check_code_unit_bounds_impl( | ||
self.view.function_handles(), | ||
*idx, | ||
bytecode_offset, | ||
)?, | ||
CallGeneric(idx) => { | ||
CallGeneric(idx) | LdFunctionGeneric(idx) => { | ||
self.check_code_unit_bounds_impl( | ||
self.view.function_instantiations(), | ||
*idx, | ||
|
@@ -657,7 +657,9 @@ impl<'a> BoundsChecker<'a> { | |
| VecPushBack(idx) | ||
| VecPopBack(idx) | ||
| VecUnpack(idx, _) | ||
| VecSwap(idx) => { | ||
| VecSwap(idx) | ||
| InvokeFunction(idx) | ||
| EarlyBindFunction(idx, _) => { | ||
self.check_code_unit_bounds_impl( | ||
self.view.signatures(), | ||
*idx, | ||
|
@@ -683,8 +685,20 @@ impl<'a> BoundsChecker<'a> { | |
|
||
for ty in ty.preorder_traversal() { | ||
match ty { | ||
Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address | Signer | TypeParameter(_) | ||
| Reference(_) | MutableReference(_) | Vector(_) => (), | ||
Bool | ||
| U8 | ||
| U16 | ||
| U32 | ||
| U64 | ||
| U128 | ||
| U256 | ||
| Address | ||
| Signer | ||
| TypeParameter(_) | ||
| Reference(_) | ||
| MutableReference(_) | ||
| Vector(_) | ||
| Function { .. } => (), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be |
||
Struct(idx) => { | ||
check_bounds_impl(self.view.struct_handles(), *idx)?; | ||
if let Some(sh) = self.view.struct_handles().get(idx.into_index()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,8 +67,20 @@ impl<'a> BinaryComplexityMeter<'a> { | |
cost = cost.saturating_add(struct_name.len() as u64 * COST_PER_IDENT_BYTE); | ||
cost = cost.saturating_add(moduel_name.len() as u64 * COST_PER_IDENT_BYTE); | ||
}, | ||
U8 | U16 | U32 | U64 | U128 | U256 | Signer | Address | Bool | Vector(_) | ||
| TypeParameter(_) | Reference(_) | MutableReference(_) => (), | ||
U8 | ||
| U16 | ||
| U32 | ||
| U64 | ||
| U128 | ||
| U256 | ||
| Signer | ||
| Address | ||
| Bool | ||
| Vector(_) | ||
| Function { .. } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| TypeParameter(_) | ||
| Reference(_) | ||
| MutableReference(_) => (), | ||
} | ||
} | ||
|
||
|
@@ -262,7 +274,7 @@ impl<'a> BinaryComplexityMeter<'a> { | |
|
||
for instr in &code.code { | ||
match instr { | ||
CallGeneric(idx) => { | ||
CallGeneric(idx) | LdFunctionGeneric(idx, ..) => { | ||
self.meter_function_instantiation(*idx)?; | ||
}, | ||
PackGeneric(idx) | UnpackGeneric(idx) => { | ||
|
@@ -291,7 +303,9 @@ impl<'a> BinaryComplexityMeter<'a> { | |
| VecPushBack(idx) | ||
| VecPopBack(idx) | ||
| VecUnpack(idx, _) | ||
| VecSwap(idx) => { | ||
| VecSwap(idx) | ||
| InvokeFunction(idx) | ||
| EarlyBindFunction(idx, _) => { | ||
self.meter_signature(*idx)?; | ||
}, | ||
|
||
|
@@ -323,6 +337,7 @@ impl<'a> BinaryComplexityMeter<'a> { | |
| PackVariant(_) | ||
| UnpackVariant(_) | ||
| TestVariant(_) | ||
| LdFunction(_) | ||
| ReadRef | ||
| WriteRef | ||
| FreezeRef | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,10 @@ fn sig_to_ty(sig: &SignatureToken) -> Option<MoveTypeLayout> { | |
SignatureToken::U128 => Some(MoveTypeLayout::U128), | ||
SignatureToken::U256 => Some(MoveTypeLayout::U256), | ||
SignatureToken::Vector(v) => Some(MoveTypeLayout::Vector(Box::new(sig_to_ty(v.as_ref())?))), | ||
SignatureToken::Function(..) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you return |
||
// TODO(LAMBDA): do we need representation in MoveTypeLayout? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need it here? It's a good question however. MoveTypeLayout is used for ser/de in the VM so it would be dependent on our implementation there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not need it here (function values cannot be constants), but we do need it otherwise. I have a PR adding it: #15442 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible that we could allow function values as constants in the future. But yeah, maybe not worth it now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, for structs it is also possible but we did not do it yet, so None is a way to go :) |
||
None | ||
}, | ||
SignatureToken::Reference(_) | ||
| SignatureToken::MutableReference(_) | ||
| SignatureToken::Struct(_) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,12 @@ impl Table { | |
} | ||
} | ||
|
||
fn read_u8_internal(cursor: &mut VersionedCursor) -> BinaryLoaderResult<u8> { | ||
cursor.read_u8().map_err(|_| { | ||
PartialVMError::new(StatusCode::MALFORMED).with_message("Unexpected EOF".to_string()) | ||
}) | ||
} | ||
|
||
fn read_u16_internal(cursor: &mut VersionedCursor) -> BinaryLoaderResult<u16> { | ||
let mut u16_bytes = [0; 2]; | ||
cursor | ||
|
@@ -1131,6 +1137,13 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult<Sign | |
arity: usize, | ||
ty_args: Vec<SignatureToken>, | ||
}, | ||
Function { | ||
args_arity: u64, | ||
res_arity: u64, | ||
args: Vec<SignatureToken>, | ||
results: Vec<SignatureToken>, | ||
abilities: AbilitySet, | ||
}, | ||
} | ||
|
||
impl TypeBuilder { | ||
|
@@ -1157,6 +1170,37 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult<Sign | |
} | ||
} | ||
}, | ||
T::Function { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not familiar with part of code, but don't we need to make sure we are V8 bytecode to even see a function? I think |
||
args_arity, | ||
res_arity, | ||
mut args, | ||
mut results, | ||
abilities, | ||
} => { | ||
if args.len() >= args_arity as usize { | ||
results.push(tok); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can results be empty? If res_arity is 0, this seems to push a token nevertheless? |
||
if results.len() >= res_arity as usize { | ||
T::Saturated(SignatureToken::Function(args, results, abilities)) | ||
} else { | ||
T::Function { | ||
args_arity, | ||
res_arity, | ||
args, | ||
results, | ||
abilities, | ||
} | ||
} | ||
} else { | ||
args.push(tok); | ||
T::Function { | ||
args_arity, | ||
res_arity, | ||
args, | ||
results, | ||
abilities, | ||
} | ||
} | ||
}, | ||
_ => unreachable!("invalid type constructor application"), | ||
} | ||
} | ||
|
@@ -1223,6 +1267,19 @@ fn load_signature_token(cursor: &mut VersionedCursor) -> BinaryLoaderResult<Sign | |
let idx = load_type_parameter_index(cursor)?; | ||
T::Saturated(SignatureToken::TypeParameter(idx)) | ||
}, | ||
S::FUNCTION => { | ||
let args_arity = load_signature_size(cursor)?; | ||
let res_arity = load_signature_size(cursor)?; | ||
let abilities = | ||
load_ability_set(cursor, AbilitySetPosition::FunctionValueType)?; | ||
T::Function { | ||
args_arity, | ||
res_arity, | ||
abilities, | ||
args: vec![], | ||
results: vec![], | ||
} | ||
}, | ||
}) | ||
} else { | ||
Err(PartialVMError::new(StatusCode::MALFORMED) | ||
|
@@ -1257,6 +1314,7 @@ enum AbilitySetPosition { | |
FunctionTypeParameters, | ||
StructTypeParameters, | ||
StructHandle, | ||
FunctionValueType, | ||
} | ||
|
||
fn load_ability_set( | ||
|
@@ -1306,11 +1364,17 @@ fn load_ability_set( | |
DeprecatedKind::RESOURCE => AbilitySet::EMPTY | Ability::Key, | ||
}; | ||
Ok(match pos { | ||
AbilitySetPosition::StructHandle => unreachable!(), | ||
AbilitySetPosition::StructHandle | AbilitySetPosition::FunctionValueType => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why these are unreachable? Adding a message would help. |
||
unreachable!() | ||
}, | ||
AbilitySetPosition::FunctionTypeParameters => set | Ability::Store, | ||
AbilitySetPosition::StructTypeParameters => set, | ||
}) | ||
}, | ||
AbilitySetPosition::FunctionValueType => { | ||
// This is a new type, shouldn't show up here. | ||
Err(PartialVMError::new(StatusCode::UNKNOWN_ABILITY)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But above we use unreachable? Should we use an error for safety there as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a message? e.g., "Unexpected function value type abilities" so we can debug? If this is unreachable, maybe invariant violation status is more appropriate? |
||
}, | ||
} | ||
} else { | ||
// The uleb here doesn't really do anything as it is bounded currently to 0xF, but the | ||
|
@@ -1814,6 +1878,16 @@ fn load_code(cursor: &mut VersionedCursor, code: &mut Vec<Bytecode>) -> BinaryLo | |
Opcodes::CAST_U16 => Bytecode::CastU16, | ||
Opcodes::CAST_U32 => Bytecode::CastU32, | ||
Opcodes::CAST_U256 => Bytecode::CastU256, | ||
|
||
Opcodes::LD_FUNCTION => Bytecode::LdFunction(load_function_handle_index(cursor)?), | ||
Opcodes::LD_FUNCTION_GENERIC => { | ||
Bytecode::LdFunctionGeneric(load_function_inst_index(cursor)?) | ||
}, | ||
Opcodes::INVOKE_FUNCTION => Bytecode::InvokeFunction(load_signature_index(cursor)?), | ||
Opcodes::EARLY_BIND_FUNCTION => Bytecode::EarlyBindFunction( | ||
load_signature_index(cursor)?, | ||
read_u8_internal(cursor)?, | ||
), | ||
}; | ||
code.push(bytecode); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename this
ty
totok
? It is annoying that we have tags, tokens and types and sometimes even layouts all as types....