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

Support thread-safety with ARMC6 #6973

Merged
merged 3 commits into from Jun 21, 2018

Conversation

@ccli8
Contributor

ccli8 commented May 22, 2018

Description

This PR tries to add thread-safety support with ARMC6 toolchain.

Related issue

#6396
ARMmbed/mbed-cloud-client-example#11

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested review from bulislaw and ARMmbed/mbed-os-core and removed request for bulislaw May 22, 2018

@geky geky requested review from bulislaw and c1728p9 May 22, 2018

@geky

This comment has been minimized.

Member

geky commented May 22, 2018

This looks good to me, but not really my area of expertise 👍

Ah! Sorry it was my first time seeing a review request for a group, glad to see that's working now, should help quite a bit with reviews moving forward

#if (defined(__ARMCC_VERSION) && (__ARMCC_VERSION >= 6010050))

// Check if Kernel has been started
static uint32_t os_kernel_is_active (void) {

This comment has been minimized.

@kegilbert

kegilbert May 22, 2018

Contributor

Minor nit: Spacing in this function is inconsistent with the rest of the file (2 spaces versus 4).

This comment has been minimized.

@ccli8

ccli8 May 23, 2018

Contributor

@kegilbert I fix spacing in a775f18.

// Acquire mutex
__USED void _mutex_acquire(mutex *m)
{
if (os_kernel_is_active()) {

This comment has been minimized.

@bulislaw

bulislaw May 22, 2018

Member

Why do we need to check that?

This comment has been minimized.

@ccli8

ccli8 May 23, 2018

Contributor

@bulislaw This code is copied from mbed-os/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c. Maybe its original owner could comment it well. I guess it is to guard from e.g. C/C++ initialization code during which kernel is not ready yet.

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_armc6_thread_safe branch from 987d31c to a775f18 May 23, 2018

@bulislaw

Actually why do we need to redefine it? The same functions from rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c should be used by default

@geky

This comment has been minimized.

Member

geky commented May 23, 2018

From #6396 (comment), I think the correct solution is to make the versions in rtx_lib.c available on all versions of ARMCC:
https://github.com/ARMmbed/mbed-os/blob/master/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c#L623

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_armc6_thread_safe branch from a775f18 to e442749 May 24, 2018

@ccli8

This comment has been minimized.

Contributor

ccli8 commented May 24, 2018

As commented, I force _mutex_acquire/_mutex_release to __USED for ARM/ARMC6 toolchains in rtx_lib.c and restore mbed_boot.c (no _mutex_acquire/_mutex_release are re-defined here). But I don't also force _mutex_initialize/_mutex_free to __USED because I meet error with ARMC6 toolchain:

Error: L6200E: Symbol _mutex_initialize multiply defined (by rtx_lib.o and mbed_boot.o).
Error: L6200E: Symbol _mutex_free multiply defined (by rtx_lib.o and mbed_boot.o).

It seems that with ARMC6 toolchain the __WEAK modifier will be cancelled by the __USED modifier.

@bulislaw

This comment has been minimized.

Member

bulislaw commented May 24, 2018

@ccli8 we've just merged update of RTX could you retest the vanilla version and if it still doesn't work rebase your PR.

@geky

This comment has been minimized.

Member

geky commented May 24, 2018

@ccli8, were the __USED and __WEAK on different declarations?

__USED _mutex_free(void*);
__WEAK _mutex_free(void *m) {
    Blah blah blah
}
@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented May 24, 2018

It seems that with ARMC6 toolchain the __WEAK modifier will be cancelled by the __USED modifier.

Can we try __WEAK in "Source/rtx_lib.c" definition and __USED in "mbed_boot.c" ?

@ccli8

This comment has been minimized.

Contributor

ccli8 commented May 25, 2018

@bulislaw Where's vanilla version?

@ccli8

This comment has been minimized.

Contributor

ccli8 commented May 25, 2018

were the __USED and __WEAK on different declarations?

@geky I've tried it and in the same line. Still meet the multiple definitions link error with ARMC6 toolchain.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented May 25, 2018

Can we try __WEAK in "Source/rtx_lib.c" definition and __USED in "mbed_boot.c" ?

@deepikabhavnani It seems that doesn't make sense. _mutex_xxx functions defined in rtx_lib.c are to be re-implemented in mbed_boot.c, so they are qualified with __WEAK. Because ARM/ARMC6 libraries refer weakly to them, they must be __USED to be present in the final image. So _mutex_xxx functions defined in rtx_lib.c must be qualified with __WEAK and __USED simultaneously. If we just have __WEAK in rtx_lib.c but the function is not re-implemented in mbed_boot.c, it won't linked in.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

@ccli8 The above recommendation was to rebase this one (there is a conflict anyway) to get the latest rtx update , and try it with that one and report back.

@0xc0170 0xc0170 added needs: work and removed needs: review labels May 25, 2018

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_armc6_thread_safe branch from e442749 to 8003834 May 28, 2018

@ccli8

This comment has been minimized.

Contributor

ccli8 commented May 28, 2018

I do rebase to fix the conflict (8003834). Just add __USED to _mutex_acquire/_mutex_release in rtx_lib.c for ARMC6 toolchain and have no other change.

@0xc0170 0xc0170 added needs: review and removed needs: work labels May 28, 2018

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jun 5, 2018

Any update?

__USED
#endif
__WEAK void _mutex_acquire(mutex *m);
__WEAK __USED void _mutex_acquire(mutex *m);

This comment has been minimized.

@0xc0170

0xc0170 Jun 6, 2018

Member

used was added to fix this issue ARM-software/CMSIS_5#129. Looking at this, do we still need weak? (the PR referenced at the bottom there, was closed, and RTX_NO_MULTITHREAD_CLIB was mentioned there).

This comment has been minimized.

@JonatanAntoni

JonatanAntoni Jun 6, 2018

Member

__WEAK and __USED seems not to make sense if used both. As discussed earlier this function cannot be defined __WEAK because there is already a weak version provided in the C library. Hence the linker cannot distinguish which weak version to use if no non-weak version is given. If its really needed for mbed to get rid of the internal implementation that is part of RTX we need to find a completely different solution.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

@ccli8 👍 Thanks for resolving this issue !

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 20, 2018

Build : SUCCESS

Build number : 2397
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6973/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@@ -404,6 +404,11 @@
#define OS_THREAD_LIBSPACE_NUM OS_THREAD_NUM
#endif


// Don't adopt default multi-thread support for ARM/ARMC6 toolchains from RTX code base.

This comment has been minimized.

@bulislaw

bulislaw Jun 20, 2018

Member

We are trying not to change the default config file. Could it be moved to rtos/TARGET_CORTEX/mbed_rtx_conf.h

This comment has been minimized.

@ccli8

ccli8 Jun 21, 2018

Contributor

@bulislaw OK. Please see 8071409.

@@ -423,6 +423,60 @@ void __rt_entry (void) {
mbed_start_main();
}

#if defined(RTX_NO_MULTITHREAD_CLIB)

This comment has been minimized.

@bulislaw

bulislaw Jun 20, 2018

Member

Could you add a comment saying it was copied from whichever file in rtx.

This comment has been minimized.

@ccli8

ccli8 Jun 21, 2018

Contributor

@bulislaw I add comment in f055c2a.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Jun 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

@bulislaw Would you mind re-reviewing?

Original changes addressed.

@bulislaw

Looks awesome!

@SenRamakri SenRamakri requested a review from deepikabhavnani Jun 21, 2018

@deepikabhavnani

Looks good to me 👍

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 21, 2018

Build : SUCCESS

Build number : 2419
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6973/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit aa25b1d into ARMmbed:master Jun 21, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 919 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10243 cycles (+1308 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Jun 21, 2018

@pan-

This comment has been minimized.

Member

pan- commented Jun 22, 2018

this patch cause an link error when an application is compiled with MBED_RTOS_SINGLE_THREAD.

@pan-

This comment has been minimized.

Member

pan- commented Jun 22, 2018

Seems like a root cause was due to a custom profile without _RTE_ defined; there's nothing to see here.

@ccli8 ccli8 deleted the OpenNuvoton:nuvoton_armc6_thread_safe branch Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment