-
Notifications
You must be signed in to change notification settings - Fork 3k
GCC small lib and OS5 #4885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Nanolib is not thread safe, it is compiled without many hooks that we would need (I recall malloc lock for instance). cc @c1728p9 |
So we should not allow to create thread in case of nanolib, |
Tracked here #4877
Please reference the exact files and what changes should be done? The test contains a check that is not supported https://github.com/ARMmbed/mbed-os/blob/master/TESTS/mbedmicro-rtos-mbed/threads/main.cpp#L24 |
Test writer is an expert, so the correct check exists :-) |
I recall there was a settting previously for older RTX that would limit number of thread, I can not locate it anymore to be related to cc @bulislaw |
@jeromecoutant Is this issue still relevant? I do not see any fix, but havent seen the failure above reported neither? |
Hi |
I've been checking where @ARMmbed/mbed-os-core Please review |
Supporting special mode for small targets makes me uncomfortable, that would add 3rd flavour: RTOS, semi supported RTOS-less and single thread. I would say we should be able to run Mbed OS on 16K+ targets with RTOS. We had some PR with a way to skip tests maybe we should use it to exclude tests requiring more ram? |
@jeromecoutant Switching to GCC nano was tried in #7604, but was not successful. |
OK Note that I never saw any clear information |
@jeromecoutant - Good point. |
@jeromecoutant - Would like to know your thoughts on adding this #5285 |
I just looked into this because I was curious on the usage of Is this a legacy flag then? i.e. perhaps it was used at some point in RTX to limit the number of threads but this is no longer the case. If so, perhaps we should remove it? |
As far as I can see, the flag is there to stop tests failing when you try to make a build using the RTOS and a non-thread-safe library. It skips tests that require multiple threads. I don't believe you're prevented from making the multiple threads, but they wouldn't be reliable, due to missing library thread protection. I'm not sure attempting that is useful, given that we now support the bare metal config, and the RTOS API for it. I would suggest use of non-thread-safe libraries should be limited to bare-metal builds, and RTOS builds should require proper thread-safe libraries. |
We have to make it clear and easy:
The second option could be default for small targets, but that should be made clear in some way in their docs. As far as i can see |
Plausible, if really small - ie blinky doesn't fit otherwise. Maybe many targets which are currently marked "Mbed OS 2-only" in targets.json |
@jeromecoutant thank you for raising this issue.Please take a look at the following comments: Could you add some more detail to the description? A good description should be at least 25 words. 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. |
@jeromecoutant it has been 5 days since the last reminder. Could you please update the issue header as previously requested? |
Does it make sense to only allow small C libs with bare metal, by having a check in the build system for example? If an application needs RTOS, most likely it uses multiple threads which small C libs are not safe with? |
I would say yes, same as quote below from @kjbracey-arm
👍 |
Uh oh!
There was an error while loading. Please reload this page.
Hi
We got some issue with tests-mbedmicro-rtos-mbed-threads and "small" OS5 targets like NUCLEO_F072RB (20k RAM)
This test is FAILED with GCC_ARM.
I can make it OK using the small lib, but this needs a patch in gcc.py script :
Can someone remind me the reason to forbid multiple thread with GCC nano ?
Thx
The text was updated successfully, but these errors were encountered: