Re: [Maria-developers] bzr commit into MariaDB 5.1, with Maria 1.5:maria branch (monty:2770)

[Kristian Nielsen]

Michael Widenius  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.c2008-02-18 22:35:17 +
>>> +++ b/storage/myisam/myisamlog.c2009-11-29 23:08:56 +
>>> @@ -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.

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] bzr commit into MariaDB 5.1, with Maria 1.5:maria branch (monty:2770)

[Michael Widenius]

#At lp:maria based on revid:mo...@askmonty.org-20091130111302-6zcyj6ucz6rnphdq

 2770 Michael Widenius  2009-11-30
  Fixes after comments from last push:
  - Removed some not needed casts
  - Change plugin.h to be 'binary compatible' with old versions
  - Added mysql_ft_size_t typedef to plugin.h to make it trivial to change 
string lengths to size_t on next ABI change
  - Made some fixes suggested by Kristian to make things more portable and 
future safe (when it comes to strict aliasing)
  modified:
include/ft_global.h
include/mysql/plugin.h
include/mysql/plugin.h.pp
mysql-test/t/information_schema.test
sql/sp_head.cc
sql/sql_select.cc
sql/table.cc
storage/maria/ma_ft_boolean_search.c
storage/maria/ma_ft_nlq_search.c
storage/maria/ma_ft_parser.c
storage/maria/ma_ftdefs.h
storage/maria/maria_ftdump.c
storage/myisam/ft_boolean_search.c
storage/myisam/ft_nlq_search.c
storage/myisam/ft_parser.c
storage/myisam/myisam_ftdump.c

per-file messages:
  include/ft_global.h
Introduced FT_WEIGTH, to handle fulltext weights in a slightly more 
portable manner
  include/mysql/plugin.h
Change plugin.h to be 'binary compatible' with old versions
Added mysql_ft_size_t typedef to plugin.h to make it trivial to change 
string lengths to size_t on next ABI change
Changed flags to unsigned (as flags should always be unsigned)
  mysql-test/t/information_schema.test
Fixed typo
  sql/sp_head.cc
Removed cast
  sql/sql_select.cc
Removed cast
  sql/table.cc
Removed cast
  storage/maria/ma_ft_boolean_search.c
Use mysql_ft_size_t instead of size_t for plugin.h code
Changed some other string lengths to size_t
  storage/maria/ma_ft_nlq_search.c
Use FT_WEIGTH to make code more portable
  storage/maria/ma_ft_parser.c
Use mysql_ft_size_t instead of size_t for plugin.h code
Changed some other string lengths to size_t
  storage/maria/ma_ftdefs.h
Changed some string lengths to size_t
  storage/maria/maria_ftdump.c
Use FT_WEIGTH to make code more portable
  storage/myisam/ft_boolean_search.c
Use mysql_ft_size_t instead of size_t for plugin.h code
  storage/myisam/ft_nlq_search.c
Use FT_WEIGTH to make code more portable
  storage/myisam/ft_parser.c
Use mysql_ft_size_t instead of size_t for plugin.h code
  storage/myisam/myisam_ftdump.c
Use FT_WEIGTH to make code more portable
=== modified file 'include/ft_global.h'
--- a/include/ft_global.h   2007-06-27 14:49:12 +
+++ b/include/ft_global.h   2009-11-30 13:36:06 +
@@ -76,6 +76,8 @@ my_bool ft_boolean_check_syntax_string(c
 
 extern const HA_KEYSEG ft_keysegs[FT_SEGS];
 
+typedef union {int32 i; float f;} FT_WEIGTH;
+
 #ifdef  __cplusplus
 }
 #endif

=== modified file 'include/mysql/plugin.h'
--- a/include/mysql/plugin.h2009-11-29 23:08:56 +
+++ b/include/mysql/plugin.h2009-11-30 13:36:06 +
@@ -564,19 +564,22 @@ typedef struct st_mysql_ftparser_boolean
   nothing.  See  enum_ftparser_mode above.
 */
 
+/* TODO: Change the following int to size_t at next ABI update */
+typedef int mysql_ft_size_t;
+
 typedef struct st_mysql_ftparser_param
 {
   int (*mysql_parse)(struct st_mysql_ftparser_param *,
- const unsigned char *doc, size_t doc_len);
+ const unsigned char *doc, mysql_ft_size_t doc_len);
   int (*mysql_add_word)(struct st_mysql_ftparser_param *,
-const unsigned char *word, size_t word_len,
+const unsigned char *word, mysql_ft_size_t word_len,
 MYSQL_FTPARSER_BOOLEAN_INFO *boolean_info);
   void *ftparser_state;
   void *mysql_ftparam;
   struct charset_info_st *cs;
-  char *doc;
-  int length;
-  int flags;
+  const unsigned char *doc;
+  mysql_ft_size_t length;
+  unsigned int flags;
   enum enum_ftparser_mode mode;
 } MYSQL_FTPARSER_PARAM;
 

=== modified file 'include/mysql/plugin.h.pp'
--- a/include/mysql/plugin.h.pp 2009-11-29 23:08:56 +
+++ b/include/mysql/plugin.h.pp 2009-11-30 13:36:06 +
@@ -70,19 +70,20 @@ typedef struct st_mysql_ftparser_boolean
   char prev;
   char *quot;
 } MYSQL_FTPARSER_BOOLEAN_INFO;
+typedef int mysql_ft_size_t;
 typedef struct st_mysql_ftparser_param
 {
   int (*mysql_parse)(struct st_mysql_ftparser_param *,
- const unsigned char *doc, size_t doc_len);
+ const unsigned char *doc, mysql_ft_size_t doc_len);
   int (*mysql_add_word)(struct st_mysql_ftparser_param *,
-const unsigned char *word, size_t word_len,
+const unsigned char *word, mysql_ft_size_t word_len,
 MYSQL_FTPARSER_BOOLEAN_INFO *boolean_info);
   void *ftparser_state;
   void *mysql_ftparam;
   struct charset_info_st *cs;
-  char *doc;
-  int length;
-  int flags;
+  const unsigned char *doc;
+  mysql_ft_size_t le

Re: [Maria-developers] bzr commit into MariaDB 5.1, with Maria 1.5:maria branch (monty:2770)

[Michael Widenius]

Hi!

> "Kristian" == Kristian Nielsen  writes:

Kristian> Michael Widenius  writes:
Kristian> Monty, thaks a log for working on fixing all of these problems!

Kristian> Below are some comments...



>> #
>> # 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.c2009-01-15 21:27:36 +
>> +++ b/mysys/lf_hash.c2009-11-29 23:08:56 +
>> @@ -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 +
>> +++ b/sql/sp_head.cc 2009-11-29 23:08:56 +
>> @@ -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 +
>> +++ b/sql/sql_select.cc  2009-11-29 23:08:56 +
>> @@ -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 +
>> +++ b/sql/table.cc   2009-11-29 23:08:56 +
>> @@ -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 

Re: [Maria-developers] bzr commit into MariaDB 5.1, with Maria 1.5:maria branch (monty:2770)

[Michael Widenius]

Hi!

> "Sergei" == Sergei Golubchik  writes:

Sergei> Hi, Michael!
Sergei> On Nov 29, Michael Widenius wrote:
>> 2770 Michael Widenius2009-11-30
>> Change type for functions in plugin.h:str_mysql_ftparser_param()
>> to const unsigned char and string lengths to size_t.

Sergei> You cannot do it that way.
Sergei> char -> const unsigned char is fine, but
Sergei> int -> size_t is not, because they may have different width and on this
Sergei> system your change breaks the ABI.

Sergei> The best solution would be to undo your change and to keep internal
Sergei> MariaDB problems (like gcc warnins in the MariaDB code) strictly
Sergei> internal and not reflected in the API. But if you absolutely must to
Sergei> change it - you have to increase the API version. It's an incompatible
Sergei> change, you have to set it to 0x0200 thus making all existent ftparser
Sergei> plugins invalid. And this will fork the API - when Sun/MySQL will change
Sergei> the API and set the version to 0x0200, there will be two different
Sergei> incompatible APIs both having 0x0200 version.

Sergei> So, again, the best solution is to keep the int type.

As discussed on IRC:
- Changed the type back to int, but did it with a typedef so that we
  can at next ABI change just change the typedef and no other code
  change is necessary.

This will also make it easier for other storage engines to prepare for
the type change.

Regards,
Monty



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] bzr commit into MariaDB 5.1, with Maria 1.5:maria branch (monty:2770)

[Kristian Nielsen]

Michael Widenius  writes:

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

Below are some comments...

 - Kristian.

> #At lp:maria based on revid:mo...@askmonty.org-20091126201933-dgoynszp2z90gknl
>
>  2770 Michael Widenius2009-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.in2009-11-07 15:56:51 +
> +++ b/configure.in2009-11-29 23:08:56 +
> @@ -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.test2009-09-29 20:19:43 +
> +++ b/mysql-test/t/information_schema.test2009-11-29 23:08:56 +
> @@ -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 +
> +++ b/mysys/lf_hash.c 2009-11-29 23:08:56 +
> @@ -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 +
> +++ b/sql/sp_head.cc  2009-11-29 23:08:56 +
> @@ -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 +
> +++ b/sql/sql_select.cc   2009-11-29 23:08:56 +
> @@ -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.cc2009-10-15 21:38:29 +
> +++ b/sql/table.cc2009-11-29 23:08:56 +
> @@ -2077,8 +2077,9 @@ ulong get_form_pos(File file, uchar *hea
>else
>{
>  char *str;

Re: [Maria-developers] bzr commit into MariaDB 5.1, with Maria 1.5:maria branch (monty:2770)

[Sergei Golubchik]

Hi, Michael!

On Nov 29, Michael Widenius wrote:
>  2770 Michael Widenius2009-11-30
>   Change type for functions in plugin.h:str_mysql_ftparser_param()
>   to const unsigned char and string lengths to size_t.

You cannot do it that way.
char -> const unsigned char is fine, but
int -> size_t is not, because they may have different width and on this
system your change breaks the ABI.

The best solution would be to undo your change and to keep internal
MariaDB problems (like gcc warnins in the MariaDB code) strictly
internal and not reflected in the API. But if you absolutely must to
change it - you have to increase the API version. It's an incompatible
change, you have to set it to 0x0200 thus making all existent ftparser
plugins invalid. And this will fork the API - when Sun/MySQL will change
the API and set the version to 0x0200, there will be two different
incompatible APIs both having 0x0200 version.

So, again, the best solution is to keep the int type.

Regards / Mit vielen Grüßen,
Sergei

-- 
   __  ___ ___   __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik 
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Engineer/Server Architect
/_/  /_/\_, /___/\___\_\___/  Sun Microsystems GmbH, HRB München 161028
   <___/  Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp