maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Michael Widenius <monty@xxxxxxxxxxxx> writes:

Monty, thaks a log for working on fixing all of these problems!

Below are some comments...

 - Kristian.

> #At lp:maria based on revid:monty@xxxxxxxxxxxx-20091126201933-dgoynszp2z90gknl
>
>  2770 Michael Widenius	2009-11-30
>       Remove compiler warnings (Including some warnings from -Wstrict-aliasing)
>       Don't use static link by default (in compile-pentium) as some new systems doesn't have all static libraries available
>       Change type for functions in plugin.h:str_mysql_ftparser_param() to const unsigned char and string lengths to size_t.
>       One effect of the above change is that one needs to include mysql_global.h or define size_t before including plugin.h
>       This fixes a case where mysql_client_test failed with newer gcc that enables strict-aliasing by default


> === modified file 'configure.in'
> --- a/configure.in	2009-11-07 15:56:51 +0000
> +++ b/configure.in	2009-11-29 23:08:56 +0000
> @@ -15,7 +15,7 @@ AC_CANONICAL_SYSTEM
>  # MySQL version number.
>  #
>  # Note: the following line must be parseable by win/configure.js:GetVersion()
> -AM_INIT_AUTOMAKE(mysql, 5.1.39-maria-beta)
> +AM_INIT_AUTOMAKE(mysql, 5.1.39-MariaDB-beta)

Ok, the package name story again...

Can you please push this to your Buildbot staging tree first

    lp:~maria-captains/maria/mariadb-5.1-monty

so that we can check that this does not break the packaging scripts before
pushing into main?

Also, you should check that this does not break automatic package upgrade
(apt-get, yum) from MariaDB 5.1.38 and 5.1.39. This is not currently tested
with the Buildbot setup (we can work together to get this tested).

Naming issues are annoying ...


> === modified file 'mysql-test/t/information_schema.test'
> --- a/mysql-test/t/information_schema.test	2009-09-29 20:19:43 +0000
> +++ b/mysql-test/t/information_schema.test	2009-11-29 23:08:56 +0000
> @@ -5,6 +5,9 @@
>  # on the presence of the log tables (which are CSV-based).
>  --source include/have_csv.inc
>  
> +# Check that innodb/xtradb is incompiled in as result depends on it

                                 ^^^ typo

> +-- source include/have_innodb.inc
> +

An alternative is to move the problematic tests to the test case
information_schema_all_engines.test, to keep some testing of
information_schema when building without innodb (this test case exists to
solve the similar problem for other engines like PBXT).


> === modified file 'mysys/lf_hash.c'
> --- a/mysys/lf_hash.c	2009-01-15 21:27:36 +0000
> +++ b/mysys/lf_hash.c	2009-11-29 23:08:56 +0000
> @@ -124,8 +124,8 @@ retry:
>          we found a deleted node - be nice, help the other thread
>          and remove this deleted node
>        */
> -      if (my_atomic_casptr((void **)cursor->prev,
> -                           (void **)&cursor->curr, cursor->next))
> +      if (my_atomic_casptr((void **) cursor->prev,
> +                           (void **)(char*) &cursor->curr, cursor->next))

I hope you are aware that the (char *) cast does nothing to change the
underlying violation of the strict aliasing rule? It can still be valid to do
to silence the warning of course (could add a comment that this unusual double
cast is due to GCC warnings).

The underlying violation of strict aliasing remains. The access of the memory
storing cursor->curr inside my_atomic_casptr() is done with type void *, which
is incompatible with the actual type of cursor->curr which is LF_SLIST *. So
to fix we would need to change the type of cursor->curr to void * (and would
then need additional casts between void * and LF_SLIST *). Or we could add
__attribute__(may_alias) in include/atomic/gcc_builtins.h. Or we could ignore
the problem for now ...

The same applies to all other uses of my_atomic_casptr().

> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc	2009-09-15 10:46:35 +0000
> +++ b/sql/sp_head.cc	2009-11-29 23:08:56 +0000
> @@ -1924,9 +1924,10 @@ sp_head::execute_procedure(THD *thd, Lis
>        if (spvar->mode == sp_param_out)
>        {
>          Item_null *null_item= new Item_null();
> +        Item *tmp_item= (Item*) null_item;

The cast here is not necessary, is it? (As Item is a parent class of
Item_null)

If not necessary, better remove it to avoid confusion. Or could even do just

    Item *null_item= new Item_null();

> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2009-11-06 17:22:32 +0000
> +++ b/sql/sql_select.cc	2009-11-29 23:08:56 +0000
> @@ -3586,8 +3586,9 @@ add_key_fields(JOIN *join, KEY_FIELD **k
>          {
>            if (!field->eq(item->field))
>            {
> +            Item *tmp_item= (Item*) item;

Again, is the (Item *) cast needed? (also more places below).


> === modified file 'sql/table.cc'
> --- a/sql/table.cc	2009-10-15 21:38:29 +0000
> +++ b/sql/table.cc	2009-11-29 23:08:56 +0000
> @@ -2077,8 +2077,9 @@ ulong get_form_pos(File file, uchar *hea
>    else
>    {
>      char *str;
> +    const char **tmp = (const char**) (char*) buf;

You don't need the double cast here, just (const char **)buf should be enough.

> === modified file 'storage/maria/ma_ft_nlq_search.c'
> --- a/storage/maria/ma_ft_nlq_search.c	2009-01-09 04:23:25 +0000
> +++ b/storage/maria/ma_ft_nlq_search.c	2009-11-29 23:08:56 +0000
> @@ -63,7 +63,8 @@ static int FT_SUPERDOC_cmp(void* cmp_arg
>  
>  static int walk_and_match(FT_WORD *word, uint32 count, ALL_IN_ONE *aio)
>  {
> -  int	       subkeys, r;
> +  int32	       subkeys;
> +  int          r;
>    uint	       doc_cnt;
>    FT_SUPERDOC  sdoc, *sptr;
>    TREE_ELEMENT *selem;
> @@ -127,7 +128,7 @@ static int walk_and_match(FT_WORD *word,
>        goto do_skip;
>      }
>  #if HA_FT_WTYPE == HA_KEYTYPE_FLOAT
> -    tmp_weight=*(float*)&subkeys;
> +    tmp_weight=*(float*) (char*) &subkeys;

Why not fix the problem properly here instead?

    union { int32 i; float f; } subkeys;

    tmp_weight= subkeys.f;

(and use subkeys.i where subkeys was previously used as int)? This will fix
the real problem and not just silence the warning.

> === modified file 'storage/maria/ma_state.c'
> --- a/storage/maria/ma_state.c	2009-10-06 06:57:22 +0000
> +++ b/storage/maria/ma_state.c	2009-11-29 23:08:56 +0000
> @@ -528,7 +528,7 @@ void _ma_remove_table_from_trnman(MARIA_
>  
>    safe_mutex_assert_owner(&share->intern_lock);
>    
> -  for (prev= (MARIA_USED_TABLES**) &trn->used_tables, tables= *prev;
> +  for (prev= (MARIA_USED_TABLES**) (char*) &trn->used_tables, tables= *prev;

I understand why the code does this here, but it does violate strict aliasing
:-(

To fix the real problem it seems one would need to do something like

    union { void *v; MARIA_USED_TABLES *m; } *prev;

which is quite ugly ...

So not sure if we should leave this (to potentially, but perhaps unlikely,
break with new compiler), or do an ugly fix ...

> === modified file 'storage/maria/maria_ftdump.c'
> --- a/storage/maria/maria_ftdump.c	2008-08-25 11:49:47 +0000
> +++ b/storage/maria/maria_ftdump.c	2009-11-29 23:08:56 +0000
> @@ -116,7 +116,7 @@ int main(int argc,char *argv[])
>  
>      subkeys=ft_sintXkorr(info->lastkey_buff + keylen + 1);
>      if (subkeys >= 0)
> -      weight=*(float*)&subkeys;
> +      weight=*(float*) (char*) &subkeys;

Use instead

    union { int i; float f; } subkeys;

    weight= subkeys.f;

> === modified file 'storage/myisam/ft_nlq_search.c'
> --- a/storage/myisam/ft_nlq_search.c	2008-04-28 16:24:05 +0000
> +++ b/storage/myisam/ft_nlq_search.c	2009-11-29 23:08:56 +0000
> @@ -63,7 +63,8 @@ static int FT_SUPERDOC_cmp(void* cmp_arg
>  
>  static int walk_and_match(FT_WORD *word, uint32 count, ALL_IN_ONE *aio)
>  {
> -  int	       subkeys, r;
> +  int32	       subkeys;
> +  int          r;
>    uint	       keylen, doc_cnt;
>    FT_SUPERDOC  sdoc, *sptr;
>    TREE_ELEMENT *selem;
> @@ -123,7 +124,8 @@ static int walk_and_match(FT_WORD *word,
>        goto do_skip;
>      }
>  #if HA_FT_WTYPE == HA_KEYTYPE_FLOAT
> -    tmp_weight=*(float*)&subkeys;
> +    /* The weight we read was actually a float */
> +    tmp_weight=*(float*) (char*) &subkeys;

Use union as above.

> === modified file 'storage/myisam/myisam_ftdump.c'
> --- a/storage/myisam/myisam_ftdump.c	2007-05-10 09:59:39 +0000
> +++ b/storage/myisam/myisam_ftdump.c	2009-11-29 23:08:56 +0000
> @@ -113,7 +113,7 @@ int main(int argc,char *argv[])
>  
>      subkeys=ft_sintXkorr(info->lastkey+keylen+1);
>      if (subkeys >= 0)
> -      weight=*(float*)&subkeys;
> +      weight= *(float*) (char*) &subkeys;

Use union for subkeys, as above.

> === modified file 'storage/myisam/myisamlog.c'
> --- a/storage/myisam/myisamlog.c	2008-02-18 22:35:17 +0000
> +++ b/storage/myisam/myisamlog.c	2009-11-29 23:08:56 +0000
> @@ -385,7 +385,7 @@ static int examine_log(char * file_name,
>        file_info.name=0;
>        file_info.show_name=0;
>        file_info.record=0;
> -      if (read_string(&cache,(uchar**) &file_info.name,
> +      if (read_string(&cache,(uchar**) (char*) &file_info.name,

Better fix the real problem and not just the warning:

      int err;
...
      uchar *tmp= 0;
      file_info.show_name=0;
      file_info.record=0;
      err= read_string(&cache, &tmp, (uint) mi_uint2korr(head));
      file_info.name= (char *)tmp;
      if (err)
...