Singleton support #2158

c1728p9 · 2016-07-12T23:04:38Z

Add a singleton wrapper class and use this for global mutexes

c1728p9 · 2016-07-12T23:05:11Z

This is an alternative and more generic version of #2051

c1728p9 · 2016-07-12T23:05:56Z

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=KL25Z,LCP1768,K64F

c1728p9 · 2016-07-12T23:29:05Z

Don't merge yet, I need to test this more tomorrow.

mbed-bot · 2016-07-12T23:36:12Z

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

c1728p9 · 2016-07-13T17:06:24Z

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=KL25Z,LCP1768,K64F

mbed-bot · 2016-07-13T18:55:25Z

[Build 621]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

c1728p9 · 2016-07-13T19:30:16Z

This patch is ready to go from my side

c1728p9 · 2016-07-13T19:41:39Z

CC: @geky, @0xc0170

geky · 2016-07-13T20:27:14Z

Looks good to me 👍

Will be useful for working around static c++ shenanigans.

geky · 2016-07-14T22:34:35Z

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=KL25Z,LCP1768,K64F

mbed-bot · 2016-07-14T23:06:34Z

[Build 629]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

geky · 2016-07-15T15:50:09Z

@sg-, @0xc0170: LGTM?

0xc0170 · 2016-07-15T16:31:35Z

hal/api/SingletonPtr.h

+
+    // This is zero initialized when in global scope
+    T *_ptr;
+    uint32_t _data[(sizeof(T) + 3 / 4)];


(sizeof(T) + 3 / 4) any details for this size (might be worth documenting here)? I see its used as a placement for the T type.

This is to ensure 4 byte alignment. Want me to add a comment here?

uint32_t _data[(sizeof(T) + sizeof(uint32_t)-1) / sizeof(uint32_t)];

Might be less ambigous, just throwing that out there.

oops, it would help if I actually had the code round up here. What would make this really easy is a macro to do the rounding - somethin like UTIL_DIV_ROUND_UP(sizeof(t), sizeof(uint32_t)).

Switch pre_main from assembly to C for ARMCC. This function does not need to be in assembly.

Create the wrapper class SingletonPtr. This provides a safe way to declare and use singletons. This class allows both the lazy initialization of a singleton, and allows the singleton to be garbage collected by the linker if it is never referenced. This patch also updates the HAL to use SingletonPtr when declaring singleton mutexes.

c1728p9 · 2016-07-15T19:38:57Z

@mbed-bot: TEST

HOST_OSES=Windows
BUILD_TOOLCHAINS=ALL
TARGETS=KL25Z,LCP1768,K64F

mbed-bot · 2016-07-16T20:39:09Z

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

bridadan · 2016-07-16T20:42:51Z

@mbed-bot: TEST

HOST_OSES=windows
BUILD_TOOLCHAINS=ALL
TARGETS=KL25Z,LCP1768,K64F

mbed-bot · 2016-07-16T21:47:28Z

[Build 639]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

0xc0170 · 2016-07-18T15:33:40Z

LGTM

0xc0170 · 2016-07-19T09:36:51Z

@bogdanm review pls

bogdanm · 2016-07-19T09:45:07Z

hal/api/SingletonPtr.h

+    }
+
+    // This is zero initialized when in global scope
+    T *_ptr;


Why aren't _ptr and _data private?

This needs to be a plain old data (POD) type to get garbage collected by the linker properly. In C++ 03 POD type classes can't have any private members.

Can confirm, what @c1728p9 says is true until C++11 : (

bogdanm · 2016-07-19T09:50:12Z

hal/common/FileBase.cpp


 FileBase::FileBase(const char *name, PathType t) : _next(NULL),
                                                   _name(name),
                                                   _path_type(t) {
-    _mutex.lock();
+    _mutex->lock();


These mutex lock()/unlock() sequences really need to be encapsulated in a RAII class.

RAII would be good to have, as it would prevent the mutex from accidentally being left locked. It its not directly related to this PR though. Feel free to add a mutex RAII utility class in another PR.

0xc0170 · 2016-07-19T10:28:13Z

@c1728p9 Please can you provide answers (update the commit message), to clarify why this class shall be POD, and thus can't be RAII class, as asked above.

How it is intended to be used, what problems does it solve (use in the global scope, linker elimination of global objects, etc). What;s the intention for singleton_mutex, can I use it for my own classes?

bogdanm · 2016-07-19T10:28:23Z

Sorry if I'm missing something, but it looks to me like in this PR SingletonPtr is used to protect from concurent calls to PlatformMutex's constructor. That I understand. What I don't understand is why you don't protect the constructor of PlatformMutex directly. My worry is that if you start doing SingletonPtr<SomethingThatIsNotPlatformMutex> you're likely to run into race conditions, since you're using a single mutex to protect all possible singleton instantiations.

bogdanm · 2016-07-19T10:36:39Z

rtos/rtx/TARGET_CORTEX_A/RTX_CM_lib.h

+void pre_main (void)
+{
+    singleton_mutex_id = osMutexCreate(osMutex(singleton_mutex));
+    __rt_lib_init((unsigned)armcc_heap_base, (unsigned)armcc_heap_top);


Are you sure it's OK to call osMutexCreate before __rt_lib_init? How was this tested?

Yep this is safe to call here. The RTOS has been initialized by the time pre_main is called. The function pre_main its actually running on an RTOS created thread. The function __rt_lib_init is responsible for finishing up the standard library initialization by calling constructors.

c1728p9 · 2016-07-19T13:58:30Z

Hi @bogdanm, thanks for the detailed review.

The SingletonPtr class does the following:

Allow global classes to be garbage collected if they are unused
Protect global objects from being created twice due to a race condition on the first access

The PlatformMutex class could be protected directly, and this was done in #2051. That PR has been closed and replaced by this singleton PR since it solves the problem in a more generic way - singletons can be created for any class using the SingletonPtr class.

There is not a race condition when creating objects other than mutexes,. Furthermore this is an intended use case. For example this is planned to be used for LWIP - #429. Using a single mutex to protect everything does not lead to race conditions. It could lead to lock contention if two threads try to access a singleton for the first time at the same time. This is not a big concern though, since any subsequent accesses would bypass this mutex.

bogdanm · 2016-07-19T14:16:14Z

It probably works if you manage to constrain it to be used only on global objects. What about something like this (simplified):

struct B {
    B() {
        printf("B");
    }
};

struct A {
    A() {
        _b = get_B();
    }
    B *_b;
};

A *get_a() {
    static SingletonPtr<A> local;
    return local->get();
}

B *get_b() {
    static SingletonPtr<B> local;
    return local->get();
}

int main() {
    A *a = get_a();
}

Wouldn't this lead to lock contention?

c1728p9 · 2016-07-19T14:27:49Z

There would only be lock contention if two threads where trying to access A and/or B at the same time. Assuming this happened, the thread that was able to acquire the singleton lock first would be responsible for initializing the object it was trying to access. The other thread would need to wait for this to complete before it could get a handle to the same object. All subsequent accesses would bypass the mutex and there would be no contention.

Also, since mutexes can be recursively acquired, there is no possibility of deadlock in your example. If a thread calls get_a() for the first time and locks it, it will hold the lock until both A and B are fully initialized.

bogdanm · 2016-07-19T14:56:10Z

Thanks for the explanation. I wasn't aware that RTX mutexes are recursive by default (it's not easy to find that information), hence the confusion. I believe this will work.

c1728p9 · 2016-07-19T19:29:44Z

This PR is ready from my end
cc @sg-, @0xc0170

…..4a19dc4 4a19dc4 Import new thread files f6a021d Removed test files b9e842a Merge branch 'release_internal' into release_external 7d5d869 Merge pull request ARMmbed#2167 from ARMmbed/release_internal_merge 26e2d43 Merge branch 'master' into release_internal f43620f Add support for India band (ARMmbed#2166) 122f158 Merge pull request ARMmbed#2165 from ARMmbed/release_internal_merge 0e65ee5 Added disabling of NA for Thread BR PPP backbone 4c50e52 Added disabling of NA for Wi-SUN BR PPP backbone d2ea325 Moved DAD enabled check to Ipv6 SLAAC handler 49994fc Added PPP interface to nanostack 3383e91 Merge pull request ARMmbed#2163 from ARMmbed/IOTTHD-3558 81f7511 MAC: print RF configs 397240a MAC: Implemented CCA threshold and TX power setting 5907042 Added check for allocation failures in EAPOL 9ed97c9 ETX update: b489415 Add own certificate handling APIS (ARMmbed#2149) 888a0fb fhss_ws: check if 0 used as divider 586f2f2 Merge pull request ARMmbed#2160 from hugueskamba/hk-iotcore-1299-remove-fp-usage-ns_monitor f1d03b1 Remove floating-point usage in Nanostack heap monitor ef88f64 Removed rank comprae and also probe 5 best on the list. a2887d6 Clean PAN id compare trace print. f37dcf2 Wi-SUN NS NUD & Probe send update f7133f8 Merge pull request ARMmbed#2158 from ARMmbed/remove_temp_debug_traces 2dc1a8e fhss_ws: removed temporary debug traces 96f962a Reduce wi-sun NS Probe 0a1beb2 GTK update trigger fix a1d172e Limit Pan config sol timeout after 5 solication. 9d7414b Limit PAE supplikant GTK re-use for authentication from 2->1. 662df08 Fixed Key request address set issue if GTK mismatch is detected. a56b908 Merge pull request ARMmbed#2153 from ARMmbed/IOTTHD-3650 9b33e98 Fixed mac_pd_sap CCA_PREPARE active ACK handler. 035af9a Enhanced ACK GEN and TX update b1beb5d fhss_ws: typecast drift to int32_t f786fc9 Merge pull request ARMmbed#2152 from ARMmbed/fhss_coverity_fix 6efff35 fhss_ws: Coverity fixes d743e91 WS LLC brodacst shedule fix 6a6fb0c Removed old configuration options from Border router API a051865 Merge pull request ARMmbed#2135 from ARMmbed/IOTTHD-3232 ff771b1 Added empty interface function for network name set e94da3c Merge pull request ARMmbed#2146 from ARMmbed/IOTTHD-3571_2 234e649 added network name change function to public API 1770465 fhss_ws: Added temporary debug traces (IOTTHD-3571) d400859 Fix Thread 1.1 unitests (ARMmbed#2145) 38978f3 wi-sun ETX update: 4a71b04 Adjust Thread functions defined for Thread 1.2 (ARMmbed#2139) 4d8dc0d remove border router from pan size calculation fb3363e Merge pull request ARMmbed#2141 from ARMmbed/IOTTHD-3571 f01c5f2 fhss_ws: conversion macros/functions to support int64 a7b0027 Suprress dio sending whenRPL is not yet ready f8c9d54 Adjust tracing (ARMmbed#2138) 678eaf8 Moved Thread 1.2 code to to correct place f39d07e Merge pull request ARMmbed#2136 from ARMmbed/IOTTHD-3571 ab23116 FHSS: temporary debug traces (IOTTHD-3571) 09d4b06 MAC: Implemented PHY statistics git-subtree-dir: features/nanostack/sal-stack-nanostack git-subtree-split: 4a19dc4

0xc0170 added needs: work needs: review labels Jul 13, 2016

c1728p9 force-pushed the singleton_support branch from c127742 to 4106c64 Compare July 13, 2016 17:01

c1728p9 force-pushed the singleton_support branch from 4106c64 to 41e46f5 Compare July 14, 2016 22:34

0xc0170 reviewed Jul 15, 2016
View reviewed changes

c1728p9 added 2 commits July 15, 2016 14:12

Update ARMCC pre_main to be in C

d462717

Switch pre_main from assembly to C for ARMCC. This function does not need to be in assembly.

c1728p9 force-pushed the singleton_support branch from 41e46f5 to 348b32c Compare July 15, 2016 19:37

0xc0170 removed needs: review needs: work labels Jul 18, 2016

bogdanm reviewed Jul 19, 2016
View reviewed changes

geky mentioned this pull request Jul 19, 2016

[rtos] Add default constructor for Semaphore #2189

Merged

sg- merged commit d89c3c1 into ARMmbed:master Jul 19, 2016

Singleton support #2158

Singleton support #2158

Conversation

c1728p9 commented Jul 12, 2016

c1728p9 commented Jul 12, 2016

c1728p9 commented Jul 12, 2016

c1728p9 commented Jul 12, 2016

mbed-bot commented Jul 12, 2016

c1728p9 commented Jul 13, 2016

mbed-bot commented Jul 13, 2016

c1728p9 commented Jul 13, 2016

c1728p9 commented Jul 13, 2016

geky commented Jul 13, 2016

geky commented Jul 14, 2016

mbed-bot commented Jul 14, 2016

geky commented Jul 15, 2016

0xc0170 Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c1728p9 commented Jul 15, 2016

mbed-bot commented Jul 16, 2016

bridadan commented Jul 16, 2016

mbed-bot commented Jul 16, 2016

0xc0170 commented Jul 18, 2016

0xc0170 commented Jul 19, 2016

bogdanm Jul 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Jul 19, 2016

bogdanm commented Jul 19, 2016 • edited Loading

bogdanm Jul 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c1728p9 commented Jul 19, 2016

bogdanm commented Jul 19, 2016 • edited Loading

c1728p9 commented Jul 19, 2016

bogdanm commented Jul 19, 2016 • edited Loading

c1728p9 commented Jul 19, 2016

0xc0170 Jul 15, 2016 •

edited

Loading

bogdanm Jul 19, 2016 •

edited

Loading

bogdanm commented Jul 19, 2016 •

edited

Loading

bogdanm Jul 19, 2016 •

edited

Loading

bogdanm commented Jul 19, 2016 •

edited

Loading

bogdanm commented Jul 19, 2016 •

edited

Loading