maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

  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@xxxxxxxxxxxx wrote:
> At file:///home/tsk/mprog/src/5.3/
>
> ------------------------------------------------------------
> revno: 3677
> revision-id: timour@xxxxxxxxxxxx-20130820123610-cjtgvaybl1ip3128
> parent: igor@xxxxxxxxxxxx-20130815235920-io2h7tlypwlbunsp
> fixes bug: https://mariadb.atlassian.net/browse/MDEV-4895
> committer: timour@xxxxxxxxxxxx
> 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"));