hathach / tinyusb

An open source cross-platform USB stack for embedded system
https://www.tinyusb.org
MIT License
4.66k stars 996 forks source link

Add a simple re-entrancy lock for tu_printf. #2653

Open HiFiPhile opened 1 month ago

HiFiPhile commented 1 month ago

Describe the PR Add a simple re-entrancy lock for TU_LOG, should resolve #2652 in most cases. An interlocked version is needed for multicore MCU.

hathach commented 2 weeks ago

Thanks @HiFiPhile, though I think printf lock should be done in user space by implementing (nano)libc lock in _write_r(). Since not all retarget needs lock e.g. segger rtt

HiFiPhile commented 2 weeks ago

though I think printf lock should be done in user space by implementing (nano)libc lock in _write_r(). Since not all retarget needs lock e.g. segger rtt

The problem is libc's printf() itself is not reentrant as it works on global data, not just the I/O part.

https://github.com/bminor/newlib/blob/1ed15161362b2eb5649b049b7ab0aaf8097bd43a/newlib/libc/stdio/printf.c#L52

hathach commented 2 weeks ago

though I think printf lock should be done in user space by implementing (nano)libc lock in _write_r(). Since not all retarget needs lock e.g. segger rtt

The problem is libc's printf() itself is not reentrant as it works on global data, not just the I/O part.

https://github.com/bminor/newlib/blob/1ed15161362b2eb5649b049b7ab0aaf8097bd43a/newlib/libc/stdio/printf.c#L52

I may miss a thing though: the newlib reentrant implementation seems to the the exact same thing as this. For logging within ISR, we only do log in isr with level 3, which I used mainly with segger RTT. I will revise the code to see if there is any log1/log2 in the isr.

PS: oh, I see you mean the global _REENT is also used by other libc like malloc() and not only printf(). For this, how about moving this guard into the _write(), I still think it should be done at user level.

HiFiPhile commented 1 week ago

Eh... I think reentrant is kind of implementation dependent, I just read IAR's manual:

Most parts of the DLIB runtime environment are reentrant, but the following functions
and parts are not reentrant because they need static data:
Heap functions—malloc, free, realloc, calloc, etc. and the C++ operators new and delete
...
Functions that use files or the heap in some way. This includes scanf, sscanf, getchar, getwchar,
putchar, and putwchar. In addition, if you are using the options --printf_multibytes and --dlib_config=Full,
the printf and sprintf functions (or any variants) can also use the heap.
...
Remedies for this are:
● Do not use non-reentrant functions in interrupt service routines
● Guard calls to a non-reentrant function by a mutex, or a secure region, etc