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

[utest]: Allow the linker to remove any part of utest if not used #2559

Merged
merged 14 commits into from
Sep 10, 2016

Conversation

pan-
Copy link
Member

@pan- pan- commented Aug 26, 2016

This PR allow application to not embed any parts of utest if they don't use it.

Different changes where operated:

  • The scope of global C++ instances like the utest serial or green tea serial has been changed. Now they are static variable in functions. This little change prevent the global variable to be constructed automatically at startup and to be a part of the final binary even if its not used.
  • Fix multiple violations of ODR: variables or functions where defined at several places.
  • Regroup constant in compilation unit instead of header.
  • Avoid unnecessary copy of C++ constants instance. This was forcing initialization in ROM and prevented the constant to be optimized.
  • Provide base POD type for C++ objects like control_t and use it for constants. This change force the object to be constructed at compile time instead of runtime.

For more information see each commit.

This PR introduce the POD type base_control_t which is the base of the non POD control_t.
Every constant of the framework now use ``base_control_tinstead ofcontrol_t`
The maximum has been made to makes these two type compatible and this change has transparent as possible.

In only one case the compatibility is not preserved: ternary operator.

@pan-
Copy link
Member Author

pan- commented Aug 26, 2016

@0xc0170 @c1728p9 @geky @sg- @adbridge

could you review this PR ?

Some numbers about the gain (blinky example on K64F):

GCC:

  • RAM: -1024 bytes (8%)
  • ROM: -7184 bytes (13%)

ARMCC:

  • RAM: -932 (8.5%)
  • ROM: - 7112 (17%)

RawSerial greentea_serial(USBTX, USBRX);

RawSerial& greentea_serial() {
static RawSerial serial(USBTX, USBRX);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this guaranteed by the C++ standard to be initialized on first use or could it also be initialized at boot? Also, I doubt this is thread safe for GCC, although if utest calls this in its initialization sequence then it will be initialized in a single threaded manner. Maybe leave a comment here? Either that or you could ensure that it is thread safe by using the singleton_lock().

Also, @bridadan I know you made this change locally. Do you have any feedback on this? This version might be cleaner that using a singleton class.

Copy link
Contributor

Choose a reason for hiding this comment

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

My change was actually just merged in, but yeah this may be another alternative. I suppose one benefit of this is that the RawSerial instance is on the stack, not the heap. But yeah I think it'd be worth verifying your concerns you voiced above @c1728p9.

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.

Both this instance and the SingletonPtr instance should be allocated globally. According to this post, gcc by default emits thread-safe static constructors, although I'm not sure if we need to tie into hooks for that:
http://stackoverflow.com/questions/1270927/are-function-static-variables-thread-safe-in-gcc

It also raises the question of thread-safety on other toolchains.

If this is garunteed to be thread-safe per the C++03 standard, it may be a better solution over the existing SingletonPtr and we may want consider deprecating the SingletonPtr class in favor of this method. But that is a big if at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update the PR to take advantages of the changes from @bridadan.

As a side note:

Is this guaranteed by the C++ standard to be initialized on first use or could it also be initialized at boot?

Yes it is guaranteed to be initialized on first use.

lso, I doubt this is thread safe for GCC, although if utest calls this in its initialization sequence then it will be initialized in a single threaded manner.

No its not thread safe in C++03 (it is in C++11) but it can be thread safe for abi conforming to the itanium ABI: https://mentorembedded.github.io/cxx-abi/abi.html#once-ctor

It is possible to overload these definitions:

  • extern "C" int __cxa_guard_acquire ( __int64_t *guard_object );
  • extern "C" void __cxa_guard_release ( __int64_t *guard_object );
  • extern "C" void __cxa_guard_abort ( __int64_t *guard_object );

because the ARMEABI is based on itanium ABI and therefore it is possible to have thread safe static objects constructions on mbed without any workaround.

Copy link
Member Author

Choose a reason for hiding this comment

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

@geky:

If this is garunteed to be thread-safe per the C++03 standard, it may be a better solution over the existing SingletonPtr and we may want consider deprecating the SingletonPtr class in favor of this method. But that is a big if at the moment.

The singleton pointer class is still good to create PODs without wrapping everything as a static, in a function and it is immune to the static initialization fiasco.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since these both solve the same problem of global-initialization, shouldn't we chose one solution and consolidate?

👍

I would be all for getting rid of the SingletonPtr class. It is not very intuitive to declare.

Though this was to be renamed to LazyInitialization

Only downside is it won't have the semantics of pointer access to a global class.

The semantics of returning a reference and not being able to overloading the dot operator is very unintuitive. Pointer access to a global class is a HUGE 👍 for readability and clarity IMO.

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.

From what I understand, the static-initializer solution has several benefits over LazyInstance:

  • Language-based solution, no delicate tip-toeing around the definition of a POD type
  • Little concerns for regressions and one less class to support
  • Can easily do non-trivial amount of work (and accept arguments)
  • Is forward compatible with non-trivial changes to the initialization of the object
    (for example global thread instances could still start the thread in the access function)

If the only issue is the semantics of a reference over pointer, these functions could be easily changed to return a pointer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pan- I created #2562 to add support for initialization of static objects inside functions. I also added a test to make sure there are no race conditions on other toolchains.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm probably missing it, but I'm not sure I see where SingletonPtr benefits over static initialization other than the syntax difference, but both cases are different from the syntax for global objects.

Ok, let me go back to the start.

When a compilation unit define global (extern or static) variable(s) that are not initialized at compile time, then the compiler generate an initialization function which will initialize these global variables of this compilation unit before the entry in the main function.

Some examples of runtime global:

extern "C" { 
int foo();
const int bar = foo();
}

// the definition is omitted but Baz is not a POD 
class Baz;

Baz baz;

This initialization function will be referenced in special section which contains all the initialization functions of the program. Before the entry in main, the c runtime will execute every functions referenced in this section.

The main issue with that model is that global variables initialized at runtime can't be removed from the output because even if it seems like the variable is not used, it is used by the c runtime.

Then the dependency chain is:

c runtime -> initialization_function_array -> compilation unit initialization -> global variable.

That was the issue with the utest code and what this PR fix. A lot of global variables initialized at runtime were here and these variables, not collectable by the linker GC, pulled code and data from the utest library.

Global runtime (static or extern) variables are never an option for embedded libraries.

Wrapping the variable inside a LazyInstance or a function scope (as static) is the only option here (both have their advantages and inconvenient).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @pan- for providing the answers.

Since these both solve the same problem of global-initialization, shouldn't we chose one solution and consolidate?

In this manner, yes they are the same, but from the perspective of singleton, they are not, thus depends on the usage. Therefore I proposed a name change for SingletonPtr, not to confuse that this class is not a singleton. As @pan- wrote:

Wrapping the variable inside a LazyInstance or a function scope (as static) is the only option here (both have their advantages and inconvenient).

As there's PR for providing locks for local static variables, any other concerns?

@bridadan
Copy link
Contributor

@pan- You are a wizard :) Nice work on this, I took a crack at this once and I admit I fell short due to my lack of C++ foo. But all the changes you've made seem quite sane to me.

The only immediate issue I see with this PR is my PR changing greentea-client to use a SingletonPtr for the RawSerial was just merged, so there is now a merge conflict. That PR should acheive the same thing your changes do though. This was the PR for your reference: #2455

@sg-
Copy link
Contributor

sg- commented Aug 26, 2016

This should provide the same code size reduction.
a7c7ced

The next step for unity and greentea-client is to use the retargeted stdio once updated to use RTOS or Events

@bridadan
Copy link
Contributor

@sg- This PR actually goes a further step by preventing parts of utest that were coming in in addition to the RawSerial instance, so this actually reduces the code size by quite a bit more! My PR only removed a few 100 bytes I think.

handlers.test_setup = defaults.get_handler(specification.setup_handler);
handlers.test_teardown = defaults.get_handler(specification.teardown_handler);
handlers.test_failure = defaults.get_handler(specification.failure_handler);
defaults() = specification.defaults;
Copy link
Contributor

Choose a reason for hiding this comment

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

sigh, C++, the only language this line could make sense in...

@c1728p9
Copy link
Contributor

c1728p9 commented Aug 26, 2016

Excellent PR @pan-. Looking at the before and after map files it looks like this PR strips out ALL mbed global constructors.

Looks like these were removed:

 .init_array    0x0000d56c        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/mbed-utest-shim.o
 .init_array    0x0000d570        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/greentea-client/source/test_env.o
 .init_array    0x0000d574        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/unity_handler.o
 .init_array    0x0000d578        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/utest_case.o
 .init_array    0x0000d57c        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/utest_default_handlers.o
 .init_array    0x0000d580        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/utest_greentea_handlers.o
 .init_array    0x0000d584        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/utest_serial.o
 .init_array    0x0000d588        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/utest_shim.o
 .init_array    0x0000d58c        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/utest_harness.o
 .init_array    0x0000d590        0x4 ./.build/K64F/GCC_ARM/mbed-os/features/frameworks/utest/source/utest_types.o

@c1728p9
Copy link
Contributor

c1728p9 commented Aug 26, 2016

/morph test

@c1728p9
Copy link
Contributor

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 852]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

@mbed-bot
Copy link

Result: FAILURE

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

/morph test

@bridadan
Copy link
Contributor

@c1728p9 This has merge conflicts that need to be fixed before running CI again

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 30, 2016

@pan- +1 for this patch. Please rebase if this is completed

Note: As you mentioned earlier in the discussion, this type of optimizations might be worth capturing in the guidelines (I imagine we would provide C++ API guideline) where we could describe what we follow to make the footprint minimal (Lazy instance, POD vs nonPOD in the global scope, ODR, etc). That would explain nicely this patch and others that landed recently. Let's write it ?

pan- added 11 commits August 30, 2016 13:23
That way it is not a global object anymore and is not attached to the
.init section and init array. If the function which contain the object is
not called then the serial object will not be present in the final binary.
The change of scope allow the linker to remove the variable if not used.
It save space and fix the ODR violation.
Replace const control_t instances by this POD.

This save memory and prevent inclusion of the constants in the binary if
they are not used.
* Add conversion operator in control_t to convert instances to base_control_t
This change allow a lot of code from utest to be removed from the final
binary if not used.
* provide missing member functions from control_t in base_control_t
* construction of control_t from reference of base_control_t instead of
  value.
* overload operator+ for all permutations between control_t and
  base_control_t.
Both branches of a ternary operator should yield the same type;
compatibility is not enough in that case.
@pan- pan- force-pushed the utest_globals_optimizations branch from 0224f6a to a9d9987 Compare August 30, 2016 13:21
@pan-
Copy link
Member Author

pan- commented Aug 30, 2016

@0xc0170 It has been rebased, please test :)

@c1728p9
Copy link
Contributor

c1728p9 commented Aug 30, 2016

/morph test

@bridadan
Copy link
Contributor

@pan- I think another change to utest needs to be made before this is ready: https://github.com/pan-/mbed/blob/a9d9987d7c375ed3915654adede8981a74ffa7e1/features/frameworks/utest/utest/utest_serial.h#L24

My previous PR allowed greentea and utest to share the same RawSerial instance. It looks like this PR reverts this. The way I did this was by moving the shared serial instance into greentea-client/greentea-serial.h, which was then included by both greentea-client and utest. Do you think something similar could be done for this PR as well? Now Greentea is using a the new statically initialized RawSerial instance and utest is using a RawSerial instance from the SingletonPtr in greentea-client/greentea-serial.h.

@bridadan
Copy link
Contributor

Awesome, thanks @pan-

/morph test

@mbed-bot
Copy link

Result: FAILURE

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

/morph test

Output

mbed Build Number: 759

Test 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: 767

Test failed!

@bridadan
Copy link
Contributor

bridadan commented Sep 1, 2016

/morph test

@pan-
Copy link
Member Author

pan- commented Sep 1, 2016

@bridadan utest with this PR on ARMCC and IAR, I'm investigating why.

@bridadan
Copy link
Contributor

bridadan commented Sep 1, 2016

@pan- Thanks, we are observing strange behavior where some USB devices are being reset mid test by the Windows OS causing some failures to occur, we are looking into this as well.

@@ -66,7 +66,10 @@ static volatile utest_v1_harness_callback_t minimal_callback;
static volatile utest_v1_harness_callback_t ticker_callback;

// Timeout object used to control the scheduling of test case callbacks
Timeout utest_timeout_object;
static Timeout& utest_timeout_object() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use SingletonPtr same as RawSerial?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this bit cause a failure on some tests with ARMCC, it will be replaced.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pan- can you provide info on the ARMCC tests that were failing? This could be indicative of a more serious problem. I would like to investigate this regardless of if function local static initialization is used by this patch.

@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: 771

Test failed!

Makes sure to initialize it, otherwize, it might be initialized in
interrupt context.
@pan-
Copy link
Member Author

pan- commented Sep 6, 2016

@bridadan I've updated the PR, it should be good now.

@sg- sg- added needs: CI and removed needs: work labels Sep 6, 2016
@@ -47,8 +47,15 @@ namespace
size_t case_failed = 0;
size_t case_failed_before = 0;

handlers_t defaults = default_handlers;
handlers_t handlers = defaults;
handlers_t& defaults() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can handlers_t not be a class to and use the SingletonPtr paradigm for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

handlers_t is not a class, it is a POD.
The cause of the trouble is related to the initialization of these handlers: the initialization is made by copy of default_handlers.
This is not something done at compile time, the initialization happen at runtime.
That mean that the variables defaults and handlers will be kept even if there is no code using them after the initialization.
That also means that the variable default_handlers will be kept and the various function pointers which initialize it will be kept.

It is not possible to use directly a SingletonPtr here because there is no way to specify the initialization sequence.
On the other hand, it can be worked around by creating a new class derived from handlers_t which default initialize to default_handlers . Do you prefer this solution ?

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, it can be worked around by creating a new class derived from handlers_t which default initialize to default_handlers . Do you prefer this solution ?

That would be great!

@pan-
Copy link
Member Author

pan- commented Sep 9, 2016

@sg- I've updated the PR. Now, then handlers in utest_harness use a SingletonPtr.
Please, let me know if their is anything remaining.

@sg-
Copy link
Contributor

sg- commented Sep 9, 2016

/morph test

@sg-
Copy link
Contributor

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

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 795

All builds and test passed!

@mbed-bot
Copy link

mbed-bot commented Sep 9, 2016

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

@sg-
Copy link
Contributor

sg- commented Sep 9, 2016

/morph test

@mbed-bot
Copy link

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 803

All builds and test passed!

@sg- sg- merged commit 3806c87 into ARMmbed:master Sep 10, 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
@pan- pan- deleted the utest_globals_optimizations branch July 3, 2018 11:03
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