maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Kristian" == Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx> writes:

Kristian> Michael Widenius <monty@xxxxxxxxxxxx> 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