You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/07 18:45:38 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #3471: arch: Fix the stack boundary calculation and check
xiaoxiang781216 opened a new pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471
## Summary
All supported arch uses a push-down stack:
The stack grows toward lower addresses in memory. The stack pointer
register points to the lowest, valid working address (the "top" of
the stack). Items on the stack are referenced as positive(include zero)
word offsets from sp.
Which means that for stack in the [begin, begin + size):
1.The initial SP point to begin + size
2.push equals sub and then store
3.pop equals load and then add
## Impact
ps report the correct stack range.
## Testing
ostest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815886024
Yes, I have an upcoming PR to change all g_intstackbase to g_intstacktop, and istackbase will pointer to g_intstackalloc, and always follow the convention of [base, top).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-816609229
Ok. @btashton - would you please have a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815979356
@xiaoxiang781216 I am trying to review it.....
if this `(sp < istackbase && sp >= istackbase - istacksize)` is `(sp < istackTOP && sp >= istackTOP - istacksize)`
Then I think the change is wrong.
based on
>Given FD-DB-IA - A test for a valid Stack would be clear:
>
>'sp > lowerbound && sp <= upperbound`
>
>'lowerbound == sp - is invalid as a push (DB) at this point would be out of bounds. It is valid if in the context of a handler that can NOT call deeper but will ONLY return pop (IA)
>upperbound` == sp - is valid if the stake is unused, as push is DB. It is invalid the context of a handler that will ONLY return pop (IA)
The correct test is: (sp <= istackTOP && sp > istackTOP - istacksize)
I am missing something or do you agree?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815830500
@xiaoxiang781216 - Do you find this super confusing? I do. Maybe it is my perspective on the naming, and you can shed some light on the naming, if not the why not fix the naming in this PR as well?
As you describe, above ARM refers to it as:
Stack-oriented suffix | For store or push instructions | For load or pop instructions
-- | -- | --
FD (Full Descending stack) | DB (Decrement Before) | IA (Increment After)
This sort of construct is very unclear.
`(sp < istackbase && sp >= istackbase - istacksize)`
For me the perspective is that `base` of a lamp is on a table, the `top` of a lamp is above the table, the higher up it is the higher the addresses is.
`low` and `high` addresses are super clear the words `base` and `bound` or 'top' are also grock-able (if used consistently)
The definition of the stack can also be clearer:
| Address increase
v
lowerbound (lower address) : AKA base
ds size
upperbound (higher address) : AKA top
If it were written in terms of `lowerbound` `upperbound`
Given FD-DB-IA - A test for a valid Stack would be clear:
'sp > lowerbound && sp <= upperbound`
'lowerbound == sp - is invalid as a push (DB) at this point would be out of bounds. It is valid if in the context of a handler that can NOT call deeper but will ONLY return pop (IA)
upperbound` == sp - is valid if the stake is unused, as push is DB. It is invalid the context of a handler that will ONLY return pop (IA)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815971113
Yes, please help review and merge my change if all change is good. I will send a new patch tomorrow to address the problem you just mentioned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817155372
This looks correct to me @davids5 if you want to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815886024
Yes, I have an upcoming PR to change all base to top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815886024
Yes, I have an upcoming PR to change all g_intstackbase to g_intstacktop, and istackbase will pointer to g_intstackalloc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#discussion_r611007050
##########
File path: arch/arm/src/tiva/common/lmxx_tm4c_start.c
##########
@@ -53,9 +53,7 @@
* ARM EABI requires 64 bit stack alignment.
*/
-#define IDLE_STACKSIZE (CONFIG_IDLETHREAD_STACKSIZE & ~7)
Review comment:
@xiaoxiang781216 this is what I am talking about. Won't the heap base be wrong if the stack size config variable is not on the right boundary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817088444
Doesn't this change now not assert that the stack size is aligned on the correct boundaries? Also if we are removing that from _start.c we should remove the comments about alignment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815962948
@xiaoxiang781216 - So we should merge this as is, then make that pass?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817333362
Could someone verify if the IDLE thread stack use on sim:nsh is resonable on master? I still get it close to 100% even if I increase it. I don't think it is related to this though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817088444
Doesn't this change now not force that the stack size is aligned on the correct boundaries? Also if we are removing that from _start.c we should remove the comments about alignment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817332569
nevermind it seems unrelated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 merged pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817155445
@btashton - Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817331953
this seems to have broken something. I'm working on sim, and I'm getting assertion on free(). When looking at stack usage of IDLE thread it reports maximum size (I increased it up to 16384 and still seems to be full). If I go back to a commit prior to this change, I don't get this problem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-816379767
> @xiaoxiang781216 I am trying to review it.....
>
> if this `(sp < istackbase && sp >= istackbase - istacksize)` is `(sp < istackTOP && sp >= istackTOP - istacksize)`
>
Yes, itstackbase equal istackTOP, the valid stack range is [istackTOP - istacksize, istackTOP)
> Then I think the change is wrong.
> based on
>
> > Given FD-DB-IA - A test for a valid Stack would be clear:
> > 'sp > lowerbound && sp <= upperbound`
No, the valid range is `sp >= lowerbound && sp < upperbound` since local variables is accessed by sp[-offset] for FD stack where offset is always a negative number.
Note: I assume the stack range is [lowerbound, upperbound)
> > 'lowerbound == sp - is invalid as a push (DB) at this point would be out of bounds. It is valid if in the context of a handler that can NOT call deeper but will ONLY return pop (IA)
Yes, it is invalid to allocate(sub sp) more local variables or make more function call, but it's valid in the current funciton context. So it's correct at this point: no stack overflow.
> > upperbound` == sp - is valid if the stake is unused, as push is DB. It is invalid the context of a handler that will ONLY return pop (IA)
But, the condition(the stack is unused) is impossible once the thread is running. On the other hand, it isn't only invalid at the return, but also invalid to access all local variables which's address equal or higher than upperbound.
>
> The correct test is: (sp <= istackTOP && sp > istackTOP - istacksize)
>
> I am missing something or do you agree?
I still think the correct test is: (sp < istackTOP && sp >= istackTOP - istacksize).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817348133
Thanks for the explanation!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-815971113
Yes, please help review merge my change. If all change is good. I will send a new patch tomorrow to address the problem you just mentioned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817345061
sim IDLE thread stack is very specific because:
1. The stack isn't created by NuttX, but reused from host main thread stack, which mean it will be dynamically extended by host OS as needed and then is never overflow.
2. All pseudo device drivers(e.g. tty, tap, x11) talk to the host devices inside the idle loop, which mean the stack consumption is much higher than the epxectation.
3. sim still initialze IDLE thread stack info from CONFIG_IDLETHREAD_STACKSIZE:
https://github.com/apache/incubator-nuttx/blob/master/arch/sim/src/sim/up_initialstate.c#L54-L60
So, from item 2 and 3, you will see the stack usage near or equals 100% frequently, but the program still can run correctly due to item 1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817345061
sim IDLE thread stack is very specific because:
1. The stack isn't created by NuttX, but reused from host main thread stack, which mean it will be dynamically extended by host OS as needed and then is never overflow.
2. All pseudo device drivers(e.g. tty, tap, x11) talk to the host devices inside the idle loop, which mean the stack consumption is much higher than the epxectation.
3. sim still initialze IDLE thread stack info from CONFIG_IDLETHREAD_STACKSIZE:
https://github.com/apache/incubator-nuttx/blob/master/arch/sim/src/sim/up_initialstate.c#L54-L60
So, from item 2 and 3, you will see the stack usage near or equals 100% frequently, but the program still can run correctly due to item 1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#issuecomment-817089416
The alignment requirement is the same as before, the real change is fixing the wrong boundary calculation(e.g. shouldn't substract 4 from the stack size).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3471: arch: Fix the stack boundary calculation and check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3471:
URL: https://github.com/apache/incubator-nuttx/pull/3471#discussion_r611010655
##########
File path: arch/arm/src/tiva/common/lmxx_tm4c_start.c
##########
@@ -53,9 +53,7 @@
* ARM EABI requires 64 bit stack alignment.
*/
-#define IDLE_STACKSIZE (CONFIG_IDLETHREAD_STACKSIZE & ~7)
Review comment:
Oh, you are talking about the idle thread stack. The heap manager will align the address for us:
https://github.com/apache/incubator-nuttx/blob/master/mm/mm_heap/mm_initialize.c#L101-L102
So, we need pass the real unaligned heap base(stack end), and the heap manager will move the address to the next aligned boundary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org