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 heap init error in rtos-less code #10078

Merged
merged 1 commit into from Apr 26, 2019

Conversation

Projects
None yet
10 participants
@ccli8
Copy link
Contributor

commented Mar 13, 2019

Description

This PR tries to fix heap initialization error in rtos-less code on ARM/ARMC6 toolchain. In rtos-less code, HEAP_START is defined in target mbed_rtx.h but not passed to _mbed_user_setup_stackheap or __rt_lib_init in mbed_retarget.cpp. The default defines for HEAP_START/HEAP_SIZE are for single-region targets. Heap initialization error will occur on two-region targets. We cannot include mbed_rtx.h in mbed_retarget.cpp because Mbed 2 targets don't provide it. Instead, this PR approaches it by weak reference to ARM_LIB_HEAP. Heap definition is fixed if ARM_LIB_HEAP is defined.

Pull request type

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

Release Notes

On ARM/ARMC6 toolchain, fix heap initialization error for two-region memory model (ARM_LIB_HEAP + ARM_LIB_STACK) in rtos-less code. Original implementation just supports one-region memory model (ARM_LIB_STACK). This release fixes for two-region memory model.

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

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

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Mar 13, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Fix looks fine to me but having mbed_rtx include in platform does not much. This is the header file that defines stack and heap macros.
This might require additional refactor if that would be the case.

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Any update? How about make it "right" first and then refactor?

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@cmonr cmonr requested a review from ARMmbed/mbed-os-hal Mar 27, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Any update?

@0xc0170

0xc0170 approved these changes Apr 8, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 8, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 8, 2019

Test run: FAILED

Summary: 2 of 9 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-IAR8
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@mprse The above failures for mbed 2 - INITIAL_SP not defined for few targets. There was a consolidation of the stack pointer recently.
Can you review the failures above why are few targets failing now?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I am slightly confused about mbed_rtx.h. and what it defines. It was created for Mbed OS 5, not Mbed 2. Thus including this by default results in some failures (some mbed 2 targets do not define INITIAL_SP). Thus we get these failures. Before we add these missing values, what about heap macros?

Also not all targets define them in mbed_rtx.h - is this documented ?

cc @deepikabhavnani

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

NUCLEO_F030R8/
NUCLEO_F031K6/
NUCLEO_F042K6/
SAMD21G18A/
SAMD21J18A/
SAMR21G18A/

These targets are failing to build.
@ARMmbed/team-st-mcd Can you review these nucleos, why they do not define INITIAL_SP?

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Can you review these nucleos, why they do not define INITIAL_SP?

Because they are not OS5, only OS2

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@ccli8 Would it be other way to fix this ? Pulling this header file has implications for mbed 2 targets.

Heap definitions should be placed somewhere else and pulled in to retarget.

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

See #9626: "INITIAL_SP definition is no more used (since #9092)"

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Thanks, missed that one today.

@ccli8 Would it be other way to fix this ? Pulling this header file has implications for mbed 2 targets.

@deepikabhavnani how to address this?

@mprse

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Generally, there is a problem with Mbed 2 and ARM compiler builds for targets which specify ARM_LIB_HEAP/ARM_LIB_STACK regions in scatter files. Is that correct?

The default defines for HEAP_START/HEAP_SIZE are for single-region targets:

#if !defined(HEAP_START)
// Heap here is considered starting after ZI ends to Stack start
extern uint32_t Image$$ARM_LIB_STACK$$ZI$$Base[];
extern uint32_t Image$$RW_IRAM1$$ZI$$Limit[];
#define HEAP_START Image$$RW_IRAM1$$ZI$$Limit
#define HEAP_SIZE ((uint32_t)((uint32_t) Image$$ARM_LIB_STACK$$ZI$$Base - (uint32_t) HEAP_START))
#endif
#define HEAP_LIMIT ((uint32_t)((uint32_t)HEAP_START + (uint32_t)HEAP_SIZE))

As far as I remember we assumed that by default only stack region is specified in the scatter files and then default defines are used(from mbed_retarget.cpp). Targets with two regions defined(ARM_LIB_HEAP and ARM_LIB_STACK) need to specify their own HEAP_START/HEAP_SIZE macros (usually in mbed_rtx.h).

Now since HEAP_START/HEAP_SIZE macros are defined in mbed_rtx.h we don't have them in case of Mbed 2 builds. This causes a failure in case of scatter files with two regions defined.

@mprse

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Personally, I think that we should add ARM_LIB_HEAP to all scatter files. Then we could remove all HEAP_*, STACK_* macros from mbed_rtx.h and relay only on linker symbols.

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Personally, I think that we should add ARM_LIB_HEAP to all scatter files. Then we could remove all HEAP_, STACK_ macros from mbed_rtx.h and relay only on linker symbols.

I will agree with @mprse on this, we should add ARM_LIB_HEAP to all scatter files and remove dependency of mbed_rtx.h.

Fix heap init error in rtos-less code
In rtos-less code, heap is defined by assuming one-region. Through weak-reference to
ARM_LIB_HEAP, heap definition is fixed if ARM_LIB_HEAP is defined.

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fix-heap-in-rtosless branch from 41578f1 to 64515c0 Apr 12, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I make the following modifications:

  1. Drop including mbed_rtx.h because Mbed 2 targets don't provide it.
  2. Through weak-reference to ARM_LIB_HEAP, heap definition is fixed if ARM_LIB_HEAP is defined.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Apr 12, 2019

@0xc0170 0xc0170 requested review from c1728p9 and 0xc0170 Apr 12, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Any update?

@0xc0170
Copy link
Member

left a comment

Looks fine to me. It would help to describe this via "Release notes" section (add it to your first comment here). This section will be added to the release notes.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

I restarted travis, CI will be started once approved.

@mprse

mprse approved these changes Apr 18, 2019

Copy link
Member

left a comment

👍

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 18, 2019

@deepikabhavnani
Copy link
Contributor

left a comment

Thanks for change 👍

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

Added "Release Notes" section.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

ci started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 26, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@adbridge adbridge merged commit 23af842 into ARMmbed:master Apr 26, 2019

26 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-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
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 9178 cycles (-1188 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
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

@ccli8 ccli8 deleted the OpenNuvoton:nuvoton_fix-heap-in-rtosless branch Apr 30, 2019

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.