refactor: fixed review findings

Author: Arnaud Riess

crates/herkos-tests/data/wat/data_segments.wat

@@ -0,0 +1,21 @@
+(module
+  (memory 1 2)
+
+  ;; Active segment 1: four known bytes at offset 0
+  (data (i32.const 0) "\01\02\03\04")
+
+  ;; Active segment 2: four known bytes at a different offset
+  (data (i32.const 16) "\0A\0B\0C\0D")
+
+  ;; Passive segment: not copied to memory at startup.
+  ;; Exercised here to verify the transpiler skips it without error.
+  ;; TODO: later, add a test that explicitly loads this segment to verify it is present and correct.
+  (data "passive bytes that are never loaded")
+
+  (func (export "load_i32") (param i32) (result i32)
+    local.get 0
+    i32.load)
+
+  (func (export "load_byte") (param i32) (result i32)
+    local.get 0
+    i32.load8_u))

crates/herkos-tests/tests/module_wrapper.rs

@@ -5,7 +5,7 @@
 //! 2. The new() constructor initializes globals and data segments
 //! 3. Exported functions work correctly as methods on WasmModule
 
-use herkos_tests::{const_global, counter, hello_data};
+use herkos_tests::{const_global, counter, data_segments, hello_data};
 
 // ── Counter: mutable global, no memory ──
 
@@ -71,6 +71,46 @@ fn test_hello_data_second_byte() {
     assert_eq!(word, expected, "bytes 1..5 of 'Hello' as LE i32");
 }
 
+// ── Data segments: active segments initialised, passive segment skipped ──
+
+#[test]
+fn test_data_segments_active_init() {
+    let mut module = data_segments::new().unwrap();
+
+    // Active segment 1: bytes [1, 2, 3, 4] at offset 0
+    let word = module.load_i32(0).unwrap();
+    assert_eq!(word, i32::from_le_bytes([1, 2, 3, 4]));
+}
+
+#[test]
+fn test_data_segments_second_active_init() {
+    let mut module = data_segments::new().unwrap();
+
+    // Active segment 2: bytes [10, 11, 12, 13] at offset 16
+    let word = module.load_i32(16).unwrap();
+    assert_eq!(word, i32::from_le_bytes([10, 11, 12, 13]));
+}
+
+#[test]
+fn test_data_segments_byte_access() {
+    let mut module = data_segments::new().unwrap();
+
+    // Individual bytes from the first active segment
+    assert_eq!(module.load_byte(0).unwrap(), 1);
+    assert_eq!(module.load_byte(1).unwrap(), 2);
+    assert_eq!(module.load_byte(2).unwrap(), 3);
+    assert_eq!(module.load_byte(3).unwrap(), 4);
+}
+
+#[test]
+fn test_data_segments_passive_does_not_crash() {
+    // Instantiating the module must succeed even though a passive data
+    // segment is present (it is silently skipped by the transpiler).
+    let _module = data_segments::new().unwrap();
+
+    // TODO: we will need a future test that checks that passive segments can be initialized via `memory.init` once we support that instruction. For now we just want to verify that the presence of a passive segment doesn't cause a constructor failure.
+}
+
 // ── Const global: immutable global as const item ──
 
 #[test]

crates/herkos/src/backend/mod.rs

@@ -7,6 +7,7 @@ mod safe;
 pub use safe::SafeBackend;
 
 use crate::ir::*;
+use anyhow::Result;
 
 /// Code generation backend trait.
 ///
@@ -33,7 +34,7 @@ pub trait Backend {
         offset: u32,
         width: MemoryAccessWidth,
         sign: Option<SignExtension>,
-    ) -> String;
+    ) -> Result<String>;
 
     /// Emit Rust code for a memory store (full or sub-width).
     fn emit_store(
@@ -43,7 +44,7 @@ pub trait Backend {
         value: VarId,
         offset: u32,
         width: MemoryAccessWidth,
-    ) -> String;
+    ) -> Result<String>;
 
     /// Emit Rust code for a function call (local function).
     fn emit_call(

crates/herkos/src/backend/safe.rs

@@ -363,7 +363,7 @@ impl Backend for SafeBackend {
         offset: u32,
         width: MemoryAccessWidth,
         sign: Option<SignExtension>,
-    ) -> String {
+    ) -> anyhow::Result<String> {
         let addr_expr = if offset > 0 {
             format!("({addr} as usize).wrapping_add({offset} as usize)")
         } else {
@@ -425,12 +425,11 @@ impl Backend for SafeBackend {
                 format!("memory.load_i32({addr_expr})? as u32 as i64")
             }
 
-            // Invalid combinations (shouldn't occur from valid Wasm)
-            // TODO: return an error instead of silently ignoring invalid stores?
-            _ => "0 /* unsupported load width */".to_string(),
+            // Invalid combinations (shouldn't occur in valid Wasm)
+            _ => anyhow::bail!("unsupported load: {ty:?} width={width:?} sign={sign:?}"),
         };
 
-        format!("                {dest} = {load_expr};")
+        Ok(format!("                {dest} = {load_expr};"))
     }
 
     fn emit_store(
@@ -440,7 +439,7 @@ impl Backend for SafeBackend {
         value: VarId,
         offset: u32,
         width: MemoryAccessWidth,
-    ) -> String {
+    ) -> anyhow::Result<String> {
         let addr_expr = if offset > 0 {
             format!("({addr} as usize).wrapping_add({offset} as usize)")
         } else {
@@ -477,12 +476,11 @@ impl Backend for SafeBackend {
                 format!("memory.store_i32({addr_expr}, {value} as i32)?")
             }
 
-            // Invalid combinations
-            // TODO: return an error instead of silently ignoring invalid stores?
-            _ => "() /* unsupported store width */".to_string(),
+            // Invalid combinations (shouldn't occur in valid Wasm)
+            _ => anyhow::bail!("unsupported store: {ty:?} width={width:?}"),
         };
 
-        format!("                {store_call};")
+        Ok(format!("                {store_call};"))
     }
 
     fn emit_call(

crates/herkos/src/codegen/constructor.rs

@@ -8,6 +8,7 @@
 
 use crate::backend::Backend;
 use crate::ir::*;
+use anyhow::Result;
 
 /// Emit preamble for generated Rust files.
 pub fn rust_code_preamble() -> String {
@@ -35,34 +36,58 @@ pub fn emit_const_globals<B: Backend>(_backend: &B, info: &ModuleInfo) -> String
 
 /// Generate element segment initialization code for a table.
 ///
-/// `table_receiver` is the expression to access the table (e.g., "module.table" or "table").
-pub fn emit_element_segments(info: &ModuleInfo, table_receiver: &str) -> String {
+/// Element segments are declared in the Wasm binary's `element` section. Each
+/// segment specifies a base offset into the table and a list of function
+/// references to write into consecutive slots starting at that offset. This
+/// function emits the Rust `table.set(...)` calls that perform those writes
+/// inside the generated module's `new()` constructor.
+pub fn emit_element_segments(info: &ModuleInfo, table_receiver: &str) -> Result<String> {
     let mut code = String::new();
+
+    // A Wasm module may declare multiple element segments, each covering a
+    // contiguous slice of table slots at a different base offset.
     for seg in &info.element_segments {
+        // `seg.offset` is the base table index for this segment.
+        // `seg.func_indices` lists the local function indices to place into
+        // consecutive slots: slot (offset+0), (offset+1), ...
+        // All indices are already in the local index space (imports subtracted).
         for (i, local_func_idx) in seg.func_indices.iter().enumerate() {
+            // Absolute table slot = segment base + position within segment.
             let table_idx = seg.offset + i;
+
+            // Each table entry records the function's canonical type index so
+            // that `call_indirect` can validate the expected signature at the
+            // call site before dispatching. We fetch it from the IR function.
             let type_idx = info
                 .ir_function(*local_func_idx)
                 .map(|f| f.type_idx.as_usize())
-                .unwrap_or(0);
+                .ok_or(anyhow::anyhow!("Invalid function index"))?;
+
+            // Emit one table initialisation statement per slot.
+            //
+            // TODO: `.unwrap()` panics if `table_idx` exceeds the table's
+            //       allocated size. The table is sized from the same Wasm module
+            //       so this should be unreachable, but it is not formally proven.
+            //       Generated code should use `?` and propagate a
+            //       `ConstructionError` to stay panic-free (required by no_std).
             code.push_str(&format!(
                 "    {}.set({}, Some(FuncRef {{ type_index: {}, func_index: {} }})).unwrap();\n",
-                table_receiver,
-                table_idx,
-                type_idx,
-                local_func_idx.as_usize()
+                table_receiver,            // "module.table" or "table"
+                table_idx,                 // absolute slot in the table
+                type_idx,                  // for type-checking on call_indirect
+                local_func_idx.as_usize()  // which function to dispatch to
             ));
         }
     }
-    code
+    Ok(code)
 }
 
 /// Generate the `pub fn new() -> WasmModule` or `pub fn new() -> WasmResult<WasmModule>` constructor.
 pub fn generate_constructor<B: Backend>(
     _backend: &B,
     info: &ModuleInfo,
     has_mut_globals: bool,
-) -> String {
+) -> Result<String> {
     let mut code = String::new();
 
     // Simple constructor for modules with no initialization
@@ -71,10 +96,10 @@ pub fn generate_constructor<B: Backend>(
         && info.data_segments.is_empty()
         && info.element_segments.is_empty()
     {
-        code.push_str("pub fn new() -> Result<WasmModule, herkos_runtime::ConstructionError> {\n");
+        code.push_str("pub fn new() -> Result<WasmModule, ConstructionError> {\n");
         code.push_str("    Ok(WasmModule(LibraryModule::new((), Table::try_new(0)?)))\n");
         code.push_str("}\n");
-        return code;
+        return Ok(code);
     }
 
     code.push_str("pub fn new() -> WasmResult<WasmModule> {\n");
@@ -130,13 +155,13 @@ pub fn generate_constructor<B: Backend>(
         }
 
         // Element segment initialization
-        code.push_str(&emit_element_segments(info, "module.table"));
+        code.push_str(&emit_element_segments(info, "module.table")?);
 
         code.push_str("    Ok(WasmModule(module))\n");
     } else if !info.element_segments.is_empty() {
         // Need mutable table for element initialization
         code.push_str(&format!("    let mut table = {};\n", table_init));
-        code.push_str(&emit_element_segments(info, "table"));
+        code.push_str(&emit_element_segments(info, "table")?);
         code.push_str(&format!(
             "    Ok(WasmModule(LibraryModule::new({}, table)))\n",
             globals_init
@@ -149,5 +174,5 @@ pub fn generate_constructor<B: Backend>(
     }
 
     code.push_str("}\n");
-    code
+    Ok(code)
 }

crates/herkos/src/codegen/function.rs

@@ -163,7 +163,7 @@ pub fn generate_function_with_info<B: Backend>(
         let block = &ir_func.blocks[0];
         for instr in &block.instructions {
             let code =
-                crate::codegen::instruction::generate_instruction_with_info(backend, instr, info);
+                crate::codegen::instruction::generate_instruction_with_info(backend, instr, info)?;
             output.push_str(&code);
             output.push('\n');
         }
@@ -196,7 +196,7 @@ pub fn generate_function_with_info<B: Backend>(
             for instr in &block.instructions {
                 let code = crate::codegen::instruction::generate_instruction_with_info(
                     backend, instr, info,
-                );
+                )?;
                 output.push_str(&code);
                 output.push('\n');
             }

crates/herkos/src/codegen/instruction.rs

@@ -5,15 +5,16 @@
 
 use crate::backend::Backend;
 use crate::ir::*;
+use anyhow::Result;
 use std::collections::HashMap;
 
 /// Generate code for a single instruction with module info.
 pub fn generate_instruction_with_info<B: Backend>(
     backend: &B,
     instr: &IrInstr,
     info: &ModuleInfo,
-) -> String {
-    match instr {
+) -> Result<String> {
+    let code = match instr {
         IrInstr::Const { dest, value } => backend.emit_const(*dest, value),
 
         IrInstr::BinOp { dest, op, lhs, rhs } => backend.emit_binop(*dest, *op, *lhs, *rhs),
@@ -27,15 +28,15 @@ pub fn generate_instruction_with_info<B: Backend>(
             offset,
             width,
             sign,
-        } => backend.emit_load(*dest, *ty, *addr, *offset, *width, *sign),
+        } => return backend.emit_load(*dest, *ty, *addr, *offset, *width, *sign),
 
         IrInstr::Store {
             ty,
             addr,
             value,
             offset,
             width,
-        } => backend.emit_store(*ty, *addr, *value, *offset, *width),
+        } => return backend.emit_store(*ty, *addr, *value, *offset, *width),
 
         IrInstr::Call {
             dest,
@@ -100,7 +101,8 @@ pub fn generate_instruction_with_info<B: Backend>(
             val2,
             condition,
         } => backend.emit_select(*dest, *val1, *val2, *condition),
-    }
+    };
+    Ok(code)
 }
 
 /// Generate code for a terminator with BlockId to index mapping.