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

Adding debugger awarness with Keil MDK #8887

Merged
merged 3 commits into from Jan 17, 2019

Conversation

Projects
None yet
9 participants
@deepikabhavnani
Copy link
Contributor

commented Nov 27, 2018

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-arm

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

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?

@0xc0170 0xc0170 requested review from kjbracey-arm and c1728p9 Nov 28, 2018

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:uVision_fix branch Nov 28, 2018

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

@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-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

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

@JonatanAntoni

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

@c1728p9 @bulislaw - Please review

@deepikabhavnani deepikabhavnani requested a review from bulislaw Dec 18, 2018

@c1728p9

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

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.

@JonatanAntoni

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

[..] 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

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

Testing PR with CI.

@mbed-ci

This comment has been minimized.

Copy link

commented Dec 27, 2018

Test run: SUCCESS

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

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

@c1728p9 @kjbracey-arm @bulislaw - Please review

@c1728p9

c1728p9 approved these changes Jan 3, 2019

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

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:uVision_fix branch 2 times, most recently Jan 3, 2019

@0xc0170

0xc0170 approved these changes Jan 8, 2019

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 10, 2019

Test run: SUCCESS

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

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

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?

deepikabhavnani added some commits Nov 27, 2018

Adding debugger awarness with Keil MDK
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.
CMSIS/RTX: Move Idle and Timer thread stack to bss section.
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 deepikabhavnani force-pushed the deepikabhavnani:uVision_fix branch Jan 15, 2019

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:uVision_fix branch to 347acd1 Jan 15, 2019

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

Client test got stuck, restarted

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

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 ?

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

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

21 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 RTOS ROM(+0 bytes) RAM(+0 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/events Passed, runtime is 9292 cycles (-1227 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+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

@deepikabhavnani deepikabhavnani deleted the deepikabhavnani:uVision_fix branch Jan 17, 2019

@chentang16

This comment has been minimized.

Copy link

commented Mar 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@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-arm

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

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

This comment has been minimized.

Copy link

commented Mar 7, 2019

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

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

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...

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.