Remove Mutex::_count

[AriParkkila]

Description of defect

Mutex::_count should be removed or implemented properly.

Mutex class is not actually using _count for anything, but because it's updated without holding a lock that breaks implementation of the friend class ConditionVariable.

I guess the original idea behind _count was to implement recursive mutex, but it's not really needed as that's already handled (properly) in rtx_mutex.c. Also if ConditionVariable need to guarantee Mutex is non-recursive then simply deassert for osMutexRecursive attribute.

Target(s) affected by this defect ?

All with rtos.present.

Toolchain(s) (name and version) displaying this defect ?

All.

What version of Mbed-os are you using (tag or sha) ?

#0c4e50abb3fa1ec3d80f19c88ceefa106ff94e64

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

How is this defect reproduced ?

E.g. this code:

    Mutex m;
    Thread t;
    t.start([&m]() {
        while (1) {
            m.lock();
            m.unlock();
        }
    });

    ConditionVariable cv(m);
    while (1) {
        m.lock();
        cv.wait_for(0);
        m.unlock();
    }

... eventually it crashes:

++ MbedOS Error Info ++
Error Status: 0x80FF0144 Code: 324 Module: 255
Error Message: Assertion failed: _mutex._count == 1
Location: 0x1DEAF
File: ./mbed-os/rtos/source/ConditionVariable.cpp+52
Error Value: 0x0
Current Thread: main Id: 0x20004700 Entry: 0x1F3DB StackSize: 0x1FA0 StackMem: 0x200053F8 SP: 0x200071D4 
For more info, visit: https://mbed.com/s/error?error=0x80FF0144&tgt=MTB_LAIRD_BL654
-- MbedOS Error Info --

[ciarmcom]

@AriParkkila thank you for raising this issue.Please take a look at the following comments:

We cannot automatically identify a release based on the version of Mbed OS that you have provided.
Please provide either a single valid sha of the form #abcde12 or #3b8265d70af32261311a06e423ca33434d8d80de
or a single valid release tag of the form mbed-os-x.y.z .
E.g. '0c4e50a' has not been matched as a valid tag or sha.It would help if you could also specify the versions of any tools you are using?

NOTE: If there are fields which are not applicable then please just add 'n/a' or 'None'.This indicates to us that at least all the fields have been considered.
Please update the issue header with the missing information, the issue will not be mirroredto our internal defect tracking system or investigated until this has been fully resolved.

[0xc0170]

@kjbracey-arm @ashok-rao Was this discussed this week in #12965 and #12964 ?

[ashok-rao]

@0xc0170 .. Nop. Looks like @AriParkkila came across "_count" and Mutex the same time we did, but our requirements are different..

[kjbracey]

Nice test. Weird lower loop, but maybe it's necessary to provoke failure. Hope you're not using it like that in practice. It should look more like

ConditionVariable cv(m);
m.lock();
while (1) {
    cv.wait_for(0);
}

to make sure the while condition is tested with the mutex held. (The mutex isn't just some random thing you have to do around the wait call for no reason - it's to make the test-and-wait combo atomic).

Looks like I broke it in b8e80dd (#10358)

Previous behaviour was always to decrement count inside the lock, before the unlock call. That was safe, but would have left it wrong in the event of any error, which is probably what I meant to fix, but that's not a big deal. Errors mean system death anyway.

The original reason for it was purely to implement the ConditionVariable assert.

I argued against the _count when it originally went in (#3648), much like I've just been arguing against Semaphore::get_count, and got it demoted to just friend access to ConditionVariable. But taking it out now would be a potentially binary-breaking change. I think we just fix the #10358 breakage, putting the decrement back where it was.

Also if ConditionVariable need to guarantee Mutex is non-recursive then simply deassert for osMutexRecursive attribute.

No can do, as we always set that flag. There's currently no option to make a non-recursive Mutex, and even if there was, requiring non-recursive rather than current count 1 would be a breaking change.

But yes, that is the approach of C++11. You have to use std::mutex, not std::recursive_mutex.

[AriParkkila]

Thanks, fixed in #12983.