From a4a80f37bc2775ebce032f491a1002f03aec61ba Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Sun, 2 Feb 2025 20:45:18 +0400 Subject: [PATCH 1/7] rewrite unary expressions too - in order to support "NOT FALSE" expressions --- core/translate/optimizer.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/translate/optimizer.rs b/core/translate/optimizer.rs index 5ad3074cd..bd69fb0f0 100644 --- a/core/translate/optimizer.rs +++ b/core/translate/optimizer.rs @@ -675,6 +675,10 @@ fn rewrite_expr(expr: &mut ast::Expr) -> Result<()> { } Ok(()) } + ast::Expr::Unary(_, arg) => { + rewrite_expr(arg)?; + Ok(()) + } _ => Ok(()), } } From c2950d144e37d12604c4bef84712dfaa351ced9b Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Sun, 2 Feb 2025 20:47:15 +0400 Subject: [PATCH 2/7] add logical expression fuzz tests --- tests/integration/fuzz/mod.rs | 88 +++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index e7baf481b..5591d13b3 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -146,4 +146,92 @@ mod tests { ); } } + + #[test] + pub fn logical_expression_fuzz_ex1() { + let _ = env_logger::try_init(); + let db = TempDatabase::new_empty(); + let limbo_conn = db.connect_limbo(); + let sqlite_conn = rusqlite::Connection::open_in_memory().unwrap(); + + for query in [ + "SELECT FALSE", + "SELECT NOT FALSE", + "SELECT ((NULL) IS NOT TRUE <= ((NOT (FALSE))))", + ] { + let limbo = limbo_exec_row(&limbo_conn, query); + let sqlite = sqlite_exec_row(&sqlite_conn, query); + assert_eq!( + limbo, sqlite, + "query: {}, limbo: {:?}, sqlite: {:?}", + query, limbo, sqlite + ); + } + } + + #[test] + pub fn logical_expression_fuzz_run() { + let _ = env_logger::try_init(); + let g = GrammarGenerator::new(); + let (expr, expr_builder) = g.create_handle(); + let (bin_op, bin_op_builder) = g.create_handle(); + let (unary_infix_op, unary_infix_op_builder) = g.create_handle(); + let (paren, paren_builder) = g.create_handle(); + + paren_builder + .concat("") + .push_str("(") + .push(expr) + .push_str(")") + .build(); + + unary_infix_op_builder + .concat(" ") + .push(g.create().choice().options_str(["NOT"]).build()) + .push(expr) + .build(); + + bin_op_builder + .concat(" ") + .push(expr) + .push( + g.create() + .choice() + .options_str(["AND", "OR", "IS", "IS NOT", "=", "<>", ">", "<", ">=", "<="]) + .build(), + ) + .push(expr) + .build(); + + expr_builder + .choice() + .option_w(unary_infix_op, 1.0) + .option_w(bin_op, 1.0) + .option_w(paren, 1.0) + // unfortunatelly, sqlite behaves weirdly when IS operator is used with TRUE/FALSE constants + // e.g. 8 IS TRUE == 1 (although 8 = TRUE == 0) + // so, we do not use TRUE/FALSE constants as they will produce diff with sqlite results + .options_str(["1", "0", "NULL", "1.0", "1.5", "-0.5", "-1.0", "'x'"]) + .build(); + + let sql = g.create().concat(" ").push_str("SELECT").push(expr).build(); + + let db = TempDatabase::new_empty(); + let limbo_conn = db.connect_limbo(); + let sqlite_conn = rusqlite::Connection::open_in_memory().unwrap(); + + let (mut rng, seed) = rng_from_time(); + log::info!("seed: {}", seed); + for _ in 0..16 * 1024 { + let query = g.generate(&mut rng, sql, 50); + log::info!("query: {}", query); + let limbo = limbo_exec_row(&limbo_conn, &query); + let sqlite = sqlite_exec_row(&sqlite_conn, &query); + assert_eq!( + limbo, sqlite, + "query: {}, limbo: {:?}, sqlite: {:?}", + query, limbo, sqlite + ); + } + } } From 17d06a1d28b06b2fd6174a0e39f3a9dc0b261bb9 Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Sun, 2 Feb 2025 20:56:32 +0400 Subject: [PATCH 3/7] adjust options for logical fuzz test --- tests/integration/fuzz/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index 5591d13b3..e155a5fb1 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -211,7 +211,7 @@ mod tests { // unfortunatelly, sqlite behaves weirdly when IS operator is used with TRUE/FALSE constants // e.g. 8 IS TRUE == 1 (although 8 = TRUE == 0) // so, we do not use TRUE/FALSE constants as they will produce diff with sqlite results - .options_str(["1", "0", "NULL", "1.0", "1.5", "-0.5", "-1.0", "'x'"]) + .options_str(["1", "0", "NULL", "2.0", "1.5", "-0.5", "-2.0", "(1 / 0)"]) .build(); let sql = g.create().concat(" ").push_str("SELECT").push(expr).build(); From 11c47f5e4467b07a57492253f2356bbb7feb523c Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Sun, 2 Feb 2025 21:22:24 +0400 Subject: [PATCH 4/7] fix miscomplation of ifnull scalar function --- core/translate/expr.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 771978cba..b0ee7fa61 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -1146,9 +1146,10 @@ pub fn translate_expr( temp_reg, resolver, )?; + let before_copy_label = program.allocate_label(); program.emit_insn(Insn::NotNull { reg: temp_reg, - target_pc: program.offset().add(2u32), + target_pc: before_copy_label, }); translate_expr( @@ -1158,6 +1159,7 @@ pub fn translate_expr( temp_reg, resolver, )?; + program.resolve_label(before_copy_label, program.offset()); program.emit_insn(Insn::Copy { src_reg: temp_reg, dst_reg: target_register, From 979612cb34bd49a7d5ad53eed2d0c4f921d0722d Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Sun, 2 Feb 2025 21:25:23 +0400 Subject: [PATCH 5/7] fix miscompilation of like function --- core/translate/expr.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index b0ee7fa61..acf218f1d 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -1227,15 +1227,21 @@ pub fn translate_expr( srf.to_string() ); }; - for arg in args { - let _ = - translate_and_mark(program, referenced_tables, arg, resolver); + let func_registers = program.alloc_registers(args.len()); + for (i, arg) in args.iter().enumerate() { + let _ = translate_expr( + program, + referenced_tables, + arg, + func_registers + i, + resolver, + )?; } program.emit_insn(Insn::Function { // Only constant patterns for LIKE are supported currently, so this // is always 1 constant_mask: 1, - start_reg: target_register + 1, + start_reg: func_registers, dest: target_register, func: func_ctx, }); From 529c2df6f6abe0aa84c0e92124c7b3eafc229032 Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Sun, 2 Feb 2025 21:25:33 +0400 Subject: [PATCH 6/7] add few scalar function with binary result in logical fuzz test --- tests/integration/fuzz/mod.rs | 42 +++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index e155a5fb1..943995359 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -10,7 +10,7 @@ mod tests { use crate::{ common::TempDatabase, - fuzz::grammar_generator::{rand_int, GrammarGenerator}, + fuzz::grammar_generator::{rand_int, rand_str, GrammarGenerator}, }; fn rng_from_time() -> (ChaCha8Rng, u64) { @@ -176,6 +176,7 @@ mod tests { let (expr, expr_builder) = g.create_handle(); let (bin_op, bin_op_builder) = g.create_handle(); let (unary_infix_op, unary_infix_op_builder) = g.create_handle(); + let (scalar, scalar_builder) = g.create_handle(); let (paren, paren_builder) = g.create_handle(); paren_builder @@ -203,11 +204,48 @@ mod tests { .push(expr) .build(); + scalar_builder + .choice() + .option( + g.create() + .concat("") + .push_str("like('") + .push_symbol(rand_str("", 2)) + .push_str("', '") + .push_symbol(rand_str("", 2)) + .push_str("')") + .build(), + ) + .option( + g.create() + .concat("") + .push_str("ifnull(") + .push(expr) + .push_str(",") + .push(expr) + .push_str(")") + .build(), + ) + .option( + g.create() + .concat("") + .push_str("iif(") + .push(expr) + .push_str(",") + .push(expr) + .push_str(",") + .push(expr) + .push_str(")") + .build(), + ) + .build(); + expr_builder .choice() .option_w(unary_infix_op, 1.0) .option_w(bin_op, 1.0) .option_w(paren, 1.0) + .option_w(scalar, 1.0) // unfortunatelly, sqlite behaves weirdly when IS operator is used with TRUE/FALSE constants // e.g. 8 IS TRUE == 1 (although 8 = TRUE == 0) // so, we do not use TRUE/FALSE constants as they will produce diff with sqlite results @@ -222,7 +260,7 @@ mod tests { let (mut rng, seed) = rng_from_time(); log::info!("seed: {}", seed); - for _ in 0..16 * 1024 { + for _ in 0..1024 { let query = g.generate(&mut rng, sql, 50); log::info!("query: {}", query); let limbo = limbo_exec_row(&limbo_conn, &query); From f43a326649da747c771a3fcd8f8311eae53c88a0 Mon Sep 17 00:00:00 2001 From: Nikita Sivukhin Date: Sun, 2 Feb 2025 21:26:13 +0400 Subject: [PATCH 7/7] add examples of tests found by fuzzer --- tests/integration/fuzz/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index 943995359..925bfe771 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -158,6 +158,8 @@ mod tests { "SELECT FALSE", "SELECT NOT FALSE", "SELECT ((NULL) IS NOT TRUE <= ((NOT (FALSE))))", + "SELECT ifnull(0, NOT 0)", + "SELECT like('a%', 'a') = 1", ] { let limbo = limbo_exec_row(&limbo_conn, query); let sqlite = sqlite_exec_row(&sqlite_conn, query);