Hi! Something similar (HAVE_IB_ATOMIC_PTHREAD_T_GCC) was removed in 2b47f8ff03845f7ffe2fa3bd583dd4123dae2b61. IIRC the reason was: relevant code didn't follow this macro properly and this code was still functional. You may use os_thread_pf(os_thread_get_curr_id()) to cast it to ulint. Yes, it is unsafe. But I'd leave it this way assuming it works. This code is subject for refactoring and I would avoid further spread of hacks. Real fix options I could immediately think of: - let all InnoDB threads have id based on some global counter, something like THD::thread_id - if all threads would have been guaranteed to have some per-thread variable (like my_thread_var), we could have used it's address as thread id - SYS_gettid, but it isn't portable (see toku_os_gettid()) - add some mutex and access m_thread_id under mutex protection The bigger problem is: this is not the only code that is accessing pthread_t concurrently. In fact this code is probably least important of these. Regards, Sergey On Mon, Apr 16, 2018 at 11:35:44AM +0200, Sergei Golubchik wrote:

Hi, Teodor!

I'd just say that this MutexDebug thing relies on pthread_t being ulint.

So, I'd add a test to innodb.cmake, like, PTHREAD_T_IS_ULINT or detected that by the compiler in sync0policy.h. And only compiled MutexDebug is pthread_t is ulint. Or, at least, sizeof(pthread_t) == sizeof(ulint).

On Apr 16, Teodor Mircea Ionita wrote:

...

Hi,

While trying to build on macOS 10.13.4 with clang 9.1.0 I originally came across this error: -- .../storage/innobase/include/sync0policy.h:79:4: error: cannot initialize a parameter of type 'int64' (aka 'long long') with an rvalue of type 'os_thread_id_t' (aka '_opaque_pthread_t *') my_atomic_storelint(&m_thread_id, os_thread_get_curr_id()); --- This problem was originally reported in MDEV-15778. Since pthread_t on macOS is a pointer to a struct named _opaque_pthread_t, the "obvious" solution for this was to explicitly cast to ULINT which in turn is defined as size_t, fine so far. Solution was tested in PR#691.

However, upon testing my patch with buildbot, on slave kvm-fulltest2, which is 64bit qemu vm with Ubuntu precise-i386 (32bit) running and gcc compiling in -march=i686. The above "fix" now yields: -- /home/buildbot/buildbot/build/mariadb-10.3.6/storage/innobase/include/sync0policy.h:79:4: error: invalid cast from type ‘os_thread_id_t {aka long unsigned int}’ to type ‘ulint {aka unsigned int}’ ---

Now reconsidering more carefully, this looks more serious then initially thought, posix pthread_t(3) is defined as: -- POSIX.1 allows an implementation wide freedom in choosing the type used to represent a thread ID; for example, representation using either an arithmetic type or a structure is permitted. Therefore, variables of type pthread_t can't portably be compared using the C equality operator (==); use pthread_equal(3)instead. ---

So in this particular platform spec, where pthread_t on Linux is defined as unsigned long int integral type and if LP64 model is used, this implies pthread_t always being 64bit in either both 32 and 64 arch and trying to convert to size_t on a 32bit arch will fail.

Now given that the standard particularly states pthread_t as being opaque and no assumptions should be made in regards to the actual type, not even an initializer type (which ULINT_UNDEFINED defines as -1), trying to cast m_thread_id to any other type ( (ULINT) ) either implicitly or explicit is not portable. Is this a bug that needs fixing or just impossible to handle pthread id atomically in any other fashion? How about a test case I can run to confirm/infirm erratic behavior?

Since I don't have access to the particular bb slave instance to check with sizeof() maybe somebody else can help me here to confirm this? Relevant links bellow:

https://jira.mariadb.org/browse/MDEV-15778 https://buildbot.askmonty.org/buildbot/builders/kvm-fulltest2/builds/12317/s... https://linux.die.net/man/3/pthread_self https://github.com/MariaDB/server/pull/691

Cheers, Teodor

Regards, Sergei Chief Architect MariaDB and security@mariadb.org

_______________________________________________ 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

Hi Sergei, If you disable just DebugMutex but leave UNIV_DEBUG enabled, how do you make assertions like this work properly: `ut_ad(mutex_own(&mutex))`? Regards, Sergey On Mon, Apr 16, 2018 at 01:30:33PM +0200, Sergei Golubchik wrote:

Hi, Sergey!

On Apr 16, Sergey Vojtovich wrote:

...

Something similar (HAVE_IB_ATOMIC_PTHREAD_T_GCC) was removed in 2b47f8ff03845f7ffe2fa3bd583dd4123dae2b61. IIRC the reason was: relevant code didn't follow this macro properly and this code was still functional.

You may use os_thread_pf(os_thread_get_curr_id()) to cast it to ulint. Yes, it is unsafe. But I'd leave it this way assuming it works. This code is subject for refactoring and I would avoid further spread of hacks.

Exactly. It's DebugMutex and I'd say it's not worth fixing for cases when pthread_t doesn't fit in ulint. DebugMutex is completely disabled in non-debug builds anyway.

If we'll ever have a bug that would need DebugMutex to analyze and cannot be repeated on a platform where pthread_t fits in ulint... in that case it might make sense to get back to this. Until then I'd just wouldn't compile DebugMutex where it doesn't work.

Regards, Sergei Chief Architect MariaDB and security@mariadb.org