xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r613180679
##########
File path: arch/arm/src/common/arm_usestack.c
##########
@@ -127,37 +117,17 @@ int up_use_stack(struct tcb_s *tcb, void *stack, size_t
stack_size)
/* Save the new stack allocation */
tcb->stack_alloc_ptr = stack;
-
- /* Align stack top */
-
- tcb->adj_stack_ptr =
- (FAR void *)STACK_ALIGN_DOWN((uintptr_t)stack + stack_size);
-
- /* Offset by tls_size */
-
- stack = (FAR void *)((uintptr_t)stack + tls_size);
-
- /* Is there enough room for at least TLS ? */
-
- if ((uintptr_t)stack > (uintptr_t)tcb->adj_stack_ptr)
- {
- return -ENOMEM;
- }
-
- tcb->adj_stack_size = (uintptr_t)tcb->adj_stack_ptr - (uintptr_t)stack;
-
- /* Initialize the TLS data structure */
-
- memset(tcb->stack_alloc_ptr, 0, tls_size);
+ tcb->adj_stack_ptr = tcb->stack_alloc_ptr;
+ tcb->adj_stack_size =
+ STACK_ALIGN_DOWN((uintptr_t)stack + stack_size) - (uintptr_t)stack;
Review comment:
> @patacongo (wrong sig above) Here is the patch:
[d485c36](https://github.com/apache/incubator-nuttx/commit/d485c3687fc9e25380706f4af09de5157c9c0a79)
>
> @xiaoxiang781216 - Thank you! That feels a lot safer. I think @patacongo
gave some important usage guidelines. Which raises the question: Do we have
documentation, all in one place, that defines stack usage, sizing and
guidelines for selecting the TLS config?
@patacongo create a very nice wiki page:
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=152115079
> If not I feel this could really hurt adoption. Nothing worse than a bad
first impression. If a new user see a 16K stack, or has blind stack related
crashes.
>
As @patacongo mentioned before, there isn't benefit to enable the aligned
stack on the FLAT build, and it's disabled by default even on the PROTECT and
KERNEL build, so it's the user's responsibility to read wiki/code and fully
understand the side effect before he turn on this feature(or any feature).
Nobody can help him, if he turn the feature blindly.
> Since the the symptom of a shallow stack crash will now corrupt the TLS
should we add guards and checks in the future usage code?
The stack overflow is a critical error regardless it overflow TLS region or
something else. The same tool(ps, up_check_stack) work as before, we don't need
invent the new one.
--
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:
us...@infra.apache.org
