Merge lp:~laurynas-biveinis/percona-server/compiler-warnings-5.1 into lp:percona-server/5.1

[Laurynas Biveinis]

Changes from the previous MP:
- UNINIT_VAR used everywhere where possible (the enum var still has a default initialization due to UNINIT_VAR incompatibility).
- Type cast whitespace fixed.
- s/(unsigned)/(uint).
- Code removed instead of commented out.

The test results of --enable-mysql-maintainer-mode are at
http://jenkins.percona.com/view/Percona%20Server%205.1/job/percona-server-5.1-param-debug/16/
(all workers are past compilation stage now)

The release configuration Jenkins test results will be at
http://jenkins.percona.com/view/Percona%20Server%205.1/job/percona-server-5.1-param/185/

-----------------------

Fix Percona Server 5.1 compilation warnings (including the GCC
4.6-specific ones which are tracked in bug 878164):

- bugfix48929.patch: declare and set ip_flags and socket_flags
  local vars in handle_connections_sockets with conditional
  compilation only if their values are going to be used. Initialize
  sock and flag local vars to silence compiler warnings.
- innodb_expand_fast_index_creation.patch: initialize has_pk local var
  in mysqldump.c:get_table_structure to silence a false positive
  compiler warning. Likewise for is_unique in
  mysqldump.c:skip_secondary_keys.
- innodb_expand_import.patch: remove declaration and setting of
  root_height local var in fil_open_single_table_dataspace.
- innodb_split_buf_pool_mutex.patch: comment out local var flush_type
  declaration and setting of its value in buf_page_io_complete to make
  it regular with commented out use of it.
- innodb_thread_concurrency_timer_based.patch: cast
  os_atomic_increment_lint result to (void) in
  srv0srv.c:srv_conc_enter_innodb_timer_based,
  srv_conc_exit_innodb_timer_based and srv_conc_force_enter_innodb.
- mysql_remove_eol_carret.patch: initialize local var out_length in
  readline.cc:batch_readline to silence a false positive compiler
  warning.
- query_cache_enhance.patch: fix comparison between signed and
  unsigned integer expressions in sql_cache.cc:QueryStripComments::set
  by casting last_space + 1, which can only be non-negative, to
  unsigned.
- New patch warning_fixes.patch for upstream warning fixes: initialize
  local var number in log.cc:find_uniq_filename to silence a false
  positive compiler warning. Initialize local var need_copy_table_res
  local var in sql_table.cc:mysql_alter_table.

Custom Jenkins configuration with -Werror testing results:
http://jenkins.percona.com/view/Percona%20Server%205.1/job/percona-server-5.1-param/184/

No failures besides the usual slave/unstable-testcase stuff.

Since it is missing "release" configuration, I'm also running Jenkins in the main configuration. Will post it once it finishes.

[Laurynas Biveinis (laurynas-biveinis)]

Jenkins results:
http://jenkins.percona.com/view/Percona%20Server%205.1/job/percona-server-5.1-param/184/

[Oleg Tsarev (tsarev)]

1) innodb_split_buf_pool_mutex.patch

According to discussion with Alexey https://code.launchpad.net/~tsarev/percona-server/test55_691404/+merge/77124, our current policy - remove source code instead of comment than.

I think for XtraDB we can do following exception:
 * If we replace original code - comment should be (or we should fill bug about remove all comments)
 * If we fix code in patch - replace without variants.

flush_type variable added by patch, and I don't see reasons to keep it with comments

2) warning_fixes.patch

Can you please fill separate bug for this?

All other LGTM.

review: Needs Fixing (dev)

[Laurynas Biveinis (laurynas-biveinis)]

1) My fix is meant to be minimal and consistent w/ original patch style. If the original patch violates our coding guidelines, let's file a separate bug for that and make separate fixes as a follow-up. If I only change the current MP minimally, then the original patch will become inconsistent in style. If this MP fixes the whole patch, then the MP won't be minimal anymore and will contain what is an unrelated change. So I believe it is best to merge as-is, and then follow up as necessary.

2) Sure, I will file separate (upstream) bugs as soon as this lands in the trunk.

[Oleg Tsarev (tsarev)]

> 1) My fix is meant to be minimal and consistent w/ original patch style. If
> the original patch violates our coding guidelines, let's file a separate bug
> for that and make separate fixes as a follow-up. If I only change the current
> MP minimally, then the original patch will become inconsistent in style. If
> this MP fixes the whole patch, then the MP won't be minimal anymore and will
> contain what is an unrelated change. So I believe it is best to merge as-is,
> and then follow up as necessary.
>
> 2) Sure, I will file separate (upstream) bugs as soon as this lands in the
> trunk.

review: Approve

[Alexey Kopytov (akopytov)]

- please use UNINIT_VAR() to suppress false positive warnings. This also applies to need_copy_table_res one.

- please add a space after a type cast. this rule applies to both server and InnoDB code.

- please use 'uint' instead of 'unsigned'

- please, please, please just remove code instead of commenting it out. Yes, that weird thing is used in existing patches. No, it's not like we have any documented policy requiring people to do so, so I don't think "style" is an appropriate term to describe this. Yes, we can file a separate bug to fix the existing code. No, having abominations like this in existing patches does not mean we must introduce new ones.

review: Needs Fixing

[Alexey Kopytov (akopytov)]

review: Approve

=== modified file 'patches/bugfix48929.patch'
--- patches/bugfix48929.patch	2011-10-07 20:36:41 +0000
+++ patches/bugfix48929.patch	2011-10-25 14:05:25 +0000
@@ -29,16 +29,25 @@
  #define mysqld_charset &my_charset_latin1
  
  #ifdef HAVE_purify
-@@ -5139,28 +5143,49 @@
+@@ -5137,31 +5141,58 @@
+ #ifndef EMBEDDED_LIBRARY
+ pthread_handler_t handle_connections_sockets(void *arg __attribute__((unused)))
  {
-   my_socket sock,new_sock;
+-  my_socket sock,new_sock;
++  my_socket UNINIT_VAR(sock),new_sock;
    uint error_count=0;
 -  uint max_used_connection= (uint) (max(ip_sock,unix_sock)+1);
 -  fd_set readFDs,clientFDs;
    THD *thd;
    struct sockaddr_in cAddr;
 -  int ip_flags=0,socket_flags=0,flags;
-+  int ip_flags=0,socket_flags=0,flags,retval;
++#if !defined(HAVE_POLL)
++  int ip_flags= 0;
++#endif
++#if defined(HAVE_SYS_UN_H) && !defined(HAVE_POLL)
++  int socket_flags= 0;
++#endif
++  int UNINIT_VAR(flags),retval;
    st_vio *vio_tmp;
 +#ifdef HAVE_POLL
 +  int socket_count= 0;
@@ -66,8 +75,9 @@
 +    socket_count++;
 +#else
      FD_SET(ip_sock,&clientFDs);
+-#ifdef HAVE_FCNTL
 +#endif
- #ifdef HAVE_FCNTL
++#if !defined (HAVE_POLL) && defined(HAVE_FCNTL)
      ip_flags = fcntl(ip_sock, F_GETFL, 0);
  #endif
    }
@@ -78,11 +88,13 @@
 +  socket_count++;
 +#else
    FD_SET(unix_sock,&clientFDs);
+-#ifdef HAVE_FCNTL
 +#endif
- #ifdef HAVE_FCNTL
++#if defined(HAVE_FCNTL) && defined(HAVE_SYS_UN_H) && !defined(HAVE_POLL)
    socket_flags=fcntl(unix_sock, F_GETFL, 0);
  #endif
-@@ -5170,12 +5195,15 @@
+ #endif
+@@ -5170,12 +5201,15 @@
    MAYBE_BROKEN_SYSCALL;
    while (!abort_loop)
    {
@@ -103,7 +115,7 @@
      {
        if (socket_errno != SOCKET_EINTR)
        {
-@@ -5185,7 +5213,7 @@
+@@ -5185,7 +5219,7 @@
        MAYBE_BROKEN_SYSCALL
        continue;
      }
@@ -112,7 +124,7 @@
      if (abort_loop)
      {
        MAYBE_BROKEN_SYSCALL;
-@@ -5193,6 +5221,21 @@
+@@ -5193,6 +5227,21 @@
      }
  
      /* Is this a new connection request ? */
@@ -134,7 +146,7 @@
  #ifdef HAVE_SYS_UN_H
      if (FD_ISSET(unix_sock,&readFDs))
      {
-@@ -5200,11 +5243,12 @@
+@@ -5200,11 +5249,12 @@
        flags= socket_flags;
      }
      else

=== modified file 'patches/innodb_expand_fast_index_creation.patch'
--- patches/innodb_expand_fast_index_creation.patch	2011-10-07 20:37:58 +0000
+++ patches/innodb_expand_fast_index_creation.patch	2011-10-25 14:05:25 +0000
@@ -113,7 +113,7 @@
 +  while (*ptr)
 +  {
 +    char *tmp, *orig_ptr, c;
-+    my_bool is_unique;
++    my_bool UNINIT_VAR(is_unique);
 +
 +    orig_ptr= ptr;
 +    /* Skip leading whitespace */
@@ -248,7 +248,7 @@
    int        len;
    MYSQL_RES  *result;
    MYSQL_ROW  row;
-+  my_bool    has_pk;
++  my_bool    UNINIT_VAR(has_pk);
    DBUG_ENTER("get_table_structure");
    DBUG_PRINT("enter", ("db: %s  table: %s", db, table));
  

=== modified file 'patches/innodb_expand_import.patch'
--- patches/innodb_expand_import.patch	2011-09-20 11:03:20 +0000
+++ patches/innodb_expand_import.patch	2011-10-25 14:05:25 +0000
@@ -591,7 +591,7 @@
  	ut_free(buf2);
  
  	if (UNIV_UNLIKELY(space_id != id
-@@ -3117,6 +3642,271 @@
+@@ -3117,6 +3642,269 @@
  	os_file_close(file);
  	mem_free(filepath);
  
@@ -599,7 +599,6 @@
 +		ulint		page_no;
 +		ulint		zip_size;
 +		ulint		height;
-+		ulint		root_height = 0;
 +		rec_t*		node_ptr;
 +		dict_table_t*	table;
 +		dict_index_t*	index;
@@ -638,7 +637,6 @@
 +
 +			if (height == ULINT_UNDEFINED) {
 +				height = btr_page_get_level(page, &mtr);
-+				root_height = height;
 +			}
 +
 +			if (height == 0) {

=== modified file 'patches/innodb_split_buf_pool_mutex.patch'
--- patches/innodb_split_buf_pool_mutex.patch	2011-10-07 20:33:10 +0000
+++ patches/innodb_split_buf_pool_mutex.patch	2011-10-25 14:05:25 +0000
@@ -1264,16 +1264,15 @@
  
  	mtr_memo_push(mtr, block, MTR_MEMO_BUF_FIX);
  
-@@ -2761,6 +2920,8 @@
+@@ -2761,6 +2920,7 @@
  	enum buf_io_fix	io_type;
  	const ibool	uncompressed = (buf_page_get_state(bpage)
  					== BUF_BLOCK_FILE_PAGE);
-+	enum buf_flush	flush_type;
 +	mutex_t*	block_mutex;
  
  	ut_a(buf_page_in_file(bpage));
  
-@@ -2894,8 +3055,17 @@
+@@ -2894,8 +3054,13 @@
  		}
  	}
  
@@ -1281,11 +1280,7 @@
 -	mutex_enter(buf_page_get_mutex(bpage));
 +	//buf_pool_mutex_enter();
 +	if (io_type == BUF_IO_WRITE) {
-+		flush_type = buf_page_get_flush_type(bpage);
-+		/* to keep consistency at buf_LRU_insert_zip_clean() */
-+		//if (flush_type == BUF_FLUSH_LRU) { /* optimistic! */
-+			mutex_enter(&LRU_list_mutex);
-+		//}
++		mutex_enter(&LRU_list_mutex);
 +	}
 +	block_mutex = buf_page_get_mutex_enter(bpage);
 +	ut_a(block_mutex);
@@ -1293,7 +1288,7 @@
  
  #ifdef UNIV_IBUF_COUNT_DEBUG
  	if (io_type == BUF_IO_WRITE || uncompressed) {
-@@ -2935,6 +3105,11 @@
+@@ -2935,6 +3100,11 @@
  
  		buf_flush_write_complete(bpage);
  
@@ -1305,7 +1300,7 @@
  		if (uncompressed) {
  			rw_lock_s_unlock_gen(&((buf_block_t*) bpage)->lock,
  					     BUF_IO_WRITE);
-@@ -2957,8 +3132,9 @@
+@@ -2957,8 +3127,9 @@
  	}
  #endif /* UNIV_DEBUG */
  
@@ -1317,7 +1312,7 @@
  }
  
  /*********************************************************************//**
-@@ -3005,7 +3181,8 @@
+@@ -3005,7 +3176,8 @@
  		freed = buf_LRU_search_and_free_block(100);
  	}
  
@@ -1327,7 +1322,7 @@
  
  	ut_ad(UT_LIST_GET_LEN(buf_pool->LRU) == 0);
  	ut_ad(UT_LIST_GET_LEN(buf_pool->unzip_LRU) == 0);
-@@ -3018,7 +3195,8 @@
+@@ -3018,7 +3190,8 @@
  	memset(&buf_pool->stat, 0x00, sizeof(buf_pool->stat));
  	buf_refresh_io_stats();
  
@@ -1337,7 +1332,7 @@
  }
  
  #if defined UNIV_DEBUG || defined UNIV_BUF_DEBUG
-@@ -3043,7 +3221,10 @@
+@@ -3043,7 +3216,10 @@
  
  	ut_ad(buf_pool);
  
@@ -1349,7 +1344,7 @@
  
  	chunk = buf_pool->chunks;
  
-@@ -3142,7 +3323,7 @@
+@@ -3142,7 +3318,7 @@
  	/* Check clean compressed-only blocks. */
  
  	for (b = UT_LIST_GET_FIRST(buf_pool->zip_clean); b;
@@ -1358,7 +1353,7 @@
  		ut_a(buf_page_get_state(b) == BUF_BLOCK_ZIP_PAGE);
  		switch (buf_page_get_io_fix(b)) {
  		case BUF_IO_NONE:
-@@ -3167,8 +3348,9 @@
+@@ -3167,8 +3343,9 @@
  
  	/* Check dirty compressed-only blocks. */
  
@@ -1369,7 +1364,7 @@
  		ut_ad(b->in_flush_list);
  
  		switch (buf_page_get_state(b)) {
-@@ -3213,6 +3395,7 @@
+@@ -3213,6 +3390,7 @@
  		}
  		ut_a(buf_page_hash_get(b->space, b->offset) == b);
  	}
@@ -1377,7 +1372,7 @@
  
  	mutex_exit(&buf_pool_zip_mutex);
  
-@@ -3224,19 +3407,27 @@
+@@ -3224,19 +3402,27 @@
  	}
  
  	ut_a(UT_LIST_GET_LEN(buf_pool->LRU) == n_lru);
@@ -1406,7 +1401,7 @@
  
  	ut_a(buf_LRU_validate());
  	ut_a(buf_flush_validate());
-@@ -3270,7 +3461,10 @@
+@@ -3270,7 +3456,10 @@
  	index_ids = mem_alloc(sizeof(dulint) * size);
  	counts = mem_alloc(sizeof(ulint) * size);
  
@@ -1418,7 +1413,7 @@
  
  	fprintf(stderr,
  		"buf_pool size %lu\n"
-@@ -3337,7 +3531,10 @@
+@@ -3337,7 +3526,10 @@
  		}
  	}
  
@@ -1430,7 +1425,7 @@
  
  	for (i = 0; i < n_found; i++) {
  		index = dict_index_get_if_in_cache(index_ids[i]);
-@@ -3376,7 +3573,7 @@
+@@ -3376,7 +3568,7 @@
  	ulint		i;
  	ulint		fixed_pages_number = 0;
  
@@ -1439,7 +1434,7 @@
  
  	chunk = buf_pool->chunks;
  
-@@ -3410,7 +3607,7 @@
+@@ -3410,7 +3602,7 @@
  	/* Traverse the lists of clean and dirty compressed-only blocks. */
  
  	for (b = UT_LIST_GET_FIRST(buf_pool->zip_clean); b;
@@ -1448,7 +1443,7 @@
  		ut_a(buf_page_get_state(b) == BUF_BLOCK_ZIP_PAGE);
  		ut_a(buf_page_get_io_fix(b) != BUF_IO_WRITE);
  
-@@ -3420,8 +3617,9 @@
+@@ -3420,8 +3612,9 @@
  		}
  	}
  
@@ -1459,7 +1454,7 @@
  		ut_ad(b->in_flush_list);
  
  		switch (buf_page_get_state(b)) {
-@@ -3444,9 +3642,10 @@
+@@ -3444,9 +3637,10 @@
  			break;
  		}
  	}
@@ -1471,7 +1466,7 @@
  
  	return(fixed_pages_number);
  }
-@@ -3504,7 +3703,11 @@
+@@ -3504,7 +3698,11 @@
  
  	ut_ad(buf_pool);
  
@@ -1484,7 +1479,7 @@
  
  	fprintf(file,
  		"Buffer pool size        %lu\n"
-@@ -3607,7 +3810,11 @@
+@@ -3607,7 +3805,11 @@
  		buf_LRU_stat_sum.unzip, buf_LRU_stat_cur.unzip);
  
  	buf_refresh_io_stats();
@@ -1497,7 +1492,7 @@
  }
  
  /**********************************************************************//**
-@@ -3634,7 +3841,7 @@
+@@ -3634,7 +3836,7 @@
  
  	ut_ad(buf_pool);
  
@@ -1506,7 +1501,7 @@
  
  	chunk = buf_pool->chunks;
  
-@@ -3651,7 +3858,7 @@
+@@ -3651,7 +3853,7 @@
  		}
  	}
  
@@ -1515,7 +1510,7 @@
  
  	return(TRUE);
  }
-@@ -3667,7 +3874,8 @@
+@@ -3667,7 +3869,8 @@
  {
  	ibool	ret;
  
@@ -1525,7 +1520,7 @@
  
  	if (buf_pool->n_pend_reads + buf_pool->n_flush[BUF_FLUSH_LRU]
  	    + buf_pool->n_flush[BUF_FLUSH_LIST]
-@@ -3677,7 +3885,8 @@
+@@ -3677,7 +3880,8 @@
  		ret = TRUE;
  	}
  
@@ -1535,7 +1530,7 @@
  
  	return(ret);
  }
-@@ -3692,11 +3901,13 @@
+@@ -3692,11 +3896,13 @@
  {
  	ulint	len;
  

=== modified file 'patches/innodb_thread_concurrency_timer_based.patch'
--- patches/innodb_thread_concurrency_timer_based.patch	2011-09-20 11:03:20 +0000
+++ patches/innodb_thread_concurrency_timer_based.patch	2011-10-25 14:05:25 +0000
@@ -112,7 +112,7 @@
 +			enter_innodb_with_tickets(trx);
 +			return;
 +		}
-+		os_atomic_increment_lint(&srv_conc_n_threads, -1);
++		(void) os_atomic_increment_lint(&srv_conc_n_threads, -1);
 +	}
 +	if (!has_yielded)
 +	{
@@ -142,7 +142,7 @@
 +static void
 +srv_conc_exit_innodb_timer_based(trx_t* trx)
 +{
-+	os_atomic_increment_lint(&srv_conc_n_threads, -1);
++	(void) os_atomic_increment_lint(&srv_conc_n_threads, -1);
 +	trx->declared_to_be_inside_innodb = FALSE;
 +	trx->n_tickets_to_enter_innodb = 0;
 +	return;
@@ -172,7 +172,7 @@
  	ut_ad(srv_conc_n_threads >= 0);
 +#ifdef HAVE_ATOMIC_BUILTINS
 +	if (srv_thread_concurrency_timer_based) {
-+		os_atomic_increment_lint(&srv_conc_n_threads, 1);
++		(void) os_atomic_increment_lint(&srv_conc_n_threads, 1);
 +		trx->declared_to_be_inside_innodb = TRUE;
 +		trx->n_tickets_to_enter_innodb = 1;
 +		return;

=== modified file 'patches/mysql_remove_eol_carret.patch'
--- patches/mysql_remove_eol_carret.patch	2011-10-03 09:29:31 +0000
+++ patches/mysql_remove_eol_carret.patch	2011-10-25 14:05:25 +0000
@@ -47,7 +47,13 @@
  static bool init_line_buffer(LINE_BUFFER *buffer,File file,ulong size,
  			    ulong max_size);
  static bool init_line_buffer_from_string(LINE_BUFFER *buffer,char * str);
-@@ -62,7 +64,7 @@
+@@ -57,12 +59,12 @@
+ char *batch_readline(LINE_BUFFER *line_buff)
+ {
+   char *pos;
+-  ulong out_length;
++  ulong UNINIT_VAR(out_length);
+ 
    if (!(pos=intern_read_line(line_buff, &out_length)))
      return 0;
    if (out_length && pos[out_length-1] == '\n')

=== modified file 'patches/query_cache_enhance.patch'
--- patches/query_cache_enhance.patch	2011-10-07 20:36:41 +0000
+++ patches/query_cache_enhance.patch	2011-10-25 14:05:25 +0000
@@ -32,7 +32,7 @@
  my_bool opt_use_global_long_query_time= 0;
  my_bool opt_slow_query_log_microseconds_timestamp= 0;
  my_bool lower_case_file_system= 0;
-@@ -5914,6 +5915,7 @@
+@@ -5922,6 +5923,7 @@
    OPT_THREAD_STATISTICS,
    OPT_OPTIMIZER_FIX,
    OPT_SUPPRESS_LOG_WARNING_1592,
@@ -40,7 +40,7 @@
    OPT_USE_GLOBAL_LONG_QUERY_TIME,
    OPT_USE_GLOBAL_LOG_SLOW_CONTROL,
    OPT_SLOW_QUERY_LOG_MICROSECONDS_TIMESTAMP,
-@@ -6969,6 +6971,10 @@
+@@ -6977,6 +6979,10 @@
    {"use_global_log_slow_control", OPT_USE_GLOBAL_LOG_SLOW_CONTROL,
      "Choose flags, wich always use the global variables. Multiple flags allowed in a comma-separated string. [none, log_slow_filter, log_slow_rate_limit, log_slow_verbosity, long_query_time, min_examined_row_limit, all]",
     0, 0, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, SLOG_UG_NONE, 0, 0},
@@ -360,7 +360,7 @@
 +    }
 +    if(insert_space)
 +    {
-+      if((last_space + 1) != position)
++      if((uint) (last_space + 1) != position)
 +      {
 +        last_space = position;
 +        buffer[position++] = ' ';

=== modified file 'patches/series'
--- patches/series	2011-10-08 00:17:56 +0000
+++ patches/series	2011-10-25 14:05:25 +0000
@@ -65,3 +65,4 @@
 bug53761.patch
 xtradb_bug317074.patch
 subunit.patch
+warning_fixes.patch

=== added file 'patches/warning_fixes.patch'
--- patches/warning_fixes.patch	1970-01-01 00:00:00 +0000
+++ patches/warning_fixes.patch	2011-10-25 14:05:25 +0000
@@ -0,0 +1,24 @@
+--- a/sql/log.cc
++++ b/sql/log.cc
+@@ -1905,7 +1905,7 @@
+ 
+ static int find_uniq_filename(char *name)
+ {
+-  long                  number;
++  long                  UNINIT_VAR(number);
+   uint                  i;
+   char                  buff[FN_REFLEN];
+   struct st_my_dir     *dir_info;
+--- a/sql/sql_table.cc
++++ b/sql/sql_table.cc
+@@ -7140,7 +7140,9 @@
+     need_copy_table= ALTER_TABLE_DATA_CHANGED;
+   else
+   {
+-    enum_alter_table_change_level need_copy_table_res;
++    enum_alter_table_change_level need_copy_table_res=
++        ALTER_TABLE_METADATA_ONLY;
++
+     /* Check how much the tables differ. */
+     if (compare_tables(table, alter_info,
+                        create_info, order_num,