[language] Binary format changes #2718

dariorussi-zz · 2020-02-26T22:27:19Z

Here are the changes to the binary format. We have few things missing around Scripts vs Modules and around functions (native function flags and function signatures).
However this works and it's the bulk of the changes.
There is more to clean up and correct but this needs to go in asap and we can all iterate over

dariorussi-zz · 2020-02-29T22:09:07Z

added some renaming of binary format structures:
*::unrestricted -> *::copyable
type_formals -> type_arguments
return_types -> return_
arg_types -> parameters
type_formals -> type_parameters
and implications of that (function and variable names)

MIRAI-bot · 2020-02-29T22:30:16Z

Hello you!

It looks like MIRAI found some warnings:

warning: possible index out of bounds
   --> language/vm/src/file_format.rs:804:35
    |
804 |             TypeParameter(idx) => tys[*idx as usize].clone(),
    |                                   ^^^^^^^^^^^^^^^^^^

warning: possible execution panic
    --> language/vm/src/file_format.rs:1791:9
     |
1791 |         self.as_inner().kind_count(kind)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: related location
    --> language/vm/src/file_format.rs:1758:49
     |
1758 |             | other @ IndexKind::FieldOffset => unreachable!("invalid kind for count: {:?}", other),
     |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: possible assertion failed: index < len
    --> language/vm/src/file_format.rs:1812:20
     |
1812 |         let main = inner.function_defs.remove(0);
     |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: possible attempt to subtract with overflow
   --> language/vm/src/gas_schedule.rs:303:33
    |
303 |         &self.instruction_table[(instr_index - 1) as usize]
    |                                 ^^^^^^^^^^^^^^^^^

warning: possible attempt to subtract with overflow
   --> language/vm/src/gas_schedule.rs:463:33
    |
463 |     precondition!(size.get() <= MAX_ABSTRACT_MEMORY_SIZE.get() - (WORD_SIZE.get() + 1));
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: possible attempt to add with overflow
   --> language/vm/src/gas_schedule.rs:463:66
    |
463 |     precondition!(size.get() <= MAX_ABSTRACT_MEMORY_SIZE.get() - (WORD_SIZE.get() + 1));
    |                                                                  ^^^^^^^^^^^^^^^^^^^^^

warning: possible attempt to add with overflow
    --> language/vm/src/serializer.rs:43:9
     |
43   |         self.as_inner().serialize(binary)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: related location
    --> language/vm/src/serializer.rs:56:9
     |
56   |         ser.serialize(&mut temp, self)?;
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: related location
    --> language/vm/src/serializer.rs:1023:9
     |
1023 |         self.serialize_function_definitions(binary, &module.function_defs)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: related location
    --> language/vm/src/serializer.rs:1093:13
     |
1093 |             self.common.table_count += 1;
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: possible attempt to add with overflow
    --> language/vm/src/serializer.rs:56:9
     |
56   |         ser.serialize(&mut temp, self)?;
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: related location
    --> language/vm/src/serializer.rs:1023:9
     |
1023 |         self.serialize_function_definitions(binary, &module.function_defs)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: related location
    --> language/vm/src/serializer.rs:1093:13
     |
1093 |             self.common.table_count += 1;
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: assumption is provably true and can be deleted
   --> language/vm/src/file_format_common.rs:211:13
    |
211 |             assume!(self._binary.len() < usize::max_value());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: assumption is provably true and can be deleted
   --> language/vm/src/file_format_common.rs:227:13
    |
227 |             assume!(self._binary.len() <= usize::max_value() - vec_len);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: possible execution panic
    --> network/src/transport.rs:128:33
     |
128  |     let noise_config = Arc::new(NoiseConfig::new(identity_keypair));
     |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: related location
    --> /home/runner/work/mirai-bot/mirai-bot/libra/network/noise/src/lib.rs:40:39
     |
40   |         let parameters: NoiseParams = NOISE_PARAMETER.parse().expect("Invalid protocol name");
     |                                       ^^^^^^^^^^^^^^^^^^^^^^^
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:4220:9: 4220:32
note: related location
    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/snow-0.6.2/src/params/mod.rs:147:29
     |
147  | ...                   split.next().ok_or(PatternProblem::TooFewParameters)?.parse()?,
     |                       ^^^^^^^^^^^^
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:974:17: 974:30
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:1124:15: 1124:40
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/pattern.rs:126:19: 126:30
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/pattern.rs:282:23: 282:38

warning: possible execution panic
    --> network/src/transport.rs:182:33
     |
182  |     let noise_config = Arc::new(NoiseConfig::new(identity_keypair));
     |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: related location
    --> /home/runner/work/mirai-bot/mirai-bot/libra/network/noise/src/lib.rs:40:39
     |
40   |         let parameters: NoiseParams = NOISE_PARAMETER.parse().expect("Invalid protocol name");
     |                                       ^^^^^^^^^^^^^^^^^^^^^^^
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:4220:9: 4220:32
note: related location
    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/snow-0.6.2/src/params/mod.rs:147:29
     |
147  | ...                   split.next().ok_or(PatternProblem::TooFewParameters)?.parse()?,
     |                       ^^^^^^^^^^^^
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:974:17: 974:30
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:1124:15: 1124:40
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/pattern.rs:126:19: 126:30
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/pattern.rs:282:23: 282:38

warning: possible execution panic
    --> network/src/transport.rs:219:33
     |
219  |     let noise_config = Arc::new(NoiseConfig::new(identity_keypair));
     |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: related location
    --> /home/runner/work/mirai-bot/mirai-bot/libra/network/noise/src/lib.rs:40:39
     |
40   |         let parameters: NoiseParams = NOISE_PARAMETER.parse().expect("Invalid protocol name");
     |                                       ^^^^^^^^^^^^^^^^^^^^^^^
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:4220:9: 4220:32
note: related location
    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/snow-0.6.2/src/params/mod.rs:147:29
     |
147  | ...                   split.next().ok_or(PatternProblem::TooFewParameters)?.parse()?,
     |                       ^^^^^^^^^^^^
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:974:17: 974:30
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/mod.rs:1124:15: 1124:40
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/pattern.rs:126:19: 126:30
note: related location /rustc/6fd8798f4de63328d743eb2a9a9c12e202a4a182/src/libcore/str/pattern.rs:282:23: 282:38

warning: possible attempt to add with overflow
   --> language/compiler/ir-to-bytecode/syntax/src/lexer.rs:188:24
    |
188 |         self.cur_end = self.cur_start + len;
    |                        ^^^^^^^^^^^^^^^^^^^^

bors-libra · 2020-03-03T03:20:54Z

☔ The latest upstream changes (presumably 5ada39a) made this pull request unmergeable. Please resolve the merge conflicts.

dariorussi-zz · 2020-03-03T16:40:11Z

preparing for FunctionHandle change to "inline" the function signature

bors-libra · 2020-03-03T22:02:22Z

☔ The latest upstream changes (presumably 02a898d) made this pull request unmergeable. Please resolve the merge conflicts.

dariorussi-zz · 2020-03-07T03:41:06Z

FunctionSignature removed, tests should compile and some fail. Uploading meanwhile...

bors-libra · 2020-03-10T01:32:27Z

☔ The latest upstream changes (presumably cea66f4) made this pull request unmergeable. Please resolve the merge conflicts.

MIRAI-bot · 2020-04-01T15:38:27Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 8a13107 did fail. See details.

MIRAI-bot · 2020-04-01T16:11:59Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 386587d did fail. See details.

dariorussi-zz · 2020-04-01T19:17:45Z

rebased on latest - after "generic script"

MIRAI-bot · 2020-04-01T19:23:51Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 6659708 did fail. See details.

vgao1996

Haven't looked into prop tests & move prover but everything else looks good to me.

vgao1996 · 2020-03-31T19:36:56Z

language/vm/src/check_bounds.rs

-        .into_iter()
-        .flatten()
-        .collect()
+    fn check_module_handle(&self, module_handle: &ModuleHandle, errors: &mut Vec<VMStatus>) {


Great we're taking &mut Vec<VMStatus> instead of returning Vec<Status>, but I wonder if makes more sense to make the Vec<VMStatus> a field of the BoundsChecker?

(This is minor, we can deal with it later.)

it's possible and it's something to look into.
Those mutable vectors passed all around don't really help with understanding of the code and I think you are right that there may be better ways to organize this code

vgao1996 · 2020-03-31T19:42:37Z

language/vm/src/file_format.rs

+#[cfg_attr(any(test, feature = "fuzzing"), proptest(no_params))]
+pub struct StructDefInstantiation {
+    pub def: StructDefinitionIndex,
+    pub type_parameters: SignatureIndex,


I'll trust your judgement then. Could you add a few comments here just to make sure we don't forget? Thanks!

vgao1996 · 2020-04-01T18:52:43Z

language/vm/src/check_bounds.rs

+        // if there are locals check that the type parameters in local signature are in bounds.
+        let locals = match &self.module.signatures.get(code_unit.locals.into_index()) {
+            Some(locals) => &locals.0,
+            None => return, // stop now


This is unreachable since we already checked that code_unit.locals was in bound.

right and we don't want to crash because it's on entry and I changed and, in fact, crashed the test :)
There is a conversation to have about what guarantees you can rely on but it may have a lot of implications...
I hope we can talk about what guarantees should the code have and when at some point soon, but most likely not too soon

vgao1996 · 2020-04-01T19:01:05Z

language/vm/src/check_bounds.rs

+        for module_handle in &self.module.module_handles {
+            self.check_module_handle(module_handle, &mut errors);
+        }
+        for struct_handle in &self.module.struct_handles {
+            self.check_struct_handle(struct_handle, &mut errors);
+        }
+        for function_handle in &self.module.function_handles {
+            self.check_function_handle(function_handle, &mut errors);
+        }
+        for field_handle in &self.module.field_handles {
+            self.check_field_handle(field_handle, &mut errors);
+        }
+        for struct_instantiation in &self.module.struct_def_instantiations {
+            self.check_struct_instantiation(struct_instantiation, &mut errors);
+        }
+        for function_instantiation in &self.module.function_instantiations {
+            self.check_function_instantiation(function_instantiation, &mut errors);
+        }
+        for field_instantiation in &self.module.field_instantiations {
+            self.check_field_instantiation(field_instantiation, &mut errors);
+        }
+        for struct_def in &self.module.struct_defs {
+            self.check_struct_def(struct_def, &mut errors);
+        }
+        for function_def in &self.module.function_defs {
+            self.check_function_def(function_def, &mut errors);
+        }
+        errors


I like this approach! Struct indices and type parameters are checked in separate phases, which simplifies a lot of things.

It would be better if you could document what properties are checked at what time here. It was unclear to me whether everything had been covered initially.

yes I will do it and we both should do it for different parts as we "finish" them.
I'm glad that you see the simplification too, I think we could do better and plan to work on this and much more as soon as this PR lands.
At that point I plan to add a bit of documentation which is critical in terms of "specification" and testing

vgao1996 · 2020-04-01T19:16:59Z

language/bytecode-verifier/src/abstract_state.rs

@@ -22,6 +25,17 @@ pub struct TypedAbstractValue {
    pub value: AbstractValue,
 }

+impl TypedAbstractValue {
+    pub fn reference_to(&self, type_sig: &SignatureToken) -> bool {


Nit: reference_to => is_reference_to to make it consistent with other helper functions?

MIRAI-bot · 2020-04-01T19:41:11Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 69a14bb did fail. See details.

vgao1996 · 2020-04-01T19:42:57Z

@wrwg @junkil-park could you please help look at the Move Prover failure? It doesn't look like an actual error to me. Wondering if updating the baseline will fix it.

MIRAI-bot · 2020-04-01T20:01:17Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 45fe473 did fail. See details.

dariorussi-zz · 2020-04-01T20:11:17Z

@wrwg @junkil-park could you please help look at the Move Prover failure? It doesn't look like an actual error to me. Wondering if updating the baseline will fix it.

yeah I am in contact with the Move Prover side and things are all good and something they know about, thanks for pinging people though

bors-libra · 2020-04-01T20:27:14Z

☔ The latest upstream changes (presumably c2f473e) made this pull request unmergeable. Please resolve the merge conflicts.

vgao1996 · 2020-04-01T20:29:57Z

Great. I assume that implies landing won't be blocked by the move prover test failure.

vgao1996

Overall this looks good to me. Let's check this in and move forward. We'll see if there are other minor changes to make as we revisit each of the components.

Here are the changes to the binary format. We have few things missing around Scripts vs Modules and around functions (native function flags and function signatures). However this works and it's the bulk of the binary changes. There is more to clean up and correct but this needs to go in asap and we can all iterate over

MIRAI-bot · 2020-04-01T21:34:32Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 1104adf did fail. See details.

dariorussi-zz · 2020-04-01T21:53:06Z

@bors-libra r=vgao1996

bors-libra · 2020-04-01T21:53:07Z

📌 Commit 1104adf has been approved by vgao1996

bors-libra · 2020-04-01T21:53:16Z

⌛ Testing commit 1104adf with merge 58f432a...

github-actions · 2020-04-01T22:08:57Z

Cluster Test Result

all up : 1032 TPS, 1890.3 ms latency, no expired txns

Repro cmd:

./scripts/cti --k8s --tag land_58f432a4 --run bench --k8s-fullnodes-per-validator=1 --k8s-num-validators=30

bors-libra · 2020-04-01T22:09:34Z

☀️ Test successful - checks-actions_land_blocking_test, checks-circle_commit_workflow
Approved by: vgao1996
Pushing 58f432a to master...

diem-cla-bot bot added the cla-signed label Feb 26, 2020

review-assign-bot bot requested a review from gedigi February 26, 2020 22:27

dariorussi-zz requested review from vgao1996, tnowacki, shazqadeer, bob-wilson, runtian-zhou, sblackshear, sunshowers and tzakian and removed request for gedigi February 26, 2020 22:27

dariorussi-zz force-pushed the binary_format branch from 69ef2bf to 59999b1 Compare February 29, 2020 22:05

dariorussi-zz force-pushed the binary_format branch 2 times, most recently from 56a1749 to e8cea7a Compare March 3, 2020 16:39

dariorussi-zz force-pushed the binary_format branch from e8cea7a to aeeaf22 Compare March 7, 2020 03:39

dariorussi-zz force-pushed the binary_format branch 2 times, most recently from 39daa3d to 1faeb7b Compare March 7, 2020 20:00

dariorussi-zz changed the title ~~[language] Binary format changes~~ [language] Binary format changes (draft but review as needed) Mar 7, 2020

dariorussi-zz force-pushed the binary_format branch 5 times, most recently from 33fc91d to f986259 Compare March 9, 2020 02:19

dariorussi-zz force-pushed the binary_format branch from facd109 to 8a13107 Compare April 1, 2020 15:32

dariorussi-zz force-pushed the binary_format branch from 8a13107 to 386587d Compare April 1, 2020 16:05

dariorussi-zz force-pushed the binary_format branch from 386587d to 6659708 Compare April 1, 2020 19:17

dariorussi-zz force-pushed the binary_format branch from 6659708 to 69a14bb Compare April 1, 2020 19:33

vgao1996 reviewed Apr 1, 2020

View reviewed changes

vgao1996 added the breaking Any changes that will require restarting the testnet label Apr 1, 2020

dariorussi-zz force-pushed the binary_format branch from 69a14bb to 45fe473 Compare April 1, 2020 19:54

vgao1996 approved these changes Apr 1, 2020

View reviewed changes

dariorussi-zz force-pushed the binary_format branch from 45fe473 to 1104adf Compare April 1, 2020 21:25

bors-libra mentioned this pull request Apr 1, 2020

[terraform] Collect Prometheus metrics from Vault #3204

Closed

bors-libra closed this in 58f432a Apr 1, 2020

dariorussi-zz deleted the binary_format branch April 3, 2020 00:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[language] Binary format changes #2718

[language] Binary format changes #2718

dariorussi-zz commented Feb 26, 2020 •

edited

dariorussi-zz commented Feb 29, 2020

MIRAI-bot commented Feb 29, 2020

bors-libra commented Mar 3, 2020

dariorussi-zz commented Mar 3, 2020

bors-libra commented Mar 3, 2020

dariorussi-zz commented Mar 7, 2020

bors-libra commented Mar 10, 2020

MIRAI-bot commented Apr 1, 2020

MIRAI-bot commented Apr 1, 2020

dariorussi-zz commented Apr 1, 2020

MIRAI-bot commented Apr 1, 2020

vgao1996 left a comment

vgao1996 Mar 31, 2020

dariorussi-zz Apr 1, 2020

vgao1996 Mar 31, 2020

vgao1996 Apr 1, 2020

dariorussi-zz Apr 1, 2020

vgao1996 Apr 1, 2020

dariorussi-zz Apr 1, 2020

vgao1996 Apr 1, 2020

MIRAI-bot commented Apr 1, 2020

vgao1996 commented Apr 1, 2020

MIRAI-bot commented Apr 1, 2020

dariorussi-zz commented Apr 1, 2020

bors-libra commented Apr 1, 2020

vgao1996 commented Apr 1, 2020

vgao1996 left a comment

MIRAI-bot commented Apr 1, 2020

dariorussi-zz commented Apr 1, 2020

bors-libra commented Apr 1, 2020

bors-libra commented Apr 1, 2020

github-actions bot commented Apr 1, 2020

bors-libra commented Apr 1, 2020

[language] Binary format changes #2718

[language] Binary format changes #2718

Conversation

dariorussi-zz commented Feb 26, 2020 • edited

dariorussi-zz commented Feb 29, 2020

MIRAI-bot commented Feb 29, 2020

bors-libra commented Mar 3, 2020

dariorussi-zz commented Mar 3, 2020

bors-libra commented Mar 3, 2020

dariorussi-zz commented Mar 7, 2020

bors-libra commented Mar 10, 2020

MIRAI-bot commented Apr 1, 2020

MIRAI-bot commented Apr 1, 2020

dariorussi-zz commented Apr 1, 2020

MIRAI-bot commented Apr 1, 2020

vgao1996 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

MIRAI-bot commented Apr 1, 2020

vgao1996 commented Apr 1, 2020

MIRAI-bot commented Apr 1, 2020

dariorussi-zz commented Apr 1, 2020

bors-libra commented Apr 1, 2020

vgao1996 commented Apr 1, 2020

vgao1996 left a comment

Choose a reason for hiding this comment

MIRAI-bot commented Apr 1, 2020

dariorussi-zz commented Apr 1, 2020

bors-libra commented Apr 1, 2020

bors-libra commented Apr 1, 2020

github-actions bot commented Apr 1, 2020

bors-libra commented Apr 1, 2020

dariorussi-zz commented Feb 26, 2020 •

edited