Skip to content
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

Fix GCC lazy init race condition and add test #2562

Merged
merged 2 commits into from
Sep 9, 2016

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Aug 26, 2016

Fix lazy initialization of objects in GCC along with adding a test for it.

singleton_unlock();
return 0;
}
MBED_ASSERT(0 == *(uint8_t*)guard_object);
Copy link
Contributor

@geky geky Aug 26, 2016

Choose a reason for hiding this comment

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

Is this assert helpful? Why not change the above to a simple boolean check:

if (*(uint8_t*)guard_object) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will catch if the guard object is uninitialized or clobbered.

Copy link
Contributor

@geky geky Aug 26, 2016

Choose a reason for hiding this comment

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

Ah ok, so it's an assert against the toolchain

@geky
Copy link
Contributor

geky commented Aug 26, 2016

Small nits, but looks good 👍

@c1728p9
Copy link
Contributor Author

c1728p9 commented Aug 26, 2016

/morph test

@c1728p9
Copy link
Contributor Author

c1728p9 commented Aug 26, 2016

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=GCC_ARM
TARGETS=K64F,NRF51_DK,NRF51_MICROBIT,NUCLEO_F411RE

@mbed-bot
Copy link

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

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 735

Test failed!

@@ -713,6 +713,39 @@ extern "C" void __env_unlock( struct _reent *_r )
{
__rtos_env_unlock(_r);
}

extern "C" int __cxa_guard_acquire (uint64_t *guard_object_p)
Copy link
Member

Choose a reason for hiding this comment

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

The signature is a bit different for ARM aeabi, an int* is used as parameter.
Same thing for __cxa_guard_release and __cxa_guard_abort.

More details here: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0043d/IHI0043D_rtabi.pdf

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @pan- . It is also described on ARM pages, that they use int* as a parameter, even for ARM6 compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

@c1728p9 Where did you take these prototypes from ? (uint64_t *guard_object_p) ?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to more closely match the run-time ABI as specified by ARM.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 30, 2016

nit:
The commit message:

Implement the functions __cxa_guard_acquire, __cxa_guard_release and
__cxa_guard_abort so any objects lazily initialized in GCC are done so
in a thread safe manner.

I assume the aim of the fix is to guard lazily initialised objects (as they are done at any time), but this fix is regarding locks for local static variable initialisation. (might be more clear to a reader).

+1 for providing locks for this to have it thread safe.

Test SingletonPtr initailization along the with lazy initailization
that is built into the C++ language itself.
Implement the functions __cxa_guard_acquire, __cxa_guard_release and
__cxa_guard_abort so lazily initialized function-local static objects
are done so in a thread safe manner in GCC.
@c1728p9
Copy link
Contributor Author

c1728p9 commented Aug 30, 2016

Rebased pr to master, updated functions to match run-time ABI from ARM and updated commit message to make it more clear.

@c1728p9
Copy link
Contributor Author

c1728p9 commented Aug 30, 2016

/morph test

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 752

Test failed!

@c1728p9
Copy link
Contributor Author

c1728p9 commented Aug 30, 2016

/morph test

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 31, 2016

Seems like this job stalled CI. I'll try to terminate it or ask someone do it

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 755

Test failed!

@c1728p9
Copy link
Contributor Author

c1728p9 commented Aug 31, 2016

/morph test

@mbed-bot
Copy link

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 765

Build Prep failed!

@bridadan
Copy link
Contributor

/morph test

@mbed-bot
Copy link

mbed-bot commented Sep 1, 2016

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 766

Test failed!

@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 1, 2016

/morph test

@mbed-bot
Copy link

mbed-bot commented Sep 1, 2016

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 770

Test failed!

@sg-
Copy link
Contributor

sg- commented Sep 8, 2016

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@sg-
Copy link
Contributor

sg- commented Sep 8, 2016

/morph test

@sg- sg- removed the needs: review label Sep 8, 2016
@mbed-bot
Copy link

mbed-bot commented Sep 9, 2016

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 788

All builds and test passed!

@mbed-bot
Copy link

mbed-bot commented Sep 9, 2016

[Build ${MBED_BUILD_ID}]
FAILURE: Something went wrong when building and testing.

@bridadan
Copy link
Contributor

bridadan commented Sep 9, 2016

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot
Copy link

mbed-bot commented Sep 9, 2016

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

@sg- sg- merged commit 0128dd2 into ARMmbed:master Sep 9, 2016
theotherjimmy added a commit that referenced this pull request Sep 19, 2016
Release mbed-os-5.1.4

Changes:

New Targets:
2504: [Disco_F769NI] adding new target [#2504]
2654: DELTA_DFBM_NQ620 platform porting [#2654]
2615: [MTM_MTCONNECT04S] Added support for MTM_MTCONNECT04S [#2615]
2548: Nucleof303ze [#2548]

Fixes:

2678: Fixing NCS36510 compile on Linux #2678
2657: [MAX326xx] Removed echoing of characters and carriage return. #2657
2651: Use lp_timer to count time in the deepsleep tests #2651
2645: NUCLEO_F446ZE - Enable mbed5 release version #2645
2643: Fix thread self termination #2643
2634: Updated USBHost for library changes #2634
2633: Updated USBDevice to use Callback #2633
2630: Test names not dependent on disk location of root #2630
2624: CFSTORE Bugfix for realloc() moving KV area and cfstore_file_t data structures not being updated correctly #2624
2623: DISCO_L476VG - Add Serial Flow Control pins + add SERIAL_FC macro #2623
2617: STM32F2xx - Enable Serial Flow Control #2617
2613: Correctly providing directories to build_apis #2613
2607: Fix uvisor memory tracing #2607
2604: Tools - Fix fill section size variation #2604
2601: Adding ON Semiconductor copyright notice to source and header files. #2601
2597: [HAL] Fixed "intrinsic is deprecated" warnings #2597
2596: [HAL] Improve memory tracer #2596
2594: Fix TCPServer constructor #2594
2593: Add app config command line switch for test and make #2593
2589: [NUC472] Fix heap configuration error with armcc #2589
2588: Timing tests drift refactor #2588
2587: add PTEx pins as option for SPI on Hexiwear - for SD Card Interface #2587
2584: Set size of callback irq array to IrqCnt #2584
2583: github issue and PR templates #2583
2582: [GCC_CR] fix runtime hang for baremetal build #2582
2580: lwip - Add check for previously-bound socket #2580
2579: lwip - Fix handling of max sockets in socket_accept #2579
2578: Fix double free in NanostackInterface #2578
2576: Add smoke test that builds example programs with mbed-cli #2576
2575: tools-config! -  Allow an empty or mal-formed config to be passed to the config system #2575
2562: Fix GCC lazy init race condition and add test #2562
2559: [utest]: Allow the linker to remove any part of utest if not used #2559
2545: Added define guards for SEQUENTIAL_FLASH_JOURNAL_MAX_LOGGED_BLOBS so  #2545
2538: STM32F4xx - Add support of ADC internal channels (Temp, VRef, VBat) #2538
2521: [NUCLEO_F207ZG] Add MBED5 capability #2521
2514: Updated FlexCan and SAI SDK drivers #2514
2487: Runtime dynamic memory tracing #2487
2442: Malloc heap info #2442
2419: [STM32F1] Add asynchronous serial #2419
2393: [tools] Prevent trace-backs from incomplete args #2393
2245: Refactor export subsystem #2245
2130: stm32 : reduce number of device.h files #2130
@bridadan bridadan mentioned this pull request Nov 21, 2016
aisair pushed a commit to aisair/mbed that referenced this pull request Apr 30, 2024
Changes:

New Targets:
#2504: [Disco_F769NI] adding new target [ARMmbed/mbed-os#2504]
#2654: DELTA_DFBM_NQ620 platform porting [ARMmbed/mbed-os#2654]
#2615: [MTM_MTCONNECT04S] Added support for MTM_MTCONNECT04S [ARMmbed/mbed-os#2615]
#2548: Nucleof303ze [ARMmbed/mbed-os#2548]

Fixes:

#2657: [MAX326xx] Removed echoing of characters and carriage return. [ARMmbed/mbed-os#2657]
#2651: Use lp_timer to count time in the deepsleep tests [ARMmbed/mbed-os#2651]
#2643: Fix thread self termination [ARMmbed/mbed-os#2643]
#2623: DISCO_L476VG - Add Serial Flow Control pins + add SERIAL_FC macro [ARMmbed/mbed-os#2623]
#2617: STM32F2xx - Enable Serial Flow Control [ARMmbed/mbed-os#2617]
#2601: Adding ON Semiconductor copyright notice to source and header files. [ARMmbed/mbed-os#2601]
#2597: [HAL] Fixed "intrinsic is deprecated" warnings [ARMmbed/mbed-os#2597]
#2589: [NUC472] Fix heap configuration error with armcc [ARMmbed/mbed-os#2589]
#2587: add PTEx pins as option for SPI on Hexiwear - for SD Card Interface [ARMmbed/mbed-os#2587]
#2584: Set size of callback irq array to IrqCnt [ARMmbed/mbed-os#2584]
#2582: [GCC_CR] fix runtime hang for baremetal build [ARMmbed/mbed-os#2582]
#2562: Fix GCC lazy init race condition and add test [ARMmbed/mbed-os#2562]
#2538: STM32F4xx - Add support of ADC internal channels (Temp, VRef, VBat) [ARMmbed/mbed-os#2538]
#2514: Updated FlexCan and SAI SDK drivers [ARMmbed/mbed-os#2514]
#2442: Malloc heap info [ARMmbed/mbed-os#2442]
#2419: [STM32F1] Add asynchronous serial [ARMmbed/mbed-os#2419]
#2130: stm32 : reduce number of device.h files [ARMmbed/mbed-os#2130]
#2678: Fixing NCS36510 compile on Linux [ARMmbed/mbed-os#2678]
#2607: Fix uvisor memory tracing [ARMmbed/mbed-os#2607]
#2596: [HAL] Improve memory tracer [ARMmbed/mbed-os#2596]
#2487: Runtime dynamic memory tracing [ARMmbed/mbed-os#2487]
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