maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Michael Widenius <monty@xxxxxxxxxxxx> writes:

Thanks for fixing all the issues? Agree with leaving as is the couple of
things you suggested.

>>> === 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).

What happens inside GCC is this:

GCC sees a store through a uchar ** inside read_string (so the lvalue is of
type uchar *).

Later it sees a load through a char ** when file_info.show_name is read (so
the lvalue here is of type char *).

Whenever there is a load, GCC needs to decide which pending stores it has that
it needs to flush from registers to memory to be sure that the correct value
is loaded.

With -fstrict-aliasing, it will decide that it does _not_ need to flush the
store from read_string, as the lvalue types are incompatible! This is where
incorrect code may be generated.

Of course, it may decide to flush the store for other reasons (out of
registers, call of or return from a function, etc.), which is why in many
cases we see no problem.

So the difference in my suggestion is that the store in read_string() is the
same lvalue type as what is read (uchar *), so GCC will be guaranteed to force
the store before the load.

So any casting of pointers, function calls, etc. do not enter into the picture
from the point of view of GCC, as they are probably already eliminated by
other optimisations when/if the issue arises...

(It's up to you which version you prefer, just wanted to explain the issue.)

> 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.

I don't think it will fix the warning (the extra (char **) cast is fine for
that).

I will check if the __attribute__(may_alias) works.

> 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.

Probably, especially as the pointer is marked volatile; strict aliasing means
the compiler _may_ theoretically generate the wrong code, not that it
necessarily will ever actually do so.

> 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.

Sounds good.

> 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).

Ok.

> 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.

Yes.

 - Kristian.