Fix heap init error in rtos-less code #10078

ccli8 · 2019-03-13T10:32:35Z

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 · 2019-03-13T12:00:33Z

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

0xc0170 · 2019-03-15T11:09:20Z

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 · 2019-03-27T06:00:37Z

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

cmonr · 2019-03-27T21:55:23Z

@ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal Please review.

ccli8 · 2019-04-08T10:12:02Z

Any update?

0xc0170 · 2019-04-08T10:27:46Z

CI started

mbed-ci · 2019-04-08T12:08:00Z

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 · 2019-04-08T12:32:39Z

@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 · 2019-04-10T10:16:11Z

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 · 2019-04-10T10:21:20Z

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 · 2019-04-10T11:26:11Z

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

Because they are not OS5, only OS2

0xc0170 · 2019-04-10T11:29:13Z

@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 · 2019-04-10T11:31:47Z

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

0xc0170 · 2019-04-10T11:36:32Z

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 · 2019-04-10T12:18:43Z

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:

mbed-os/platform/mbed_retarget.cpp

Lines 923 to 931 in ffe9ddf

    
           #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 · 2019-04-10T12:29:06Z

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 · 2019-04-10T14:13:59Z

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.

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 · 2019-04-12T06:43:08Z

I make the following modifications:

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

ccli8 · 2019-04-18T10:10:17Z

Any update?

0xc0170

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 · 2019-04-18T11:23:39Z

I restarted travis, CI will be started once approved.

mprse

👍

deepikabhavnani

Thanks for change 👍

ccli8 · 2019-04-19T01:44:38Z

Added "Release Notes" section.

adbridge · 2019-04-25T15:18:17Z

ci started

mbed-ci · 2019-04-26T00:37:29Z

Test run: SUCCESS

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

ciarmcom requested review from a team March 13, 2019 12:00

ciarmcom added the needs: review label Mar 13, 2019

cmonr requested a review from a team March 27, 2019 21:55

cmonr added the release-version: 5.12.1 label Mar 27, 2019

c1728p9 approved these changes Mar 28, 2019

View reviewed changes

0xc0170 approved these changes Apr 8, 2019

View reviewed changes

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

cmonr added needs: work and removed needs: CI release-version: 5.12.1 labels Apr 8, 2019

Fix heap init error in rtos-less code

64515c0

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 force-pushed the nuvoton_fix-heap-in-rtosless branch from 41578f1 to 64515c0 Compare April 12, 2019 06:40

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

0xc0170 requested review from c1728p9 and 0xc0170 April 12, 2019 09:44

0xc0170 reviewed Apr 18, 2019

View reviewed changes

mprse approved these changes Apr 18, 2019

View reviewed changes

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

deepikabhavnani approved these changes Apr 18, 2019

View reviewed changes

adbridge added ready for merge release-version: 5.12.3 and removed needs: CI labels Apr 26, 2019

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

adbridge removed the ready for merge label Apr 26, 2019

ccli8 deleted the nuvoton_fix-heap-in-rtosless branch April 30, 2019 01:30

0xc0170 mentioned this pull request May 21, 2019

FCCE fails to run on CY8CKIT_062_WIFI_BT_PSA target with ARMC6 compiler #10613

Closed

Fix heap init error in rtos-less code #10078

Fix heap init error in rtos-less code #10078

Uh oh!

Conversation

ccli8 commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Release Notes

Uh oh!

ciarmcom commented Mar 13, 2019

Uh oh!

0xc0170 commented Mar 15, 2019

Uh oh!

ccli8 commented Mar 27, 2019

Uh oh!

cmonr commented Mar 27, 2019

Uh oh!

ccli8 commented Apr 8, 2019

Uh oh!

0xc0170 commented Apr 8, 2019

Uh oh!

mbed-ci commented Apr 8, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Apr 8, 2019

Uh oh!

0xc0170 commented Apr 10, 2019

Uh oh!

0xc0170 commented Apr 10, 2019

Uh oh!

jeromecoutant commented Apr 10, 2019

Uh oh!

0xc0170 commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromecoutant commented Apr 10, 2019

Uh oh!

0xc0170 commented Apr 10, 2019

Uh oh!

mprse commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mprse commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented Apr 10, 2019

Uh oh!

ccli8 commented Apr 12, 2019

Uh oh!

ccli8 commented Apr 18, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 18, 2019

Uh oh!

mprse left a comment

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani left a comment

Choose a reason for hiding this comment

Uh oh!

ccli8 commented Apr 19, 2019

Uh oh!

adbridge commented Apr 25, 2019

Uh oh!

mbed-ci commented Apr 26, 2019

Test run: SUCCESS

Uh oh!

Uh oh!

ccli8 commented Mar 13, 2019 •

edited

Loading

0xc0170 commented Apr 10, 2019 •

edited

Loading

mprse commented Apr 10, 2019 •

edited

Loading

mprse commented Apr 10, 2019 •

edited

Loading