Bug#21554 (sp_cache.cc: violates C++ aliasing rules)

The problem reported is a compile bug,
reported by the development GCC team with GCC 4.2.

The original issue can no longer be reproduced in MySQL 5.1,
since the configure script no longer define HAVE_ATOMIC_ADD,
which caused the Linux atomic functions to be used (and cause a problem
with an invalid cast).

This patch implements some code cleanup for 5.1 only, which was identified
during the investigation of this issue.

With this patch, statistics maintained in THD::status_var are by definition
owned by the running thread, and do not need to be protected against race
conditions. These statistics are maintained by the status_var_* helpers,
which do not require any lock.

include/my_global.h

@@ -441,17 +441,7 @@ C_MODE_END
 #ifdef HAVE_ALLOCA_H
 #include <alloca.h>
 #endif
-#ifdef HAVE_ATOMIC_ADD
-#define new my_arg_new
-#define need_to_restore_new 1
-C_MODE_START
-#include <asm/atomic.h>
-C_MODE_END
-#ifdef need_to_restore_new /* probably safer than #ifdef new */
-#undef new
-#undef need_to_restore_new
-#endif
-#endif
+
 #include <errno.h>				/* Recommended by debian */
 /* We need the following to go around a problem with openssl on solaris */
 #if defined(HAVE_CRYPT_H)
@@ -1449,10 +1439,13 @@ do { doubleget_union _tmp; \
 
 #ifndef THREAD
 #define thread_safe_increment(V,L) (V)++
+#define thread_safe_decrement(V,L) (V)--
 #define thread_safe_add(V,C,L)     (V)+=(C)
 #define thread_safe_sub(V,C,L)     (V)-=(C)
 #define statistic_increment(V,L)   (V)++
+#define statistic_decrement(V,L)   (V)--
 #define statistic_add(V,C,L)       (V)+=(C)
+#define statistic_sub(V,C,L)       (V)-=(C)
 #endif
 
 #ifdef HAVE_CHARSET_utf8

include/my_pthread.h

@@ -710,33 +710,60 @@ extern uint my_thread_end_wait_time;
 
 extern uint thd_lib_detected;
 
-	/* statistics_xxx functions are for not essential statistic */
-
-#ifndef thread_safe_increment
-#ifdef HAVE_ATOMIC_ADD
-#define thread_safe_increment(V,L) atomic_inc((atomic_t*) &V)
-#define thread_safe_decrement(V,L) atomic_dec((atomic_t*) &V)
-#define thread_safe_add(V,C,L)     atomic_add((C),(atomic_t*) &V)
-#define thread_safe_sub(V,C,L)     atomic_sub((C),(atomic_t*) &V)
-#else
+/*
+  thread_safe_xxx functions are for critical statistic or counters.
+  The implementation is guaranteed to be thread safe, on all platforms.
+  Note that the calling code should *not* assume the counter is protected
+  by the mutex given, as the implementation of these helpers may change
+  to use my_atomic operations instead.
+*/
+
+/*
+  Warning:
+  When compiling without threads, this file is not included.
+  See the *other* declarations of thread_safe_xxx in include/my_global.h
+*/
+#ifdef THREAD
 #define thread_safe_increment(V,L) \
         (pthread_mutex_lock((L)), (V)++, pthread_mutex_unlock((L)))
 #define thread_safe_decrement(V,L) \
         (pthread_mutex_lock((L)), (V)--, pthread_mutex_unlock((L)))
-#define thread_safe_add(V,C,L) (pthread_mutex_lock((L)), (V)+=(C), pthread_mutex_unlock((L)))
+#define thread_safe_add(V,C,L) \
+        (pthread_mutex_lock((L)), (V)+=(C), pthread_mutex_unlock((L)))
 #define thread_safe_sub(V,C,L) \
         (pthread_mutex_lock((L)), (V)-=(C), pthread_mutex_unlock((L)))
-#endif /* HAVE_ATOMIC_ADD */
+#endif
+
+/*
+  statistics_xxx functions are for non critical statistic,
+  maintained in global variables.
+  When compiling with SAFE_STATISTICS:
+  - race conditions can not occur.
+  - some locking occurs, which may cause performance degradation.
+
+  When compiling without SAFE_STATISTICS:
+  - race conditions can occur, making the result slightly inaccurate.
+  - the lock given is not honored.
+*/
 #ifdef SAFE_STATISTICS
-#define statistic_increment(V,L)   thread_safe_increment((V),(L))
-#define statistic_decrement(V,L)   thread_safe_decrement((V),(L))
-#define statistic_add(V,C,L)       thread_safe_add((V),(C),(L))
+#define statistic_increment(V,L) thread_safe_increment((V),(L))
+#define statistic_decrement(V,L) thread_safe_decrement((V),(L))
+#define statistic_add(V,C,L)     thread_safe_add((V),(C),(L))
+#define statistic_sub(V,C,L)     thread_safe_sub((V),(C),(L))
 #else
 #define statistic_decrement(V,L) (V)--
 #define statistic_increment(V,L) (V)++
 #define statistic_add(V,C,L)     (V)+=(C)
+#define statistic_sub(V,C,L)     (V)-=(C)
 #endif /* SAFE_STATISTICS */
-#endif /* thread_safe_increment */
+
+/*
+  No locking needed, the counter is owned by the thread
+*/
+#define status_var_increment(V) (V)++
+#define status_var_decrement(V) (V)--
+#define status_var_add(V,C)     (V)+=(C)
+#define status_var_sub(V,C)     (V)-=(C)
 
 #ifdef  __cplusplus
 }

sql/filesort.cc

@@ -171,11 +171,11 @@ ha_rows filesort(THD *thd, TABLE *table, SORT_FIELD *sortorder, uint s_length,
 
   if (select && select->quick)
   {
-    statistic_increment(thd->status_var.filesort_range_count, &LOCK_status);
+    status_var_increment(thd->status_var.filesort_range_count);
   }
   else
   {
-    statistic_increment(thd->status_var.filesort_scan_count, &LOCK_status);
+    status_var_increment(thd->status_var.filesort_scan_count);
   }
 #ifdef CAN_TRUST_RANGE
   if (select && select->quick && select->quick->records > 0L)
@@ -1120,8 +1120,7 @@ int merge_buffers(SORTPARAM *param, IO_CACHE *from_file,
   THD::killed_state not_killable;
   DBUG_ENTER("merge_buffers");
 
-  statistic_increment(current_thd->status_var.filesort_merge_passes,
-		      &LOCK_status);
+  status_var_increment(current_thd->status_var.filesort_merge_passes);
   if (param->not_killable)
   {
     killed= &not_killable;

sql/handler.cc

@@ -639,7 +639,7 @@ int ha_prepare(THD *thd)
     for (; *ht; ht++)
     {
       int err;
-      statistic_increment(thd->status_var.ha_prepare_count,&LOCK_status);
+      status_var_increment(thd->status_var.ha_prepare_count);
       if ((*ht)->prepare)
       {
         if ((err= (*(*ht)->prepare)(*ht, thd, all)))
@@ -734,7 +734,7 @@ int ha_commit_trans(THD *thd, bool all)
           my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
           error= 1;
         }
-        statistic_increment(thd->status_var.ha_prepare_count,&LOCK_status);
+        status_var_increment(thd->status_var.ha_prepare_count);
       }
       DBUG_EXECUTE_IF("crash_commit_after_prepare", abort(););
       if (error || (is_real_trans && xid &&
@@ -781,7 +781,7 @@ int ha_commit_one_phase(THD *thd, bool all)
         my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
         error=1;
       }
-      statistic_increment(thd->status_var.ha_commit_count,&LOCK_status);
+      status_var_increment(thd->status_var.ha_commit_count);
       *ht= 0;
     }
     trans->nht=0;
@@ -837,7 +837,7 @@ int ha_rollback_trans(THD *thd, bool all)
         my_error(ER_ERROR_DURING_ROLLBACK, MYF(0), err);
         error=1;
       }
-      statistic_increment(thd->status_var.ha_rollback_count,&LOCK_status);
+      status_var_increment(thd->status_var.ha_rollback_count);
       *ht= 0;
     }
     trans->nht=0;
@@ -1252,8 +1252,7 @@ int ha_rollback_to_savepoint(THD *thd, SAVEPOINT *sv)
       my_error(ER_ERROR_DURING_ROLLBACK, MYF(0), err);
       error=1;
     }
-    statistic_increment(thd->status_var.ha_savepoint_rollback_count,
-                        &LOCK_status);
+    status_var_increment(thd->status_var.ha_savepoint_rollback_count);
     trans->no_2pc|=(*ht)->prepare == 0;
   }
   /*
@@ -1268,7 +1267,7 @@ int ha_rollback_to_savepoint(THD *thd, SAVEPOINT *sv)
       my_error(ER_ERROR_DURING_ROLLBACK, MYF(0), err);
       error=1;
     }
-    statistic_increment(thd->status_var.ha_rollback_count,&LOCK_status);
+    status_var_increment(thd->status_var.ha_rollback_count);
     *ht=0; // keep it conveniently zero-filled
   }
   DBUG_RETURN(error);
@@ -1301,7 +1300,7 @@ int ha_savepoint(THD *thd, SAVEPOINT *sv)
       my_error(ER_GET_ERRNO, MYF(0), err);
       error=1;
     }
-    statistic_increment(thd->status_var.ha_savepoint_count,&LOCK_status);
+    status_var_increment(thd->status_var.ha_savepoint_count);
   }
   sv->nht=trans->nht;
 #endif /* USING_TRANSACTIONS */
@@ -1489,7 +1488,7 @@ handler *handler::clone(MEM_ROOT *mem_root)
 
 void handler::ha_statistic_increment(ulong SSV::*offset) const
 {
-  statistic_increment(table->in_use->status_var.*offset, &LOCK_status);
+  status_var_increment(table->in_use->status_var.*offset);
 }
 
 void **handler::ha_data(THD *thd) const
@@ -2836,7 +2835,7 @@ int ha_discover(THD *thd, const char *db, const char *name,
     error= 0;
 
   if (!error)
-    statistic_increment(thd->status_var.ha_discover_count,&LOCK_status);
+    status_var_increment(thd->status_var.ha_discover_count);
   DBUG_RETURN(error);
 }
 

sql/sql_insert.cc

@@ -2538,7 +2538,7 @@ bool Delayed_insert::handle_inserts(void)
 
     if (table->s->blob_fields)
       free_delayed_insert_blobs(table);
-    thread_safe_sub(delayed_rows_in_use,1,&LOCK_delayed_status);
+    thread_safe_decrement(delayed_rows_in_use,&LOCK_delayed_status);
     thread_safe_increment(delayed_insert_writes,&LOCK_delayed_status);
     pthread_mutex_lock(&mutex);
 

sql/sql_parse.cc

@@ -721,8 +721,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
   case COM_INIT_DB:
   {
     LEX_STRING tmp;
-    statistic_increment(thd->status_var.com_stat[SQLCOM_CHANGE_DB],
-			&LOCK_status);
+    status_var_increment(thd->status_var.com_stat[SQLCOM_CHANGE_DB]);
     thd->convert_string(&tmp, system_charset_info,
 			packet, packet_length-1, thd->charset());
     if (!mysql_change_db(thd, &tmp, FALSE))
@@ -757,7 +756,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
       break;
     }
 
-    statistic_increment(thd->status_var.com_other, &LOCK_status);
+    status_var_increment(thd->status_var.com_other);
     thd->enable_slow_log= opt_log_slow_admin_statements;
     db.str= thd->alloc(db_len + tbl_len + 2);
     db.length= db_len;
@@ -773,7 +772,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
   }
   case COM_CHANGE_USER:
   {
-    statistic_increment(thd->status_var.com_other, &LOCK_status);
+    status_var_increment(thd->status_var.com_other);
     char *user= (char*) packet, *packet_end= packet+ packet_length;
     char *passwd= strend(user)+1;
 
@@ -956,8 +955,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
     /* used as fields initializator */
     lex_start(thd);
 
-    statistic_increment(thd->status_var.com_stat[SQLCOM_SHOW_FIELDS],
-			&LOCK_status);
+    status_var_increment(thd->status_var.com_stat[SQLCOM_SHOW_FIELDS]);
     bzero((char*) &table_list,sizeof(table_list));
     if (thd->copy_db_to(&table_list.db, &dummy))
       break;
@@ -1023,8 +1021,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
       LEX_STRING db, alias;
       HA_CREATE_INFO create_info;
 
-      statistic_increment(thd->status_var.com_stat[SQLCOM_CREATE_DB],
-			  &LOCK_status);
+      status_var_increment(thd->status_var.com_stat[SQLCOM_CREATE_DB]);
       if (thd->LEX_STRING_make(&db, packet, packet_length -1) ||
           thd->LEX_STRING_make(&alias, db.str, db.length) ||
           check_db_name(&db))
@@ -1043,8 +1040,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
     }
   case COM_DROP_DB:				// QQ: To be removed
     {
-      statistic_increment(thd->status_var.com_stat[SQLCOM_DROP_DB],
-			  &LOCK_status);
+      status_var_increment(thd->status_var.com_stat[SQLCOM_DROP_DB]);
       LEX_STRING db;
 
       if (thd->LEX_STRING_make(&db, packet, packet_length - 1) ||
@@ -1073,7 +1069,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
       ushort flags;
       uint32 slave_server_id;
 
-      statistic_increment(thd->status_var.com_other,&LOCK_status);
+      status_var_increment(thd->status_var.com_other);
       thd->enable_slow_log= opt_log_slow_admin_statements;
       if (check_global_access(thd, REPL_SLAVE_ACL))
 	break;
@@ -1099,8 +1095,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
   case COM_REFRESH:
   {
     bool not_used;
-    statistic_increment(thd->status_var.com_stat[SQLCOM_FLUSH],
-                        &LOCK_status);
+    status_var_increment(thd->status_var.com_stat[SQLCOM_FLUSH]);
     ulong options= (ulong) (uchar) packet[0];
     if (check_global_access(thd,RELOAD_ACL))
       break;
@@ -1112,7 +1107,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
 #ifndef EMBEDDED_LIBRARY
   case COM_SHUTDOWN:
   {
-    statistic_increment(thd->status_var.com_other, &LOCK_status);
+    status_var_increment(thd->status_var.com_other);
     if (check_global_access(thd,SHUTDOWN_ACL))
       break; /* purecov: inspected */
     /*
@@ -1164,8 +1159,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
 #endif
 
     general_log_print(thd, command, NullS);
-    statistic_increment(thd->status_var.com_stat[SQLCOM_SHOW_STATUS],
-			&LOCK_status);
+    status_var_increment(thd->status_var.com_stat[SQLCOM_SHOW_STATUS]);
     calc_sum_of_all_status(&current_global_status_var);
     if (!(uptime= (ulong) (thd->start_time - server_start_time)))
       queries_per_second1000= 0;
@@ -1201,12 +1195,11 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
     break;
   }
   case COM_PING:
-    statistic_increment(thd->status_var.com_other, &LOCK_status);
+    status_var_increment(thd->status_var.com_other);
     send_ok(thd);				// Tell client we are alive
     break;
   case COM_PROCESS_INFO:
-    statistic_increment(thd->status_var.com_stat[SQLCOM_SHOW_PROCESSLIST],
-			&LOCK_status);
+    status_var_increment(thd->status_var.com_stat[SQLCOM_SHOW_PROCESSLIST]);
     if (!thd->security_ctx->priv_user[0] &&
         check_global_access(thd, PROCESS_ACL))
       break;
@@ -1217,15 +1210,14 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
     break;
   case COM_PROCESS_KILL:
   {
-    statistic_increment(thd->status_var.com_stat[SQLCOM_KILL], &LOCK_status);
+    status_var_increment(thd->status_var.com_stat[SQLCOM_KILL]);
     ulong id=(ulong) uint4korr(packet);
     sql_kill(thd,id,false);
     break;
   }
   case COM_SET_OPTION:
   {
-    statistic_increment(thd->status_var.com_stat[SQLCOM_SET_OPTION],
-			&LOCK_status);
+    status_var_increment(thd->status_var.com_stat[SQLCOM_SET_OPTION]);
     uint opt_command= uint2korr(packet);
 
     switch (opt_command) {
@@ -1244,7 +1236,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
     break;
   }
   case COM_DEBUG:
-    statistic_increment(thd->status_var.com_other, &LOCK_status);
+    status_var_increment(thd->status_var.com_other);
     if (check_global_access(thd, SUPER_ACL))
       break;					/* purecov: inspected */
     mysql_print_status();
@@ -1783,8 +1775,7 @@ mysql_execute_command(THD *thd)
 #ifdef HAVE_REPLICATION
   } /* endif unlikely slave */
 #endif
-  statistic_increment(thd->status_var.com_stat[lex->sql_command],
-                      &LOCK_status);
+  status_var_increment(thd->status_var.com_stat[lex->sql_command]);
 
   switch (lex->sql_command) {
   case SQLCOM_SHOW_EVENTS:

sql/sql_prepare.cc

@@ -2430,7 +2430,7 @@ void mysql_stmt_fetch(THD *thd, char *packet, uint packet_length)
 
   /* First of all clear possible warnings from the previous command */
   mysql_reset_thd_for_next_command(thd);
-  statistic_increment(thd->status_var.com_stmt_fetch, &LOCK_status);
+  status_var_increment(thd->status_var.com_stmt_fetch);
   if (!(stmt= find_prepared_statement(thd, stmt_id, "mysql_stmt_fetch")))
     DBUG_VOID_RETURN;
 
@@ -2494,7 +2494,7 @@ void mysql_stmt_reset(THD *thd, char *packet)
   /* First of all clear possible warnings from the previous command */
   mysql_reset_thd_for_next_command(thd);
 
-  statistic_increment(thd->status_var.com_stmt_reset, &LOCK_status);
+  status_var_increment(thd->status_var.com_stmt_reset);
   if (!(stmt= find_prepared_statement(thd, stmt_id, "mysql_stmt_reset")))
     DBUG_VOID_RETURN;
 
@@ -2598,7 +2598,7 @@ void mysql_stmt_get_longdata(THD *thd, char *packet, ulong packet_length)
 #endif
   DBUG_ENTER("mysql_stmt_get_longdata");
 
-  statistic_increment(thd->status_var.com_stmt_send_long_data, &LOCK_status);
+  status_var_increment(thd->status_var.com_stmt_send_long_data);
 #ifndef EMBEDDED_LIBRARY
   /* Minimal size of long data packet is 6 bytes */
   if (packet_length <= MYSQL_LONG_DATA_HEADER)
@@ -2849,7 +2849,7 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len)
     However, it seems handy if com_stmt_prepare is increased always,
     no matter what kind of prepare is processed.
   */
-  statistic_increment(thd->status_var.com_stmt_prepare, &LOCK_status);
+  status_var_increment(thd->status_var.com_stmt_prepare);
 
   /*
     alloc_query() uses thd->memroot && thd->query, so we should call
@@ -2972,7 +2972,7 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor)
   Query_arena *old_stmt_arena;
   bool error= TRUE;
 
-  statistic_increment(thd->status_var.com_stmt_execute, &LOCK_status);
+  status_var_increment(thd->status_var.com_stmt_execute);
 
   /* Check if we got an error when sending long data */
   if (state == Query_arena::ERROR)
@@ -3094,7 +3094,7 @@ error:
 bool Prepared_statement::deallocate()
 {
   /* We account deallocate in the same manner as mysql_stmt_close */
-  statistic_increment(thd->status_var.com_stmt_close, &LOCK_status);
+  status_var_increment(thd->status_var.com_stmt_close);
   if (flags & (uint) IS_IN_USE)
   {
     my_error(ER_PS_NO_RECURSION, MYF(0));

sql/sql_select.cc

@@ -6282,8 +6282,7 @@ make_join_readinfo(JOIN *join, ulonglong options)
 	join->thd->server_status|=SERVER_QUERY_NO_GOOD_INDEX_USED;
 	tab->read_first_record= join_init_quick_read_record;
 	if (statistics)
-	  statistic_increment(join->thd->status_var.select_range_check_count,
-			      &LOCK_status);
+	  status_var_increment(join->thd->status_var.select_range_check_count);
       }
       else
       {
@@ -6293,15 +6292,13 @@ make_join_readinfo(JOIN *join, ulonglong options)
 	  if (tab->select && tab->select->quick)
 	  {
 	    if (statistics)
-	      statistic_increment(join->thd->status_var.select_range_count,
-				  &LOCK_status);
+	      status_var_increment(join->thd->status_var.select_range_count);
 	  }
 	  else
 	  {
 	    join->thd->server_status|=SERVER_QUERY_NO_INDEX_USED;
 	    if (statistics)
-	      statistic_increment(join->thd->status_var.select_scan_count,
-				  &LOCK_status);
+	      status_var_increment(join->thd->status_var.select_scan_count);
 	  }
 	}
 	else
@@ -6309,15 +6306,13 @@ make_join_readinfo(JOIN *join, ulonglong options)
 	  if (tab->select && tab->select->quick)
 	  {
 	    if (statistics)
-	      statistic_increment(join->thd->status_var.select_full_range_join_count,
-				  &LOCK_status);
+	      status_var_increment(join->thd->status_var.select_full_range_join_count);
 	  }
 	  else
 	  {
 	    join->thd->server_status|=SERVER_QUERY_NO_INDEX_USED;
 	    if (statistics)
-	      statistic_increment(join->thd->status_var.select_full_join_count,
-				  &LOCK_status);
+	      status_var_increment(join->thd->status_var.select_full_join_count);
 	  }
 	}
 	if (!table->no_keyread)
@@ -9377,7 +9372,7 @@ create_tmp_table(THD *thd,TMP_TABLE_PARAM *param,List<Item> &fields,
               (int) distinct, (int) save_sum_fields,
               (ulong) rows_limit,test(group)));
 
-  statistic_increment(thd->status_var.created_tmp_tables, &LOCK_status);
+  status_var_increment(thd->status_var.created_tmp_tables);
 
   if (use_temp_pool && !(test_flags & TEST_KEEP_TMP_TABLES))
     temp_pool_slot = bitmap_lock_set_next(&temp_pool);
@@ -10249,8 +10244,7 @@ static bool create_myisam_tmp_table(TABLE *table,TMP_TABLE_PARAM *param,
     table->db_stat=0;
     goto err;
   }
-  statistic_increment(table->in_use->status_var.created_tmp_disk_tables,
-		      &LOCK_status);
+  status_var_increment(table->in_use->status_var.created_tmp_disk_tables);
   share->db_record_offset= 1;
   DBUG_RETURN(0);
  err: