Use temp variable in mutex release loop #52
Conversation
|
@kjbracey-arm @TeroJaasko @SeppoTakalo @tommikas Don't merge yet, I will test this still tomorrow morning. |
| @@ -454,8 +454,12 @@ void mbed_vtracef(uint8_t dlevel, const char* grp, const char *fmt, va_list ap) | |||
|
|
|||
| end: | |||
| if ( m_trace.mutex_release_f ) { | |||
| while (m_trace.mutex_lock_count > 0) { | |||
| m_trace.mutex_lock_count--; | |||
| // Store the mutex lock count to temp variable so that it won't get | |||
There was a problem hiding this comment.
"temp_mutex_lock_count" is long-winded to the point of making it hard to read - looks visually like the global.
Just "count" would do.
Could make it a "do/while" loop, as we "know" count is at least 1, and we hold the mutex at least once.
Logic seems solid though. (But then I didn't spot the problem in the last one either.)
There was a problem hiding this comment.
Maybe
int count = m_trace.mutex_lock_count;
m_trace.mutex_lock_count = 0;
do {
m_trace.mutex_release_f();
} while (--count > 0);
There was a problem hiding this comment.
This might be a good time to add some kind of comments why the unlocking loop exists at all. Every time I have looked that code it has given a impression that something must be very, very wrong here.
+1 for functional part.
There was a problem hiding this comment.
There's these where the mutex is acquired: https://github.com/ARMmbed/mbed-trace/blob/6cf459bca0fc744ef52be428100e896cb861f2b6/source/mbed_trace.c#L480
But agreed, more wouldn't hurt.
There was a problem hiding this comment.
Agree on adding more comment on this. I thought I did already but I guess I forgot to commit it last night.
|
Guys does this look good now? The unittest coverage fails because this removes 1 line completely and resulting coverage is 0.04% less than previously.... |
|
@SeppoTakalo @TeroJaasko |
|
@kuggenhoffen It's green now. @SeppoTakalo : A comment in the original PR: #30 (comment) |
|
That comment seems fairly explicit. |
|
@SeppoTakalo Please make a PR to mbed-os for this fix as well. We need to get this in mbed-os 5.2 |
* origin/master: (22 commits) output/ added to gitignore list (#66) Added include_directories(${CMAKE_CURRENT_SOURCE_DIR}/) include to resolve compilation error for Linux. (#63) Ensure tr_array doesn't print <null> when len == 0 (#65) Add CMakeLists.txt to support PAL Tools (#61) Fix header documentation (#58) Update README.md (#57) Provide environment-agnostic configuration macros. (#54) Flash size can be limited by defining MBED_TRACE_MAX_LEVEL Use temp variable in mutex release loop (#52) Fix race condition when unlocking mutex Add instructions for enabling the library in mbed OS 5. (#50) document update. (#47) Allow setting mutex locking/unlocking callbacks before trace init. (#46) Ipv6 helpers update (#44) Fix disabling traces through the YOTTA_CFG_MBED_TRACE flag. (#45) Deprecate the misnamed YOTTA_CFG_MTRACE_TMP_LINE_LENGTH flag, which is to be removed in the next release. (#43) Changed dummy definitions to match the actual functions' return types (#40) FEA_TRACE_SUPPORT flag is always set when traces are enabled. (#39) Release merge to master (#38) Add nanostack-libservice lib to CMakeLists (#31) ...
Fix in #51 still had possibility of unexpected behavior as the mutex_lock_count variable might get modified after releasing mutex during last loop iteration.