Support thread-safety with ARMC6 #6973

ccli8 · 2018-05-22T03:51:04Z

Description

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

Related issue

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

Pull request type

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

geky · 2018-05-22T16:00:21Z

~~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

kegilbert · 2018-05-22T16:02:37Z

rtos/TARGET_CORTEX/mbed_boot.c

+#if (defined(__ARMCC_VERSION) && (__ARMCC_VERSION >= 6010050))
+
+// Check if Kernel has been started
+static uint32_t os_kernel_is_active (void) {


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

@kegilbert I fix spacing in a775f18.

bulislaw · 2018-05-22T20:48:22Z

rtos/TARGET_CORTEX/mbed_boot.c

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


Why do we need to check that?

@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.

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 · 2018-05-23T19:20:26Z

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 · 2018-05-24T10:37:51Z

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 · 2018-05-24T14:13:29Z

@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 · 2018-05-24T14:53:05Z

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

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

deepikabhavnani · 2018-05-24T15:29:49Z

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 · 2018-05-25T02:05:18Z

@bulislaw Where's vanilla version?

ccli8 · 2018-05-25T02:09:12Z

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 · 2018-05-25T02:21:54Z

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 · 2018-05-25T13:49:52Z

@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.

ccli8 · 2018-05-28T03:08:42Z

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.

ccli8 · 2018-06-05T09:47:01Z

Any update?

0xc0170 · 2018-06-06T14:00:57Z

rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c

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


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).

__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 · 2018-06-20T09:31:20Z

@ccli8 👍 Thanks for resolving this issue !

/morph build

mbed-ci · 2018-06-20T10:23:40Z

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

bulislaw · 2018-06-20T11:25:40Z

rtos/TARGET_CORTEX/rtx5/RTX/Config/RTX_Config.h

@@ -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.


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

@bulislaw OK. Please see 8071409.

bulislaw · 2018-06-20T11:26:54Z

rtos/TARGET_CORTEX/mbed_boot.c

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

+#if defined(RTX_NO_MULTITHREAD_CLIB)


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

@bulislaw I add comment in f055c2a.

mbed-ci · 2018-06-20T11:30:58Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-06-20T14:24:48Z

Test : SUCCESS

Build number : 2184
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6973/2184

This is to avoid change to RTX code base.

cmonr · 2018-06-21T12:40:09Z

@bulislaw Would you mind re-reviewing?

Original changes addressed.

bulislaw

Looks awesome!

deepikabhavnani

Looks good to me 👍

cmonr · 2018-06-21T14:44:26Z

/morph build

mbed-ci · 2018-06-21T15:40:26Z

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 · 2018-06-21T17:12:33Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-06-21T19:28:37Z

Test : SUCCESS

Build number : 2200
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6973/2200

pan- · 2018-06-22T11:21:05Z

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

pan- · 2018-06-22T12:05:15Z

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

0xc0170 added the needs: review label May 22, 2018

0xc0170 requested review from bulislaw and a team and removed request for bulislaw May 22, 2018 07:50

geky requested review from bulislaw and c1728p9 May 22, 2018 15:58

kegilbert reviewed May 22, 2018

View reviewed changes

bulislaw reviewed May 22, 2018

View reviewed changes

ccli8 force-pushed the nuvoton_armc6_thread_safe branch from 987d31c to a775f18 Compare May 23, 2018 01:46

bulislaw reviewed May 23, 2018

View reviewed changes

deepikabhavnani mentioned this pull request May 23, 2018

Is malloc() thread-safe in ARMC6? #6396

Closed

ccli8 force-pushed the nuvoton_armc6_thread_safe branch from a775f18 to e442749 Compare May 24, 2018 10:28

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

ccli8 force-pushed the nuvoton_armc6_thread_safe branch from e442749 to 8003834 Compare May 28, 2018 03:05

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

0xc0170 reviewed Jun 6, 2018

View reviewed changes

bulislaw previously requested changes Jun 20, 2018

View reviewed changes

ccli8 added 2 commits June 21, 2018 09:54

Add comment with RTX_NO_MULTITHREAD_CLIB movement

f055c2a

Move RTX_NO_MULTITHREAD_CLIB from RTX_Config.h to mbed_rtx_conf.h

8071409

This is to avoid change to RTX code base.

ccli8 dismissed 0xc0170’s stale review via 8071409 June 21, 2018 02:04

0xc0170 approved these changes Jun 21, 2018

View reviewed changes

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

bulislaw approved these changes Jun 21, 2018

View reviewed changes

SenRamakri requested a review from deepikabhavnani June 21, 2018 14:40

deepikabhavnani approved these changes Jun 21, 2018

View reviewed changes

cmonr added needs: CI release-version: 5.9.2 and removed needs: review labels Jun 21, 2018

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

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

ccli8 deleted the nuvoton_armc6_thread_safe branch June 28, 2018 05:49

0xc0170 mentioned this pull request Jul 9, 2018

Link error: Symbol __user_perthread_libspace multiply defined #7422

Closed

mysterywolf mentioned this pull request Feb 14, 2021

[TODO][libc] Roadmap for libc RT-Thread/rt-thread#4337

Closed

Support thread-safety with ARMC6 #6973

Support thread-safety with ARMC6 #6973

Uh oh!

Conversation

ccli8 commented May 22, 2018

Description

Related issue

Pull request type

Uh oh!

geky commented May 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

geky commented May 23, 2018

Uh oh!

ccli8 commented May 24, 2018

Uh oh!

bulislaw commented May 24, 2018

Uh oh!

geky commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented May 24, 2018

Uh oh!

ccli8 commented May 25, 2018

Uh oh!

ccli8 commented May 25, 2018

Uh oh!

ccli8 commented May 25, 2018

Uh oh!

0xc0170 commented May 25, 2018

Uh oh!

ccli8 commented May 28, 2018

Uh oh!

ccli8 commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 20, 2018

Uh oh!

mbed-ci commented Jun 20, 2018

Triggering tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Jun 20, 2018

Uh oh!

mbed-ci commented Jun 20, 2018

Uh oh!

cmonr commented Jun 21, 2018

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jun 21, 2018

Uh oh!

mbed-ci commented Jun 21, 2018

Triggering tests

Uh oh!

mbed-ci commented Jun 21, 2018

geky commented May 24, 2018 •

edited

Loading