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

Update to 2-region model for HEAP and Stack Memory #9571

Merged
merged 40 commits into from Feb 27, 2019

Conversation

Projects
None yet
@mprse
Copy link
Member

commented Jan 31, 2019

Description

The following commits: #8039, #9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.

Verified that blinky example works on rtos-less build with this fix.

Pull request type

[ ] Fix
[x] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@deepikabhavnani Please review.

Release Notes

RAM memory model is updated to 2-region model. RTOS and non-RTOS versions of Mbed OS across all targets and toolchains will have separate stack and heap sections.
ISR stack size is unified across all targets and is set as 1K for Mbed OS with RTOS. In case of no RTOS, stack size is also the main thread size and is set as 4K.

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

@mprse, thank you for your changes.
@deepikabhavnani @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from deepikabhavnani and ARMmbed/mbed-os-maintainers Jan 31, 2019

@0xc0170 0xc0170 requested review from ARMmbed/team-nuvoton Jan 31, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

platform/mbed_retarget.cpp Outdated
@@ -904,21 +904,68 @@ extern "C" long PREFIX(_flen)(FILEHANDLE fh)
return size;
}

extern "C" char Image$$RW_IRAM1$$ZI$$Limit[];
#if defined(TARGET_NUMAKER_IOT_M487) || defined(TARGET_NUMAKER_PFM_M2351) || defined(TARGET_NUMAKER_PFM_M453) || defined(TARGET_NUMAKER_PFM_M487) || defined(TARGET_NUMAKER_PFM_NANO130) || defined(TARGET_NUMAKER_PFM_NUC472) || defined(TARGET_GR_LYCHEE) || defined(TARGET_RZ_A1H) || defined(TARGET_RZ_A1XX)

This comment has been minimized.

Copy link
@ccli8

ccli8 Feb 1, 2019

Contributor

All Nuvoton targets adopt two-region model, so this list of Nuvoton target defines can simplify to TARGET_NUVOTON.

platform/mbed_retarget.cpp Outdated
r.heap_base = zi_limit;
r.heap_limit = sp_limit;
r.heap_base = (uint32_t)HEAP_BASE;
r.heap_limit = (uint32_t)STACK_BASE;

This comment has been minimized.

Copy link
@ccli8

ccli8 Feb 1, 2019

Contributor

Why assign STACK_BASE to r.heap_limit rather than e.g. Image$$ARM_LIB_HEAP$$ZI$$Limit? Heap and stack may arrange non-consecutive or stack in lower memory/heap in higher memory (like Nuvoton targets)? The same question to $Sub$$__rt_lib_init.

This comment has been minimized.

Copy link
@mprse

mprse Feb 1, 2019

Author Member

So yeah it looks strange. The problem is that currently on most targets ARM_LIB_HEAP is not defined, but ARM_LIB_STACK is. This should be changed in the future.
Since it is misleading I think I add another macro HEAP_LIMIT.

@mprse mprse force-pushed the mprse:fix_9523_rtos_less_issue branch Feb 1, 2019

platform/mbed_retarget.cpp Outdated
#if defined(TARGET_NUVOTON) || defined(TARGET_RZ_A1XX)
extern char Image$$ARM_LIB_HEAP$$Base[];
extern char Image$$ARM_LIB_STACK$$Base[];
extern char Image$$ARM_LIB_HEAP$$Limit[];

This comment has been minimized.

Copy link
@ccli8

ccli8 Feb 1, 2019

Contributor

According to Image$$ execution region symbols, Image$$ARM_LIB_HEAP$$Limit doesn't include ZI section. Change to Image$$ARM_LIB_HEAP$$ZI$$Limit?

This comment has been minimized.

Copy link
@mprse

mprse Feb 1, 2019

Author Member

Fixed.

@mprse mprse force-pushed the mprse:fix_9523_rtos_less_issue branch 2 times, most recently Feb 1, 2019

platform/mbed_retarget.cpp Outdated

extern "C" __value_in_regs struct __argc_argv $Sub$$__rt_lib_init(unsigned heapbase, unsigned heaptop)
{
return $Super$$__rt_lib_init((unsigned)HEAP_BASE, (unsigned)STACK_BASE);

This comment has been minimized.

Copy link
@ccli8

ccli8 Feb 1, 2019

Contributor

Change STACK_BASE to HEAP_LIMIT?

This comment has been minimized.

Copy link
@mprse

mprse Feb 1, 2019

Author Member

Missed that. Sorry. Should be fixed now.

@mprse mprse force-pushed the mprse:fix_9523_rtos_less_issue branch 2 times, most recently to ec86ee1 Feb 1, 2019

@ccli8

ccli8 approved these changes Feb 1, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Thanks @ccli8 for review !

@0xc0170

0xc0170 approved these changes Feb 1, 2019

@deepikabhavnani
Copy link
Contributor

left a comment

Design and docs changes are needed for new memory model, I will add them next week.

@cmonr cmonr removed the needs: review label Feb 2, 2019

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

@mprse - I cannot add my commits to this PR, please cherry pick below listed commits, else I can create another PR (if that is preferred).

https://github.com/deepikabhavnani/mbed-os/commits/2-region

  1. 8ba28c0
  2. a883150
  3. 548f5fe
platform/mbed_retarget.cpp Outdated
#endif

#if !defined(HEAP_START)
#if defined(Image$$ARM_LIB_HEAP$$ZI$$Base) && defined(Image$$ARM_LIB_HEAP$$ZI$$Length)

This comment has been minimized.

Copy link
@ccli8

ccli8 Feb 5, 2019

Contributor

@deepikabhavnani Image$$ARM_LIB_HEAP$$ZI$$Base and Image$$ARM_LIB_HEAP$$ZI$$Length are linker-generated symbols. Can they be used for preprocessor defines?

This comment has been minimized.

Copy link
@deepikabhavnani

deepikabhavnani Feb 5, 2019

Contributor

I have not tested it, yes you are right linker symbols will not be valid during pre-processing step... will have to find another condition or define heap_start in target specific file mbed_rtx.h as it was done earlier

@d-kato

d-kato approved these changes Feb 5, 2019

Copy link
Contributor

left a comment

Sorry for the delay of the review. I have no problem with that.
And thanks @ccli8. Your code was helpful.

@mprse mprse changed the title Fix for issue #9523 (Non Rtos build fails for ARM compiler) Update to 2-region model for HEAP and Stack Memory Feb 5, 2019

rtos/TARGET_CORTEX/TOOLCHAIN_ARM_STD/mbed_boot_arm_std.c Outdated

extern uint32_t Image$$RW_IRAM1$$ZI$$Limit[];

extern uint32_t Image$$ARM_LIB_HEAP$$ZI$$Base[];

This comment has been minimized.

Copy link
@mprse

mprse Feb 5, 2019

Author Member

Currently ARM_LIB_HEAP is specified only for few targets (ARM_LIB_STACK for all), this means that the following change will cause build failure for most of the targets.

To fully move to the 2 region memory model we need to specify ARM_LIB_HEAP in all scatter files and __HeapLimit for GCC linker scripts. Then we will be able to completely remove ISR_STACK_SIZE, ISR_STACK_START, ISR_HEAP_SIZE, ISR_HEAP_START macros and relay only on the linker script symbols to define heap and stack.

I already tried that, but I took a step back since there was too much changes and we have discussed this here (and few next posts):
#9092 (comment)

This comment has been minimized.

Copy link
@deepikabhavnani

deepikabhavnani Feb 5, 2019

Contributor

Agree on __HeapLimit, will be needed for all GCC linker files, for now exploring the options to not have ARM_LIB_HEAP in all targets, moving it to mbed_rtx.h back seems reasonable.

This comment has been minimized.

Copy link
@deepikabhavnani

deepikabhavnani Feb 5, 2019

Contributor

Also I will not remove existing symbols for HEAP and Stack, since we have few targets who use completely different ram bank for heap, or want to expand heap to multiple banks. Symbols will be needed for those cases.

This comment has been minimized.

Copy link
@deepikabhavnani

deepikabhavnani Feb 10, 2019

Contributor

Added __HeapLimit for all targets in GCC linker scripts.

@deepikabhavnani deepikabhavnani force-pushed the mprse:fix_9523_rtos_less_issue branch Feb 5, 2019

@cmonr cmonr merged commit e1736cd into ARMmbed:master Feb 27, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(-16 bytes) RAM(-48 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9127 cycles (-1080 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 27, 2019

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@deepikabhavnani @mprse please remember to update handbook, there's a page about memory layout (not 100% sure it needs updating, but that needs to be confirmed)

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Also this change was in works for longer time did you guys remembered to update the new targets like PSoC6 based ones?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Also this change was in works for longer time did you guys remembered to update the new targets like PSoC6 based ones?

@mprse Can you please verify all targets on master are covered? I'll review now any outstanding targets in PRs to follow this as well.

@mprse

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

I'll check if all targets are covered.

@mprse

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

I checked all ARM_STD scatter files and GCC linker scripts and it looks like all are ok. Do we have successful CI run on master after this has been merged?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I've referenced this one to all current opened PR for new targets. See above

I checked all ARM_STD scatter files and GCC linker scripts and it looks like all are ok. Do we have successful CI run on master after this has been merged?

I can't access logs for the latest nightly. One job was triggered now, so might have results later today afternoon.

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Also this change was in works for longer time did you guys remembered to update the new targets like PSoC6 based ones?

I did check the linker scripts of PSoC6 and they were compatible. Also Musca was checked and I did shared the updated linker scripts with Musca

@deepikabhavnani deepikabhavnani deleted the mprse:fix_9523_rtos_less_issue branch Feb 27, 2019

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

please remember to update handbook, there's a page about memory layout (not 100% sure it needs updating, but that needs to be confirmed)

@bulislaw Bootstrap document is updated, please review ARMmbed/mbed-os-5-docs#951

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

there's a page about memory layout

@AnotherButler - Do we have any other doc in handbook describing memory layout (apart from one updated in ARMmbed/mbed-os-5-docs#951)

@AnotherButler

This comment has been minimized.

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

All, We have not changed the location of stack and heap, just separated them so the memory model is still the same.

@AnotherButler - Thanks, I have another concern related to bootstrap.md - This document is updated by this is not visible on mbed.com website.

@AnotherButler

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@LMESTM

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Has this been tested with uARM toolchain ?
When on top of master I see errors like:

$ python tools/singletest.py --auto --tc uARM  -n MBED_23 -v -V
MBEDLS: Detecting connected mbed-enabled devices...
MBEDLS: Detected DISCO_L496AG[FC3B], port: COM3, mounted: D:
[...]
Link: ticker_2
Error: L6218E: Undefined symbol $Super$$__rt_lib_init (referred from mbed_retarget.o).
Error: L6218E: Undefined symbol __use_two_region_memory (referred from mbed_retarget.o).
Finished: 0 information, 0 warning and 2 error messages.

whereas this works ok with ARM tc ...

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@LMESTM - #9766 should fix uARM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.