Michael Widenius <monty@askmonty.org> 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.