Partial commit of the greater MDEV-34348 scope.
MDEV-34348: MariaDB is violating clang-16 -Wcast-function-type-strict
Change the type of my_hash_get_key to:
1) Return const
2) Change the context parameter to be const void*
Also fix casting in hash adjacent areas.
Reviewed By:
============
Marko Mäkelä <marko.makela@mariadb.com>
PFS_atomic class contains wrappers around my_atomic_* operations, which
are macros to GNU atomic operations (__atomic_*). Due to different
implementations of compilers, clang may encounter errors when compiling
on x86_32 architecture.
The following functions are replaced with C++ std::atomic type in
performance schema code base:
- PFS_atomic::store_*()
-> my_atomic_store*
-> __atomic_store_n()
=> std::atomic<T>::store()
- PFS_atomic::load_*()
-> my_atomic_load*
-> __atomic_load_n()
=> std::atomic<T>::load()
- PFS_atomic::add_*()
-> my_atomic_add*
-> __atomic_fetch_add()
=> std::atomic<T>::fetch_add()
- PFS_atomic::cas_*()
-> my_atomic_cas*
-> __atomic_compare_exchange_n()
=> std::atomic<T>::compare_exchange_strong()
and PFS_atomic class could be dropped completely.
Note that in the wrapper memory order passed to original GNU atomic
extensions are hard-coded as `__ATOMIC_SEQ_CST`, which is equivalent to
`std::memory_order_seq_cst` in C++, and is the default parameter for
std::atomic_* functions.
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services.
A few different incorrect function type UBSAN issues have been
grouped into this patch.
The only real potentially undefined behavior is an error about
show_func_mutex_instances_lost, which when invoked in
sql_show.cc::show_status_array(), puts 5 arguments onto the stack;
however, the implementing function only actually has 3 parameters (so
only 3 would be popped). This was fixed by adding in the remaining
parameters to satisfy the type mysql_show_var_func.
The rest of the findings are pointer type mismatches that wouldn't
lead to actual undefined behavior. The lf_hash_initializer function
type definition is
typedef void (*lf_hash_initializer)(LF_HASH *hash, void *dst, const void *src);
but the MDL_lock and table cache's implementations of this function
do not have that signature. The MDL_lock has specific MDL object
parameters:
static void lf_hash_initializer(LF_HASH *hash __attribute__((unused)),
MDL_lock *lock, MDL_key *key_arg)
and the table cache has specific TDC parameters:
static void tdc_hash_initializer(LF_HASH *,
TDC_element *element, LEX_STRING *key)
leading to UBSAN runtime errors when invoking these functions.
This patch fixes these type mis-matches by changing the
implementing functions to use void * and const void * for their
respective parameters, and later casting them to their expected
type in the function body.
Note too the functions tdc_hash_key and tc_purge_callback had
a similar problem to tdc_hash_initializer and was fixed
similarly.
Reviewed By:
============
Sergei Golubchik <serg@mariadb.com>
This column (`COUNT_TRANSACTIONS_RETRIES`) is defined as `BIGINT UNSIGNED`
(64-bit unsigned integer) in the user-visible SQL definition:
182ff21ace/storage/perfschema/table_replication_applier_status.cc (L66)
"COUNT_TRANSACTIONS_RETRIES BIGINT unsigned not null comment 'The number of retries that were made because the replication SQL thread failed to apply a transaction.',"
And its value is internally set/updated using the `set_field_ulonglong`
function:
182ff21ace/storage/perfschema/table_replication_applier_status.cc (L231-L233)
case 3: /* total number of times transactions were retried */
set_field_ulonglong(f, m_row.count_transactions_retries);
break;
… but the structure where it is stored allocates only `ulong` for it:
182ff21ace/storage/perfschema/table_replication_applier_status.h (L62)
ulong count_transactions_retries;
As a result of this inconsistency:
1. On any platform where `ulong` is `uint32_t` and `ulonglong` is `uint64_t`,
setting this value would corrupt the 4 bytes of memory *following* the 4
bytes actually allocated for it.
Likely this problem was never noticed because this is the final element in
the structure, and the structure is padded by the compiler to prevent
memory corruption errors.
2. On any BIG-ENDIAN platform where `ulong` is `uint32_t` and `ulonglong`
is `uint64_t`, reading back the value of this column will result in
total garbage.
Likely this problem was never noticed because MariaDB has not been
tested on 32-bit big-endian platforms.
In order not to affect the user-visible/SQL definition of this column, the
correct way to fix this issue is to change it to `ulonglong` in the
structure definition. See
https://github.com/MariaDB/server/pull/2763/files#r1329110832 for the
original identification and discussion of this issue.
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the BSD-new
license. I am contributing on behalf of my employer Amazon Web Services
This reverts commit 03c9a4ef4a.
The fix is wrong. It was doing this: if the uninitialized
wait->m_class has some specific value, then don't initialize it.
When one session SELECT ... FOR UPDATE and holds the lock, subsequent
sessions that SELECT ... FOR UPDATE will wait to get the lock.
Currently, that event is labeled as `wait/io/table/sql/handler`, which
is incorrect. Instead, it should have been
`wait/lock/table/sql/handler`.
Two factors contribute to this bug:
1. Instrumentation interface and the heavy usage of `TABLE_IO_WAIT` in
`sql/handler.cc` file. See interface [^1] for better understanding;
2. The balancing act [^2] of doing instrumentation aggregration _AND_
having good performance. For example, EVENTS_WAITS_SUMMARY... is
aggregated using EVENTS_WAITS_CURRENT. Aggregration needs to be based
on the same wait class, and the code was overly aggressive in label a
LOCK operation as an IO operation in this case.
The proposed fix is pretty simple, but understanding the bug took a
while. Hence the footnotes below. For future improvement and
refactoring, we may want to consider renaming `TABLE_IO_WAIT` and making
it less coarse and more targeted.
Note that newly added test case, events_waits_current_MDEV-29091,
initially didn't pass Buildbot CI for embedded build tests. Further
research showed that other impacted tests all included not_embedded.inc.
This oversight was fixed later.
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
[^1]: To understand `performance_schema` instrumentation interface, I
found this URL is the most helpful:
https://dev.mysql.com/doc/dev/mysql-server/latest/PAGE_PFS_PSI.html
[^2]: The best place to understand instrumentation projection,
composition, and aggregration is through the source file. Although I
prefer reading Doxygen produced html file, but for whatever reason, the
rendering is not ideal. Here is link to 10.6's pfs.cc:
https://github.com/MariaDB/server/blob/10.6/storage/perfschema/pfs.cc
This patch is the result of running
run-clang-tidy -fix -header-filter=.* -checks='-*,modernize-use-equals-default' .
Code style changes have been done on top. The result of this change
leads to the following improvements:
1. Binary size reduction.
* For a -DBUILD_CONFIG=mysql_release build, the binary size is reduced by
~400kb.
* A raw -DCMAKE_BUILD_TYPE=Release reduces the binary size by ~1.4kb.
2. Compiler can better understand the intent of the code, thus it leads
to more optimization possibilities. Additionally it enabled detecting
unused variables that had an empty default constructor but not marked
so explicitly.
Particular change required following this patch in sql/opt_range.cc
result_keys, an unused template class Bitmap now correctly issues
unused variable warnings.
Setting Bitmap template class constructor to default allows the compiler
to identify that there are no side-effects when instantiating the class.
Previously the compiler could not issue the warning as it assumed Bitmap
class (being a template) would not be performing a NO-OP for its default
constructor. This prevented the "unused variable warning".
The macro my_offsetof() performs pointer arithmetics that may be
undefined behavior. As reported in MDEV-26272, it may cause
clang -fsanitize=undefined to generate invalid memory references.
struct PFS_events_statements: Convert to std::is_standard_layout
by encapsulating the standard-layout struct PFS_events instead of
deriving from it, so that the standard macro offsetof() can be used.
PFS_events_statements::copy(): Renamed from copy_events_statements().
A cast to void* is now needed in memcpy() to avoid GCC -Wclass-memaccess
"writing to an object ... leaves 64 bytes unchanged".
set_item() uses 1UL << bit, so is_set_item() must do the same.
This fixes sporadic perfschema.show_aggregate failures
(sporadic, because `bit` is the thread id, so depending on how many
tests were run before perfschema.show_aggregate it can be above or
below 32).
Improve documentation of performance_schema tables by appending COLUMN
comments to tables. Additionally improve test coverage and update corresponding
tests.
This is part of the patch covering newer columns and tables in 10.5.
Improve documentation of performance_schema tables by appending COLUMN
comments to tables. Additionally improve test coverage and update corresponding
tests.
init_mutex_v1_t: Stop lying that the mutex parameter is const.
GCC 11.2.0 assumes that it is and could complain about any mysql_mutex_t
being uninitialized even after mysql_mutex_init() as long as
PLUGIN_PERFSCHEMA is enabled.
init_rwlock_v1_t, init_cond_v1_t: Remove untruthful const qualifiers.
Note: init_socket_v1_t is expecting that the socket fd has already
been created before PSI_SOCKET_CALL(init_socket), and therefore that
parameter really is being treated as a pointer to const.
1. rename option DEPENDENCIES in MYSQL_ADD_PLUGIN() to DEPENDS
to be consistent with other cmake commands and macros
2. use this DEPENDS option in plugins
3. add dependencies to the plugin embedded target too
4. plugins don't need to add GenError dependency explicitly,
all plugins depend on it automatically
The problem is that array binding uses net buffer to read parameters for each
execution while each execiting with RETURNING write in the same buffer.
Solution is to allocate new net buffer to avoid changing buffer we are reading
from.
Though these will all get case to unsigned long
long where it is populated into the perfschema's BIGINT
type.
Use uintptr_t for NetBSD per Nia Alarie's original #1836.
