As reported by Christian and Sergey, nested mutexes following TIP 509 are broken.

Christian has provided the fix by a core list message titled Re: "[TCLCORE] Review of branch bug-87b69745be" 2025-08-13 17:50 UTC.

This fix is now in commit [cdc8e88f7b], which starts branch [893f8cc5-tip509-nested-mutex].

Here are additional informations given in the thread by Christian

For the record: the Win32 implementation of wait on condition seems broken, too. Due to Win32 critical sections supporting nesting, the logical unlock before the wait on condition *must* leave the critical section as many times as it was entered and restore this afterwards. Otherwise the mutex is locked during the wait, and that violates the contract.

Note, that the used wall clock has issues, and eventually TIP 509:

Now we have the "wait on condition" operation, which in the POSIX sense can be timely limited to an absolute point of time, when it is to time out. Why the heck you might ask? Dude, due to the spuriously wakeupery, when you've raced and won not. This is not reflected in any current implementation of the Tcl wrappers. And the fine problem in POSIX was, that absolute meant wall clock in the early days. Until POSIX condition variables learned about CLOCK_MONOTONIC. Which IMO is the only usable way to operate "waiting on condition". Since wall clockery might do things under the hood and out of control.

Ashok

Christian, however you think about it, I appreciate your contribution to this ticket! I consider it done too (unless Ashok sees some value in your latest diff's). Those diff's are - in my opinion - nice-to-haves, but not crucial to testing, since we are not responsible for the internal platform behavior of mutexes.

Again, many thanks!

OK, Jan. Good point. I am finished now with this ticket,
once and for all. As sebres stated many times and me too,
those recursive mutexen are bogus and no sane programmer
will use them. Good luck with carrying on TIP#509 forever
due to being voted positive (although bonkers from its very
beginnings).

I will take this for my future thoughts as a beacon of how
TCT is able to correct problems introduced by TIPs which
later get proved to be wrong.

Thank you for your attention to this matter.

I think I prefer to wait for diff5 ;-)

Now viewed with some distance, I think we should make the start line
of our worker threads into something which resembles a real barrier
as in Formula 1 in order to get the timing as much predictable as possible.
So please try out the 4th diff which should implement this approach.

tclMutexTest.diff3 put in the branch now, for testing

Uploaded too fast, the 2nd diff is buggy. Use the 3rd please.

The first streamlining diff is timing sensitive, see the
attached tclMutexTest.diff2 for an improved version.

How about slightly streamlining the startup of the test threads? See attached diff for the idea.

Yes, I noticed that on macos CI as well and was wondering the same thing.

Really that check was to ensure there was no egregious bug that only ran one thread on wake up. Otherwise, that test only lands up testing the fairness of the OS scheduler, I think. So I think it is reasonable to make that 40% or even 25%.

Could you please make the change? I'm just about packed up and ready for my travel.

@ashok. I see the following failure - sometimes - on MacOS:

==== mutex-condition-3 mutex condition 10/1/20000/0 FAILED
==== Contents of test case:

lassign  [testmutex condition $nthreads $recursion $iters $yield]  enqTotal enqPerThread enqTimeouts  deqTotal deqPerThread deqTimeouts
                list  $enqTotal [fairness $totalOps $enqPerThread] $enqTimeouts  $deqTotal [fairness $totalOps $deqPerThread] $deqTimeouts


---- Result was:
200000 {Thread share 19134 < 0.5 fair share 40000} 0 200000 {} 0
---- Result should have been (exact matching):
200000 {} 0 200000 {} 0
==== mutex-condition-3 FAILED

I don't think this is related to recursive mutexes at all, since it only shows on MacOS. Can this testcase be backported to 8.6, so we can see if it happens there too? Could we change the factor from 0.5 to 0.4, would that be reasonable?

> I don't known such __atomic_something that can be affected by misalignment. Atomic remains atomic, no matter intrinsic (matter of compiler) or functions (the barrier is provided internally, e.g. with proper CPU instruction). In worse case you'd get a compiler error by misalignment. 

Even better: creative struct alignment can according to this [read](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110773) nicely crash the SIGBUS.

> And last but not least, I'm basically against home made as well as reentrant mutexes - neither the former nor the latter is really needed by Tcl core. 

Fully agree. TIP#511 was the reason to fix the initial and nonfunctional use case of TIP#509.

> So now we want out of pure esoterism enter the land of undefined behavior?

There are architectures and ABIs, for instance Solaris (-m32), which require 4 bytes alignment to work properly (for most of the CPU instructions), but 8 bytes alignment to guarantee atomic read/write.

> We want our home grown mutex struct specially crafted to have the thread id misaligned?

It is enough to inline "home grown mutex struct" into another structure without alignment after odd count of int16/int32 members. Or use some memory allocator without necessarily aligned memory. Or something like that.

> What about the fine __atomic_something stuff on unaligned memory then?

I don't known such __atomic_something that can be affected by misalignment. Atomic remains atomic, no matter intrinsic (matter of compiler) or functions (the barrier is provided internally, e.g. with proper CPU instruction). In worse case you'd get a compiler error by misalignment.

And last but not least, I'm basically against home made as well as reentrant mutexes - neither the former nor the latter is really needed by Tcl core.

So now we want out of pure esoterism enter the land of undefined behavior?  
We want our home grown mutex struct specially crafted to have the thread id misaligned?  
What about the fine __atomic_something stuff on unaligned memory then?

> This could indeed happen, when on a target platform a pthread_t were a complex struct

No. As I already wrote, this can also happen with scalar data (e. g. 64-bit int or pointer on 64-bit platform) if the variable is not properly aligned in memory, e. g. placed in a structure without alignment after some members not aligned to 64-bit (e. g. after int16 or int32). However it depends (platform, CPU, etc).

> Now take into account, that the pMutex->thread variable is not written atomically, then we could have:
>
>    00 => 0M => MM => M0 => 00 => 0Y => YY => Y0 => 00 

If this claim were true, atomic read operations are most likely at no avail and thus no remedy and we then have the need of a pthread_spin_lock_t. This could indeed happen, when on a target platform a pthread_t were a complex struct, which is explicitly possible according to the opengroup.org explanation.

To handle this case we could do: 

```
#ifndef PTHREAD_NULL
#   define PTHREAD_NULL (pthread_t)NULL
#endif

typedef struct PMutex {
    pthread_mutex_t mutex;
    pthread_spin_lock_t lock;  
    volatile pthread_t thread;
    int counter;
} PMutex;

static void
PMutexInit(PMutex *pmutexPtr)
{
    pthread_mutex_init(&pmutexPtr->mutex, NULL);
    if (sizeof(pmutexPtr->thread) != sizeof(void *)) {
	pthread_spin_init(&pmutexPtr->lock, PTHREAD_PROCESS_PRIVATE);
    }
    pthread_mutex_lock(&pmutexPtr->mutex);
    pmutexPtr->thread = PTHREAD_NULL;
    pmutexPtr->counter = 0;
    pthread_mutex_unlock(&pmutexPtr->mutex);
}

static void
PMutexDestroy(PMutex *pmutexPtr)
{
    assert(pthread_equal(pmutexPtr->thread, PTHREAD_NULL) && !pmutexPtr->counter);
    if (sizeof(pmutexPtr->thread) != sizeof(void *)) {
	pthread_spin_destroy(&pmutexPtr->lock);
    }
    pthread_mutex_destroy(&pmutexPtr->mutex);
}

static void
PMutexLock(PMutex *pmutexPtr)
{
    pthread_t mythread = pthread_self();
    int equal;

    if (sizeof(pmutexPtr->thread) != sizeof(void *)) {
        pthread_spin_lock(&pmutexPtr->lock);
    }
    equal = pthread_equal(pmutexPtr->thread, mythread);
    if (sizeof(mythread) != sizeof(void *)) {
        pthread_spin_unlock(&pmutexPtr->lock);
    }
    if (equal) {
	pmutexPtr->counter++;
    } else {
	pthread_mutex_lock(&pmutexPtr->mutex);
	pmutexPtr->thread = mythread;
    }
}
  
static void
PMutexUnlock(PMutex *pmutexPtr)
{
    if (sizeof(pmutexPtr->thread) != sizeof(void *)) {
#ifndef NDEBUG
	int equal;
#endif
	assert(
	    pthread_spin_lock(&pmutexPtr->lock),
	    equal = pthread_equal(pmutexPtr->thread, pthread_self()),
	    pthread_spin_unlock(&pmutexPtr->lock),
	    equal
	);
    } else {
	assert(pthread_equal(pmutexPtr->thread, pthread_self()));
    }
    if (pmutexPtr->counter) {
	pmutexPtr->counter--;
    } else {
	pmutexPtr->thread = PTHREAD_NULL;
	pthread_mutex_unlock(&pmutexPtr->mutex);
    }
}  
```

OK, you could argue, that there could be a pathological case were a sixteen core 128 bit system has a 16 bit memory interface. But does such a beast exist in practice?

> So the conclusion is that we do not need atomic reads for the tests.

I share that conclusion, but let's elaborate.

We can see the pMutex->thread variable as a state machine. When thread MM does some operation after thread YY, the state transitions are:

00 => MM => 00 => YY => 00

Now take into account, that the pMutex->thread variable is not written atomically, then we could have:

00 => 0M => MM => M0 => 00 => 0Y => YY => Y0 => 00

That's a lot more states to consider. Now consider how thread MM sees this. Within the same thread, the processor always guarantees that operations involving the same memory are always kept in order. When thread MM sets the pMutex->thread variable to 'MM' and later to '00', it will never see the intermediate states as other threads might see them. So, we get:

00 => MM => 00 => 0Y => YY => Y0 => 00

Now the only situation left we have to worry about is when thread MM tries to acuire the lock. Before that, it has to check that pMutex->thread == 'MM', then it already has the lock. But the states '00', '0Y', 'YY' and 'Y0' are all not equal to 'MM'. So it doesn't really matter whether thread MM sees the intermediate states, the result of the == operator stays the same.

The conclusion is that we don't need the atomic states for the reads.

I also changed the code to use THREAD_NULL and pthread_equal(), as suggested.

See: [8d7d7b1c144bfca2|here]

Ashok's testcases already succeed: https://github.com/tcltk/tcl/actions/runs/18186353547!

Now lemme refine my reasoning:

0. We understand from the opengroup.org explanation, that both, mutex lock and unlock operations perform some memory barrier or fence magic to ensure memory consistency of the memory protected by the mutex.

1. Then in our mutex init function we can do  
   pthread_init();  
   pthread_mutex_lock();  
   set thread id to PTHREAD_NULL  
   set counter to 0  
   pthread_mutex_unlock();  

2. We have ensured, that our home grown mutex is usable only after its init function has completed. Therefore the thread id in our freshly initialized mutex is safely set to PTHREAD_NULL.

3. In our mutex lock function we test the thread id of the mutex against our own thread id. This need not be atomic since we guarantee that we write the mutexes' thread id only when we really locked the native mutex.

4. In our mutex unlock function we write the mutexes' thread id to PTHREAD_NULL before we unlock the native mutex to indicate that we gave up ownership.

When the mutexes' thread id is read unlocked for testing of ownership, it thus can have these three possible values:

1. PTHREAD_NULL
2. own thread id
3. some other thread id

The test in lock and unlock (and in the condition variable cases) looks for the mutexes' thread id being equal to our own thread id (which implies that we hold the native mutex) or anything else including PTHREAD_NULL.

So the conclusion is that we do not need atomic reads for the tests.

Other than that, to enhance portability, please use pthread_equal() for testing thread ids and use PTHREAD_NULL (with an own definition to 0 if needed) instead of assigning 0 directly.

@ashok, +1 to all your remarks. You testcases are great, I'm a happy user.

Next step in Christians reasoning (it's indeed about over-engineering) can be found [27ba494cc3a179e0|here]. We need to assure that pmutexPtr->thread is fully initialized when the mutex is used first. Idea: initialize pmutexPtr->thread _before_ initializing pmutexPtr->mutex. The resulting assembler-code for PMutexInit becomes much shorter:

        mov     QWORD PTR [rdi+40], 0; # pthread->thread = 0
        xor     esi, esi             ; # NULL argument of pthread_mutex_init
        mov     DWORD PTR [rdi+48], 0; # pthread->counter = 0
        jmp     pthread_mutex_init

Since pthread_mutex_init addresses [rdi+0] and memory-operations are not re-ordered by the processor, this also assures that [rdi+40] is fully written to memory before [rdi+0]. We see that the null-argument of pthread_mutex_init() is set before pthread->counter is set, this gives pthread->thread a little bit more time to be really written to memory.

Since memory-operations are not re-ordered by the processor, we can be sure this way that pthread->thread (and pthread->counter) are initialized before the lock is used. Without using any extra locks.

With this change, PMutexLock() is the last function which uses atomic operations. But that's for another day.

And fwiw, the tests in apn-mutex-tests hang with 9.0.2 (as expected!) but not with core-9-0-branch so that's a good sign :-)

Jan,

> Ashok, Sergey, do you as well? 

As I said on the call, while I have experience *using* synchronization constructs, I don't feel I am competent enough in terms of *implementing* them so I think my contribution will have to be limited to writing test cases :-)

I think since you and Sergey are at agreement (Sergey's feelings on recursive mutexes being a separate issue) and if I understood Christian correctly, his comments were more on modifications being unneeded/over-engineered as opposed to faulty behavior, I think there is no disagreement.

/Ashok

Not directly connected to this ticket but related - I have added tests for mutex locks and condition variables in branch apn-mutex-tests. 

Purpose is to eventually change the Windows implementation to use native condition variables as in the POSIX implementation instead of rolling our own. I don't feel current tests in the test suite are adequate to ensure no breakage occurs, particularly since the github CI skips them entirely!

Review and improvements from anyone with copious time on their hands would be welcomed :-)

Well, I still hope we'd revert everything to pre TIP 509 and provide that as an extra API (if needed at all).

Regarding [6c6604aee18eae14], are not there some typos - `#ifndef NDEBUG` vs `#ifdef NDEBUG` (some panics are in debug another vice versa)? Another thing - if debug only, why cannot it be simple rewritten as an assert instead?

As for Christians statement, I guess he meant the initial variant without atomic stuff completely, so one'd "speculate" that the thread/counter vars going changed in lock only and read operations on them are either safe without lock, or happen atomically (again, speculating they are properly aligned, access not reordered by compiler, etc). My opinion is that interlocking makes it more robust and may prevent against some UBs in certain circumstances (e. g. compiled there, where pointer to Tcl_Mutex gets not properly aligned, so read cannot happen atomic, because high and lower words becomes read separately). But...

My objections have orthogonal nature either and goes to the reentrant mutexes as such. Like associated with it extra overhead, cache washouts, additional sensibility to memory fragmentation, especially by "neglecting" of CPU cache granularity (which was always there due to thread-alloc, but with reentrant mutexes becomes worse)... Everything that what is influenced by new vars and totally unneeded in Tcl-core in all general facilities, currently protected by Tcl_Mutex.
To this day, no one has explained to me why a feature was implemented this way and not as new API. Getting ahead of the game, the hypothetic argument that this will immediately "fix" the existing code (using Tcl_Mutex but theoretically implying recursion) is not an argument at all, IMHO.

So, Christian, that's a long explanation. Let's divide it into parts.

First you say that there is no need to put a memory barrier around memory access, when there is already one.

In addition (not in your story), I think new panic's are only needed in DEBUG mode.

This results in [6c6604aee18eae14]. Then the only places where still atomic functions left are the MutexInit, MutexDestroy and MutexLock (not in the Unlock and ConditionWait any more). That's already an improvement. I think I understand this. Ashok, Sergey, do you as well?

(part 2 for another day, let's first look at this)

[CI failed again](https://github.com/tcltk/tcl/actions/runs/18056877013/job/51387842541) in aaa_exit.test (build with symbols and mem-debug).

However this time it was definitely performance thing - the time ran out after 1 second (test took longer than a timeout event).

So I increased the timeout for mem-debug build (for all branches) and rewrote the tests a bit, but...

This could just be a temporary slowdown of GHA, as well as a sign of slowing down caused by the new reentrant mutex. Just kidding... although, when I think about it twice...

Ashok, as far as I am able to remember, the problem of the
initial implementation of the recursive mutexes (the POSIX
variant without native recursion support) raised sooner or
later the question about memory safety, consistency, or
whatever we like to name it, which was you brought up by you.

In the follow up additional code and complexity to the
mutex implementation was added.

Now the fine point is, reading 

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12

that consistence is ensured within the critical region.
The reverse conclusion thus must be: if the critical
region is initialized "critically" and only written
"critically" the state read from outside is somewhat 
consistent, too. At least can it only be in two
possible states in between (thread identifier being
PTHREAD_NULL or not). And that fits tests of a
thread identifier being not equal to some other thread
identifier. And moreover does it not require accesses
to be of somehow atomic or otherwise nuclear kind.

My only point is, that we could have lived well with
the initial implementation, if we would have added
initial locks, when we initialize our mutex structure.
And of course ditched the native recursive POSIX stuff
in favor of our own breed due to the misfit with
POSIX condition variables.

So the pmutexPtr->thread value is either our pthread_self()
or not. And this condition is *always* ensured when we
modify it only when we hold the critical section. Even
when we give up ownership by writing PTHREAD_NULL.

Most of this argumentation can be translated to Win32
as well.

Sorry Christian, I'm not sure which of my positions you are arguing against. To be clear, 

- I'm not against recursive mutexes, it's just that I don't think they (at least currently) have a use case. I am uncertain about whether you are in favor of recursive mutex or not (not always being able to distinguish the sarcasm!)

- my last post had nothing to do with recursive mutexes. The value check followed by locking issue is applicable to non-recursive cases as well. And having said that, I understand Sergey's point about complex structure vs scalars.

Anyways, I think what Jan and Sergey currently have in place is fine until this debate arises again!

I didn't misunderstand... And although, **generally** spoken, you are right and the role of the mutex is exactly to protect data and to provide consistence, but in most cases to protect non-volatile data - e. g. complex structures with more than 1 variable, to behave consistently... to read single value it is not always needed to be "protected", till it is consistently changed (and properly aligned for atomic read, etc). Timing issue is orthogonal to mutex protection (and in case of mentioned encoding switch, we have exactly the timing issue and not some inconsistence).

Anyway, that always depends, but changes nothing what I said about single lock or futility of recursive mutexes.

> ... the reason why the double checked locking checks for the value **again**
> after obtaining the lock and does not rely on the preceding check.

Ashok, and here we are again. Let us read carefully the beginnings of
the recursive mutexing mystery (POSIX without native recursive support):

  /* unlocked initial test */
  if (currentthread != owningthread || counter == 0) {
      pthread_mutex_lock(mutex)
  /* ... */

owningthread can be PTHREAD_NULL or a thread identifier. If it were
the current thread, the counter should be greater than zero, else
the RAM and caches would be bollocks. Otherwise and all else the mutex
*must* be locked. And memory *must* be consistent then and we initialize
all respective memory to a well known state.

My conclusion after all is that we are in the process of full spectrum
overengineering.

Moreover, this *must* be the same with Win32 CRITICIAL_SECTION, i.e.
the mutex entry must imply some kind of memory barrier, fence or how
it might be named in the jargon of the processor architecture.

Even more did I prove with the accepted TIP#511, that recursive
mutexen add zero value to the POSIX signals process versus threads
dilemma. And was in the year of 2018 already strongly against those
recursive mutexes.

So please ditch it. Ultimately. And maybe keep for the boneheads some
alternative interface. Maybe calling it even

 Tcl_RecursiveMutex(Unl|L)ockForTheBoneheads()

with an explicit statement in the documentation that this kind of
idiotsyncrazy is a powder keg for condition variables.

Thank you for your attention to this matter.

> Both effects are a timing issue...

I think you misunderstood what I was trying to say. Talking about the **general** case, not just this situation where it does not matter (as I already pointed out), when an action is taken that is dependent on a value, both the value comparison and the action must be under the same lock. It is not a question of which thread wins. That is always true. It is a question of consistency. It is the reason why the double checked locking checks for the value **again** after obtaining the lock and does not rely on the preceding check.

> Still, in this case no harm is done because even in the very unlikely case this happens, the only effect is a needless encoding change and epoch bump.

Or vice versa, it may not detect the change of encoding by another thread, but then, it'd return before lock (and think the encoding is still the same as given). Both effects are simple a timing issue and a lock has nothing to do with that. Few nanoseconds earlier or later it'd behave different and the last thread wins here, no matter with or without lock.

> So we have 2 dogma's: "thy shall not use recursive mutex" and "thy shall use a single lock within a compare-and-modify operation".

No. The "usage of single lock" is mostly the same as "not use recursive mutex"... in that particular case it'd be enough to provide GetEncoding() without lock (in opposite to Tcl_GetEncoding(), similar to FreeEncoding()/Tcl_FreeEncoding() pair) and use that GetEncoding within the single lock inside Tcl_SetSystemEncoding. But as already convinced - unneeded here, because there is no harm at all.

Recursive mutex can be only needed if in some case is too hard to predict whether mutex was already locked by current thread or not. 99.99% of any cases may be rewritten to guarantee that, especially if one'd keep in mind that to be optimal a lock must be as short as possible, what is contradicting with a recursion a bit. Yes, there are surely cases where a recursion shall be protected by mutex, but it is never unavoidable, so either it can be protected once at first level (or outside) or much better unfolded to cycle or data items somehow marked as busy on every iteration... There are many known programming patterns to achieve that.

> we could implement an internal TclMutexStrictLock() function which does the same as Tcl_MutexLock(), except it panics when it is called recursively.

Well, the implementation like this would get all cons of recursive mutex and has no pros at all, excepting a possible panic instead of hang in case of nesting invocation. The idea is not bad, but for debug (+ assert) mode maybe.

Anyway, I think the best we can do would be to revert everything to non-recursive mutexes and to provide recursive mutexes as new API, e. g. like Tcl_RecMutexLock() / Tcl_RecMutexUnlock(). But, as already said it is something outside of best practice and the percent of real use-cases for that are so measly, and if really needed can be almost always achieved on user side without to have reentrant mutexes.

Thanks!

So we have 2 dogma's: "thy shall not use recursive mutex" and "thy shall use a single lock within a compare-and-modify operation". Here we have an example where those two rules are conflicting. In some situations the first rule is more important, in other situations the second one.

That also means we have a valid use-case for recursive mutexes. For some more complicated compare-and-modify operations it might be not so easy to implement it without a recursive mutex. This one is quite easy, so I tend to agree with both Ashok and Sergey that it's best to change tclEncoding.c. This way we don't have to bother with recursive mutexes any more in the core (but it might be useful for more complicated situations in extensions).

So, we could implement an internal TclMutexStrictLock() function which does the same as Tcl_MutexLock(), except it panics when it is called recursively. In order to do that we still need a Interlocked function to retrieve the "thread" field, in order to determine that. So we don't save anything at all compared to just using Tcl_MutexLock() everywhere. I don't think it's worth it, neither to export this function as Tcl_MutexStrictLock(). It would be even less worth to duplicate all mutex functions in a non-recursive and a recursive variant.

Let's talk about this next meeting.

I think it's ok although in general a compare-and-modify operation should be within a single lock. Here the comparison with systemEncoding is done outside of the mutex and so could change (from another thread) by the time the lock is obtained. Still, in this case no harm is done because even in the very unlikely case this happens, the only effect is a needless encoding change and epoch bump.

So fine with me.

@Ashok, would you like to review [137d478d0e0c5b6f|this]?

Thanks, Sergey, for all your help!

CI failed again - GHA#runs/18000251395/job/51215850058, this time with symbols only (mem-debug was not affected because it doesn't use thread-alloc).

Fixed in [cfc51c43785413bd], since the mutex is not a simple critical section anymore, so allocMutex is larger now (and therefore the heap going broken).

But... (I'll not stop to repeat)...

It bothers now the thread-alloc too... The recursive mutex is completely unneeded there as basically everywhere in 100% of Tcl code, so please, lets revert this completely and in best case provide the TIP 509 as new API (for people that need it), however I don't ever think we shall do that within tcl itself (and not as some module). Just because TIP 509 was and still is an error and recursive mutexes are evil, introduce conceptual bugs and needed only on some very special cases. Anyway definitely not as replacement for default Tcl_Mutex.

Yep... The fix looks good to me.<br/>
Moreover it is pretty well reproducible with VC memdebug build, exactly like within GHA before the fix:

<details>

```
win> nmake -f makefile.vc OPTS=symbols STATS=compdbg,memdbg test TESTFLAGS="-f \"*exit.test enc*.test\""
...
d:\Projects\tcl9.0-core-upstream\win\Debug_AMD64_VC1942\tcltest90.exe "d:/Projects/tcl9.0-core-upstream/win/../tests/all.tcl" -f "*exit.test enc*.test" -loadfile T:\Temp\nm32.tmp
Tests running in interp:  d:/Projects/tcl9.0-core-upstream/win/Debug_AMD64_VC1942/tcltest90.exe
Tests located in:  D:/Projects/tcl9.0-core-upstream/tests
Tests running in:  D:/Projects/tcl9.0-core-upstream/win
Temporary files stored in D:/Projects/tcl9.0-core-upstream/win
Test files run in separate interpreters
Running tests that match:  *
Skipping test files that match:  l.*.test
Only running test files that match:  *exit.test enc*.test
Tests began at 2025-09-24 22:09:52 +0200
aaa_exit.test


==== exit-1.2 full-finalized exit FAILED
==== Contents of test case:

    set f [open "|[interpreter] << \"exec [interpreter] << {set ::env(TCL_FINALIZE_ON_EXIT) 1;exit}\"" r]
    set aft [after 1000 {set done "Full-finalized exit hangs !!!"}]
    fileevent $f readable {after cancel $aft;set done OK}
    vwait done
    if {$done != "OK"} {
        fconfigure $f -blocking 0
        close $f
    } else {
        if {[catch {close $f} err]} {
            set done "Full-finalized exit misbehaves: $err"
        }
    }
    set done

---- Result was:
Full-finalized exit hangs !!!
---- Result should have been (exact matching):
OK
==== exit-1.2 FAILED

encoding.test


==== encoding-24.2 EscapeFreeProc on open channels FAILED
==== Contents of test case:

    # Bug #524674 output
    runInSubprocess {
        encoding system cp1252; # Bug #2891556 crash revelator
        fconfigure stdout -encoding iso2022-jp
        puts ab乎棙g
        set env(TCL_FINALIZE_ON_EXIT) 1
        exit
    }

---- Test generated error; Return code was: 1
---- Return code should have been one of: 0 2
---- errorInfo: ab8CD%g
child killed: segmentation violation
    while executing
"exec [interpreter] $theFile"
    (procedure "runInSubprocess" line 4)
    invoked from within
"runInSubprocess {
        encoding system cp1252; # Bug #2891556 crash revelator
        fconfigure stdout -encoding iso2022-jp
        puts ab乎棙g
        set env(TCL_FINALIZ..."
    ("uplevel" body line 3)
    invoked from within
"uplevel 1 $script"
---- errorCode: CHILDKILLED 22336 SIGSEGV {segmentation violation}
==== encoding-24.2 FAILED


Tests ended at 2025-09-24 22:10:34 +0200
all.tcl:        Total   234     Passed  228     Skipped 4       Failed  2
Sourced 2 Test Files.
Files with failing tests: aaa_exit.test encoding.test
Number of tests skipped for each constraint:
        4       perf
```
</details>

And not reproducible anymore after latest fix of allocMutex.

Strange is that it was not reproducible also within [gcc (--enable-symbols=all)](https://github.com/tcltk/tcl/actions/runs/17969599841/job/51113679485), which implies mem-debug too.
I expected it to fail there too.

My guess is the problem is caused by Tcl_GetAllocMutex().

exit-1.2 and encoding-24.2 are failing on Windows (OPTS=symbols STATS=compdbg,memdbg only). Investigating (ideas welcome!)

Thanks Sergey! Good idea

I added small amend [54c1a0b44dddce9b] with atomic load (similar to unix now) - although the read of (properly aligned) integer variable is always atomic, it was not synchronized with another (write) access, so to ensure there is no some RC and to implement it more similar to unix mutexes, it got a full mem-barrier now (by an intrinsic or an instruction).

> however for unix, since in both of else-cases in PMutexLock, the mutex gets always locked (with pthread_mutex_lock), the atomic exchange is not really needed

You are right, and calling the atomic exchange doesn't save us time either. I now merged the two else's.

UNIX part of the fix now committed to core-9-0-branch and trunk.

For Windows, I will have a further look.

> What if we let "counter" only count the additional locks after the first one in a thread. Then we only update "counter" when we already have the lock

But exactly that happened in original PMutexLock (what Ashok complained) and that was exactly my point - the counter would be only incremented if the thread acquired lock. The difficulty thereby (which is the potential for the hypothetical RC at the same time) to do this second time without locking twice (because otherwise it'd be an UB or even a deadlock). Thus one'd need 2 variables (lock owner and counter), which state cannot ever be changed atomically, only in lock and the safe handling will be achieved solely through correct and well thought out logic.

The issues with RC start always on border line under certain circumstances (especially if there are many factors, like compiler optimization etc).

Your change looks fine at first glance, however for unix, since in both of else-cases in PMutexLock, the mutex gets always locked (with pthread_mutex_lock), the atomic exchange is not really needed and then we are again by initial version. Windows version of WMutexLock looks similar (however doesn't compare `wmPtr->thread == mythread` in atomic way). As already said, I don't think even original version was affected by RC, although my doubts still remain more orthogonal to it - the performance losses, the cache washout and increased context switches.

So I'd not stop to repeat - Tcl doesn't need recursive mutexes at all. And if yes - it'd be a "feature" either and one shall provide it as completely new API, not as replacement for non-reentrant mutexes.

I re-implemented everything in tclUnixThrd.c, with the following idea:

What if we let "counter" only count the additional locks after the first one in a thread. Then we only update "counter" when we already have the lock, so we don't have to worry about it doing it atomic. It makes things much simpler. Also, as long as the mutex is only used non-recursive, the "counter" will always be 0.

Windows not updated yet, so don't look there ;-)

This way, it is also easier to follow the possible race conditions, I don't think there are anymore.

Please find my second attempt with spin locks in the spin2.diff attachment.

Ashok, I like to spin the lock even more after this read:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12

> there is no guaranteed that those memory locations are written back into main memory or synchronized with other caches.

There is (as for single volatile variable).

The cache of modern CPUs is always coherent (if we don't speak about GPUs or some special processors) - that is simply a basic requirement.
Volatile would "guarantee" the "atomicity" and durability (of single variable) and prevents against reordering or aggressive optimization issues. One cannot use volatile to synchronize access to non-volatile data, but it doesn't happen here.
So without memory barriers we can only doubt the atomicity of two variables (thread and counter) together, but because they would be changed only in the lock and because counter is interesting only if the thread owned the lock, it is OK for the second thread, because if it enters the lock, the counter must be again 0 (after unlock of other thread). I don't see the RC here right now (for the code pieces you provided before), but one have to lock closer on the whole solution (no time currently).

However I am anyway for real atomic facilities, which would provide much more better variants to handle that, without to think about memory barriers and may prevent many RC scenarios.

@Christian, @Jan,

Do not know exact semantics of those _atomic* operations but I get really nervous about building higher level synchronization primitives on top of the low level atomic operations. I liken it to implementing crypto algorithms. Minefields are well hidden.  I'd much rather punt on recursive mutexes but if the collective decision is otherwise, in my opinion anything along those lines needs really careful independent review by **multiple** pairs of eyes before acceptance. 

Just my opinion.

Sorry, no time to review and comment on the code for at least a few days.

> I don't think so, and even with aggressive optimization level the compiler wouldn't generate the code doing that.

My concerns had nothing to do with compilers or optimization levels, with or without volatile attributes. They have to do with cache coherency in multiprocessor systems. Thinking about it further, I'm even more convinced there is a race condition. Scenario:

1. Thread A on processor X successfully takes out the lock.
2. It updates the thread id and count fields in the mutex wrapper struct. However, **until there is a memory barrier or similar instruction, generally effected by synchronization primitives there is no guaranteed that those memory locations are written back into main memory or synchronized with other caches.**
3. Thread B on processor Y checks the value of the count and thread id BEFORE thread A releases the lock (which would have caused the processor cache flush). However, the values B sees are stale because neither main memory not processor Y cache have been updated with the new values from processor X cache.
4. What happens next depends on the stale values. If the stale thread id is X and thread count is not 0, B will think it already owns the mutex and proceed without trying to lock it.
5. Thus both A and B think they own the lock and have a race condition.

> Or like in [a1c84c80695cce88|this] commit?

Yes, such a thing would be very usefull!

For Tcl 9.1 we plan to only support C compilers "supporting the mandatory features of the C11 standard". See TIP #715, which isn't voted upon yet. I don't know if the atomic functions are mandatory in C11, but wrapping them in a simpler API would be a good idea anyway (IMHO).

Thanks!

> I am pretty sure accessing the thread and counter fields outside of any locks or memory barriers can result in stale values being read with the caller erroneously thinking it is holding the lock when it isn't and foregoing executing the `pthread_mutex_lock`.

I don't think so, and even with aggressive optimization level the compiler wouldn't generate the code doing that. However I can imagine necessity for volatile declarations for some vars.

What doesn't change my opinion about recursive mutexes.

> Can we fix this using atomic operations? Something like this

Or like in this commit? It uses native atomic functions. Also it'd not need C11 or C23 (I ported that atomic facilities initially from nginx, where they appeared dozen years before C11).

Anyway having that interlocked facilities in Tcl would definitely provide more benefits (I missed it often in core tcl for atomic incr/decr reference counters in structures used across threads, like in that encoding commit). Often one doesn't need mutex at all (where is no lock - there is no wait).

> So we want to enforce C11? Why not C23 then?

That's why my experiment is still WIP. I'm open to any possible solution!

So we want to enforce C11? Why not C23 then?

Or can't we stick at good'ole POSIX using spinlocks,
as outlined in the attached diff.

Can we fix this using atomic operations? Something like [10bcfc0e125e9575|this]?

I confirmed that Windows native condition variables also only work when the critical section lock is held exactly once so Christian's changes are appropriate for Windows as well.

However, now having looked at the recursive implementation for POSIX (which is essentially now cloned for Windows), I am not convinced the implementation is correct in the first place (i.e. before Christian's changes).

Consider the lock function:

```
static void
PMutexLock(
    PMutex *pmutexPtr)
{
    if (pmutexPtr->thread != pthread_self() || pmutexPtr->counter == 0) {
	pthread_mutex_lock(&pmutexPtr->mutex);
	pmutexPtr->thread = pthread_self();
	pmutexPtr->counter = 0;
    }
    pmutexPtr->counter++;
}
```

I am no expert on architectures, but I am pretty sure accessing the thread and counter fields outside of any locks or memory barriers can result in stale values being read with the caller erroneously thinking it is holding the lock when it isn't and foregoing executing the `pthread_mutex_lock`.

The equivalent code on Windows (added by Christian) is

```
    if (wmPtr->thread != Tcl_GetCurrentThread() || wmPtr->counter == 0) {
	EnterCriticalSection(&wmPtr->crit);
	wmPtr->thread = Tcl_GetCurrentThread();
	wmPtr->counter = 0;
    }
    wmPtr->counter++;
```

and suffers from the same issue.

On Windows, the fix should be simple by moving the EnterCriticalSection above the `if` statement since critical sections can be recursively locked. This is not possible on POSIX for obvious chicken and egg reasons as the mutex is not recursively lockable.

A possible solution is to add another internal mutex to the mutex wrapper that controls access to the counter and thread but that needs deeper thought to determine whether it is indeed a fix and does not cause problems of its own.

This gets back to Sergey's comment about implementing synchronization mechanisms being dicey and even if the above suggested additional mutex is deemed to work, we should explore eliminating recursive mutexes.

As an example, it was only a few years ago that I learnt that the double locking idiom that I had long presumed was safe (and is in fact extensively used in Tcl) in fact has race conditions on some architectures and platforms (thankfully, I *think* not on x86 or ARM).

> I should also look at the use of recursive mutexes in my patch as referenced by Sergey in relation to that.

For now, I prefer to keep it as-is: it's a use-case for the recursive mutex implementation ;-)

Eventually it's better to modify it, not to use recursive mutex any more. But that can be done later, in a separate commit.

@sebres, thanks for this explanation. Makes sense.

@Ashok wrote:

> I tend to agree that recursive mutexes are to be discouraged though I do think there are times they can simplify the code

+1

Jan, thanks for the reminder, it was on my plate but got pushed further and further down the stack. My primary motivation was to check whether the Windows native condition variables already deal with recursive locks. I'll get to this tomorrow.

As to Sergey's comments, I tend to agree that recursive mutexes are to be discouraged though I do think there are times they can simplify the code. But merging Christian's patch need not wait until we take a call on that. I should also look at the use of recursive mutexes in my patch as referenced by Sergey in relation to that.

> In the mean time, I saw sebres' branch (sebres--non-reentrant-mutex)...

Although my own preference is to use non-reentrant mutexes further (neither see the necessity for TIP#509, nor I think it is good idea at all), but this branch was made for Schelte, to test his strange issue with 9.0, where in opposite to 8.6, the CPU load (and eval times) grows with the time and new findings are - tracing with bpftrace shows that mutexes may be responsible for that.

Here is a quote from tcl-chat:

<schelte> I finally managed to get some data that may be useful regarding the cpu usage increasing on Tcl 9: After the application has been running for 3 weeks, calls to Tcl_MutexLock() take 70% longer than just after the application is started.

<schelte> Tcl_GetIndexFromObjStruct() increased by 20%. But Tcl_MutexLock() shows the biggest difference.

My assumption - since the unix mutexes use common alloc, and change the memory (lock counter, locked thread) in the block that may be shared across threads (potentially with other memory, if it is not aligned to CPU cache granularity), so we probably end up with something like [3d1c409f1a] after potential defragmentation.

To test this, I provided a small patch for his 9.0, reverting reentrant mutexes, but it'd not work anymore after apn's encoding fix [0433b67adc5fb0b9], what expects reentrant mutexes now and would cause a deadlock by setting of system encoding. Therefore, the branch in case Schelte would need it for newer versions as well.

That's it.

P.S. Basic system functions such as mutexes are not for naive attempts at improvement and definitely require deeper knowledge of the subject matter. Unfortunately I didn't see the TIP at that time point, otherwise I had definitely express my doubts about it.
Regarding the reentrant mutexes - someone, who may need it, doesn't understand the primary essential prerequisite of locking mechanisms - the lock shall be as short as possible. From this point of view - recursive usage of them is totally absurd. Moreover it'd quasi encourage the devs to write "bad" code.

The only reason Christian's patch is not merged to 9.0 (and trunk) yet is because Ashok (in the telco) requested time to review.

In the mean time, I saw sebres' branch (sebres--non-reentrant-mutex), undoing all of TIP #509, which - as far as I know - is not something we agreed upon already.

I'm convinced we need to do something, since - in the mean time - it also turned out that linux recursive mutexes have performance problems. Christian's patch has my preference.

So I propose to merge the "893f8cc5-tip509-nested-mutex" branch to 9.0 and trunk. That solves the linux performance problem, and it keeps recursive mutexes usable as described in TIP #509 . It probably won't work well with UNIX signals, but's that's a tricky issue in Tcl anyway.

Christian underlines, that TIP 509 is critical in implementation and has no practical usage.

The Bi-Weekly telco also reported, that there are no known usages.

https://core.tcl-lang.org/tips/doc/trunk/tip/509.md

Perhaps Brian may comment, as the TIP gives TclX as application and Brian is currently porting TclX to Tcl 9.

Thanks for any comment, Harald

Guys, I'm now an almost happy drama queen!

Now let us see: to ditch or not to ditch, wasn't that once upon a
time a question? Dramatically?

Really, I had some 40% Williams Christ and am patient. Take your time.
Think over TIP#509. It helps not in POSIX signalary for sure. But who
knows, when and whom sometime in the bright future TIP#509 might help?

Thanks, Jan, great. The solution sounds to be to easy to be true... See you in 4 minuites... Harald

Everything brought together in branch 893f8cc5-tip509-nested-mutex now.

> But POSIX native in 1) cannot be fixed at all.

So, use the emulated recursion counter for POSIX always. Done in the above branch now :-)

Thanks, great.
So, the branch [893f8cc5-tip509-nested-mutex] is still useful for the emulated and non native POSIX case -> Branch re-opened.
Bad, that the POSIX native case may not be fixed. The word "native" sounds, that this is quite common.

Thanks for all,
Harald

Harald, I cannot attend tomorrow, so let me try to describe the
current situation with this example:

  Tcl_MutexLock(&mutex);   /* A */
  Tcl_MutexLock(&mutex);   /* B */
  while (something) {
      Tcl_ConditionWait(&cond, &mutex);   /* C */
  }
  Tcl_MutexUnlock(&mutex); /* D */
  Tcl_MutexUnlock(&mutex); /* E */

1) For POSIX with native recursive option:

* step C decrements the lock only by one
* this means the critical section remains locked
* and most likely the wake up can never happen due to the lock
* deadlock is the outcome

2) For POSIX with emulated recursion counter:

* step C releases the mutex without accounting for the lock count
* the critical section is unlocked
* the wake up can happen
* after wake up the critical section is locked again
* but the bookkeeping of the ownership and the lock count get lost
* bad things happen later

3) For Win32 see 1)

Both patches for POSIX emulated 2) and Win32 3) try to fix the
bookkeeping at the end of Tcl_ConditionWait() and would allow for
a natural expected behavior.

But POSIX native in 1) cannot be fixed at all.

Ok, thanks, great.

The MS-Windows patch is now in commit [c2b0208c78] starting branch [893f8cc5-tip509-nested-mutex-win].

I understand right, that the Unix patch does not work? Ok, I have closed branch [893f8cc5-tip509-nested-mutex].

Hope we will have a great discussion tomorrow !

Thanks for all, I appreciate, Christian. I suppose, you had a full week-end on engeneering on this.

Take care, Harald

Oh, I forgot option C):

Make the emulated POSIX variant, that it deadlocks like the native
POSIX and Win32 variants. Such that all three variants error in
the same way and the platform differences are kept minimal.

Thinking further I see two options:

A) Fix Tcl_Mutex/Tcl_Condition in the true spirit of TIP#509 by using the
emulated (not the native recursive) variant on POSIX with fixes around
"wait on condition". Apply a similar strategy on Win32.

B) Don't worry about recursive mutexes and be happy with the pre TIP509
state of affairs.

Option A) would give up on using PTHREAD_MUTEX_RECURSIVE for the benefit
to have the Tcl_Mutex/Tcl_Condition combination working without implicit
danger of deadlock. Which could as well be achieved for Win32.

It gets worse every day: now read this fine print quoted from
https://forums.oracle.com/ords/apexds/post/recursive-mutex-and-conditional-wait-4703

  In pthread_mutexattr_settype man page on Solaris 7, it is stated that
  "It is advised that an application should not use a PTHREAD_MUTEX_RECURSIVE mutex
  with condition variables PTHREAD_MUTEX_RECURSIVE because the implicit unlock performed
  for a pthread_cond_wait() or pthread_cond_timedwait() will not actually release the
  mutex (if it had been locked multiple times)...

So the combination of recursive mutex and condition variable is
a classical two component explosive. And my initial thoughts on
trying to fix it for systems which natively do not support
recursive mutexes thus were most likely a complete waste of time.

Now, when we take the quoted statement (which is 25 years old) serious
we should provide both recursive and plain mutexes or ditch recursive
mutexes entirely. I'm in favor of the latter (and am still not sorry
for my notorious noise).

BTW: here is a snippet with which the quoted statement can nicely be
reproduced (it panics) when the snippet is compiled in tcl trunk's ready
built unix subdirectory:

---snip---
#include <tcl.h>

TCL_DECLARE_MUTEX(mutex);
Tcl_Condition cond;
static int deadlock;
static int stopit;
static int state;

static Tcl_ThreadCreateType
WatchDog(void *arg)
{
    int n = 0;

    while (!stopit && n < 20) {
        Tcl_Sleep(100);
        ++n;
    }
    if (deadlock) {
        Tcl_Panic("Deadlock detected");
    }
    TCL_THREAD_CREATE_RETURN;
}

static Tcl_ThreadCreateType
StrayCat(void *arg)
{
    while (!stopit) {
        Tcl_MutexLock(&mutex);
        if (state == 0) {
            state = 1;
            Tcl_ConditionNotify(&cond);
        } else if (state == 1) {
            state = 2;
            Tcl_ConditionNotify(&cond);
        }
        Tcl_MutexUnlock(&mutex);
        Tcl_Sleep(100);
    }
    TCL_THREAD_CREATE_RETURN;
}

static void
TestFunc(void)
{
    Tcl_ThreadId tid1, tid2;
    int dummy;

    deadlock = 1;
    stopit = 0;
    state = 0;
    Tcl_CreateThread(&tid1, WatchDog, NULL,
            TCL_THREAD_STACK_DEFAULT, TCL_THREAD_JOINABLE);
    Tcl_MutexLock(&mutex);
    Tcl_CreateThread(&tid1, StrayCat, NULL,
            TCL_THREAD_STACK_DEFAULT, TCL_THREAD_JOINABLE);
    while (state != 1) {
        Tcl_ConditionWait(&cond, &mutex, NULL);
    }
    Tcl_MutexLock(&mutex);
    while (state != 2) {
        Tcl_ConditionWait(&cond, &mutex, NULL);
    }
    Tcl_MutexUnlock(&mutex);
    Tcl_MutexUnlock(&mutex);
    stopit = 1;
    Tcl_JoinThread(tid2, &dummy);
    deadlock = 0;
    Tcl_JoinThread(tid1, &dummy);
}

int
main(int argc, char **argv)
{
    TestFunc();
    return 0;
}

/*
 * Local Variables:
 * mode: c
 * c-basic-offset: 4
 * fill-column: 78
 * compile-command: "cc test.c -I../generic -I. -L. -ltcl9.1"
 * End:
 */

See attached untested POC patch to fix this on Win32, too.