Hi Timour, The patch looks Okey to push for me. Please find two small neat-peaking comments below though :) On 08/20/2013 04:36 PM, timour@askmonty.org wrote:
At file:///home/tsk/mprog/src/5.3/
------------------------------------------------------------ revno: 3677 revision-id: timour@askmonty.org-20130820123610-cjtgvaybl1ip3128 parent: igor@askmonty.org-20130815235920-io2h7tlypwlbunsp fixes bug: https://mariadb.atlassian.net/browse/MDEV-4895 committer: timour@askmonty.org branch nick: 5.3 timestamp: Tue 2013-08-20 15:36:10 +0300 message: Fix bug MDEV-4895 Valgrind warnings (Conditional jump or move depends on uninitialised value) in Field_datetime::get_date on GREATEST(..) IS NULL
Analysis: The cause of the valgrind warning was an attempt to evaluate a Field that was not yet read. The reason was that on one hand Item_func_isnotnull was marked as constant by Item_func_isnotnull::update_used_tables, and this allowed eval_const_cond() to be called. On the other hand Item_func_isnotnull::val_int() evaluated its argument as if it was not constant.
Solution: The fix make sure that Item_func_isnotnull::val_int() doesn't evaluate its argument when it is constant and cannot be NULL, because the result is known in this case.
=== modified file 'mysql-test/r/null.result' --- a/mysql-test/r/null.result 2011-11-21 13:16:16 +0000 +++ b/mysql-test/r/null.result 2013-08-20 12:36:10 +0000 @@ -343,3 +343,33 @@ Field Type Null Key Default Extra IFNULL(NULL, b) decimal(1,0) YES NULL DROP TABLE t1, t2; # End of 5.0 tests +# +# MDEV-4895 Valgrind warnings (Conditional jump or move depends on uninitialised value) in Field_datetime::get_date on GREATEST(..) IS NULL +# +CREATE TABLE t1 (dt DATETIME NOT NULL); +INSERT INTO t1 VALUES (NOW()),(NOW()); +EXPLAIN +SELECT * FROM t1 WHERE concat( dt, '2012-12-21 12:12:12' ) IS NULL; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL Impossible WHERE +SELECT * FROM t1 WHERE concat( dt, '2012-12-21 12:12:12' ) IS NULL; +dt +drop table t1; +CREATE TABLE t1 (dt INT NOT NULL); +INSERT INTO t1 VALUES (1),(2); +EXPLAIN +SELECT * FROM t1 WHERE concat( dt, '1' ) IS NULL; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL Impossible WHERE +SELECT * FROM t1 WHERE concat( dt, '1' ) IS NULL; +dt +drop table t1; +CREATE TABLE t1 (dt INT NOT NULL); +INSERT INTO t1 VALUES (1),(2); +EXPLAIN +SELECT * FROM t1 WHERE NOT (concat( dt, '1' ) IS NOT NULL); +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL Impossible WHERE +SELECT * FROM t1 WHERE NOT (concat( dt, '1' ) IS NOT NULL); +dt +drop table t1;
Please fix "drop table" to upper case here, and in the other places.
=== modified file 'mysql-test/t/null.test' --- a/mysql-test/t/null.test 2009-02-10 03:00:11 +0000 +++ b/mysql-test/t/null.test 2013-08-20 12:36:10 +0000 @@ -255,3 +255,31 @@ DESCRIBE t2; DROP TABLE t1, t2;
--echo # End of 5.0 tests + +--echo # +--echo # MDEV-4895 Valgrind warnings (Conditional jump or move depends on uninitialised value) in Field_datetime::get_date on GREATEST(..) IS NULL +--echo # + +CREATE TABLE t1 (dt DATETIME NOT NULL); +INSERT INTO t1 VALUES (NOW()),(NOW()); + +EXPLAIN +SELECT * FROM t1 WHERE concat( dt, '2012-12-21 12:12:12' ) IS NULL; +SELECT * FROM t1 WHERE concat( dt, '2012-12-21 12:12:12' ) IS NULL; + +drop table t1; +CREATE TABLE t1 (dt INT NOT NULL); +INSERT INTO t1 VALUES (1),(2); +EXPLAIN +SELECT * FROM t1 WHERE concat( dt, '1' ) IS NULL; +SELECT * FROM t1 WHERE concat( dt, '1' ) IS NULL; + +drop table t1; +CREATE TABLE t1 (dt INT NOT NULL); +INSERT INTO t1 VALUES (1),(2); + +EXPLAIN +SELECT * FROM t1 WHERE NOT (concat( dt, '1' ) IS NOT NULL); +SELECT * FROM t1 WHERE NOT (concat( dt, '1' ) IS NOT NULL); + +drop table t1;
=== modified file 'sql/item_cmpfunc.cc' --- a/sql/item_cmpfunc.cc 2013-08-15 23:59:20 +0000 +++ b/sql/item_cmpfunc.cc 2013-08-20 12:36:10 +0000 @@ -4621,6 +4621,8 @@ Item *and_expressions(Item *a, Item *b, longlong Item_func_isnull::val_int() { DBUG_ASSERT(fixed == 1); + if (const_item() && !args[0]->maybe_null) + return false;
"return 0" should be better in a longlong function.
return args[0]->is_null() ? 1: 0; }
@@ -4628,6 +4630,8 @@ longlong Item_is_not_null_test::val_int( { DBUG_ASSERT(fixed == 1); DBUG_ENTER("Item_is_not_null_test::val_int"); + if (const_item() && !args[0]->maybe_null) + DBUG_RETURN(1); if (args[0]->is_null()) { DBUG_PRINT("info", ("null"));