Fix heap base/limit error with ARM_LIB_STACK/ARM_LIB_HEAP in RTOS-less

[ccli8]

Description

This PR tries to fix heap memory error with ARM_LIB_STACK/ARM_LIB_HEAP in RTOS-less code. Per our check, heap base/limit passed to __rt_lib_init are incorrect in the condition that __user_setup_stackheap and ARM_LIB_STACK/ARM_LIB_HEAP co-exist. Both ARM/ARMC6 have the same issue.

The issue occurs only in RTOS-less, not in RTOS. This PR doesn't influence RTOS code.

Related issue

#6899

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

[0xc0170]

@SenRamakri @c1728p9 Can you please review this fix?

[c1728p9]

Hi @ccli8 rtos-less builds have a different memory map than the RTOS builds. The interrupt stack becomes the main stack. Additionally, the heap no longer has a strict limit, and instead is checked against the stack pointer.

The memory map looks something like this:

      +--------+   Last Address of RAM
      | Stack  |
      |   |    |
      |   v    |
      +--------+
RAM   |        |
      |        |
      +--------+
      |   ^    |
      |   |    |
      | Heap   |
      +--------+
      |   ZI   |
      +--------+
      |   RW   |  
      +========+  First Address of RAM
      |        |
Flash |        |

For reference, the RTOS code which turns on the two region memory model (and turns off the rtos-less one region mode) can be found here:
https://github.com/ARMmbed/mbed-os/blob/master/rtos/TARGET_CORTEX/mbed_boot.c#L410

[ccli8]

@c1728p9 Yes. In rtos-less build, stack/heap share one region. This holds for most targets, but not for Nuvoton targets (NUMAKER_PFM_NUC472/NUMAKER_PFM_M453/NUMAKER_PFM_M487/NUMAKER_PFM_NANO130). On Nuvoton targets, ARM_LIB_STACK/ARM_LIB_HEAP are defined in *.sct, so stack/heap occupy separate regions (two-region model).

LR_IROM1 0x0 {
  ER_IROM1 0x0 {
   *(RESET, +First)
   *(InRoot$$Sections)
   .ANY (+RO)
  }
  
  ARM_LIB_STACK 0x20000000 EMPTY 0x800 {
  }
  
  ER_IRAMVEC 0x20000800 EMPTY (4*(16 + 96)) {
  }
  
  RW_IRAM1 AlignExpr(+0, 16) {
   .ANY (+RW +ZI)
  }
  
  ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (0x20000000 + 0x28000 - AlignExpr(ImageLimit(RW_IRAM1), 16)) {
  }
}

This PR just targets the targets in which ARM_LIB_STACK/ARM_LIB_HEAP are defined and doesn't influence one-region targets in rtos-less build.

__value_in_regs struct __argc_argv $Sub$$__rt_lib_init (unsigned heapbase, unsigned heaptop)
{
    if (Image$$ARM_LIB_HEAP$$ZI$$Limit) {
        return $Super$$__rt_lib_init((unsigned) Image$$ARM_LIB_HEAP$$ZI$$Base, (unsigned) Image$$ARM_LIB_HEAP$$ZI$$Limit);
    } else {
        return $Super$$__rt_lib_init(heapbase, heaptop);
    }
}

In rtos build, there's no such issue because __rt_entry is overridden in mbed_boot.c as you pointed out.

[c1728p9]

@ccli8 I would recommend updating the .sct file to match other mbed 2 targets. For just one ram bank I don't know of a technical reason this Nuvoton should be different than the other mbed 2 targets.

[ccli8]

This is to locate stack at start of SRAM and detect stack overflow by H/W bus error in rtos-less program. After scanning all target folder, there are other few targets which also define ARM_LIB_STACK/ARM_LIB_HEAP, so I think this is common issue for these targets.

I don't prefer changing to one-region model because its influence range is large. Besides, all toolchains (ARM/ARMC6/GCC_ARM/IAR) on Nuvoton targets all apply two-region model. It is easier for maintenance with such consistency. If you worry this PR would influence other targets, I could add #if TARGET_NUVOTON to apply this PR for only Nuvoton targets.

@c1728p9

[ccli8]

@c1728p9 Any comment?

[cmonr]

@c1728p9 Ping

[samchuarm]

Hi can anyone help comment why this PR is not progressing for Nuvoton? Thanks.

[0xc0170]

There are other targets that have the similar memory layout. Browsing their linker files and startup files, some differences are there.

Retarget implementation implements __user_setup_stackheap that invokes _mbed_user_setup_stackheap. Function _mbed_user_setup_stackheap is weakly linked, therefore can be overwritten. Nuvoton target define this function.

From the http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0179b/CHDEGGBA.html, you should define __use_two_region_memory. This is missing for nuvoton ? This is defined for RTOS build, but in this case should also be imported for non rtos. Would that fix the issue?

From the note:

If you use the stack function above, you must also include an IMPORT __use_two_region_memory in your assembly source or a #pragma import(__use_two_region_memory) in your C/C++ source, because the two-region model is not automatically selected.

[deepikabhavnani]

Retarget implementation implements __user_setup_stackheap that invokes _mbed_user_setup_stackheap

__user_setup_stackheap is not called in case of RTOS less code.

I would suggest moving __rt_lib_init code to target specific file TARGET_NUVOTON\TARGET_M451\device\TOOLCHAIN_ARM_STD\sys.cpp

index abf213c62..2f6810385 100644
--- a/targets/TARGET_NUVOTON/TARGET_M451/device/TOOLCHAIN_ARM_STD/sys.cpp
+++ b/targets/TARGET_NUVOTON/TARGET_M451/device/TOOLCHAIN_ARM_STD/sys.cpp
@@ -19,6 +19,16 @@ extern "C" {
 extern char Image$$ARM_LIB_STACK$$ZI$$Limit[];
 extern char Image$$ARM_LIB_HEAP$$Base[];
 extern char Image$$ARM_LIB_HEAP$$ZI$$Limit[];
+
+#ifndef MBED_CONF_RTOS_PRESENT
+extern char Image$$ARM_LIB_HEAP$$ZI$$Base[];
+extern __value_in_regs struct __argc_argv $Super$$__rt_lib_init(unsigned heapbase, unsigned heaptop);
+
+__value_in_regs struct __argc_argv $Sub$$__rt_lib_init (unsigned heapbase, unsigned heaptop)
+{
+    return $Super$$__rt_lib_init((unsigned) Image$$ARM_LIB_HEAP$$ZI$$Base, (unsigned) Image$$ARM_LIB_HEAP$$ZI$$Limit);
+}
+#else
 extern __value_in_regs struct __initial_stackheap _mbed_user_setup_stackheap(uint32_t R0, uint32_t R1, uint32_t R2, uint32_t R3) {

     struct __initial_stackheap r;
@@ -26,6 +36,7 @@ extern __value_in_regs struct __initial_stackheap _mbed_user_setup_stackheap(uin
     r.heap_limit = (uint32_t)Image$$ARM_LIB_HEAP$$ZI$$Limit;
     return r;
 }
+#endif

 #ifdef __cplusplus

[0xc0170]

__user_setup_stackheap is not called in case of RTOS less code.

I see it now, rt entry invokes this function only for v5.

I would suggest moving __rt_lib_init code to target specific file TARGET_NUVOTON\TARGET_M451\device\TOOLCHAIN_ARM_STD\sys.cpp

It's moved now to only for nuvoton.

@ARMmbed/team-st-mcd there are few targets that define TWO_RAM_REGIONS, isn't this issue also present there and the fix should be applied?

@ARMmbed/mbed-os-core Please review. Also is two rams regions documented ?

@ccli8 I would recommend updating the .sct file to match other mbed 2 targets. For just one ram bank I don't know of a technical reason this Nuvoton should be different than the other mbed 2 targets.

@c1728p9 Is this hard requirement? There are couple of targets that have two ram regions memory defined. As noted, this is working fine for all supported toolchains.

[ccli8]

I make the following modifications:

1. Do rebase
2. Merge multiple sys.cpp files with ARM/ARMC6 toolchains into one
3. Move overriding __rt_lib_init to Nuvoton sys.cpp @deepikabhavnani
4. Add __use_two_region_memory in Nuvoton sys.cpp @0xc0170

[deepikabhavnani]

there are few targets that define TWO_RAM_REGIONS, isn't this issue also present there and the fix should be applied

I noted that few targets overwrite stack/heap settings in assembly startup files hence recommended to set it in Nuvoton specific code. Below is example for few

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_Silicon_Labs/TARGET_EFM32/TARGET_EFM32GG/device/TARGET_1024K/TOOLCHAIN_ARM_STD/startup_efm32gg.S

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_ONSEMI/TARGET_NCS36510/device/TOOLCHAIN_ARM/startup_NCS36510.S

[cmonr]

/morph build

[mbed-ci]

Build : SUCCESS

Build number : 2692
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6985/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

[mbed-ci]

Exporter Build : SUCCESS

Build number : 2322
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6985/

[mbed-ci]

Test : SUCCESS

Build number : 2423
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6985/2423