arch: arm: move stack switching back to ASM code #16099

wentongwu · 2019-05-13T10:28:59Z

With -O0 optimizion, gcc compiler doesn't inline "static inline"
marked function. So when function call return from function
set_and_switch_to_psp which is to switch sp from MSP to PSP, the
ending "mov sp, r7" instruction will overwrite the just updated
sp value(PSP) with the beginning stack pointer(should be MSP)
stored in r7 register, so the switch doesn't happen. And it causes
unpredictable problems in the initialization process, the backward
analysis for this problem can be found on Github issue #15794.

Also increase stack buffer when code coverage enabled.

Fixes: #15794.

ioannisg

Although this seems to work, I wonder if we should just revert to using inline assembly for setting and switching to PSP.

psidhu · 2019-05-13T16:04:33Z

Although this seems to work, I wonder if we should just revert to using inline assembly for setting and switching to PSP.

@ioannisg Why did we switch away from asm here anyways? I'm assuming it was something to do with maintainability, but I'm not totally sure.

ioannisg · 2019-05-13T16:11:18Z

Although this seems to work, I wonder if we should just revert to using inline assembly for setting and switching to PSP.

@ioannisg Why did we switch away from asm here anyways? I'm assuming it was something to do with maintainability, but I'm not totally sure.

Yeah, it was an effort to make the code more readable, but I guess we didn't review all the cases, unfortunately. and ended up with this kind of issues. You can look at the history, if you are interested in the original discussion around the refactoring; can't recall the author on top of my head.

I favor reverting the switch-and-set-psp() to assembly, but other reviewers are welcome to comment on the current way of fixing the bug.

psidhu · 2019-05-13T16:16:41Z

Yeah, it was an effort to make the code more readable, but I guess we didn't review all the cases, unfortunately. and ended up with this kind of issues. I favor reverting the switch-and-set-psp() to assembly, but other reviewers are welcome to comment on the current way of fixing the bug.

I agree with you to revert to this back to asm instead of fixing it this way. This is the second time I've seen a project attempt to do this, then decide to backoff so as not to rely on the compiler.

I think there's much merit in sticking with asm, but add in a block comment the roughly equivalent C code if anyone can't understand the asm, but would like to still understand what's happening.

andrewboie · 2019-05-13T20:18:30Z

I think we should go back to assembly. Switching the stack pointer within C code is incredibly perilous, one bad assumption by the compiler about what is on the stack causes everything to come crashing down. You could consider x86_64's approach as a model however, look for _switch_top / _switch_bottom

ioannisg · 2019-05-14T07:13:30Z

I think we should go back to assembly. Switching the stack pointer within C code is incredibly perilous, one bad assumption by the compiler about what is on the stack causes everything to come crashing down. You could consider x86_64's approach as a model however, look for _switch_top / _switch_bottom

Will you be able to provide the patch @wentongwu ?

wentongwu · 2019-05-14T07:18:26Z

I think we should go back to assembly. Switching the stack pointer within C code is incredibly perilous, one bad assumption by the compiler about what is on the stack causes everything to come crashing down. You could consider x86_64's approach as a model however, look for _switch_top / _switch_bottom

Will you be able to provide the patch @wentongwu ?

Yes, I'm preparing the patch. Thanks.

ioannisg · 2019-05-14T08:19:41Z

I think we should go back to assembly. Switching the stack pointer within C code is incredibly perilous, one bad assumption by the compiler about what is on the stack causes everything to come crashing down. You could consider x86_64's approach as a model however, look for _switch_top / _switch_bottom

Will you be able to provide the patch @wentongwu ?

Yes, I'm preparing the patch. Thanks.

Thank you!

andrewboie · 2019-05-14T20:41:28Z

@ioannisg please have another look

ioannisg

Did a pass - left some review comments, and a question for @andrewboie

arch/arm/core/cortex_m/reset.S

ioannisg · 2019-05-15T09:01:23Z

arch/arm/core/cortex_m/reset.S

 #ifdef CONFIG_WDOG_INIT
    /* board-specific watchdog initialization is necessary */
    bl _WdogInit
 #endif

+#ifdef CONFIG_INIT_STACKS


This is OK with me, I think we can just keep it as is. Although, still the C code should work, cause I don't see any SP manipulation.

arch/arm/core/cortex_m/reset.S

ioannisg · 2019-05-16T13:16:03Z

arch/arm/core/cortex_m/reset.S

+     * CONTROL_SPSEL_Msk
+     */
+    mrs r0, CONTROL
+    movs r1, #2


Pls, add the /* CONTROL_SPSEL_Msk */ comment in this line: orrs r0, r1
Actually, probably, you can do "orrs" directly with an immediate.

yes, it seems ARMV7-m can orrs immediate, but ARMV6-M can not (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0662a/CIHBDBJH.html )

ioannisg

just a minor suggestion for the inline comment

With -O0 optimizion, gcc compiler doesn't inline "static inline" marked function. So when function call return from function set_and_switch_to_psp which is to switch sp from MSP to PSP, the ending "mov sp, r7" instruction will overwrite the just updated sp value(PSP) with the beginning stack pointer(should be MSP) stored in r7 register, so the switch doesn't happen. And it causes unpredictable problems in the initialization process, the backward analysis for this problem can be found on Github issue zephyrproject-rtos#15794. Fixes: zephyrproject-rtos#15794. Signed-off-by: Wentong Wu <wentong.wu@intel.com>

increase stack buffer when code coverage enabled. Fixes: zephyrproject-rtos#15794. Signed-off-by: Wentong Wu <wentong.wu@intel.com>

manochavikas · 2019-05-23T01:38:09Z

With -O0 optimizion, gcc compiler doesn't inline "static inline" marked function

just saw this revert, an easy fix seems to be using always inline (__always_inline).

Cheers,
Vikas

manochavikas · 2019-05-24T15:56:41Z

@wentongwu
Can you check with always_inline, it should fix the issue with -O0.

Cheers,
Vikas

wentongwu requested review from andrewboie, andyross, galak, MaureenHelm and nashif as code owners May 13, 2019 10:28

wentongwu requested review from ioannisg and a user May 13, 2019 10:30

ioannisg reviewed May 13, 2019

View reviewed changes

ioannisg added the priority: high High impact/importance bug label May 13, 2019

ioannisg mentioned this pull request May 13, 2019

ARM: Core Stack Improvements/Bug fixes for 2.0 release #14744

Closed

26 tasks

galak added the backport v1.14-branch label May 13, 2019

ioannisg added the area: ARM ARM (32-bit) Architecture label May 14, 2019

wentongwu force-pushed the coverage branch from 9831c6c to 1a00295 Compare May 14, 2019 15:16

andrewboie approved these changes May 14, 2019

View reviewed changes

andrewboie changed the title ~~arch: arm: force static inline on changing stack function~~ arch: arm: move stack switching back to ASM code May 14, 2019

ioannisg requested changes May 15, 2019

View reviewed changes

wentongwu force-pushed the coverage branch 2 times, most recently from 9a369ec to 772ff5b Compare May 16, 2019 11:16

wentongwu requested a review from ioannisg May 16, 2019 13:02

ioannisg reviewed May 16, 2019

View reviewed changes

wentongwu force-pushed the coverage branch from 772ff5b to 481a118 Compare May 16, 2019 15:40

tests: kernel: increase stack buffer when code coverage enabled

481a118

increase stack buffer when code coverage enabled. Fixes: zephyrproject-rtos#15794. Signed-off-by: Wentong Wu <wentong.wu@intel.com>

wentongwu requested a review from ioannisg May 16, 2019 16:18

ioannisg approved these changes May 16, 2019

View reviewed changes

andrewboie merged commit fc3270d into zephyrproject-rtos:master May 16, 2019

backporting bot mentioned this pull request May 16, 2019

[Backport v1.14-branch] arch: arm: move stack switching back to ASM code #16214

Merged

wentongwu mentioned this pull request May 23, 2019

ssanitycheck --coverage -p mps2_an385: Some remaing test cases that are still failing #15446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arch: arm: move stack switching back to ASM code #16099

arch: arm: move stack switching back to ASM code #16099

wentongwu commented May 13, 2019 •

edited by andrewboie

Loading

ioannisg left a comment

psidhu commented May 13, 2019

ioannisg commented May 13, 2019 •

edited

Loading

psidhu commented May 13, 2019

andrewboie commented May 13, 2019

ioannisg commented May 14, 2019

wentongwu commented May 14, 2019

ioannisg commented May 14, 2019

andrewboie commented May 14, 2019

ioannisg left a comment

ioannisg May 15, 2019

ioannisg May 16, 2019 •

edited

Loading

wentongwu May 16, 2019

ioannisg left a comment

manochavikas commented May 23, 2019

manochavikas commented May 24, 2019

arch: arm: move stack switching back to ASM code #16099

arch: arm: move stack switching back to ASM code #16099

Conversation

wentongwu commented May 13, 2019 • edited by andrewboie Loading

ioannisg left a comment

Choose a reason for hiding this comment

psidhu commented May 13, 2019

ioannisg commented May 13, 2019 • edited Loading

psidhu commented May 13, 2019

andrewboie commented May 13, 2019

ioannisg commented May 14, 2019

wentongwu commented May 14, 2019

ioannisg commented May 14, 2019

andrewboie commented May 14, 2019

ioannisg left a comment

Choose a reason for hiding this comment

ioannisg May 15, 2019

Choose a reason for hiding this comment

ioannisg May 16, 2019 • edited Loading

Choose a reason for hiding this comment

wentongwu May 16, 2019

Choose a reason for hiding this comment

ioannisg left a comment

Choose a reason for hiding this comment

manochavikas commented May 23, 2019

manochavikas commented May 24, 2019

wentongwu commented May 13, 2019 •

edited by andrewboie

Loading

ioannisg commented May 13, 2019 •

edited

Loading

ioannisg May 16, 2019 •

edited

Loading