List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:December 16 2010 12:07am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195)
Bug#58699
View as plain text  
Hi Tor,

On 12/15/10 6:32 AM, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/5.5-bugteam-bug58137/ based on
> revid:gleb.shchepa@stripped
>
>   3195 Tor Didriksen	2010-12-13
>        Bug #58699 cannot build with gcc dbg on solaris
>       @ include/my_pthread.h
>          On solaris pthread_once_t contains an array, so we must wrap the
> initialization in {}
>       @ include/mysql/psi/mysql_file.h
>          Include my_global.h first, to get correct platform definitions.
>       @ mysys/ptr_cmp.c
>          Hide the unused static functions in #ifdef's on solaris.
>       @ sql/my_decimal.cc
>          Include my_global.h first, to get correct platform definitions.
>       @ sql/mysqld.cc
>          Fix signed/unsigned comparison warning.
>       @ sql/sql_audit.h
>          Include my_global.h first, to get correct platform definitions.
>       @ sql/sql_plugin.h
>          Include my_global.h first, to get correct platform definitions.
>       @ sql/sql_show.cc
>          Fix: warning: cast from pointer to integer of different size
>       @ sql/sys_vars.h
>          Use reinterpret_cast rather than c-style cast.
>       @ storage/perfschema/pfs_instr.cc
>          Include my_global.h first, to get correct platform definitions.

Some comments below.

>      modified:
>        include/my_pthread.h
>        include/mysql/psi/mysql_file.h
>        mysys/ptr_cmp.c
>        sql/my_decimal.cc
>        sql/mysqld.cc
>        sql/sql_audit.h
>        sql/sql_plugin.h
>        sql/sql_show.cc
>        sql/sys_vars.h
>        storage/perfschema/pfs_instr.cc
> === modified file 'include/my_pthread.h'
> --- a/include/my_pthread.h	2010-10-20 14:48:19 +0000
> +++ b/include/my_pthread.h	2010-12-13 16:04:03 +0000
> @@ -214,7 +214,11 @@ int pthread_cancel(pthread_t thread);
>   typedef void *(* pthread_handler)(void *);
>
>   #define my_pthread_once_t pthread_once_t
> +#if defined __GNUC__ &&  defined TARGET_OS_SOLARIS

defined(__GNUC__) && defined(TARGET_OS_SOLARIS) as used in other places 
in this file. I believe you told me something very similar in a review 
this week :-)

> +#define MY_PTHREAD_ONCE_INIT { PTHREAD_ONCE_INIT }

Please reference, in the per-file comment, the bug report about this:

http://bugs.opensolaris.org/bugdatabase/printableBug.do?bug_id=6611808

Also, it seems to me that this is something we should detect at cmake 
time, or tie it to a set of specific Solaris versions (8 and 9). A test 
at cmake time for Solaris 8 and 9 to ensure that { PTHREAD_ONCE_INIT } 
compiles is one option.

> +#else
>   #define MY_PTHREAD_ONCE_INIT PTHREAD_ONCE_INIT
> +#endif
>   #define my_pthread_once(C,F) pthread_once(C,F)
>
>   /* Test first for RTS or FSU threads */
>
> === modified file 'include/mysql/psi/mysql_file.h'
> --- a/include/mysql/psi/mysql_file.h	2010-07-09 23:00:24 +0000
> +++ b/include/mysql/psi/mysql_file.h	2010-12-13 16:04:03 +0000
> @@ -16,6 +16,8 @@
>   #ifndef MYSQL_FILE_H
>   #define MYSQL_FILE_H
>
> +#include "my_global.h"

<my_global.h>

Just a style preference in line with the other includes in the file. I 
wonder if there are any practical implications for MySQL code (other 
then the normal difference).

> +
>   /* For strlen() */
>   #include<string.h>
>   /* For MY_STAT */
>
> === modified file 'mysys/ptr_cmp.c'
> --- a/mysys/ptr_cmp.c	2009-11-20 19:01:43 +0000
> +++ b/mysys/ptr_cmp.c	2010-12-13 16:04:03 +0000
> @@ -50,11 +50,15 @@ static int ptr_compare_3(size_t *compare
>
>   	/* Get a pointer to a optimal byte-compare function for a given size */
>
> +#ifdef TARGET_OS_SOLARIS
>   qsort2_cmp get_ptr_compare (size_t size)
>   {
> -#ifdef TARGET_OS_SOLARIS
> +  (void) size;

Use the unused attribute tag. It can be used even when the argument is 
being used.

Also, let's get rid of TARGET_OS_SOLARIS? It's only used in a few places 
and we should actually be using a macro provided by Solaris.

>     return (qsort2_cmp) native_compare;
> +}
>   #else
> +qsort2_cmp get_ptr_compare (size_t size)
> +{
>     if (size<  4)
>       return (qsort2_cmp) ptr_compare;
>     switch (size&  3) {
> @@ -64,8 +68,8 @@ qsort2_cmp get_ptr_compare (size_t size)
>       case 3: return (qsort2_cmp) ptr_compare_3;
>       }
>     return 0;					/* Impossible */
> -#endif /* TARGET_OS_SOLARIS */
>   }
> +#endif /* TARGET_OS_SOLARIS */
>
>
>   	/*
> @@ -75,6 +79,8 @@ qsort2_cmp get_ptr_compare (size_t size)
>
>   #define cmp(N) if (first[N] != last[N]) return (int) first[N] - (int) last[N]
>
> +#ifndef TARGET_OS_SOLARIS
> +
>   static int ptr_compare(size_t *compare_length, uchar **a, uchar **b)
>   {
>     reg3 int length= *compare_length;
> @@ -177,6 +183,8 @@ static int ptr_compare_3(size_t *compare
>     return (0);
>   }
>
> +#endif /* !TARGET_OS_SOLARIS */
> +
>   void my_store_ptr(uchar *buff, size_t pack_length, my_off_t pos)
>   {
>     switch (pack_length) {
>
> === modified file 'sql/my_decimal.cc'
> --- a/sql/my_decimal.cc	2010-12-14 16:26:18 +0000
> +++ b/sql/my_decimal.cc	2010-12-13 16:04:03 +0000
> @@ -13,6 +13,7 @@
>      along with this program; if not, write to the Free Software
>      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
>
> +#include "my_global.h"
>   #include "sql_priv.h"
>   #include<time.h>
>
>
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc	2010-12-14 14:34:23 +0000
> +++ b/sql/mysqld.cc	2010-12-13 16:04:03 +0000
> @@ -3258,8 +3258,8 @@ static int init_common_variables()
>        size_t *pagesize = (size_t *) malloc(sizeof(size_t) * nelem);
>        if (pagesize != NULL&&  getpagesizes(pagesize, nelem)>  0)
>        {
> -       size_t i, max_page_size= 0;
> -       for (i= 0; i<  nelem; i++)
> +       size_t max_page_size= 0;
> +       for (int i= 0; i<  nelem; i++)

This is prohibited in the CodingStyle because some of old compilers in 
platforms we support for 5.1. Need to double check if we support those 
platforms for 5.5. I also vaguely remember that this might have been 
changed in some CodingStyle meeting.. but I'm not sure.

>          {
>            if (pagesize[i]>  max_page_size&&
>                pagesize[i]<= max_desired_page_size)
>
> === modified file 'sql/sql_audit.h'
> --- a/sql/sql_audit.h	2010-12-14 14:34:23 +0000
> +++ b/sql/sql_audit.h	2010-12-13 16:04:03 +0000
> @@ -17,6 +17,8 @@
>      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
>
>
> +#include "my_global.h"
> +
>   #include<mysql/plugin_audit.h>
>   #include "sql_class.h"
>
>
> === modified file 'sql/sql_plugin.h'
> --- a/sql/sql_plugin.h	2010-12-01 22:15:14 +0000
> +++ b/sql/sql_plugin.h	2010-12-13 16:04:03 +0000
> @@ -16,6 +16,8 @@
>   #ifndef _sql_plugin_h
>   #define _sql_plugin_h
>
> +#include "my_global.h"
> +
>   /*
>     the following #define adds server-only members to enum_mysql_show_type,
>     that is defined in plugin.h
>
> === modified file 'sql/sql_show.cc'
> --- a/sql/sql_show.cc	2010-12-14 10:46:00 +0000
> +++ b/sql/sql_show.cc	2010-12-13 16:04:03 +0000
> @@ -2246,7 +2246,7 @@ static bool show_status_array(THD *thd,
>             end= int10_to_str(*(long*) value, buff, 10);
>             break;
>           case SHOW_LONGLONG_STATUS:
> -          value= ((char *) status_var + (ulonglong) value);
> +          value= ((char *) status_var + (ulong) value);
>             /* fall through */
>           case SHOW_LONGLONG:
>             end= longlong10_to_str(*(longlong*) value, buff, 10);
>

Otherwise, looks good.

Regards,

Davi
Thread
bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195) Bug#58699Tor Didriksen15 Dec
  • Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195)Bug#58699Davi Arnaut16 Dec
    • Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195) Bug#58699Tor Didriksen20 Dec