Hi!
"Kristian" == Kristian Nielsen <knielsen@knielsen-hq.org> writes:
Kristian> Michael Widenius <monty@askmonty.org> writes: Kristian> Monty, thaks a log for working on fixing all of these problems! Kristian> Below are some comments... <cut>
# # 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)
Kristian> Ok, the package name story again... Kristian> Can you please push this to your Buildbot staging tree first Kristian> lp:~maria-captains/maria/mariadb-5.1-monty Kristian> so that we can check that this does not break the packaging scripts before Kristian> pushing into main? Will do. Kristian> Also, you should check that this does not break automatic package upgrade Kristian> (apt-get, yum) from MariaDB 5.1.38 and 5.1.39. This is not currently tested Kristian> with the Buildbot setup (we can work together to get this tested). Kristian> Naming issues are annoying ... Agree, but they have to get done sometimes and the sooner the better. (As after 'final' they can't be done)
+-- source include/have_innodb.inc +
Kristian> An alternative is to move the problematic tests to the test case Kristian> information_schema_all_engines.test, to keep some testing of Kristian> information_schema when building without innodb (this test case exists to Kristian> solve the similar problem for other engines like PBXT). Agree, but better to leave this to another patch. We have to do a lot of work in the future to split the test suite to two parts, one for all engines and engine specific tests.
=== 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))
Kristian> I hope you are aware that the (char *) cast does nothing to change the Kristian> underlying violation of the strict aliasing rule? It can still be valid to do Kristian> to silence the warning of course (could add a comment that this unusual double Kristian> cast is due to GCC warnings). Yes, I am aware of that nothing is really changed. This is just to avoid a warning in gcc that is not relevant as the my_atomic_casptr() macro should be safe as such. As this is done in a lot of places for my_atomic_casptr(), I don't think a comment is needed (we can't have comment in every place). Kristian> The underlying violation of strict aliasing remains. The access of the memory Kristian> storing cursor->curr inside my_atomic_casptr() is done with type void *, which Kristian> is incompatible with the actual type of cursor->curr which is LF_SLIST *. So Kristian> to fix we would need to change the type of cursor->curr to void * (and would Kristian> then need additional casts between void * and LF_SLIST *). Or we could add Kristian> __attribute__(may_alias) in include/atomic/gcc_builtins.h. Or we could ignore Kristian> the problem for now ... Could you check if adding the attribute to gcc_builtins.h would help ? Even if I (and Serg) think the code is 100% safe, it can only be better if we give the compiler more information. In this particular case I think it's safe to ignore the problem even if the above attribute isn't done or doesn't help.
=== 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;
Kristian> The cast here is not necessary, is it? (As Item is a parent class of Kristian> Item_null) Kristian> If not necessary, better remove it to avoid confusion. Or could even do just I thought it would be necessary, but apparently it worked without it. I have now removed the cast. Kristian> 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;
Kristian> Again, is the (Item *) cast needed? (also more places below). fixed.
=== 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;
Kristian> You don't need the double cast here, just (const char **)buf should be enough. Fixed. (Got a bit paranoid with the (char*) and added the above without testing or thinking.
=== 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;
Kristian> Why not fix the problem properly here instead? Kristian> union { int32 i; float f; } subkeys; Kristian> tmp_weight= subkeys.f; Kristian> (and use subkeys.i where subkeys was previously used as int)? This will fix Kristian> the real problem and not just silence the warning. Good idea. Done
=== 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;
Kristian> I understand why the code does this here, but it does violate strict aliasing Kristian> :-( For this case, I have now clue how to do this without doing a lot of changes in used_tables. Kristian> To fix the real problem it seems one would need to do something like Kristian> union { void *v; MARIA_USED_TABLES *m; } *prev; Kristian> which is quite ugly ... Kristian> So not sure if we should leave this (to potentially, but perhaps unlikely, Kristian> break with new compiler), or do an ugly fix ... This case should be 100 % safe as we are never referring to the changed pointers in this function or any functions that could inline this function. <cut>
=== 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,
Kristian> Better fix the real problem and not just the warning: Kristian> int err; Kristian> ... Kristian> uchar *tmp= 0; Kristian> file_info.show_name=0; Kristian> file_info.record=0; Kristian> err= read_string(&cache, &tmp, (uint) mi_uint2korr(head)); Kristian> file_info.name= (char *)tmp; Kristian> if (err) Kristian> ... I think my code is the same thing from all practical point of view. In general, I think it's stupid for any compiler to assume that if you take a pointer of an object in *any circumstances* for a function call that the object may not change (strict-alias or not). The little benefit you *may* get for this particular optimization (in the function call case) is not worth it (or the discussion around it). Regards, Monty