Merge branch 'andriy/run-785-charge-functions-loops' into 'master'

fix: RUN-785: Set the weight of a Wasm function prologue to 1 instead of 0 This MR follows up on the `New WASM instrumentation` and partially addresses the recommendation of the security team to avoid 0-weight Wasm instructions. Note this change should not have any significant impact on the overall cycle consumption of canisters because the cost of a function call is dominated by the existing weights of the `call` instructions. Closes RUN-785 Closes RUN-785 See merge request dfinity-lab/public/ic!15592
dfinity · Oct 25, 2023 · 074f6d1 · 074f6d1
2 parents 687d783 + 488e99f
commit 074f6d1
Show file tree

Hide file tree

Showing 24 changed files with 126 additions and 92 deletions.
diff --git a/rs/embedders/src/wasm_utils/instrumentation.rs b/rs/embedders/src/wasm_utils/instrumentation.rs
@@ -1148,9 +1148,9 @@ struct InjectionPoint {
 }
 
 impl InjectionPoint {
-    fn new_static_cost(position: usize, scope: Scope) -> Self {
+    fn new_static_cost(position: usize, scope: Scope, cost: u64) -> Self {
         InjectionPoint {
-            cost_detail: InjectionPointCostDetail::StaticCost { scope, cost: 0 },
+            cost_detail: InjectionPointCostDetail::StaticCost { scope, cost },
             position,
         }
     }
@@ -1525,24 +1525,25 @@ fn injections_old(code: &[Operator]) -> Vec<InjectionPoint> {
     let mut stack = Vec::new();
     use Operator::*;
     // The function itself is a re-entrant code block.
-    let mut curr = InjectionPoint::new_static_cost(0, Scope::ReentrantBlockStart);
+    let mut curr = InjectionPoint::new_static_cost(0, Scope::ReentrantBlockStart, 0);
     for (position, i) in code.iter().enumerate() {
         curr.cost_detail.increment_cost(instruction_to_cost(i));
         match i {
             // Start of a re-entrant code block.
             Loop { .. } => {
                 stack.push(curr);
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::ReentrantBlockStart);
+                curr = InjectionPoint::new_static_cost(position + 1, Scope::ReentrantBlockStart, 0);
             }
             // Start of a non re-entrant code block.
             If { .. } | Block { .. } => {
                 stack.push(curr);
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::NonReentrantBlockStart);
+                curr =
+                    InjectionPoint::new_static_cost(position + 1, Scope::NonReentrantBlockStart, 0);
             }
             // End of a code block but still more code left.
             Else | Br { .. } | BrIf { .. } | BrTable { .. } => {
                 res.push(curr);
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd);
+                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd, 0);
             }
             // `End` signals the end of a code block. If there's nothing more on the stack, we've
             // gone through all the code.
@@ -1581,34 +1582,37 @@ fn injections_new(code: &[Operator]) -> Vec<InjectionPoint> {
     let mut res = Vec::new();
     use Operator::*;
     // The function itself is a re-entrant code block.
-    let mut curr = InjectionPoint::new_static_cost(0, Scope::ReentrantBlockStart);
+    // Start with at least one fuel being consumed because even empty
+    // functions should consume at least some fuel.
+    let mut curr = InjectionPoint::new_static_cost(0, Scope::ReentrantBlockStart, 1);
     for (position, i) in code.iter().enumerate() {
         curr.cost_detail.increment_cost(instruction_to_cost_new(i));
         match i {
             // Start of a re-entrant code block.
             Loop { .. } => {
                 res.push(curr);
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::ReentrantBlockStart);
+                curr = InjectionPoint::new_static_cost(position + 1, Scope::ReentrantBlockStart, 0);
             }
             // Start of a non re-entrant code block.
             If { .. } => {
                 res.push(curr);
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::NonReentrantBlockStart);
+                curr =
+                    InjectionPoint::new_static_cost(position + 1, Scope::NonReentrantBlockStart, 0);
             }
             // End of a code block but still more code left.
             Else | Br { .. } | BrIf { .. } | BrTable { .. } => {
                 res.push(curr);
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd);
+                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd, 0);
             }
             End => {
                 res.push(curr);
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd);
+                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd, 0);
             }
             Return | Unreachable | ReturnCall { .. } | ReturnCallIndirect { .. } => {
                 res.push(curr);
                 // This injection point will be unreachable itself (most likely empty)
                 // but we create it to keep the algorithm uniform
-                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd);
+                curr = InjectionPoint::new_static_cost(position + 1, Scope::BlockEnd, 0);
             }
             // Bulk memory instructions require injected metering __before__ the instruction
             // executes so that size arguments can be read from the stack at runtime.

diff --git a/rs/embedders/tests/instrumentation.rs b/rs/embedders/tests/instrumentation.rs
@@ -298,7 +298,8 @@ fn metering_plain() {
     assert_eq!(g[0], Global::I64(10));
 
     let instructions_used = instr_used(&mut instance);
-    assert_eq!(instructions_used, cost_a(10));
+    // Function is 1 instruction.
+    assert_eq!(instructions_used, 1 + cost_a(10));
 
     // Now run the same with insufficient instructions
     let mut instance = new_instance(&wat, instructions_used - 1);
@@ -331,7 +332,8 @@ fn metering_plain() {
 
     let instructions_used = instr_used(&mut instance);
     let cret = instruction_to_cost_new(&wasmparser::Operator::Return);
-    assert_eq!(instructions_used, cost_a(10) + cret);
+    // Function is 1 instruction.
+    assert_eq!(instructions_used, 1 + cost_a(10) + cret);
 
     // Now run the same with insufficient instructions
     let mut instance = new_instance(&wat, instructions_used - 1);
@@ -361,7 +363,8 @@ fn metering_plain() {
 
     let instructions_used = instr_used(&mut instance);
     let ctrap = instruction_to_cost_new(&wasmparser::Operator::Unreachable);
-    assert_eq!(instructions_used, cost_a(10) + ctrap);
+    // Function is 1 instruction.
+    assert_eq!(instructions_used, 1 + cost_a(10) + ctrap);
 }
 
 #[test]
@@ -388,7 +391,8 @@ fn metering_block() {
     assert_eq!(g[0], Global::I64(10));
 
     let instructions_used = instr_used(&mut instance);
-    assert_eq!(instructions_used, cost_a(10));
+    // Function is 1 instruction.
+    assert_eq!(instructions_used, 1 + cost_a(10));
 
     // another one, more complex
     let wat = format!(
@@ -431,7 +435,8 @@ fn metering_block() {
 
     let instructions_used = instr_used(&mut instance);
     let cbr = instruction_to_cost_new(&wasmparser::Operator::Br { relative_depth: 1 });
-    assert_eq!(instructions_used, cost_a(100) + cost_a(10) * 2 + cbr);
+    // Function is 1 instruction.
+    assert_eq!(instructions_used, 1 + cost_a(100) + cost_a(10) * 2 + cbr);
 
     // another one, with return
     let wat = format!(
@@ -474,7 +479,8 @@ fn metering_block() {
 
     let instructions_used = instr_used(&mut instance);
     let cret = instruction_to_cost_new(&wasmparser::Operator::Return);
-    assert_eq!(instructions_used, cost_a(100) + cost_a(10) + cret);
+    // Function is 1 instruction.
+    assert_eq!(instructions_used, 1 + cost_a(100) + cost_a(10) + cret);
 }
 
 #[test]
@@ -525,7 +531,8 @@ fn metering_if() {
     let instructions_used = instr_used(&mut instance);
     assert_eq!(
         instructions_used,
-        cost_a(5) + cost_a(20) + cost_a(30) + cc + cif
+        // Function is 1 instruction.
+        1 + cost_a(5) + cost_a(20) + cost_a(30) + cc + cif
     );
 
     let wat = format!(
@@ -570,7 +577,11 @@ fn metering_if() {
     let cret = instruction_to_cost_new(&wasmparser::Operator::Return);
 
     let instructions_used = instr_used(&mut instance);
-    assert_eq!(instructions_used, cost_a(5) + cost_a(10) + cc + cif + cret);
+    // Function is 1 instruction.
+    assert_eq!(
+        instructions_used,
+        1 + cost_a(5) + cost_a(10) + cc + cif + cret
+    );
 }
 
 #[test]
@@ -633,7 +644,8 @@ fn metering_loop() {
     let instructions_used = instr_used(&mut instance);
     assert_eq!(
         instructions_used,
-        cost_a(5) + (c_loop) * 5 + cost_a(20) + cost_a(30)
+        // Function is 1 instruction.
+        1 + cost_a(5) + (c_loop) * 5 + cost_a(20) + cost_a(30)
     );
 }
 
@@ -679,7 +691,8 @@ fn charge_for_dirty_heap() {
         .get();
 
     let instructions_used = instr_used(&mut instance);
-    assert_eq!(instructions_used, 5 * cc + cg + 2 * cs + cl + 2 * cd);
+    // Function is 1 instruction.
+    assert_eq!(instructions_used, 1 + 5 * cc + cg + 2 * cs + cl + 2 * cd);
 
     // Now run the same with insufficient instructions
     // We should still succeed (to avoid potentially failing pre-upgrades
@@ -777,7 +790,8 @@ fn run_charge_for_dirty_stable64_test(native_stable: FlagStatus) {
     // 2 dirty stable pages and one heap
     assert_eq!(
         instructions_used,
-        cdrop + ccall * 4 + csg + cc * 15 + cs * 2 + cd * 3 + csw * 2 + csr + cl + cg
+        // Function is 1 instruction.
+        1 + cdrop + ccall * 4 + csg + cc * 15 + cs * 2 + cd * 3 + csw * 2 + csr + cl + cg
     );
 
     // Now run the same with insufficient instructions
@@ -887,7 +901,8 @@ fn run_charge_for_dirty_stable_test(native_stable: FlagStatus) {
     // 2 dirty stable pages and one heap
     assert_eq!(
         instructions_used,
-        cdrop + ccall * 4 + csg + cc * 15 + cs * 2 + cd * 3 + csw * 2 + csr + cl + cg
+        // Function is 1 instruction.
+        1 + cdrop + ccall * 4 + csg + cc * 15 + cs * 2 + cd * 3 + csw * 2 + csr + cl + cg
     );
 
     // Now run the same with insufficient instructions
@@ -933,7 +948,8 @@ fn test_metering_for_table_fill() {
     let instructions_used = instr_used(&mut instance);
     assert_eq!(
         instructions_used,
-        param1 + param2 + param3 + table_fill + dynamic_cost_table_fill
+        // Function is 1 instruction.
+        1 + param1 + param2 + param3 + table_fill + dynamic_cost_table_fill
     );
 
     let mut instance = new_instance(wat, instructions_used);

diff --git a/rs/embedders/tests/snapshots/instrumentation__app.snap b/rs/embedders/tests/snapshots/instrumentation__app.snap
@@ -1,6 +1,6 @@
 ---
 source: rs/embedders/tests/instrumentation.rs
-assertion_line: 54
+assertion_line: 55
 expression: out
 ---
 (module
@@ -20,7 +20,7 @@ expression: out
   (func (;5;) (type 0) (param i32) (result i32)
     (local i32 i32)
     global.get 0
-    i64.const 18
+    i64.const 19
     i64.sub
     global.set 0
     global.get 0
@@ -111,7 +111,7 @@ expression: out
   )
   (func (;6;) (type 0) (param i32) (result i32)
     global.get 0
-    i64.const 3
+    i64.const 4
     i64.sub
     global.set 0
     global.get 0

diff --git a/rs/embedders/tests/snapshots/instrumentation__app2.snap b/rs/embedders/tests/snapshots/instrumentation__app2.snap
@@ -1,6 +1,6 @@
 ---
 source: rs/embedders/tests/instrumentation.rs
-assertion_line: 54
+assertion_line: 55
 expression: out
 ---
 (module
@@ -19,7 +19,7 @@ expression: out
   (func (;5;) (type 0) (param i64) (result i64)
     (local i64)
     global.get 0
-    i64.const 15
+    i64.const 16
     i64.sub
     global.set 0
     global.get 0
@@ -104,7 +104,7 @@ expression: out
   )
   (func (;6;) (type 0) (param i64) (result i64)
     global.get 0
-    i64.const 3
+    i64.const 4
     i64.sub
     global.set 0
     global.get 0
@@ -119,7 +119,7 @@ expression: out
   )
   (func (;7;) (type 0) (param i64) (result i64)
     global.get 0
-    i64.const 3
+    i64.const 4
     i64.sub
     global.set 0
     global.get 0

diff --git a/rs/embedders/tests/snapshots/instrumentation__basic.snap b/rs/embedders/tests/snapshots/instrumentation__basic.snap
@@ -1,6 +1,6 @@
 ---
 source: rs/embedders/tests/instrumentation.rs
-assertion_line: 54
+assertion_line: 55
 expression: out
 ---
 (module
@@ -19,7 +19,7 @@ expression: out
   (import "__" "stable_read_first_access" (func (;4;) (type 5)))
   (func (;5;) (type 0) (param i32 i32) (result i32)
     global.get 0
-    i64.const 3
+    i64.const 4
     i64.sub
     global.set 0
     global.get 0

diff --git a/rs/embedders/tests/snapshots/instrumentation__basic_import.snap b/rs/embedders/tests/snapshots/instrumentation__basic_import.snap
@@ -1,6 +1,6 @@
 ---
 source: rs/embedders/tests/instrumentation.rs
-assertion_line: 54
+assertion_line: 55
 expression: out
 ---
 (module
@@ -21,7 +21,7 @@ expression: out
   (import "ic0" "msg_arg_data_size" (func (;5;) (type $b)))
   (func (;6;) (type $a) (param i32 i32) (result i32)
     global.get 0
-    i64.const 3
+    i64.const 4
     i64.sub
     global.set 0
     global.get 0

diff --git a/rs/embedders/tests/snapshots/instrumentation__basic_import_call.snap b/rs/embedders/tests/snapshots/instrumentation__basic_import_call.snap
@@ -1,6 +1,6 @@
 ---
 source: rs/embedders/tests/instrumentation.rs
-assertion_line: 54
+assertion_line: 55
 expression: out
 ---
 (module
@@ -20,7 +20,7 @@ expression: out
   (import "ic0" "msg_cycles_accept" (func (;5;) (type $b)))
   (func (;6;) (type $a) (param i32 i32) (result i32)
     global.get 0
-    i64.const 10
+    i64.const 11
     i64.sub
     global.set 0
     global.get 0

diff --git a/rs/embedders/tests/snapshots/instrumentation__control_flow.snap b/rs/embedders/tests/snapshots/instrumentation__control_flow.snap
@@ -1,6 +1,6 @@
 ---
 source: rs/embedders/tests/instrumentation.rs
-assertion_line: 54
+assertion_line: 55
 expression: out
 ---
 (module
@@ -21,7 +21,7 @@ expression: out
   (func (;6;) (type 1)
     (local i64)
     global.get 0
-    i64.const 2
+    i64.const 3
     i64.sub
     global.set 0
     global.get 0
@@ -65,7 +65,7 @@ expression: out
   (func (;7;) (type 0) (param i64)
     (local i64)
     global.get 0
-    i64.const 2
+    i64.const 3
     i64.sub
     global.set 0
     global.get 0
@@ -101,7 +101,7 @@ expression: out
   )
   (func (;8;) (type 0) (param i64)
     global.get 0
-    i64.const 5
+    i64.const 6
     i64.sub
     global.set 0
     global.get 0