Skip to content

Thread safe #1765

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

Merged
merged 3 commits into from
Jun 1, 2016
Merged

Thread safe #1765

merged 3 commits into from
Jun 1, 2016

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented May 24, 2016

Update IAR and GCC to improve thread safety.

@@ -66,10 +66,10 @@ def __init__(self, target, options=None, notify=None, macros=None, silent=False,
self.asm = [join(IAR_BIN, "iasmarm")] + ["--cpu", cpuchoice]
if not "analyze" in self.options:
self.cc = [main_cc] + c_flags
self.cppc = [main_cc, "--c++", "--no_rtti", "--no_exceptions"] + c_flags
self.cppc = [main_cc, "--c++", "--no_rtti", "--no_exceptions", "--guard_calls"] + c_flags
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to myself: we need to clean up IAR flags. otherwise when you export you wont get all these new flags in the project file 😭

@theotherjimmy
Copy link
Contributor

This is really odd, but I don't see the same errors as the CI when I run workspace_tools/build_travis.py on this branch.

@theotherjimmy
Copy link
Contributor

I was using gcc-arm-embedded, whoops. I can reproduce the error with gcc-arm-none-eabi (version 4.9.3.2015q3-1trusty1). Also, I have the full assembly file if anyone would like to see it.

@theotherjimmy
Copy link
Contributor

removing this line: https://github.com/c1728p9/mbed/blob/807ef62af47b2ff31a8e85a9420b3db3691de631/rtos/rtx/TARGET_CORTEX_M/RTX_CM_lib.h#L646 fixes the CI errors. Tested with both ARMCC and GCC, the only compilers that build that line.

@adamgreen
Copy link
Contributor

Using assembler directives in the inline assembly language code is probably not a good idea since it can leave the assembler in an incorrect state when it attempts to assemble later code which was generated by the C/C++ compiler. Dropping the .syntax and .thumb directives would be best.

We hit that problem previously with pre_main() and fixed it by just dropping the assembly language altogether and re-writing in C.

@c1728p9
Copy link
Contributor Author

c1728p9 commented May 25, 2016

Thanks for looking into this @theotherjimmy! That was a tricky bug to find. Updated the PR to remove these assembler directives, along with adding a definition for the _reent structure.

@c1728p9
Copy link
Contributor Author

c1728p9 commented May 25, 2016

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot
Copy link

[Build 397]
FAILURE: Something went wrong when building and testing.

// Opaque declaration of _reent structure
struct _reent;

void __malloc_lock( struct _reent *_r )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix the formatting : ( struct _reent *_r ), no spacing around ( ) - (struct _reent *_r)

@0xc0170
Copy link
Contributor

0xc0170 commented May 31, 2016

I am happy with this in general but not that gcc flag. To proceed, I would propose to keep that gcc as it now (we got boilerplate, we just need to figure out how to switch nano on/off), and discuss how to switch nano lib on and off (my comment #1765 (comment)).

What about ARMCC, everything is setup correctly as it is now? With this PR (besides gcc nano flag), we would have armcc and iar thread safe standard library

c1728p9 added 3 commits May 31, 2016 14:41
Add the locks and flags necessary to make the IAR standard library
thread safe.  These changes consist of:
-Add compiler flag "--guard_calls" to ensure C++ function-static
    variables with dynamic initializers are initialized in a
    thread safe manner
-Add the linker flag "--threaded_lib" so the thread safe version of
    the standard library is used
-Implement mutex functions required for IAR thread safety
-Create a set of stub functions in retarget.c for when the rtos is not present
Add lock functions so that malloc and environment variable access are
thread safe.  Add the compiler option "-o thread-safe" to use the full
version of newlib which is thread safe.

Note that this patch does NOT make file access thread safe.
Remove the assembler directives from the inline assembly in
RTX_CM_lib.h since these can have unintended side effects in
the surrounding C code.
@c1728p9
Copy link
Contributor Author

c1728p9 commented May 31, 2016

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@c1728p9
Copy link
Contributor Author

c1728p9 commented May 31, 2016

Updated the patch to keep backwards compatibility. The option "-o thread-safe" can be used to build in the thread safe version of the standard library.

In the future toolchain handling will need to be overhauled so by default all targets which can use the thread safe version of the standard library do.

@mbed-bot
Copy link

[Build 425]
FAILURE: Something went wrong when building and testing.

@0xc0170 0xc0170 merged commit 02b0267 into ARMmbed:master Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants