Fix Cranelift frem codegen and Grammar2 parse stack overflow

darmie · darmie · commit 978a8dc3a4df · 2026-02-28T07:32:50.000Z
diff --git a/crates/compiler/src/cranelift_backend.rs b/crates/compiler/src/cranelift_backend.rs
@@ -1173,8 +1173,7 @@ impl CraneliftBackend {
                                 BinaryOp::FMul => builder.ins().fmul(lhs, rhs),
                                 BinaryOp::FDiv => builder.ins().fdiv(lhs, rhs),
                                 BinaryOp::FRem => {
-                                    // TODO: Re-implement fmod import in multi-block context
-                                    builder.ins().f64const(0.0)
+                                    Self::call_libm_fmod(&mut self.module, &mut builder, lhs, rhs)?
                                 }
                                 // Float comparisons also always return bool (i8)
                                 BinaryOp::FEq
@@ -4148,27 +4147,49 @@ impl CraneliftBackend {
         }
     }
 
-    /// Call libm fmod function for float remainder operation
-    fn call_libm_fmod(
-        &mut self,
+    /// Call libm fmod function for float remainder operation.
+    /// Supports scalar f32 (fmodf) and f64 (fmod); vector frem is rejected.
+    fn call_libm_fmod<M: Module>(
+        module: &mut M,
         builder: &mut FunctionBuilder,
         lhs: Value,
         rhs: Value,
     ) -> CompilerResult<Value> {
-        // Declare fmod signature: double fmod(double x, double y)
-        let mut sig = self.module.make_signature();
-        sig.params.push(AbiParam::new(types::F64));
-        sig.params.push(AbiParam::new(types::F64));
-        sig.returns.push(AbiParam::new(types::F64));
+        let lhs_ty = builder.func.dfg.value_type(lhs);
+        let rhs_ty = builder.func.dfg.value_type(rhs);
+        if lhs_ty != rhs_ty {
+            return Err(CompilerError::CodeGen(format!(
+                "frem operand type mismatch: lhs={:?}, rhs={:?}",
+                lhs_ty, rhs_ty
+            )));
+        }
 
-        // Declare fmod as an external function
-        let fmod_id = self
-            .module
-            .declare_function("fmod", Linkage::Import, &sig)
-            .map_err(|e| CompilerError::Backend(format!("Failed to declare fmod: {}", e)))?;
+        let (symbol_name, param_ty) = match lhs_ty {
+            types::F64 => ("fmod", types::F64),
+            types::F32 => ("fmodf", types::F32),
+            other => {
+                return Err(CompilerError::CodeGen(format!(
+                    "frem currently supports scalar f32/f64 only, got {:?}",
+                    other
+                )));
+            }
+        };
+
+        // Declare fmod signature for the selected scalar float width.
+        let mut sig = module.make_signature();
+        sig.params.push(AbiParam::new(param_ty));
+        sig.params.push(AbiParam::new(param_ty));
+        sig.returns.push(AbiParam::new(param_ty));
+
+        // Declare fmod/fmodf as an external function.
+        let fmod_id = module
+            .declare_function(symbol_name, Linkage::Import, &sig)
+            .map_err(|e| {
+                CompilerError::Backend(format!("Failed to declare {}: {}", symbol_name, e))
+            })?;
 
         // Import the function into the current function
-        let fmod_func = self.module.declare_func_in_func(fmod_id, builder.func);
+        let fmod_func = module.declare_func_in_func(fmod_id, builder.func);
 
         // Call fmod
         let call = builder.ins().call(fmod_func, &[lhs, rhs]);
@@ -4467,7 +4488,7 @@ impl CraneliftBackend {
                     BinaryOp::FMul => builder.ins().fmul(lhs, rhs),
                     BinaryOp::FDiv => builder.ins().fdiv(lhs, rhs),
                     // Float remainder requires libm fmod function call
-                    BinaryOp::FRem => self.call_libm_fmod(builder, lhs, rhs)?,
+                    BinaryOp::FRem => Self::call_libm_fmod(&mut self.module, builder, lhs, rhs)?,
 
                     // Bitwise
                     BinaryOp::And => builder.ins().band(lhs, rhs),
@@ -4618,7 +4639,7 @@ impl CraneliftBackend {
                     BinaryOp::FDiv => builder.ins().fdiv(lhs, rhs),
                     BinaryOp::FRem => {
                         // Cranelift doesn't have frem, use libm fmod
-                        self.call_libm_fmod(builder, lhs, rhs)?
+                        Self::call_libm_fmod(&mut self.module, builder, lhs, rhs)?
                     }
                     // Floating point comparisons - always return i8 (bool), never uextend
                     BinaryOp::FEq => builder.ins().fcmp(FloatCC::Equal, lhs, rhs),
@@ -6079,7 +6100,7 @@ impl CraneliftBackend {
                     BinaryOp::FSub => builder.ins().fsub(lhs, rhs),
                     BinaryOp::FMul => builder.ins().fmul(lhs, rhs),
                     BinaryOp::FDiv => builder.ins().fdiv(lhs, rhs),
-                    BinaryOp::FRem => self.call_libm_fmod(builder, lhs, rhs)?,
+                    BinaryOp::FRem => Self::call_libm_fmod(&mut self.module, builder, lhs, rhs)?,
                     // Float comparisons - also always return bool (i8)
                     BinaryOp::FEq
                     | BinaryOp::FNe
diff --git a/crates/compiler/tests/cranelift_backend_tests.rs b/crates/compiler/tests/cranelift_backend_tests.rs
@@ -226,6 +226,38 @@ fn test_vector_f32x4_add_compilation() {
         .expect("Failed to compile f32x4 add function");
 }
 
+/// Test scalar float remainder codegen (f64 + f32).
+#[test]
+fn test_scalar_float_remainder_compilation() {
+    let mut backend = CraneliftBackend::new().expect("Failed to create backend");
+
+    let f64_func = create_float_arithmetic_function("f64_rem", BinaryOp::FRem, HirType::F64);
+    backend
+        .compile_function(f64_func.id, &f64_func)
+        .expect("Failed to compile f64 remainder function");
+
+    let f32_func = create_float_arithmetic_function("f32_rem", BinaryOp::FRem, HirType::F32);
+    backend
+        .compile_function(f32_func.id, &f32_func)
+        .expect("Failed to compile f32 remainder function");
+}
+
+/// Vector float remainder is currently unsupported in Cranelift path.
+#[test]
+fn test_vector_float_remainder_rejected() {
+    let mut backend = CraneliftBackend::new().expect("Failed to create backend");
+    let func = create_vector_arithmetic_function("vec_f32x4_rem", BinaryOp::FRem, HirType::F32, 4);
+    let err = backend
+        .compile_function(func.id, &func)
+        .expect_err("Expected vector frem to be rejected");
+    let msg = err.to_string();
+    assert!(
+        msg.contains("frem currently supports scalar f32/f64 only"),
+        "unexpected error: {}",
+        msg
+    );
+}
+
 /// Test comparison operations
 #[test]
 fn test_comparison_operations() {
@@ -438,6 +470,57 @@ fn create_vector_arithmetic_function(
     func
 }
 
+/// Helper function to create scalar float arithmetic operations.
+fn create_float_arithmetic_function(name: &str, op: BinaryOp, ty: HirType) -> HirFunction {
+    let name = create_test_string(name);
+
+    let sig = HirFunctionSignature {
+        params: vec![
+            HirParam {
+                id: HirId::new(),
+                name: create_test_string("a"),
+                ty: ty.clone(),
+                attributes: ParamAttributes::default(),
+            },
+            HirParam {
+                id: HirId::new(),
+                name: create_test_string("b"),
+                ty: ty.clone(),
+                attributes: ParamAttributes::default(),
+            },
+        ],
+        returns: vec![ty.clone()],
+        type_params: vec![],
+        const_params: vec![],
+        lifetime_params: vec![],
+        is_variadic: false,
+        is_async: false,
+        effects: vec![],
+        is_pure: false,
+    };
+
+    let mut func = HirFunction::new(name, sig);
+    let entry_block_id = func.entry_block;
+    let param_a = func.create_value(ty.clone(), HirValueKind::Parameter(0));
+    let param_b = func.create_value(ty.clone(), HirValueKind::Parameter(1));
+    let result = func.create_value(ty.clone(), HirValueKind::Instruction);
+    let inst = HirInstruction::Binary {
+        op,
+        result,
+        ty,
+        left: param_a,
+        right: param_b,
+    };
+
+    let block = func.blocks.get_mut(&entry_block_id).unwrap();
+    block.add_instruction(inst);
+    block.set_terminator(HirTerminator::Return {
+        values: vec![result],
+    });
+
+    func
+}
+
 /// Create a function with control flow (if-else)
 fn create_control_flow_function() -> HirFunction {
     let name = create_test_string("abs");
diff --git a/crates/zyntax_embed/src/grammar2.rs b/crates/zyntax_embed/src/grammar2.rs
@@ -113,35 +113,65 @@ impl Grammar2 {
     ) -> Grammar2Result<TypedProgram> {
         use zyntax_typed_ast::source::SourceFile;
 
-        let interpreter = GrammarInterpreter::new(&self.grammar);
-
-        let mut builder = TypedASTBuilder::new();
-        let mut registry = TypeRegistry::new();
-        let mut state = ParserState::new(source, &mut builder, &mut registry);
-
-        // Parse from the entry rule
-        let result = interpreter.parse(&mut state);
-
-        match result {
-            ParseResult::Success(ParsedValue::Program(mut program), _) => {
-                // Add source file for diagnostics
-                program.source_files =
-                    vec![SourceFile::new(filename.to_string(), source.to_string())];
-                Ok(*program)
-            }
-            ParseResult::Success(other, _) => {
-                // If we get something other than a program, wrap it
-                eprintln!(
-                    "[Grammar2] Warning: parse returned {:?}, expected Program",
-                    std::mem::discriminant(&other)
-                );
-                Err(Grammar2Error::UnexpectedResult)
-            }
-            ParseResult::Failure(e) => Err(Grammar2Error::SourceParseError(format!(
-                "Parse error at {}:{}: expected {:?}",
-                e.line, e.column, e.expected
-            ))),
-        }
+        const PARSE_STACK_SIZE_BYTES: usize = 64 * 1024 * 1024;
+
+        let grammar = Arc::clone(&self.grammar);
+        let source_owned = source.to_string();
+        let filename_owned = filename.to_string();
+
+        let handle = std::thread::Builder::new()
+            .name("grammar2-parse".to_string())
+            .stack_size(PARSE_STACK_SIZE_BYTES)
+            .spawn(move || {
+                let interpreter = GrammarInterpreter::new(&grammar);
+
+                let mut builder = TypedASTBuilder::new();
+                let mut registry = TypeRegistry::new();
+                let mut state = ParserState::new(&source_owned, &mut builder, &mut registry);
+
+                // Parse from the entry rule
+                let result = interpreter.parse(&mut state);
+
+                match result {
+                    ParseResult::Success(ParsedValue::Program(mut program), _) => {
+                        // Add source file for diagnostics
+                        program.source_files = vec![SourceFile::new(
+                            filename_owned.clone(),
+                            source_owned.clone(),
+                        )];
+                        Ok(*program)
+                    }
+                    ParseResult::Success(other, _) => {
+                        // If we get something other than a program, wrap it
+                        eprintln!(
+                            "[Grammar2] Warning: parse returned {:?}, expected Program",
+                            std::mem::discriminant(&other)
+                        );
+                        Err(Grammar2Error::UnexpectedResult)
+                    }
+                    ParseResult::Failure(e) => Err(Grammar2Error::SourceParseError(format!(
+                        "Parse error at {}:{}: expected {:?}",
+                        e.line, e.column, e.expected
+                    ))),
+                }
+            })
+            .map_err(|e| {
+                Grammar2Error::SourceParseError(format!("Failed to spawn parser thread: {}", e))
+            })?;
+
+        handle.join().map_err(|panic_payload| {
+            let panic_msg = if let Some(s) = panic_payload.downcast_ref::<&str>() {
+                (*s).to_string()
+            } else if let Some(s) = panic_payload.downcast_ref::<String>() {
+                s.clone()
+            } else {
+                "unknown panic payload".to_string()
+            };
+            Grammar2Error::SourceParseError(format!(
+                "Grammar2 parser thread panicked: {}",
+                panic_msg
+            ))
+        })?
     }
 
     /// Parse source code with plugin signatures (for proper extern declarations)
