A question on mutexes - normal v. recursive - and printf

[peter-h]

Context:
https://www.eevblog.com/forum/microcontrollers/32f4-hard-fault-trap-how-to-track-this-down/
https://www.eevblog.com/forum/programming/st-cube-gcc-how-to-override-a-function-not-defined-as-weak-(no-sources)/

Last night I was tracing through some long and float printf code.

The "classic" C printf library is known to use the heap, at least for floats and evidently for longs (uint32_t, with an arm32) too. It is also generally thought to not be thread-safe, due to use of static storage. The library commonly used is from 1990 and that contains both mutex calls and malloc calls.

Sometimes the one I have calls the mutexes and sometimes it calls malloc as well, and it turns out that the heap code I have (probably of the same origin) has its own mutex protection and actually uses the same calls.

Unfortunately the mutex calls are to empty functions. Some silly bugger at STM compiled this stuff into a library, didn't include the source, and left the mutex functions as stubs which can't be replaced (easily) because they were not declared as "weak".

Anyway I have a Q on mutexes. I have only so far used simple mutexes, where a call has to be made during init to obtain a handle for the mutex. But this printf and malloc stuff uses recursive mutexes, and I see no "init" call anywhere. But perhaps recursive ones don't need a pre-init function. Also they don't need a parameter (a handle) because, in a recursive situation, how could you keep track of the handles?

My printf calls __retarget_lock_acquire_recursive and then __retarget_lock_release_recursive. However, the library contains symbols for all these (ignore the weaken-symbol strings)

 --weaken-symbol __retarget_lock_init
 --weaken-symbol __retarget_lock_init_recursive
 --weaken-symbol __retarget_lock_close
 --weaken-symbol __retarget_lock_close_recursive
 --weaken-symbol __retarget_lock_acquire
 --weaken-symbol __retarget_lock_acquire_recursive
 --weaken-symbol __retarget_lock_try_acquire
 --weaken-symbol __retarget_lock_try_acquire_recursive
 --weaken-symbol __retarget_lock_release
 --weaken-symbol __retarget_lock_release_recursive
 --weaken-symbol __lock___arc4random_mutex
 --weaken-symbol __lock___dd_hash_mutex
 --weaken-symbol __lock___tz_mutex
 --weaken-symbol __lock___env_recursive_mutex
 --weaken-symbol __lock___malloc_recursive_mutex
 --weaken-symbol __lock___at_quick_exit_mutex
 --weaken-symbol __lock___atexit_recursive_mutex
 --weaken-symbol __lock___sfp_recursive_mutex
 --weaken-symbol __lock___sinit_recursive_mutex

Does anyone recognise these, and what might __retarget_lock_init be?

Remember this goes back to at least 1990, back to before the days embedded systems would have had a heap, or even a full printf.

[PlainName]

A recursive mutex is basically a counting one. You use it where you would hit the same mutex from the same execution thread, such that with a simple mutex the first call would block the second and you'd be stuck there forever. With a recursive mutex instead of stopping, the count increases and you carry on - that's safe because it's in the same thread so there is no resource serialisation required. On freeing the mutex, the count is decremented and it only because free for other threads when the count reaches 0.

An example of needing the recursive mutex is where you're protecting access to, say, a printer. A function write_hex_string() grabs the printer mutex because it could be called from anywhere, but printf also grabs the mutex so other processes don't output stuff in the middle of a string. Printf then calls write_hex_char() and there is a mutex lockup. A recursive mutex would allow that to work just fine (printf and write_hex_char are in the same thread, so serialised) while still stopping other threads from getting a foot in.

The xxx_init functions may just allow for the creation, and possible initialisation, of any mutex used in the other calls.

[Nominal Animal]

Is there a reason you do not replace printf interface with something saner?

Both %nd and %n.df are easy to implement if you do a separate call to format each variable.  Both can be done in the target buffer, and if the caller specifies that buffer, then these formatting functions would be re-entrant and thread-safe.

Integer to decimal string conversion is trivial, if you use division by ten, and store the remainder as the decimal digit, in order of increasing significance; sign last.
Floating-point to decimal string conversion is easy, if you handle the integer and the fractional part separately, fractional part first.  For the fractional part, repeatedly multiply by ten, and pick off the integer part as the decimal.  If you find that the final digit rounds up, you can ripple the rounding up to the integer part.  You do need to know beforehand how many decimal digits you want to print.
For both integers and floating-point numbers, start at the end of the available buffer, and then move it to the beginning of it.

More efficient algorithms do exist, but even naïve implementation of these will beat printf implementations.
    printf("i=%d, x=%.4f\n", i, x);
will become something like
    append_string(&bufferstruct, "i=");
    append_int(&bufferstruct, i);
    append_string(&bufferstruct, ", x=");
    append_float(&bufferstruct, x, 4);
    append_char(&bufferstruct, '\n');
but as long as you have a buffer to print to, it'll be efficient, re-entrant (assuming different bufferstruct instances), and thread-safe (assuming different bufferstruct instances).  Optimally, bufferstruct refers to the output buffer related to the stream you want to print to.

If you don't mind using a default number of decimals for floats and doubles, then you can use either C++ type-specifics or C99 _Generic() and macro argument expansion to generate the above from just
    append(&bufferstruct, "i=", i, ", x=", x, '\n');
where append() is a preprocessor macro that essentially first expands it into
    append_item(&bufferstruct, "i=");
    append_item(&bufferstruct, i);
    append_item(&bufferstruct, ", x=");
    append_item(&bufferstruct, x);
    append_item(&bufferstruct, '\n');
and the type-specific calls or macro expansion via _Generic() handles the conversion to type_specific member/function calls.

[peter-h]

Is there a reason you do not replace printf interface with something saner?

Yes - lack of time (I am working on this alone, getting help from people on forums) and the need for a full standard printf lib because the product will have modules done by other people later.

If it was a one off embedded job, I would use itoa() and the other variants. OTOH I have 1MB FLASH available and the code is only 300k currently (and half of that is that pile called MbedTLS), so, why not? I am using snprintf for outputting uint64_t, and sscanf for reading uint64_t, and floats, etc.

I have it nearly done but it was a very long marathon. I could not work out how to do the wrapping mentioned here
https://www.eevblog.com/forum/programming/st-cube-gcc-how-to-override-a-function-not-defined-as-weak-(no-sources)/
without it being very messy because it looks like it needs to be on the compiler command line. So I used the --weaken-symbol option on objcopy. I will post details later in that thread but it was awfully complicated even just to locate the right libc.a file. Then getting the linker (in Cube) to see the modified lib took all day of trial and error.

I have managed to intercept the mutex calls and got most of it running but it isn't quite there.

It looks like all the mutexes do need initialisation. In FreeRTOS there isn't a "self initialising" mutex. I collected them into init_newlib_mutexes, here

Code: [Select]

/*
 * newlib_locking.c
 *
 *  Created on: 24 Jul 2022
 *      Author: peter
 *
 *  These functions replace the empty ones in the newlib (printf etc) code.
 *  The printf family uses the heap for floats and longs (regardless of
 *  whether newlib-nano is selected) and when floats or longs are used, these
 *  functions use the heap, and use mutexes to make that thread-safe.
 *
 *  Based around
 *  [url]https://gist.github.com/thomask77/3a2d54a482c294beec5d87730e163bdd[/url]
 *
 *  Also see
 *  [url]https://www.eevblog.com/forum/programming/st-cube-gcc-how-to-override-a-function-not-defined-as-weak-(no-sources)[/url]
 *
 *
 */


#include "FreeRTOS.h"
#include "semphr.h"
#include "task.h"
#include <newlib_locking.h>


struct __lock {
    SemaphoreHandle_t   sem;
};


struct __lock __lock___sinit_recursive_mutex;
struct __lock __lock___sfp_recursive_mutex;
struct __lock __lock___atexit_recursive_mutex;
struct __lock __lock___at_quick_exit_mutex;
struct __lock __lock___malloc_mutex;
struct __lock __lock___env_recursive_mutex;
struct __lock __lock___tz_mutex;
struct __lock __lock___dd_hash_mutex;
struct __lock __lock___arc4random_mutex;


// This mutex init file is called from main.c

//__attribute__((constructor))
void init_newlib_mutexes(void)
{
    __lock___sinit_recursive_mutex.sem  = xSemaphoreCreateRecursiveMutex();
    __lock___sfp_recursive_mutex.sem    = xSemaphoreCreateRecursiveMutex();
    __lock___atexit_recursive_mutex.sem = xSemaphoreCreateRecursiveMutex();
    __lock___at_quick_exit_mutex.sem    = xSemaphoreCreateMutex();
    __lock___malloc_mutex.sem = xSemaphoreCreateMutex();
    __lock___env_recursive_mutex.sem    = xSemaphoreCreateRecursiveMutex();
    __lock___tz_mutex.sem               = xSemaphoreCreateMutex();
    __lock___dd_hash_mutex.sem          = xSemaphoreCreateMutex();
    __lock___arc4random_mutex.sem       = xSemaphoreCreateMutex();
}


// The mutex lock is *not* recursive (as is shown in many examples) because
// that would be really stupid.
// These two functions are used by both malloc and free.

void __malloc_lock(_LOCK_T lock)
{
    xSemaphoreTake(lock->sem, portMAX_DELAY);
}

void __malloc_unlock (_LOCK_T lock)
{
    xSemaphoreGive(lock->sem);
}

// This one is not used
void __malloc_lock_acquire(_LOCK_T lock)
{
    xSemaphoreTake(lock->sem, portMAX_DELAY);
}

void __retarget_lock_init(_LOCK_T *lock_ptr)
{
    *lock_ptr = pvPortMalloc(sizeof(struct __lock));
    (*lock_ptr)->sem = xSemaphoreCreateMutex();
}


void __retarget_lock_init_recursive(_LOCK_T *lock_ptr)
{
    *lock_ptr = pvPortMalloc(sizeof(struct __lock));
    (*lock_ptr)->sem = xSemaphoreCreateRecursiveMutex();
}



// This one seems to exist as a symbol only; not even as an empty function

void __retarget_lock_close(_LOCK_T lock)
{
    vSemaphoreDelete(lock->sem);
    vPortFree(lock);
}


// This one is never actually called

void __retarget_lock_close_recursive(_LOCK_T lock)
{
    vSemaphoreDelete(lock->sem);
    vPortFree(lock);
}


void __retarget_lock_acquire(_LOCK_T lock)
{
    xSemaphoreTake(lock->sem, portMAX_DELAY);
}


void __retarget_lock_acquire_recursive(_LOCK_T lock)
{
    xSemaphoreTakeRecursive(lock->sem, portMAX_DELAY);
}

// This one seems to exist as a symbol only; not even as an empty function

int __retarget_lock_try_acquire(_LOCK_T lock)
{
    return xSemaphoreTake(lock->sem, 0);
}

// This one seems to exist as a symbol only; not even as an empty function

int __retarget_lock_try_acquire_recursive(_LOCK_T lock)
{
    return xSemaphoreTakeRecursive(lock->sem, 0);
}


void __retarget_lock_release(_LOCK_T lock)
{
    xSemaphoreGive(lock->sem);
}


void __retarget_lock_release_recursive(_LOCK_T lock)
{
    xSemaphoreGiveRecursive(lock->sem);
}


I think there is still something wrong with some handle somewhere but this stuff is on the limit of my knowledge of C, given all the typedefs and structures So if anyone can see anything wrong with the above, I am all ears

One thing I concluded is that it is dumb to use a recursive mutex for the heap because the heap cannot possibly be multi threaded. Yet this is what the newlib code was doing. Malloc calls __retarget_lock_acquire_recursive, r0= 0x2000c725 = __lock___malloc_recursive_mutex, then on the way out it calls __retarget_lock_release_recursive. I changed this for a normal mutex but maybe it is meant to work after all?

This was the conversion process:

arm-none-eabi-objcopy @locksyms.txt libc.a libc-weakened.a
arm-none-eabi-objdump -t libc-weakened.a > listing.txt

I could have just weakened the entire library and that would have been fine. Maybe I should (it takes about 1 second) because then one can replace the other functions like printf one day.

One consequence of this method is that as Cube is updated, the libc.a will not be. New libs are loaded into the c:\st\... tree and then according to your Cube linker settings (e.g. which version of the printf lib you want, and according to the CPU, whether it has hardware double floats etc) one of these many libc.a files is picked up and copied to another "working" part of the tree. All this stuff gets overwritten with each Cube update, but my weakened libc.a will never change. That should be ok since that code seems to be from 1990. The linker, having got a pointer to the weakened libc.a does indeed load it in preference to the stock one.

[PlainName]

One thing I concluded is that it is dumb to use a recursive mutex for the heap because the heap cannot possibly be multi threaded.

Recursive mutexes are to prevent lockup from repeat 'take's by a single thread. If there is not multi-thread issue (that is, you don't need a mutex at all) then using a recursive won't be a problem - it's just a waste of effort over a simple mutex.

Malloc calls __retarget_lock_acquire_recursive, r0= 0x2000c725 = __lock___malloc_recursive_mutex, then on the way out it calls __retarget_lock_release_recursive.

Looks to me like malloc needs the mutex to prevent other threads getting in the way (that is, make the access atomic), but allow for the same thread to interleave additional malloc calls. By making it a simple mutex you stand the risk of having a mutex lock and your box grinding to a halt for no apparent reason.

[peter-h]

OK thanks. Took me a while to get my head around this, but I think the heap mutex can be recursive, and indeed should be.

Well, one has to assume that somebody thought about this in 1990, before all coders moved over to javascript I still find it funny that the same mutex names were used in 1990, and today they pop up in embedded systems.

Unfortunately I don't understand structs and C well enough and this doesn't compile



Previously these were all like this



and I have no idea what this means



Maybe someone can help me with the C

I do know that every mutex protecting a different object needs to have a distinct handle. One could use the same handle for the mutex for malloc() as the handle for the mutex for free() but the printf mutex needs a different handle.

[PlainName]

The 'previously like this' example is how they should be - the function is passed a pointer to the appropriate lock structure, which reduces cohesion. In __malloc_unlock() you hit the structure directly (so you should be using __lock__malloc_mutex.sem instead of __lock__malloc_mutex->sem), which is what the compiler if complaining about there. And where I say 'should be using' I really mean 'you shouldn't do it like this but you can fix the error this way'. Note that if you do this in  one function you MUST do it in all of them, otherwise you stand the risk that some functions will be looking at a different mutex handle to other related functions and it will crash horribly.

OK, so I would normally think that __malloc_lock() is the way it's done, so I don't know why you are getting the error - what is the reported problem?

The example of what it previously was surely isn't correct since xSemaphoreTakeRecursive() is a FreeRTOS call and your problems seem to be due to trying to fit FreeRTOS into something that used something else. So what was the actual code originally?

The thing you have no idea what it means is, ah, almost a kind of write-only stuff that supposedly makes things simpler. Which it does so long as you don't have to keep working out WTF it means every time. Essentially you give it a name (via __LOCK_INIT()) and it autogenerates a structure bearing that name plus some standard wrapping.

[peter-h]

Many thanks again.

I decided to cut my losses (of time) and sidestepped this by installing a completely different printf library: https://github.com/mpaland/printf

Yes I know others suggested this but I wanted to be sure I could not fix the existing one, which does seem to be an absolutely standard implementation. I have seen so many open source ones and when looking at them, they have all kinds of bits missing.

Another one is https://github.com/MaJerle/lwprintf which seems ok and has some nice extras, but nothing big.

I used 4 steps to integrate the new printf:

1) make a local copy of the current libc.a (the full-everything hardware-float version - was an absolute bastard to track down, among many libc.a files under c:\st\)
2) run objcopy to weaken all symbols in it
3) linker options to include it (in preference to the default one in c:\st\...)
4) compiler option to avoid the need for #include "printf.h" in every source file

It seems to work ok. The existing bloatware is still used for scanf stuff, and heap stuff, of course. I am tempted to replace the heap also.

Funnily enough, the total flash used is down by 1k, so it looks like the linker realised that some modules (.o I think) in libc.a contained wholly unreferenced functions, and removed them.

[langwadt]

Many thanks again.

I decided to cut my losses (of time) and sidestepped this by installing a completely different printf library: https://github.com/mpaland/printf

afaict it hasn't been updated in years so it's been superseded by this fork, https://github.com/eyalroz/printf with several bug fixes etc.
 

[peter-h]

Interesting. I read somewhere he is back... Is there a list of bug fixes?

Thanks for that lead. I will drop it in.