Adding debugger awarness with Keil MDK #8887

deepikabhavnani · 2018-11-27T23:13:31Z

Description

Main thread in Mbed OS is statically allocated and was not available in call stack of Keil MDK. The RTX5 kernel requires statically allocated thread information objects that are placed into a specific section to enable RTOS thread awareness in Keil MDK. This fix is to keep main thread in specific section of memory.

Issue: #8793

Pull request type

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

kjbracey · 2018-11-28T07:44:17Z

How does this interact with #7244? If that still applies, this will cost a lot of ROM in ARMCC?

Presumably nothing you could do about the underlying issue, but maybe you could restrict the section attribute to MBED_DEBUG builds?

deepikabhavnani · 2018-11-28T16:59:50Z

Tested blinky example with and without change to see the impact on ROM size, please see the results below:

Without Fix

| Module                                                       |         .text |       .data |        .bss |
|--------------------------------------------------------------|---------------|-------------|-------------|
| mbed-os\rtos\TARGET_CORTEX\mbed_rtos_rtx.o                   |     216(+216) |       4(+4) | 4248(+4248) |

| Subtotals                                                    | 39461(+39461) | 2549(+2549) | 8292(+8292) |
Total Static RAM memory (data + bss): 10841(+10841) bytes
Total Flash memory (text + data): 42010(+42010) bytes

With Fix

| Module                                                       |         .text |       .data |        .bss |
| mbed-os\rtos\TARGET_CORTEX\mbed_rtos_rtx.o                   |   216(+0)     |   72(+68)   | 4180(-68)   |
| Subtotals                                                    | 39517(+56)    | 2617(+68)   | 8224(-68)   |
Total Static RAM memory (data + bss): 10841(+0) bytes
Total Flash memory (text + data): 42134(+124) bytes

deepikabhavnani · 2018-11-28T17:32:46Z

@kjbracey-arm - Having just the control block in BSS section resolves the issue, i.e. we are not putting the main thread stack in .bss here. https://github.com/ARMmbed/mbed-os/pull/8887/files#diff-91e672466b09fb21b174f0f44e07b8aaL31 hence the size impact is not much.

On another thought can we do same do Idle / timer threads?


C:\Users\deebha02\work_mbed\KeilIDE_fix\mbed-os>git diff
diff --git a/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c b/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c
index 8fe704f80..f35481981 100644
--- a/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c
+++ b/rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c
@@ -122,8 +122,7 @@ static osRtxThread_t os_idle_thread_cb \
 __attribute__((section(".bss.os.thread.cb")));

 // Idle Thread Stack
-static uint64_t os_idle_thread_stack[OS_IDLE_THREAD_STACK_SIZE/8] \
-__attribute__((section(".bss.os.thread.stack")));
+static uint64_t os_idle_thread_stack[OS_IDLE_THREAD_STACK_SIZE/8];

 // Idle Thread Attributes
 static const osThreadAttr_t os_idle_thread_attr = {
@@ -179,8 +178,7 @@ static osRtxThread_t os_timer_thread_cb \
 __attribute__((section(".bss.os.thread.cb")));

 // Timer Thread Stack
-static uint64_t os_timer_thread_stack[OS_TIMER_THREAD_STACK_SIZE/8] \
-__attribute__((section(".bss.os.thread.stack")));
+static uint64_t os_timer_thread_stack[OS_TIMER_THREAD_STACK_SIZE/8];

| mbed-os\rtos\TARGET_CORTEX\rtx5\RTX\Source\rtx_lib.o | 285(+0) | 332(-768) | 1280(+768) |

kjbracey · 2018-12-12T12:04:37Z

We could make such a local change to RTX, #7244 closed with a change not happening upstream, and not making a local change either, but we could still do something locally.

And just removing the area directive from the stacks altogether seems plausible - if we have no special handling for the .bss.os area in our linker maps.

If this PR was a net ROM reduction by taking the stacks out of ROM and putting this control block in, @bulislaw couldn't complain.

deepikabhavnani · 2018-12-13T18:25:15Z

if we have no special handling for the .bss.os area in our linker maps.

I am not sure of any special handling for .bss.os.
@c1728p9 - your inputs please..

deepikabhavnani · 2018-12-13T20:21:45Z

@JonatanAntoni - Please review and share your thoughts on this PR

JonatanAntoni · 2018-12-14T09:12:28Z

Hi @deepikabhavnani,

this looks meaningful to me. The relevant part for RTX5 awareness in MDK (and potentially other IDEs) is to place all the object control blocks to their specific memory sections. This applies to all RTOS objects (not only threads) when control blocks are allocated manually (from RTX point of view).

Cheers,
Jonatan

deepikabhavnani · 2018-12-18T19:38:06Z

@c1728p9 @bulislaw - Please review

c1728p9 · 2018-12-19T23:26:41Z

The relevant part for RTX5 awareness in MDK (and potentially other IDEs) is to place all the object control blocks to their specific memory sections. This applies to all RTOS objects (not only threads) when control blocks are allocated manually (from RTX point of view).

@JonatanAntoni does this mean that threads created by using the Thread class in mbed-os will not show up in MDKs RTX5 awareness? The Thread class contains the control block as one of its members - _obj_mem. Note that mbed_rtos_storage_thread_t is a typedef of osRtxThread_t.

tools/profiles/debug.json

JonatanAntoni · 2018-12-20T08:19:57Z

[..] does [it] mean that threads created by using the Thread class in mbed-os will not show up in MDKs RTX5 awareness?

I have never tried to debug a mbed application using MDK, but I assume it won't show up. The RTOS-awareness (generally speaking component awareness) in MDK is generated using a generic Software Component View Description. The section names are used as a common entry point for control block look up. Control blocks spread randomly over the memory cannot be found.

Is the --keep actually needed with more recent versions of RTX?

--keep is not needed for Arm Compiler 6 but for Arm Compiler 5. The symbol os_cb_sections denotes the top level entry for control block look up, see rtx_lib.c:583.

Cheers,
Jonatan

cmonr · 2018-12-27T17:30:59Z

Testing PR with CI.

mbed-ci · 2018-12-27T20:30:28Z

Test run: SUCCESS

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

deepikabhavnani · 2019-01-03T16:39:05Z

@c1728p9 @kjbracey-arm @bulislaw - Please review

rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c

cmonr · 2019-01-09T18:26:08Z

CI started

mbed-ci · 2019-01-10T09:09:29Z

Test run: SUCCESS

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

deepikabhavnani · 2019-01-15T17:42:59Z

Conflicts :-(
@ARMmbed/mbed-os-maintainers - This PR is tagged for 5.12 and was ready to merge, do we have any restrictions on merge to master for PR's tagged for minor release?

Main thread in Mbed OS is statically allocated and was not available in call stack of Keil MDK. The RTX5 kernel requires statically allocated thread information objects that are placed into a specific section to enable RTOS thread awareness in Keil MDK. This fix is to keep main thread in specific section of memory.

In case of ARM compiler, idle and timer thread stack though assigned to `.bss.os` section since not zero initialized are part of `data` section. In this commit, we are moving stacks of idle and timer thread to bss section and thereby saving ROM space.

deepikabhavnani · 2019-01-15T18:11:06Z

Rebased and conflicts resolved. Changing label from ready to merge to needs:CI

cmonr · 2019-01-17T05:12:10Z

@deepikabhavnani Nothing hard, but we definitely prefer if PRs for minior releases come in a bit later than right after a minor release is made, if that makes sense.

Lowers the risk that early 5.x.{1,2,3} patch releases will be troublesome.

cmonr · 2019-01-17T05:12:19Z

CI started

mbed-ci · 2019-01-17T11:31:14Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_cloud-client-test

0xc0170 · 2019-01-17T11:55:47Z

Client test got stuck, restarted

deepikabhavnani · 2019-01-17T15:45:56Z

we definitely prefer if PRs for minior releases come in a bit later than right after a minor release is made, if that makes sense.

👍 This helps, will consider this when adding changes for minor release. Else can we add and park those changes ?

chentang16 · 2019-03-07T14:36:31Z

I just found out the proposed "patch" has only been applied to the "_main_obj" thread. Actually all internal RTX5 threads defined and created by mbedOS need to have such a patch.
The change or improvement made so far can only make OS awareness debugging in MDK feasible for one single main thread, which isn't sufficient.

deepikabhavnani · 2019-03-07T14:40:36Z

@chentang16 - Yes the patch is applied to main thread, for other threads it will be change in design since we allocate dynamic memory for thread stack. Issue can be closed for Keil MDK debugger but it will remain open in Mbed OS

kjbracey · 2019-03-07T14:45:20Z

Yes, this just handles the static built-in threads.

It's unclear what the path forward here is. Mbed OS is currently permitting general allocation, including thread control blocks and stacks even being on the heap, rather than being restricted to static allocation. If MDK can only debug threads allocated via fixed static memory, it's never going to be able to handle the general case, as represented by our Thread class.

I don't understand what MDK actually does with the special area names - if you were to just stick __attribute__((section(".bss.os.thread.cb"))) on a static Thread object, would that make it work?

chentang16 · 2019-03-07T14:50:06Z

"custom control block" plays the key role here in the MDK debugger, as explained here http://www.keil.com/support/docs/4026.htm

kjbracey · 2019-03-07T14:55:57Z

Is the requirement that the section be an array of the raw RTX control blocks? I imagine it is, which would mean putting an entire Thread object into it isn't going to work.

Other debuggers like pyOCD manage full OS awareness without any special handling, as they can follow the linked lists of the RTX kernel data structures. I can see some robustness advantages to MDK's approach - it would handle OS data structure corruption better - but it does seem limiting.

But if MDK is a hybrid, where it's following the linked lists and only using the section address as a sanity-check for values, then putting the Thread in there would work...

0xc0170 added the needs: review label Nov 28, 2018

0xc0170 requested review from kjbracey and c1728p9 November 28, 2018 08:17

deepikabhavnani force-pushed the uVision_fix branch from c00d9a4 to 3254fe9 Compare November 28, 2018 16:58

deepikabhavnani requested a review from bulislaw December 18, 2018 19:38

c1728p9 reviewed Dec 19, 2018

View reviewed changes

tools/profiles/debug.json Show resolved Hide resolved

c1728p9 approved these changes Jan 3, 2019

View reviewed changes

bulislaw reviewed Jan 3, 2019

View reviewed changes

rtos/TARGET_CORTEX/rtx5/RTX/Source/rtx_lib.c Outdated Show resolved Hide resolved

deepikabhavnani force-pushed the uVision_fix branch 2 times, most recently from 03bdb0d to c75ac42 Compare January 3, 2019 20:08

kjbracey approved these changes Jan 8, 2019

View reviewed changes

0xc0170 approved these changes Jan 8, 2019

View reviewed changes

cmonr added needs: CI and removed needs: review labels Jan 9, 2019

0xc0170 added the ready for merge label Jan 10, 2019

0xc0170 removed the needs: CI label Jan 10, 2019

cmonr added release-version: 5.11.2 release-version: 5.12.0-rc1 and removed release-version: 5.11.2 labels Jan 11, 2019

Deepika added 2 commits January 15, 2019 12:04

deepikabhavnani force-pushed the uVision_fix branch from 3aa5ea6 to 4ca0122 Compare January 15, 2019 18:07

Add SHA for "Move Idle and Timer thread stack to bss section" commit

347acd1

deepikabhavnani force-pushed the uVision_fix branch from 4ca0122 to 347acd1 Compare January 15, 2019 18:09

deepikabhavnani added needs: CI and removed ready for merge labels Jan 15, 2019

0xc0170 added ready for merge and removed needs: CI labels Jan 17, 2019

0xc0170 merged commit 1a8844e into ARMmbed:master Jan 17, 2019

0xc0170 removed the ready for merge label Jan 17, 2019

deepikabhavnani deleted the uVision_fix branch January 17, 2019 17:01

adbridge mentioned this pull request Jan 25, 2019

FPU_USED to be set based on HW FPU support + Squash commits #9437

Merged

0xc0170 mentioned this pull request Sep 25, 2019

Profiles contain RTX symbol (bare metal error) #11565

Closed

Adding debugger awarness with Keil MDK #8887

Adding debugger awarness with Keil MDK #8887

Uh oh!

Conversation

deepikabhavnani commented Nov 27, 2018

Description

Pull request type

Uh oh!

kjbracey commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented Nov 28, 2018

Uh oh!

deepikabhavnani commented Nov 28, 2018

Uh oh!

kjbracey commented Dec 12, 2018

Uh oh!

deepikabhavnani commented Dec 13, 2018

Uh oh!

deepikabhavnani commented Dec 13, 2018

Uh oh!

JonatanAntoni commented Dec 14, 2018

Uh oh!

deepikabhavnani commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c1728p9 commented Dec 19, 2018

Uh oh!

Uh oh!

JonatanAntoni commented Dec 20, 2018

Uh oh!

cmonr commented Dec 27, 2018

Uh oh!

mbed-ci commented Dec 27, 2018

Test run: SUCCESS

Uh oh!

deepikabhavnani commented Jan 3, 2019

Uh oh!

Uh oh!

cmonr commented Jan 9, 2019

Uh oh!

mbed-ci commented Jan 10, 2019

Test run: SUCCESS

Uh oh!

deepikabhavnani commented Jan 15, 2019

Uh oh!

deepikabhavnani commented Jan 15, 2019

Uh oh!

cmonr commented Jan 17, 2019

Uh oh!

cmonr commented Jan 17, 2019

Uh oh!

mbed-ci commented Jan 17, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Jan 17, 2019

Uh oh!

deepikabhavnani commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chentang16 commented Mar 7, 2019

Uh oh!

deepikabhavnani commented Mar 7, 2019

Uh oh!

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

Uh oh!

Uh oh!

chentang16 commented Mar 7, 2019

Uh oh!

kjbracey commented Mar 7, 2019

Uh oh!

Uh oh!

kjbracey commented Nov 28, 2018 •

edited

Loading

deepikabhavnani commented Dec 18, 2018 •

edited

Loading

deepikabhavnani commented Jan 17, 2019 •

edited

Loading

kjbracey commented Mar 7, 2019 •

edited

Loading