address PR feedback

Veridise · Oct 30, 2023 · 08d46e8 · 08d46e8
1 parent c1ee0be
commit 08d46e8
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 17 deletions.
diff --git a/circom/tests/zzz/unreachable_code_crash.circom b/circom/tests/zzz/unreachable_code_crash.circom
@@ -1,24 +1,26 @@
 pragma circom 2.0.2;
 // REQUIRES: circom
-// RUN: rm -rf %t && mkdir %t && %circom --llvm -o %t %s
+// RUN: rm -rf %t && mkdir %t && %circom --llvm -o %t %s | sed -n 's/.*Written successfully:.* \(.*\)/\1/p' | xargs cat | FileCheck %s
 
 template OR() {
     signal input a;
 }
 
+// This test demonstrates the need for the UnusedFuncRemovalPass.
+// Here's what happens without that pass. The outer loop unrolls first, with 2 copies of its body
+//  because it has 2 iterations. In the second iteration (i.e. the second copy of the inner loop),
+//  the 'true' branch of the if-else will never execute so in the "loop.body" function generated
+//  for that second copy of the inner loop, this branch is dead code. Because it is dead code, no
+//  parameter was added to the generated function to reference the destination of the StoreBucket
+//  in that branch and therefore the location information in that StoreBucket was not updated thus
+//  leaving an invalid parameter reference that causes 'functions.rs::get_arg_ptr' to crash.
+//
 template InvalidArgIndex(n, k) {
     component has_prev_non_zero[k * n];
     for (var i = k - 1; i >= 0; i--) {
         for (var j = n - 1; j >= 0; j--) {
             has_prev_non_zero[n * i + j] = OR();
             if (i == k - 1 && j == n - 1) {
-                // StoreBucket here causes a crash in `get_arg_ptr` 
-                // Here's what happens. The outer loop unrolls first, 2 iterations. In the second
-                //  iteration, this branch of the if-else will never execute so in the generated
-                //  "loop.body" function, this branch is dead code, thus no parameter was added
-                //  to the function to reference the destination of this StoreBucket and the 
-                //  location information was not updated so there is an invalid parameter reference
-                //  that causes 'functions.rs::get_arg_ptr' to crash, but it's in dead code.
                 has_prev_non_zero[n * i + j].a <-- 99;
             } else {
                 has_prev_non_zero[n * i + j].a <-- 33;
@@ -32,10 +34,10 @@ component main = InvalidArgIndex(3, 2);
 //// Check that only the proper versions of the generated functions remain
 //// (i.e. the initial one was removed after conditional flattening).
 //
-//CHECK-NOT: define void @..generated..loop.body.[[[0-9]+]](
-//CHECK:     define void @..generated..loop.body.[[[0-9]+]].F(
-//CHECK-NOT: define void @..generated..loop.body.[[[0-9]+]](
-//CHECK:     define void @..generated..loop.body.[[[0-9]+]].T(
-//CHECK-NOT: define void @..generated..loop.body.[[[0-9]+]](
-//CHECK:     define void @..generated..loop.body.[[[0-9]+]].F.T(
-//CHECK-NOT: define void @..generated..loop.body.[[[0-9]+]](
+//CHECK-NOT: define void @..generated..loop.body.{{[0-9]+}}(
+//CHECK:     define void @..generated..loop.body.[[NAME_1:[0-9]+]].F(
+//CHECK-NOT: define void @..generated..loop.body.{{[0-9]+}}(
+//CHECK:     define void @..generated..loop.body.[[NAME_1]].T(
+//CHECK-NOT: define void @..generated..loop.body.{{[0-9]+}}(
+//CHECK:     define void @..generated..loop.body.[[NAME_2:[0-9]+]].F.T(
+//CHECK-NOT: define void @..generated..loop.body.{{[0-9]+}}(
diff --git a/circuit_passes/src/bucket_interpreter/env/extracted_func_env.rs b/circuit_passes/src/bucket_interpreter/env/extracted_func_env.rs
@@ -207,7 +207,7 @@ impl<'a> ExtractedFuncEnvData<'a> {
                             _ => unreachable!(), //ASSERT: 'cmp_address' was formed by 'loop_unroll::new_u32_value'
                         };
                         if counter_override {
-                            todo!()
+                            unreachable!() // there is no counter for a counter reference
                         } else {
                             self.base.subcmp_counter_is_zero(subcmp)
                         }
@@ -238,7 +238,7 @@ impl<'a> ExtractedFuncEnvData<'a> {
                             _ => unreachable!(), //ASSERT: 'cmp_address' was formed by 'loop_unroll::new_u32_value'
                         };
                         if counter_override {
-                            todo!()
+                            unreachable!() // there is no counter for a counter reference
                         } else {
                             self.base.subcmp_counter_equal_to(subcmp, value)
                         }