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/11 10:46:12 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
xiaoxiang781216 opened a new pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517
## Summary
and allocate tls_info_s through up_stack_frame too
BTW, all g_intstackbase is renamed to g_intstacktop
## Impact
1. Move TLS specific code from arch to the common place(sched/)
2. Unify the per-thread storage and per-task storage handling
## Testing
Pass CI, ostest and smp
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817957982
> > Please reread my reply in the previous thread.
>
> So this is just a redesign of working code with no value added. It is just for your personal preference. That is not in the spirit of a good open source project. It is bad to humiliate people by throwing away there perfectly good code just so you can replace with your own NIH code. Very bad style.
>
No, it isn't just preference, here is the reason I made this change:
1. Unify the memory allocation from stack with up_stack_frame regardless it's TLS data or argv.
2. Avoid skip TLS space explicitly in many place since TLS is allocated(remove from picture) like other stack variable
3. Don't need save tg_libvars pointer in task_group_s and tls_info_s
The most important thing is that ``Modular Architecture```get improved because almost TLS special handling is decoupled and removed from arch, the benefit include:
1. Avoid the code duplication in each arch(reduce 600+ line 31.7%)
2. Fix #1631 in the common place(sched/) and avoid the similar problem happen again in new arch
> Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
--
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 edited a comment on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
v01d edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818284700
> > Sorry, I can't really review this change as I'm not really familiar with this part of the codebase.
>
> There are five commits. Most deal with other issues that you could probably have input on. Otherwise, @davids5 will need to do the merge (ONLY after we are sure that 10.1 is protected from the changes).
I looked at the changes of each commit. I don't really understand much of this so I can't provide a meaningful review (I get the gist of it mostly). Anyway, I think even for an expert it would still be hard to catch a bug here. I think a test that can be run for each arch would be ideal. Maybe there's something that could be setup specially for this change?
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817957982
> > Please reread my reply in the previous thread.
>
> So this is just a redesign of working code with no value added. It is just for your personal preference. That is not in the spirit of a good open source project. It is bad to humiliate people by throwing away there perfectly good code just so you can replace with your own NIH code. Very bad style.
>
No, it isn't just preference, here is the reason I made this change:
1. Unify the memory allocation from stack with up_stack_frame regardless it's TLS data or argv.
2. Avoid skip TLS space explicitly in many place since TLS is allocated(remove from picture) like other stack variable
3. Don't need save tg_libvars pointer in task_group_s and tls_info_s
The most important thing is that ```Modular Architecture```get improved because almost TLS special handling is decoupled and removed from arch, the benefit include:
1. Avoid the code duplication in each arch(reduce 600+ line 31.7%)
2. Fix #1631 in the common place(sched/) and avoid the similar problem happen again in new arch
> Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817860767
> > Please reread my reply in the previous thread.
>
> Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
As I reply before, there are five changes in this PR:
0. Rename g_intstackbase to g_intstacktop
1. Allocate the space from the lowest stack(up_stack_frame)
2. The stack dump/check need adjust because adj_stack_ptr point to the low end of stack
3. TLS data need to explicitly be allocated by up_stack_frame to ensure it's address always equals stack_alloc_ptr
4. vfork also need adjust since the implementation of vfork couple with how the stack memory allocate
Item 0 is already separated. Item 1 to 4 is coupled with each other, it's hard to separate them without breaking the runtime. For example, After apply item 1:
1. If we don't change item 2, the stack check and dump report the wrong info.
2. If we don't change item 3, TLS data will overwrite by argv in the main thread.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612949820
##########
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:
Ok, I will bring back tls_info_s/task_info_s, but put it in pthread_create/task_init.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. You logic is wrong. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the above would point at the end of TLS info in that case and needs an offset. This kind of access (without system calls) is required for user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611195837
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
This is not good. Other than initial setup, TLS is 100% a user-space function. It must be implemented entirely in libc (the system call to get the stack base is a KLUDGE to avoid aligning stacks). But moving the TLS pointer into the TCB is just wrong. I cannot merge this kind of change.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611645689
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
Before your questionaable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817854300
> Please reread my reply in the previous thread.
Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817960154
> adj_stack_ptr `Adjusted stack base pointer after the TLS Data and Arguments has been removed from the stack allocation.`
@davids5 the comment change to the above line, please check.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817309644
> You might also look how TLS is implemented in Zephyr.
I will took a look.
> Much nicer than our implementation: https://docs.zephyrproject.org/latest/guides/thread_local_storage.html
>
Zephyr implementation need toolchain support(```__thread``` keyword introduced by gcc 3.3.1 2003-08-04):
https://github.com/zephyrproject-rtos/zephyr/blame/3c79b565fe7e6afed3687d95dd3e70bb93699a88/doc/guides/thread_local_storage.rst#L10
So, I think it's better to support both approach(explicit and implicit TLS) or we give up the old toolchain support.
> This is good too: https://chao-tic.github.io/blog/2018/12/25/tls#tls-access-in-executables-and-shared-objects
>
> I think if we want to re-architect TLS, we should do a better job.
This patch try to move the most tls action from arch to sched/libc, which will simplify the implementation of the implicit TLS in the future.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611198625
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> This is not good. Other than initial setup, TLS is 100% a user-space function. It must be implemented entirely in libc (the system call to get the stack base is a KLUDGE to avoid aligning stacks). But moving the TLS pointer into the TCB is just wrong. I cannot merge this kind of change.
I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72
Two alternative select:
1. Kee the push-down stack assumption as before, and remove tls_info_ptr
2. Guard tls_info_ptr by CONFIG_TLS_ALIGNED since tls_info_ptr isn't really used in this case.
Which one do you prefer?
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817312023
> The names do not carry enough context and are still too confusing.
>
> The Deinitions should be clearer - This one is not so bad, the the description can use some work
> `stack_alloc_ptr`: Pointer to the memory allocated for the stack and optional TLS this is the base address returned by an alloc operation.
>
> This one needs work:
>
> `adj_stack_ptr`: Adjusted `stack_alloc_ptr` for HW. - add information as to why it is adjusted in the comment.
>
Ok, I will add more comment.
> Then consider changing the name - See it is not a stack pointer and yet is is called adj_stack_ptr -> adj_stackbase_ptr
adj_stack_ptr is same as stack_alloc_ptr at the beginning. up_stack_frame will increase adj_stack_ptr by frame_size to allocate the space for caller(e.g. tls, argv...).
--
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] yamt commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r647210124
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -69,6 +69,16 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
qtcb = rtcb;
}
+ else if (pid == -1)
+ {
+ /* We can always query our main thread */
Review comment:
it seems that it isn't always the case.
https://github.com/apache/incubator-nuttx/issues/3868
@xiaoxiang781216 can you take 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] xiaoxiang781216 commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612100073
##########
File path: include/nuttx/arch.h
##########
@@ -308,9 +308,26 @@ int up_use_stack(FAR struct tcb_s *tcb, FAR void *stack, size_t stack_size);
*
* - adj_stack_size: Stack size after removal of the stack frame from
* the stack
- * - adj_stack_ptr: Adjusted initial stack pointer after the frame has
- * been removed from the stack. This will still be the initial value
- * of the stack pointer when the task is started.
+ * - adj_stack_ptr: Adjusted stack base pointer after the TLS Data and
+ * Arguments has been removed from the stack allocation.
+ *
+ * Here is the diagram after some allocation(tls, arg):
+ *
+ * +-------------+ <-stack_alloc_ptr(lowest)
+ * | TLS Data |
+ * +-------------+
+ * | Arguments |
+ * adj_stack_ptr-> +-------------+\
+ * | Available | +
+ * | Stack | |
+ * | | | |
+ * | | | |
+ * v | | +->adj_stack_size
+ * | | |
+ * | | |
+ * +-------------+ |
+ * | Thread Data | +
+ * +-------------+/
Review comment:
Ok, add to here:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-b0e4e9a1378b2a5ded4af14faa20cd4fea7cc12ae8d832df45817891718734eaR126-R142
--
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] masayuki2009 commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-820879975
>I'm still investigating why rtcb->xcp.sigdeliver is set to NULL when arm_sigdeliver() is called.
@xiaoxiang781216
I can not still find the root cause of the issue because if I add some debug code, the issue does not happen.
Also, I confirmed that the ostest works with other configurations. (e.g. lc823450-xgevk:nsh and lc823450-xgevk:audio which also work in SMP)
So I'd like to merge this PR to the master and I want to send a new PR to add ```CONFIGU_DEBUG_ASSERTIONS=y``` to lc82450-xgevk:rndis for workaround after I merge the PR.
What do you think?
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
Push Down Push Up
+-------------+ +-------------+ <- Allocation address (lowest)
| TLS Data | | TLS Data |
+-------------+ +-------------+
| | | Task Data* |
| Available | +-------------+
| Stack | | Arguments |
| | +-------------+
| | | |
| | | Available | |
| | | Stack | v
| | ^ | |
+-------------+ | | |
| Arguments | | |
+-------------+ | |
| Task Data* | | |
+-------------+ +-------------+
* Main thread only
NOTE: The comment at the referenced location is wrong. That code has nothing to do with push-up or push-down. The code is correct. PR #3519 fixes that bad comment.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817860767
> > Please reread my reply in the previous thread.
>
> Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
As I reply before, there are five changes in this PR:
0. Rename g_intstackbase to g_intstacktop
1. Allocate the space from the lowest stack(up_stack_frame)
2. The stack dump/check need adjust because adj_stack_ptr point to the low end of stack
3. TLS data need to explicitly be allocated by up_stack_frame to ensure it's address always equals stack_alloc_ptr
4. vfork also need adjust since the implementation of vfork couple with how the stack memory allocate
Item 0 is already separated. Item 1 to 4 is coupled with each other, it's hard to separate them without breaking the runtime. For example, After apply item 1, if we don't change item 3, TLS data will overwrite by argv in the main thread.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817306168
You might also look how TLS is implemented in Zephyr. Much nicer than our implementation: https://docs.zephyrproject.org/latest/guides/thread_local_storage.html
This is good too: https://chao-tic.github.io/blog/2018/12/25/tls#tls-access-in-executables-and-shared-objects
I think if we want to re-architect TLS, we should do a better job.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817310778
> @xiaoxiang781216 Given the size of this PR, please make separate commits for review comments until the review is done so we can look a the deltas, then you can squash it.
Sure.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611216800
##########
File path: include/nuttx/sched.h
##########
@@ -402,7 +401,7 @@ struct stackinfo_s
FAR void *stack_alloc_ptr; /* Pointer to allocated stack */
/* Needed to deallocate stack */
FAR void *adj_stack_ptr; /* Adjusted stack_alloc_ptr for HW */
- /* The initial stack pointer value */
+ FAR void *tls_info_ptr; /* Thread local storage information */
Review comment:
This change is wrong. Please remove tls_info_ptr which must always have the same valueas stack_alloc_ptr.
--
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] patacongo edited a comment on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817345621
> The comment mislead my understanding. Anyway, I will remove tls_info_ptr.
It is easy to get confused. I was confused when I wrote the comments.
it would be helpful to separate all of the pointless renaming from the core changes. It is very difficult to review and make sense out of. There is no real technical description of the change which makes it harder to review.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819447362
@xiaoxiang781216 - Is it time to squash or do the docs need to updated too? Once squashed, can we all test this on as many arches' real HW before merging?
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612556240
##########
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:
Both the old and new code require that:
1. up_[create|use]_stack align the stack size and base to the boundary
2. up_stack_frame align frame size to the boundary
If you inspect these variables in debugger, all(stack_alloc_ptr, adj_stack_ptr and adj_stack_size) should be aligned correctly. So there isn't real difference from both the requirement and implementation.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612930562
##########
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:
> > Before this change it looked (CONFIG_TLS_ALIGNED not lit) like 300+sizeof(tls) which I got what I asked for and the OS got what it needed. Is that still the case?
>
> I believe you will get 300-sizeof(tls). That could be dangerous for finely tuned, minimal stack sizes.
That could be fixed by adding sizeof(struct task_info_s) or sizeof(struct tls_info_s) in the up_create_stack alls.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your logic is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] gustavonihei commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611905418
##########
File path: arch/arm/src/common/arm_vfork.c
##########
@@ -167,26 +146,27 @@ pid_t up_vfork(const struct vfork_s *context)
* effort is overkill.
*/
- newsp = (uint32_t)child->cmn.adj_stack_ptr - stackutil;
+ newtop = (uint32_t)child->cmn.adj_stack_ptr +
+ child->cmn.adj_stack_size;
+ newsp = newtop - stackutil;
memcpy((void *)newsp, (const void *)context->sp, stackutil);
/* Was there a frame pointer in place before? */
- if (context->fp <= (uint32_t)parent->adj_stack_ptr &&
- context->fp >= (uint32_t)parent->adj_stack_ptr - stacksize)
+ if (context->fp >= context->sp && context->fp < stacktop)
{
- uint32_t frameutil = (uint32_t)parent->adj_stack_ptr - context->fp;
- newfp = (uint32_t)child->cmn.adj_stack_ptr - frameutil;
+ uint32_t frameutil = stacktop - context->fp;
+ newfp = newtop - frameutil;
}
else
{
newfp = context->fp;
}
- sinfo("Parent: stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp, context->fp);
- sinfo("Child: stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp, newfp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ stacktop, context->sp, context->fp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ newtop, newsp, newfp);
Review comment:
```suggestion
sinfo("Old stack top:%08" PRIx32 " SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
stacktop, context->sp, context->fp);
sinfo("New stack top:%08" PRIx32 " SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
newtop, newsp, newfp);
```
Missing whitespace between printed values.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611186107
##########
File path: sched/task/task_init.c
##########
@@ -130,21 +127,15 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority,
goto errout_with_group;
}
-#ifndef CONFIG_BUILD_KERNEL
- /* Allocate a stack frame to hold task-specific data */
-
- group = tcb->cmn.group;
- group->tg_libvars = up_stack_frame(&tcb->cmn, sizeof(struct libvars_s));
- DEBUGASSERT(group->tg_libvars != NULL);
-
- /* Initialize the task-specific data */
-
- memset(group->tg_libvars, 0, sizeof(struct libvars_s));
-
- /* Save the allocated task data in TLS */
+ /* Initialize thread local storage */
- tls_set_taskdata(&tcb->cmn);
-#endif
+ tcb->cmn.tls_info_ptr = up_stack_frame(&tcb->cmn,
+ sizeof(struct task_info_s));
+ if (tcb->cmn.tls_info_ptr == NULL)
+ {
+ ret = -ENOMEM;
+ goto errout_with_group;
+ }
Review comment:
This is difficult to understand. But it looks like you are moving TLS data from its correct position at the far end of the stack. That is very wrong. In ALL systems, including NuttX in KERNEL MODE, the TLS data must lie at the lowest address of the stack.
Generally, TLS is obtained very efficiently like:
tls = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK);
No system call is involved. This standard behavior must be preserved.
In FLAT mode, we can also use aligned stacks, but it is not a good use of memory. In the FLAT mode, the overhead of a system call is preferable.
But I would reject this change as I understand it because it destroys the possibility of high performance, standard TLS.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817309644
> You might also look how TLS is implemented in Zephyr.
I will took a look.
> Much nicer than our implementation: https://docs.zephyrproject.org/latest/guides/thread_local_storage.html
>
Zephyr implementation need toolchain support(```__thread``` keyword introduced by gcc 3.3.1 2003-08-04):
https://github.com/zephyrproject-rtos/zephyr/blame/3c79b565fe7e6afed3687d95dd3e70bb93699a88/doc/guides/thread_local_storage.rst#L10
So, I think it's better to support both approach(explicit and implicit TLS).
> This is good too: https://chao-tic.github.io/blog/2018/12/25/tls#tls-access-in-executables-and-shared-objects
>
> I think if we want to re-architect TLS, we should do a better job.
This patch try to move the most tls action from arch to sched/libc, which is a step to implement the implicit TLS.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612613171
##########
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:
if both adj_stack_ptr and adj_stack_size align on some value(e.g. 8), it's obvious that adj_stack_ptr + adj_stack_size still align on the same value(e.g. 8). Don't you think so?
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611188584
##########
File path: sched/task/task_init.c
##########
@@ -130,21 +127,15 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority,
goto errout_with_group;
}
-#ifndef CONFIG_BUILD_KERNEL
- /* Allocate a stack frame to hold task-specific data */
-
- group = tcb->cmn.group;
- group->tg_libvars = up_stack_frame(&tcb->cmn, sizeof(struct libvars_s));
- DEBUGASSERT(group->tg_libvars != NULL);
-
- /* Initialize the task-specific data */
-
- memset(group->tg_libvars, 0, sizeof(struct libvars_s));
-
- /* Save the allocated task data in TLS */
+ /* Initialize thread local storage */
- tls_set_taskdata(&tcb->cmn);
-#endif
+ tcb->cmn.tls_info_ptr = up_stack_frame(&tcb->cmn,
+ sizeof(struct task_info_s));
+ if (tcb->cmn.tls_info_ptr == NULL)
+ {
+ ret = -ENOMEM;
+ goto errout_with_group;
+ }
Review comment:
> This is difficult to understand. But it looks like you are moving TLS data from its correct position at the far end of the stack. That is very wrong. In ALL systems, including NuttX in KERNEL MODE, the TLS data must lie at the lowest address of the stack.
>
No, TLS is still at the lowest address of the stack, because I modify up_frame_stack to allocate the memory from the beginning of stack:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-5c9cf944a960b9718953b2cb399f5ba99fafdc22445ddd6fb9ab27ac1b808ef6R96-R122
> Generally, TLS is obtained very efficiently like:
>
> ```
> tls = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK);
> ```
>
> No system call is involved. This standard behavior must be preserved.
>
Yes, the trick still work as before if CONFIG_TLS_ALIGNED turn on.
> In FLAT mode, we can also use aligned stacks, but it is not a good use of memory. In the FLAT mode, the overhead of a system call is preferable.
>
> But I would reject this change as I understand it because it destroys the possibility of high perfromance, standard TLS.
I think my reply could resolve your concern now.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611217188
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
Ok, if TLS is always put at the lowest address regardless "push down stack" or "push up stack". The code is right, we just need fix the comment.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. You logic is wrong. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK).
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611198625
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> This is not good. Other than initial setup, TLS is 100% a user-space function. It must be implemented entirely in libc (the system call to get the stack base is a KLUDGE to avoid aligning stacks). But moving the TLS pointer into the TCB is just wrong. I cannot merge this kind of change.
I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72
Two alternative I can come up:
1. Kee the push-down stack assumption as before, and remove tls_info_ptr
2. Guard tls_info_ptr by CONFIG_TLS_ALIGNED since tls_info_ptr isn't really used in this case.
Which one do you prefer?
--
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] gustavonihei commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611906396
##########
File path: arch/mips/src/mips32/mips_vfork.c
##########
@@ -191,32 +168,33 @@ pid_t up_vfork(const struct vfork_s *context)
* effort is overkill.
*/
- newsp = (uint32_t)child->cmn.adj_stack_ptr - stackutil;
+ newtop = (uintptr_t)child->cmn.adj_stack_ptr +
+ child->cmn.adj_stack_size;
+ newsp = newtop - stackutil;
memcpy((void *)newsp, (const void *)context->sp, stackutil);
/* Was there a frame pointer in place before? */
#ifdef CONFIG_MIPS32_FRAMEPOINTER
- if (context->fp <= (uint32_t)parent->adj_stack_ptr &&
- context->fp >= (uint32_t)parent->adj_stack_ptr - stacksize)
+ if (context->fp >= context->sp && context->fp < stacktop)
{
- uint32_t frameutil = (uint32_t)parent->adj_stack_ptr - context->fp;
- newfp = (uint32_t)child->cmn.adj_stack_ptr - frameutil;
+ uint32_t frameutil = stacktop - context->fp;
+ newfp = newtop - frameutil;
}
else
{
newfp = context->fp;
}
- sinfo("Old stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp, context->fp);
- sinfo("New stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp, newfp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ stacktop, context->sp, context->fp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ newtop, newsp, newfp);
#else
- sinfo("Old stack base:%p SP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp);
- sinfo("New stack base:%p SP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 "\n",
+ stacktop, context->sp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 "\n",
+ newtop, newsp);
Review comment:
```suggestion
sinfo("Old stack top:%08" PRIx32 " SP:%08" PRIx32 "\n",
stacktop, context->sp);
sinfo("New stack top:%08" PRIx32 " SP:%08" PRIx32 "\n",
newtop, newsp);
```
Missing whitespace between printed values.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817813912
There is really too many unrelated changes in this PR to review properly. It should be separated into several PRs that can be properly reviewed.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612096107
##########
File path: arch/avr/src/avr/up_createstack.c
##########
@@ -86,10 +86,6 @@
int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
{
- /* Add the size of the TLS information structure */
-
- stack_size += sizeof(struct tls_info_s);
-
#ifdef CONFIG_TLS_ALIGNED
/* The allocated stack size must not exceed the maximum possible for the
* TLS feature.
Review comment:
Yes, TLS_MAXSTACK is still valid because it's key invariant of the aligned TLS. The real difference is that the stack size is always equals to caller supplied value(e.g. 2048) without adjustion(+ sizeof(struct tls_info_s)).
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817312023
> The names do not carry enough context and are still too confusing.
>
> The Deinitions should be clearer - This one is not so bad, the the description can use some work
> `stack_alloc_ptr`: Pointer to the memory allocated for the stack and optional TLS this is the base address returned by an alloc operation.
>
> This one needs work:
>
> `adj_stack_ptr`: Adjusted `stack_alloc_ptr` for HW. - add information as to why it is adjusted in the comment.
>
Ok, I will add more comment.
> Then consider changing the name - See it is not a stack pointer and yet is is called adj_stack_ptr -> adj_stackbase_ptr
adj_stack_ptr is same as stack_alloc_ptr at the beginning. up_stack_frame will increase adj_stack_ptr and decrease adj_stack_size by frame_size to allocate the space for caller(e.g. tls, argv...). since adj_stack_ptr is paired with adj_stack_size, it is still a name better than adj_stackbase_ptr in the current implementationt, I think.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611661458
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
> This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
>
No, main thread allocate ```struct task_info_s```:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-06984fa681053d61fe6d98716085fb7d1e9099deecbb332fa5da35d05b28ee1fR177-R185
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-3e8be9133fa805536122dc95c4303786822b38382e44e053b399e5a43824ae9fR133-R140
pthread allocate ``` struct tls_info_s```:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-936dfc6e164f9162faee0b703c30f31bea8d6f5a60a6a92b0965f179f7c763a4R308-R315
Since tls_info_s is the first field of task_info_s, all tls specific function and data work with the main thread. But the main thread could add more state for task wide state.
> Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
>
> It is VERY important to restore this to the way it was. The number of per-task variables will grow a lot (see #3168). It is critical that there be only a single copy of the per-task data. Please revert this change.
Here is the diagram:
```
Main thread Pthread
/+-------------+ +-------------+
Thread specific< | ta_tls | | tl_elem |
\| | | tl_errno |
/+-------------+ +-------------+
Task specific< | ta_getopt | | |
\+-------------+ | |
| argv | | |
+-------------+ | |
| | | |
| Available | | Available |
| Stack | | Stack |
| | | |
| | | |
| | ^ | | ^
| | | | | |
| | | |
+-------------+ +-------------+
| Task Data* | | Task Data* |
+-------------+ +-------------+
```
--
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] gustavonihei commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611907582
##########
File path: arch/risc-v/src/rv32im/riscv_vfork.c
##########
@@ -150,39 +148,18 @@ pid_t up_vfork(const struct vfork_s *context)
sinfo("Parent=%p Child=%p\n", parent, child);
- /* Get the size of the parent task's stack. Due to alignment operations,
- * the adjusted stack size may be smaller than the stack size originally
- * requested.
- */
-
- stacksize = parent->adj_stack_size + CONFIG_STACK_ALIGNMENT - 1;
-
- /* Allocate the stack for the TCB */
-
- ret = up_create_stack((FAR struct tcb_s *)child, stacksize + argsize,
- parent->flags & TCB_FLAG_TTYPE_MASK);
- if (ret != OK)
- {
- serr("ERROR: up_create_stack failed: %d\n", ret);
- nxtask_abort_vfork(child, -ret);
- return (pid_t)ERROR;
- }
-
- /* Allocate the memory and copy argument from parent task */
-
- argv = up_stack_frame((FAR struct tcb_s *)child, argsize);
- memcpy(argv, parent->adj_stack_ptr, argsize);
-
/* How much of the parent's stack was utilized? The RISC-V uses
* a push-down stack so that the current stack pointer should
* be lower than the initial, adjusted stack pointer. The
* stack usage should be the difference between those two.
*/
- DEBUGASSERT((uint32_t)parent->adj_stack_ptr > context->sp);
- stackutil = (uint32_t)parent->adj_stack_ptr - context->sp;
+ stacktop = (uint32_t)parent->adj_stack_ptr +
+ parent->adj_stack_size;
+ DEBUGASSERT(stacktop > context->sp);
+ stackutil = stacktop - context->sp;
- sinfo("stacksize:%d stackutil:%d\n", stacksize, stackutil);
+ sinfo("stackutil:%u\n", stackutil);
Review comment:
```suggestion
sinfo("Parent: stackutil:%" PRIu32 "\n", stackutil);
```
Reusing the same template form other archs
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611825470
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This will need to be in https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=152115079
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818262380
> Sorry, I can't really review this change as I'm not really familiar with this part of the codebase.
There are five commits. Most deal with other issues that you could probably have input on. Otherwise, @davids5 will need to do the merge (ONLY after we are sure that 10.1 is protected from the changes).
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612489165
##########
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:
In the old code the task/thread SP was set to the adj_stack_ptr. That value by inspection was properly aligned.
This change splits that alignment across the pointer and the size. Making all this very pron to errors.
##########
File path: arch/arm/src/armv6-m/arm_initialstate.c
##########
@@ -74,7 +74,8 @@ void up_initial_state(struct tcb_s *tcb)
/* Save the initial stack pointer */
- xcp->regs[REG_SP] = (uint32_t)tcb->adj_stack_ptr;
+ xcp->regs[REG_SP] = (uint32_t)tcb->adj_stack_ptr +
Review comment:
@xiaoxiang781216 - the old code's naming make sense and matched the semantic. `xcp->regs[REG_SP] = (uint32_t)tcb->adj_stack_ptr`
In the new code you have changed the semantics and I feel very strongly that the naming has change.
tcb->adj_stack_ptr is NOW the stack base. Please call it that. `stackbase_ptr`
Then a the stack penetration test will start at the `stackbase_ptr` upward for the guard value.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817339328
> You conception of stacks and TLS positioning in the stacks is incorrect. The TLS data must always lie at the stack allocation pointer.
The comment mislead my understanding. Anyway, I will remove tls_info_ptr.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818669760
> The ESP32 tests have passed. Did also some manual testing.
> Will give the Xtensa part a look this evening if you don't mind.
Thanks, let's wait your test on Xtensa. It's important to test on all arch we are working daily.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817829909
> > > @davids5 the comment is added.
> >
> >
> > @xiaoxiang781216 Thank you!
> > This is still un clear to me. What is the frame in this context?
> > Can you add a ASCII diagram that show the points that the pointers represent now.
>
> @davids5 here is the diagram:
> https://github.com/apache/incubator-nuttx/pull/3517/files#diff-b60d398c141520d01499c0154109c89556dc639d5895018f328455f443c229a6R314-R330
Thanks!
This helps to have the conversation
I'm I correct in assuming this is in lowest address first?
```
* Here is the diagram after some allocation:
*
* +-------------+ <-stack_alloc_ptr (lowest)
* | TLS Data |
* +-------------+
* | Arguments |
* adj_stack_ptr-> +-------------+\
* | Available | +
* | Stack | |
* | | | |
* | | | |
* v | | +->adj_stack_size
* | | |
* | | |
* +-------------+ |
* |Thread Data* | +
* +-------------+/
```
"adj_stack_ptr: Adjusted initial stack pointer after the frame has been removed from the stack"
How can this be the adjusted initial stack pointer? It is at the base of the stack.
The inital stack pointer is the what is used to align and set the SP - it is the TOP of the stack.
So what is confusing is "Adjusted initial stack pointer after the frame has been removed from the stack allocation."
adj_stack_ptr `Adjusted stack base pointer after the TLS Data and Arguments has been removed from the stack allocation.`
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612613171
##########
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:
if both adj_stack_ptr and adj_stack_size align on some value(e.g. 8), it's obvious that adj_stack_ptr + adj_stack_size still align on the same value(e.g. 8).
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611645689
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
It is VERY important to restore this to the way it was. The number of per-task variables will grow a lot (see #3168). It is critical that there be only a single copy of the per-task data. Please revert this change.
You have implemented this as a per-thread variables. That is inconsistent with the standard behavior.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611645689
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
It is VERY important to restore this to the way it was. The number of per-task variables will grow a lot (see #3168). It is critical that there be only a single copy of the per-task data. Please revert this change.
You have implemented this as a per-thread variables. That is inconsistent with the standard behavior. NOTE that the behavior is different if CONFIG_BUILD_KERNEL. Then the behavior is correct: We have standard per-process (or per-task) getopt() data as is expected.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817310256
The names do not carry enough context and are still too confusing.
The Deinitions should be clearer - This one is not so bad, the the description can use some work
``stack_alloc_ptr``: Pointer to the memory allocated for the stack and optional TLS this is the base address returned by an alloc operation.
This one needs work:
``adj_stack_ptr``: Adjusted ``stack_alloc_ptr`` for HW. - add information as to why it is adjusted in the comment.
Then consider changing the name - See it is not a stack pointer and yet is is called adj_stack_ptr -> adj_stackbase_ptr
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
Push Down Push Up
+-------------+ +-------------+
| TLS Data | | TLS Data |
+-------------+ +-------------+
| | | Task Data |
| Available | +-------------+
| Stack | | Arguments |
| | +-------------+
| | | |
| | | Available | |
| | | Stack | v
| | ^ | |
+-------------+ | | |
| Arguments | | |
+-------------+ | |
| Task Data | | |
+-------------+ +-------------+
NOTE: The comment at the referenced location is wrong. That code has nothing to do with push-up or push-down. The code is correct. PR #3519 fixes that bad comment.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611216800
##########
File path: include/nuttx/sched.h
##########
@@ -402,7 +401,7 @@ struct stackinfo_s
FAR void *stack_alloc_ptr; /* Pointer to allocated stack */
/* Needed to deallocate stack */
FAR void *adj_stack_ptr; /* Adjusted stack_alloc_ptr for HW */
- /* The initial stack pointer value */
+ FAR void *tls_info_ptr; /* Thread local storage information */
Review comment:
This change is wrong. Please remove tls_info_ptr which must always have the same values stack_alloc_ptr.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
NOTE: The comment at the referenced location is wrong. That code has nothing to do with push-up or push-down. The code is correct. PR #3519 fixes that bad comment.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] gustavonihei commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611907582
##########
File path: arch/risc-v/src/rv32im/riscv_vfork.c
##########
@@ -150,39 +148,18 @@ pid_t up_vfork(const struct vfork_s *context)
sinfo("Parent=%p Child=%p\n", parent, child);
- /* Get the size of the parent task's stack. Due to alignment operations,
- * the adjusted stack size may be smaller than the stack size originally
- * requested.
- */
-
- stacksize = parent->adj_stack_size + CONFIG_STACK_ALIGNMENT - 1;
-
- /* Allocate the stack for the TCB */
-
- ret = up_create_stack((FAR struct tcb_s *)child, stacksize + argsize,
- parent->flags & TCB_FLAG_TTYPE_MASK);
- if (ret != OK)
- {
- serr("ERROR: up_create_stack failed: %d\n", ret);
- nxtask_abort_vfork(child, -ret);
- return (pid_t)ERROR;
- }
-
- /* Allocate the memory and copy argument from parent task */
-
- argv = up_stack_frame((FAR struct tcb_s *)child, argsize);
- memcpy(argv, parent->adj_stack_ptr, argsize);
-
/* How much of the parent's stack was utilized? The RISC-V uses
* a push-down stack so that the current stack pointer should
* be lower than the initial, adjusted stack pointer. The
* stack usage should be the difference between those two.
*/
- DEBUGASSERT((uint32_t)parent->adj_stack_ptr > context->sp);
- stackutil = (uint32_t)parent->adj_stack_ptr - context->sp;
+ stacktop = (uint32_t)parent->adj_stack_ptr +
+ parent->adj_stack_size;
+ DEBUGASSERT(stacktop > context->sp);
+ stackutil = stacktop - context->sp;
- sinfo("stacksize:%d stackutil:%d\n", stacksize, stackutil);
+ sinfo("stackutil:%u\n", stackutil);
Review comment:
```suggestion
sinfo("Parent: stackutil:%" PRIu32 "\n", stackutil);
```
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612573540
##########
File path: arch/arm/src/armv6-m/arm_initialstate.c
##########
@@ -74,7 +74,8 @@ void up_initial_state(struct tcb_s *tcb)
/* Save the initial stack pointer */
- xcp->regs[REG_SP] = (uint32_t)tcb->adj_stack_ptr;
+ xcp->regs[REG_SP] = (uint32_t)tcb->adj_stack_ptr +
Review comment:
Done.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-820015894
@masayuki2009 thanks for testing on the different board. Does lc823450-xgevk:rndis pass the test before applying the patch?
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611645689
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
It is VERY important to restore this to the way it was. The number of per-task variables will grow a lot (see #3168). It is critical that there be only a single copy of the per-task data. Please revert this change.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817843672
> This change puts a copy y of the getopt() variables in every thread. That is incorrect and wasteful of stack memory. There must be only a single, per-task copy of the getopt() variables. Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that.
Please reread my reply in the previous thread.
--
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] masayuki2009 commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-820032116
> @masayuki2009 thanks for testing on the different board. Does lc823450-xgevk:rndis pass the test before applying the patch?
@xiaoxiang781216
Yes. The latest upstream has no such issue.
However, I suspect there might be a potential bug outside this PR.
Anyway, I will continue to investigate what is happening.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817349564
> @patacongo please review again, tls_info_ptr is replaced by DEBUGASSERT.
I will look at this tonight or in the morning. I have some obligations for the rest of today.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612099566
##########
File path: arch/risc-v/src/rv32im/riscv_vfork.c
##########
@@ -150,39 +148,18 @@ pid_t up_vfork(const struct vfork_s *context)
sinfo("Parent=%p Child=%p\n", parent, child);
- /* Get the size of the parent task's stack. Due to alignment operations,
- * the adjusted stack size may be smaller than the stack size originally
- * requested.
- */
-
- stacksize = parent->adj_stack_size + CONFIG_STACK_ALIGNMENT - 1;
-
- /* Allocate the stack for the TCB */
-
- ret = up_create_stack((FAR struct tcb_s *)child, stacksize + argsize,
- parent->flags & TCB_FLAG_TTYPE_MASK);
- if (ret != OK)
- {
- serr("ERROR: up_create_stack failed: %d\n", ret);
- nxtask_abort_vfork(child, -ret);
- return (pid_t)ERROR;
- }
-
- /* Allocate the memory and copy argument from parent task */
-
- argv = up_stack_frame((FAR struct tcb_s *)child, argsize);
- memcpy(argv, parent->adj_stack_ptr, argsize);
-
/* How much of the parent's stack was utilized? The RISC-V uses
* a push-down stack so that the current stack pointer should
* be lower than the initial, adjusted stack pointer. The
* stack usage should be the difference between those two.
*/
- DEBUGASSERT((uint32_t)parent->adj_stack_ptr > context->sp);
- stackutil = (uint32_t)parent->adj_stack_ptr - context->sp;
+ stacktop = (uint32_t)parent->adj_stack_ptr +
+ parent->adj_stack_size;
+ DEBUGASSERT(stacktop > context->sp);
+ stackutil = stacktop - context->sp;
- sinfo("stacksize:%d stackutil:%d\n", stacksize, stackutil);
+ sinfo("stackutil:%u\n", stackutil);
Review comment:
Done.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612556240
##########
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:
Both the old and new code require that:
1. up_[create|use]_stack align the stack size and base to the boundary
2. up_stack_frame align frame size to the boundary
If you inspect these variables in debugger, all(stack_alloc_ptr, adj_stack_ptr and adj_stack_size) should align correctly.
--
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] Ouss4 commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818667730
The ESP32 tests have passed. Did also some manual testing.
Will give the Xtensa part a look this evening if you don't mind.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611661458
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
> This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
>
No, main thread allocate ```struct task_info_s```:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-06984fa681053d61fe6d98716085fb7d1e9099deecbb332fa5da35d05b28ee1fR177-R185
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-3e8be9133fa805536122dc95c4303786822b38382e44e053b399e5a43824ae9fR133-R140
pthread allocate ``` struct tls_info_s```:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-936dfc6e164f9162faee0b703c30f31bea8d6f5a60a6a92b0965f179f7c763a4R308-R315
Since tls_info_s is the first field of task_info_s, all tls specific function and data work with the main thread. But the main thread could add more state for task wide state.
> Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
>
> It is VERY important to restore this to the way it was. The number of per-task variables will grow a lot (see #3168). It is critical that there be only a single copy of the per-task data. Please revert this change.
Here is the diagram:
```
Main thread Pthread
+-------------+ +-------------+
| ta_tls | | tl_elem |
| | | tl_errno |
+-------------+ +-------------+
| ta_getopt | | |
+-------------+ | |
| argv | | |
+-------------+ | |
| | | |
| Available | | Available |
| Stack | | Stack |
| | | |
| | | |
| | ^ | | ^
| | | | | |
| | | |
+-------------+ +-------------+
| Task Data* | | Task Data* |
+-------------+ +-------------+
```
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817310692
@xiaoxiang781216 Given the size of this PR, please make separate commits for review comments until the review is done so we can look a the deltas, then you can squash it.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612099751
##########
File path: arch/mips/src/mips32/mips_vfork.c
##########
@@ -191,32 +168,33 @@ pid_t up_vfork(const struct vfork_s *context)
* effort is overkill.
*/
- newsp = (uint32_t)child->cmn.adj_stack_ptr - stackutil;
+ newtop = (uintptr_t)child->cmn.adj_stack_ptr +
+ child->cmn.adj_stack_size;
+ newsp = newtop - stackutil;
memcpy((void *)newsp, (const void *)context->sp, stackutil);
/* Was there a frame pointer in place before? */
#ifdef CONFIG_MIPS32_FRAMEPOINTER
- if (context->fp <= (uint32_t)parent->adj_stack_ptr &&
- context->fp >= (uint32_t)parent->adj_stack_ptr - stacksize)
+ if (context->fp >= context->sp && context->fp < stacktop)
{
- uint32_t frameutil = (uint32_t)parent->adj_stack_ptr - context->fp;
- newfp = (uint32_t)child->cmn.adj_stack_ptr - frameutil;
+ uint32_t frameutil = stacktop - context->fp;
+ newfp = newtop - frameutil;
}
else
{
newfp = context->fp;
}
- sinfo("Old stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp, context->fp);
- sinfo("New stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp, newfp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ stacktop, context->sp, context->fp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ newtop, newsp, newfp);
Review comment:
Done
##########
File path: arch/arm/src/common/arm_vfork.c
##########
@@ -167,26 +146,27 @@ pid_t up_vfork(const struct vfork_s *context)
* effort is overkill.
*/
- newsp = (uint32_t)child->cmn.adj_stack_ptr - stackutil;
+ newtop = (uint32_t)child->cmn.adj_stack_ptr +
+ child->cmn.adj_stack_size;
+ newsp = newtop - stackutil;
memcpy((void *)newsp, (const void *)context->sp, stackutil);
/* Was there a frame pointer in place before? */
- if (context->fp <= (uint32_t)parent->adj_stack_ptr &&
- context->fp >= (uint32_t)parent->adj_stack_ptr - stacksize)
+ if (context->fp >= context->sp && context->fp < stacktop)
{
- uint32_t frameutil = (uint32_t)parent->adj_stack_ptr - context->fp;
- newfp = (uint32_t)child->cmn.adj_stack_ptr - frameutil;
+ uint32_t frameutil = stacktop - context->fp;
+ newfp = newtop - frameutil;
}
else
{
newfp = context->fp;
}
- sinfo("Parent: stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp, context->fp);
- sinfo("Child: stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp, newfp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ stacktop, context->sp, context->fp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ newtop, newsp, newfp);
Review comment:
Done.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612109849
##########
File path: include/nuttx/arch.h
##########
@@ -308,9 +308,26 @@ int up_use_stack(FAR struct tcb_s *tcb, FAR void *stack, size_t stack_size);
*
* - adj_stack_size: Stack size after removal of the stack frame from
* the stack
- * - adj_stack_ptr: Adjusted initial stack pointer after the frame has
- * been removed from the stack. This will still be the initial value
- * of the stack pointer when the task is started.
+ * - adj_stack_ptr: Adjusted stack base pointer after the TLS Data and
+ * Arguments has been removed from the stack allocation.
+ *
+ * Here is the diagram after some allocation(tls, arg):
+ *
+ * +-------------+ <-stack_alloc_ptr(lowest)
+ * | TLS Data |
+ * +-------------+
+ * | Arguments |
+ * adj_stack_ptr-> +-------------+\
+ * | Available | +
+ * | Stack | |
+ * | | | |
+ * | | | |
+ * v | | +->adj_stack_size
+ * | | |
+ * | | |
+ * +-------------+ |
+ * | Thread Data | +
+ * +-------------+/
Review comment:
>
>
> Ok, add to here:
> https://github.com/apache/incubator-nuttx/pull/3517/files#diff-b0e4e9a1378b2a5ded4af14faa20cd4fea7cc12ae8d832df45817891718734eaR126-R142
Is that correct? Isn't "Thread Data" at the other end of the stack? Like the version I added here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=152115079 Or maybe I don't know what Thread Data is???
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817353172
I made some minor updates to the TLS Wiki page: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=152115079
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612735629
##########
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:
Yes. My head is stuck in the days were the alloc pointer was not aligned. (it was also freed if not 0) Now it is aligned on allocation. So that is one change not in the same place that added to my not understanding it.
Am I understanding the sizing correctly: If I have 300 byte stack it will now be either much larger with CONFIG_TLS_ALIGNED lit or it will be smaller by the sizeof(tls) and args? Before this change it looked (CONFIG_TLS_ALIGNED not lit) like 300+sizeof(tls) which I got what I asked for and the OS got what it needed. Is that still the case?
Also it is correct that if I ask for an 11K stack it may get truncated by 3K without warning if the TLS_MAXSTACK (log and friends) ends up at 8K?
##########
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:
Yes. My head is stuck in the days were the alloc pointer was not aligned. (it was also freed if not 0) Now it is aligned on allocation. So that is one change not in the same place that added to my not understanding it.
Am I understanding the sizing correctly: If I have 300 byte stack it will now be either much larger with CONFIG_TLS_ALIGNED lit or it will be smaller by the sizeof(tls) and args? Before this change it looked (CONFIG_TLS_ALIGNED not lit) like 300+sizeof(tls) which I got what I asked for and the OS got what it needed. Is that still the case?
Also is it correct that if I ask for an 11K stack it may get truncated by 3K without warning if the TLS_MAXSTACK (log and friends) ends up at 8K?
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612111652
##########
File path: include/nuttx/arch.h
##########
@@ -308,9 +308,26 @@ int up_use_stack(FAR struct tcb_s *tcb, FAR void *stack, size_t stack_size);
*
* - adj_stack_size: Stack size after removal of the stack frame from
* the stack
- * - adj_stack_ptr: Adjusted initial stack pointer after the frame has
- * been removed from the stack. This will still be the initial value
- * of the stack pointer when the task is started.
+ * - adj_stack_ptr: Adjusted stack base pointer after the TLS Data and
+ * Arguments has been removed from the stack allocation.
+ *
+ * Here is the diagram after some allocation(tls, arg):
+ *
+ * +-------------+ <-stack_alloc_ptr(lowest)
+ * | TLS Data |
+ * +-------------+
+ * | Arguments |
+ * adj_stack_ptr-> +-------------+\
+ * | Available | +
+ * | Stack | |
+ * | | | |
+ * | | | |
+ * v | | +->adj_stack_size
+ * | | |
+ * | | |
+ * +-------------+ |
+ * | Thread Data | +
+ * +-------------+/
Review comment:
Yes, "Thread Data" make confusion here. I will remove it, thanks.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817950927
@patacongo Ok, the change separate to five patch, you can review here one by one:
https://github.com/apache/incubator-nuttx/pull/3517/commits
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819464216
> @xiaoxiang781216 - Is it time to squash or do the docs need to updated too? Once squashed, can we all test this on as many arches' real HW before merging?
Ok, I will squach it. the doc already update to reflect the change.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817828085
> There is really too many unrelated changes in this PR to review properly. It should be separated into several PRs that can be properly reviewed.
The rename change is already separated to a new patch, you can review it here:
https://github.com/apache/incubator-nuttx/pull/3517/commits/401d8cce169340e50fe74487bb8a3ed9144c086a
The rest change relate to each other:
1. Allocate the space from the lowest stack(up_stack_frame)
2. The stack dump/check need adjust because adj_stack_ptr point to the low end of stack
3. TLS data need to explicitly be allocated by up_stack_frame to ensure it's address always equals stack_alloc_ptr
4. vfork also need adjust since the implementation of vfork couple with how the stack memory allocate
You can review this patch here:
https://github.com/apache/incubator-nuttx/pull/3517/commits/07f0019fe0ecb53f3870069408dabfc57949307a
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817309644
> You might also look how TLS is implemented in Zephyr.
I will took a look.
> Much nicer than our implementation: https://docs.zephyrproject.org/latest/guides/thread_local_storage.html
>
Zephyr implementation need toolchain support(```__thread``` keyword introduced by gcc 3.3.1 2003-08-04):
https://github.com/zephyrproject-rtos/zephyr/blame/3c79b565fe7e6afed3687d95dd3e70bb93699a88/doc/guides/thread_local_storage.rst#L10
So, I think it's better to support both approach(explicit and implicit TLS) or we give up the old toolchain support.
> This is good too: https://chao-tic.github.io/blog/2018/12/25/tls#tls-access-in-executables-and-shared-objects
>
> I think if we want to re-architect TLS, we should do a better job.
This patch try to move the most tls action from arch to sched/libc, which is a step to implement the implicit TLS.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818257149
> @davids5 @v01d last person to approve should do the merge.
Sorry, I can't really review this change as I'm not really familiar with this part of the codebase.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611188584
##########
File path: sched/task/task_init.c
##########
@@ -130,21 +127,15 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority,
goto errout_with_group;
}
-#ifndef CONFIG_BUILD_KERNEL
- /* Allocate a stack frame to hold task-specific data */
-
- group = tcb->cmn.group;
- group->tg_libvars = up_stack_frame(&tcb->cmn, sizeof(struct libvars_s));
- DEBUGASSERT(group->tg_libvars != NULL);
-
- /* Initialize the task-specific data */
-
- memset(group->tg_libvars, 0, sizeof(struct libvars_s));
-
- /* Save the allocated task data in TLS */
+ /* Initialize thread local storage */
- tls_set_taskdata(&tcb->cmn);
-#endif
+ tcb->cmn.tls_info_ptr = up_stack_frame(&tcb->cmn,
+ sizeof(struct task_info_s));
+ if (tcb->cmn.tls_info_ptr == NULL)
+ {
+ ret = -ENOMEM;
+ goto errout_with_group;
+ }
Review comment:
> This is difficult to understand. But it looks like you are moving TLS data from its correct position at the far end of the stack. That is very wrong. In ALL systems, including NuttX in KERNEL MODE, the TLS data must lie at the lowest address of the stack.
>
No, TLS is still at the lowest address of the stack, because I modify up_frame_stack to allocate the memory from the beginning of stack:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-9944a69f3cb6b7de3620c2b924464f4cde28d4a4c3225134e5f7562ea5fefd47R197-R198
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-5c9cf944a960b9718953b2cb399f5ba99fafdc22445ddd6fb9ab27ac1b808ef6R96-R122
> Generally, TLS is obtained very efficiently like:
>
> ```
> tls = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK);
> ```
>
> No system call is involved. This standard behavior must be preserved.
>
Yes, the trick still work as before if CONFIG_TLS_ALIGNED turn on.
> In FLAT mode, we can also use aligned stacks, but it is not a good use of memory. In the FLAT mode, the overhead of a system call is preferable.
>
> But I would reject this change as I understand it because it destroys the possibility of high perfromance, standard TLS.
I think my reply could resolve your concern now.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
Push Down Push Up
+-------------+ +-------------+ <- Allocation address (lowest)
| TLS Data | | TLS Data |
+-------------+ +-------------+
| | | Task Data |
| Available | +-------------+
| Stack | | Arguments |
| | +-------------+
| | | |
| | | Available | |
| | | Stack | v
| | ^ | |
+-------------+ | | |
| Arguments | | |
+-------------+ | |
| Task Data | | |
+-------------+ +-------------+
NOTE: The comment at the referenced location is wrong. That code has nothing to do with push-up or push-down. The code is correct. PR #3519 fixes that bad comment.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611186107
##########
File path: sched/task/task_init.c
##########
@@ -130,21 +127,15 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority,
goto errout_with_group;
}
-#ifndef CONFIG_BUILD_KERNEL
- /* Allocate a stack frame to hold task-specific data */
-
- group = tcb->cmn.group;
- group->tg_libvars = up_stack_frame(&tcb->cmn, sizeof(struct libvars_s));
- DEBUGASSERT(group->tg_libvars != NULL);
-
- /* Initialize the task-specific data */
-
- memset(group->tg_libvars, 0, sizeof(struct libvars_s));
-
- /* Save the allocated task data in TLS */
+ /* Initialize thread local storage */
- tls_set_taskdata(&tcb->cmn);
-#endif
+ tcb->cmn.tls_info_ptr = up_stack_frame(&tcb->cmn,
+ sizeof(struct task_info_s));
+ if (tcb->cmn.tls_info_ptr == NULL)
+ {
+ ret = -ENOMEM;
+ goto errout_with_group;
+ }
Review comment:
This is difficult to understand. But it looks like you are moving TLS data from its correct position at the far end of the stack. That is very wrong. In ALL systems, including NuttX in KERNEL MODE, the TLS data must lie at the lowest address of the stack.
Generally, TLS is obtained very efficiently like:
tls = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK);
No system call is involved. This standard behavior must be preserved.
In FLAT mode, we can also use aligned stacks, but it is not a good use of memory. In the FLAT mode, the overhead of a system call is preferable.
But I would reject this change as I understand it because it destroys the possibility of high perfromance, standard TLS.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612556240
##########
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:
Both the old and new code require that:
1. up_[create|use]_stack align the stack size and base to the boundary
2. up_stack_frame align frame size to the boundary
If you inspect these variables in debugger, all(stack_alloc_ptr, adj_stack_ptr and adj_stack_size) should be aligned correctly.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612099619
##########
File path: arch/mips/src/mips32/mips_vfork.c
##########
@@ -191,32 +168,33 @@ pid_t up_vfork(const struct vfork_s *context)
* effort is overkill.
*/
- newsp = (uint32_t)child->cmn.adj_stack_ptr - stackutil;
+ newtop = (uintptr_t)child->cmn.adj_stack_ptr +
+ child->cmn.adj_stack_size;
+ newsp = newtop - stackutil;
memcpy((void *)newsp, (const void *)context->sp, stackutil);
/* Was there a frame pointer in place before? */
#ifdef CONFIG_MIPS32_FRAMEPOINTER
- if (context->fp <= (uint32_t)parent->adj_stack_ptr &&
- context->fp >= (uint32_t)parent->adj_stack_ptr - stacksize)
+ if (context->fp >= context->sp && context->fp < stacktop)
{
- uint32_t frameutil = (uint32_t)parent->adj_stack_ptr - context->fp;
- newfp = (uint32_t)child->cmn.adj_stack_ptr - frameutil;
+ uint32_t frameutil = stacktop - context->fp;
+ newfp = newtop - frameutil;
}
else
{
newfp = context->fp;
}
- sinfo("Old stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp, context->fp);
- sinfo("New stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp, newfp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ stacktop, context->sp, context->fp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ newtop, newsp, newfp);
#else
- sinfo("Old stack base:%p SP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp);
- sinfo("New stack base:%p SP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 "\n",
+ stacktop, context->sp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 "\n",
+ newtop, newsp);
Review comment:
Done.
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611992693
##########
File path: include/nuttx/arch.h
##########
@@ -308,9 +308,26 @@ int up_use_stack(FAR struct tcb_s *tcb, FAR void *stack, size_t stack_size);
*
* - adj_stack_size: Stack size after removal of the stack frame from
* the stack
- * - adj_stack_ptr: Adjusted initial stack pointer after the frame has
- * been removed from the stack. This will still be the initial value
- * of the stack pointer when the task is started.
+ * - adj_stack_ptr: Adjusted stack base pointer after the TLS Data and
+ * Arguments has been removed from the stack allocation.
+ *
+ * Here is the diagram after some allocation(tls, arg):
+ *
+ * +-------------+ <-stack_alloc_ptr(lowest)
+ * | TLS Data |
+ * +-------------+
+ * | Arguments |
+ * adj_stack_ptr-> +-------------+\
+ * | Available | +
+ * | Stack | |
+ * | | | |
+ * | | | |
+ * v | | +->adj_stack_size
+ * | | |
+ * | | |
+ * +-------------+ |
+ * | Thread Data | +
+ * +-------------+/
Review comment:
Could you add this to the docs as well (for the `up_use_stack` function)? I think it is critical to understand all the variables.
--
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] Ouss4 edited a comment on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
Ouss4 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818524721
> I think a test that can be run for each arch would be ideal. Maybe there's something that could be setup specially for this change?
I'll push a branch internally with this PR to run the tests we have for ESP32 chips.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818287213
> I get the gist of it mostly
Same with me. There is just too much unrelated stuff crammed into the same PR. It is not humanly possible to review properly. It was worse originally... there was only one commit with everything in it. We need to try avoid PRs like this in future if we want proper reviews.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819894606
@patacongo Sqush was done
> can we all test this on as many arches' real HW before merging?
I want to run it on some real HW , but I am in the I am in the middle of something else, but as soon as I get time. I will test it.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819892298
What is the status of the PR? I approved it a couple of days ago. Looking over the comments it looks like we are now waiting for @Ouss4 to complete some testing. Is that right? @Ouss4 can you merge this when you are comfortable? I did not merge originally because I was waiting for others to complete their review.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817860767
> > Please reread my reply in the previous thread.
>
> Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
As I reply before, there are five changes in this PR:
0. Rename g_intstackbase to g_intstacktop
1. Allocate the space from the lowest stack(up_stack_frame)
2. The stack dump/check need adjust because adj_stack_ptr point to the low end of stack
3. TLS data need to explicitly be allocated by up_stack_frame to ensure it's address always equals stack_alloc_ptr
4. vfork also need adjust since the implementation of vfork couple with how the stack memory allocate
Item 0 is already separated. Item 1 to 4 is coupled with each other, it's hard to separate them without breaking the runtime. For example, After apply item 1, if we don't change item 3, TLS data will overwrite by argv in the main thread.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611829810
##########
File path: sched/pthread/pthread_create.c
##########
@@ -301,11 +303,16 @@ int pthread_create(FAR pthread_t *thread, FAR const pthread_attr_t *attr,
goto errout_with_join;
}
-#ifndef CONFIG_BUILD_KERNEL
- /* Save the allocated task data in TLS */
+ /* Initialize thread local storage */
- tls_set_taskdata(&ptcb->cmn);
-#endif
+ info = up_stack_frame(&ptcb->cmn, sizeof(struct tls_info_s));
+ if (info == NULL)
+ {
+ errcode = ENOMEM;
+ goto errout_with_join;
+ }
+
Review comment:
I still don't understand this. The high performance access to aligned stacks requires that the TLS data lie at the stack allocation access. For a push down stack, up_stack_frame() allocates memory downward from the highest stack addresses.
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611982253
##########
File path: arch/avr/src/avr/up_createstack.c
##########
@@ -86,10 +86,6 @@
int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
{
- /* Add the size of the TLS information structure */
-
- stack_size += sizeof(struct tls_info_s);
-
#ifdef CONFIG_TLS_ALIGNED
/* The allocated stack size must not exceed the maximum possible for the
* TLS feature.
Review comment:
are the lines below (checking stack_size against TLS_MAXSTACK) still valid? I understand that stack_size no longer includes TLS size, right?
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819464216
> @xiaoxiang781216 - Is it time to squash or do the docs need to updated too? Once squashed, can we all test this on as many arches' real HW before merging?
Done, I will squach it. the doc already update to reflect the change.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r613718196
##########
File path: arch/avr/src/avr/up_createstack.c
##########
@@ -86,10 +86,6 @@
int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
{
- /* Add the size of the TLS information structure */
-
- stack_size += sizeof(struct tls_info_s);
-
#ifdef CONFIG_TLS_ALIGNED
/* The allocated stack size must not exceed the maximum possible for the
* TLS feature.
Review comment:
The problem is resovled by moving the adjustiont to common place:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-3e8be9133fa805536122dc95c4303786822b38382e44e053b399e5a43824ae9fR123-R125
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-06984fa681053d61fe6d98716085fb7d1e9099deecbb332fa5da35d05b28ee1fR166-R169
So the behaviour is same as before.
--
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] patacongo edited a comment on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817854300
> Please reread my reply in the previous thread.
So this is just a redesign of working code with no value added. It is just for your personal preference. That is not in the spirit of a good open source project. It is bad to humiliate people by throwing away there perfectly good code just so you can replace with your own NIH code. Very bad style.
Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818284700
> > Sorry, I can't really review this change as I'm not really familiar with this part of the codebase.
>
> There are five commits. Most deal with other issues that you could probably have input on. Otherwise, @davids5 will need to do the merge (ONLY after we are sure that 10.1 is protected from the changes).
I looked at the changes of each commit. I don't really understand much of this so I can't provide a meaningful review (I get the gist of it mostly). Anyway, I think even for an expert it would still be hard to catch a bug here. I think a test that can be run for each would be ideal. Maybe there's something that could be setup specially for this change?
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r613195609
##########
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:
> 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.
Of course. If it always was a liner blow through and simply detected, I would agree with you, But in the real world it is not. I have seen a call at the bottom of a stack that do not corrupt the bottom but under the base by chunks of locals. When this hits a a well defined struct (like TCB) you can tell, when it hits something in else not clearly defined like TLS, it is harder to debug.
Some other OS (pSOS) protect label and protect their data. It is just a suggestion to make debugging efficient.
--
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] masayuki2009 commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-820240380
> @xiaoxiang781216
>
> Yes. The latest upstream has no such issue.
I ran the ostest with ```CONFIG_TESTING_OSTEST_LOOPS=50``` but the test succeeded.
> However, I suspect there might be a potential bug outside this PR.
> Anyway, I will continue to investigate what is happening.
I noticed that the hardfault happened because ```rtcb->xcp.sigdeliver == NULL``` in arm_sigdeliver().
However, if I enabled ```CONFIGU_DEBUG_ASSERTIONS=y```, the ostest finished successfully.
I'm still investigating why ```rtcb->xcp.sigdeliver``` is set to NULL when arm_sigdeliver() is called.
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r613162096
##########
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? 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.
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?
--
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] masayuki2009 commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819929100
I checked the following configurations and found that the ostest with lc823450-xgevk failed.
I will look into the issue later.
qemu-i486:nsh -> OK
qemu-intel64:nsh -> OK
sim:smp (ubuntu18.04 x86_64) -> OK
raspberrypi-pico:smp -> OK
lm3s6965-ek:discover (QEMU) -> OK
lc823450-xgevk:rndis -> NG (ostest failed)
stm32f4discovery:wifi -> OK
spresense:smp, spresense:wifi -> OK
sabre-6quad:smp (QEMU) -> OK
hifive1-revb:nsh (QEMU) -> OK
maix-bit:smp (QEMU) -> OK
esp32-devkitc:smp (QEMU) -> OK
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611645689
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
It is VERY important to restore this to the way it was. The number of per-task variables will grow a lot (see #3168). It is critical that there be only a single copy of the per-task data. Please revert this change.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817810272
> > @davids5 the comment is added.
>
> @xiaoxiang781216 Thank you!
>
> This is still un clear to me. What is the frame in this context?
>
> Can you add a ASCII diagram that show the points that the pointers represent now.
@davids5 here is the diagram:
https://github.com/apache/incubator-nuttx/pull/3517/commits/3415b90cfd8cb373af4190137f5c251a97806569#diff-b60d398c141520d01499c0154109c89556dc639d5895018f328455f443c229a6R314-R330
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817957982
> > Please reread my reply in the previous thread.
>
> So this is just a redesign of working code with no value added. It is just for your personal preference. That is not in the spirit of a good open source project. It is bad to humiliate people by throwing away there perfectly good code just so you can replace with your own NIH code. Very bad style.
>
No, it isn't just preference, here is the reason I made this change:
1. Unify the memory allocation from stack with up_stack_frame regardless it's TLS data or argv.
2. Avoid skip TLS space explicitly in many place since TLS is allocated(remove from picture) like other stack variable
3. Don't need save tg_libvars pointer in task_group_s and tls_info_s
The most important thing is that ``Modular Architecture```get improved because almost TLS special handling is decoupled and removed from arch:
1. Avoid the code duplication in each arch(reduce 600+ line 31.7%)
2. Fix #1631 in the common place(sched/) and avoid the similar problem happen again in new arch
> Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-820883027
Sure, thanks you very much!
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
Push Down Push Up
+-------------+ +-------------+ <- Allocation address (lowest)
| TLS Data | | TLS Data |
+-------------+ +-------------+
| | | Task Data |
| Available | +-------------+
| Stack | | Arguments |
| | +-------------+
| | | |
| | | Available | |
| | | Stack | v
| | ^ | |
+-------------+ | | |
| Arguments | | |
+-------------+ | |
| Task Data | | |
+-------------+ +-------------+
NOTE: The comment at the referenced location is wrong. That code has nothing to do with push-up or push-down. The code is correct. PR #3519 fixes that bad comment.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] Ouss4 commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819894811
I rebased the internal branch earlier today and the tests passed as well.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r613244799
##########
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:
Yes, but the stack color could give the max stack depth reliablely. As a general rule, it's better that each thread has 20% free stack space in anytime.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817669329
@davids5 the comment is added.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612096107
##########
File path: arch/avr/src/avr/up_createstack.c
##########
@@ -86,10 +86,6 @@
int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
{
- /* Add the size of the TLS information structure */
-
- stack_size += sizeof(struct tls_info_s);
-
#ifdef CONFIG_TLS_ALIGNED
/* The allocated stack size must not exceed the maximum possible for the
* TLS feature.
Review comment:
Yes, TLS_MAXSTACK is still valid because it's key invariant of the aligned TLS. The real difference is that the stack size is always equals to caller supplied value without adjustion(+ sizeof(struct tls_info_s)).
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. You logic is wrong. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. This is required for user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612952857
##########
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:
Here is the patch: https://github.com/apache/incubator-nuttx/pull/3517/commits/d485c3687fc9e25380706f4af09de5157c9c0a79
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817758194
> @davids5 the comment is added.
@xiaoxiang781216 Thank you!
This is still un clear to me. What is the frame in this context?
Can you add a ASCII diagram that show the points that the pointers represent now.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817345621
> The comment mislead my understanding. Anyway, I will remove tls_info_ptr.
It is easy to get confused. I was confused when I wrote the comments.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611645689
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This is wrong.
Before this questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
It is VERY important to restore this to the way it was. The number of per-task variables will grow a lot (see #3168). It is critical that there be only a single copy of the per-task data. Please revert this change.
--
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] Ouss4 commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818524721
> I think a test that can be run for each arch would be ideal. Maybe there's something that could be setup specially for this change?
I'll push a branch internally with this PR to run the tests we have.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818393928
> > I get the gist of it mostly
>
> Same with me. There is just too much unrelated stuff crammed into the same PR. It is not humanly possible to review properly. It was worse originally... there was only one commit with everything in it. We need to try avoid PRs like this in future if we want proper reviews.
Yes, I also want to split the patch into small one as I do normally. But since the change in up_stack_frame(allocate from the low end of stack) impact many thing:
1. Stack check and dump
2. TLS data allocation
3. vfork stack duplication
It's more simple to only focus on arm arch, since the same change duplicate in each arch.
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612735629
##########
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:
Yes. My head is stuck in the days were the alloc pointer was not aligned. (it was also freed if not 0) Now it is aligned on allocation. So that is one change not in the same place that added to my not understanding it.
Am I understanding the sizing correctly: If I have 300 byte stack it will now be either much larger with CONFIG_TLS_ALIGNED lit or it will be smaller by the sizeof(tls) and args? Before this change it looked (CONFIG_TLS_ALIGNED not lit) like 300+sizeof(tls) which I got what I asked for and the OS git what it needed. Is that still the case?
Also it is correct that if I ask for an 11K stack it may get truncated by 3K without warning if the TLS_MAXSTACK (log and friends) ends up at 8K?
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817807748
> > The comment mislead my understanding. Anyway, I will remove tls_info_ptr.
>
> It is easy to get confused. I was confused when I wrote the comments.
>
> it would be helpful to separate all of the pointless renaming from the core changes. It is very difficult to review and make sense out of. There is no real technical description of the change which makes it harder to review.
Ok, I create a new patch to contain the rename change.
--
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] masayuki2009 edited a comment on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-819929100
I checked the following configurations and found that the ostest with lc823450-xgevk failed.
I will look into the issue later.
qemu-i486:nsh -> OK
qemu-intel64:nsh -> OK
sim:smp (ubuntu18.04 x86_64) -> OK
raspberrypi-pico:smp -> OK
lm3s6965-ek:discover (QEMU) -> OK
lc823450-xgevk:rndis -> NG (ostest failed)
stm32f4discovery:wifi -> OK
spresense:smp, spresense:wifi -> OK
sabre-6quad:smp (QEMU) -> OK
hifive1-revb:nsh (QEMU) -> OK
maix-bit:smp (QEMU) -> OK
esp32-devkitc:smp (QEMU) -> OK
```
user_main: nested signal handler test
signest_test: Starting signal waiter task at priority 101
waiter_main: Waiter started
signest_test: Started waiter_main pid=61
waiter_main: Setting signal mask
signest_test: Starting interfering task at priority 102
waiter_main: Registering signal handler
signest_test: Started interfere_main pid=62
waiter_main: Waiting on semaphore
interfere_main: Waiting on semaphore
arm_hardfault: Hard Fault:
arm_hardfault: IRQ: 3 regs: 0x2092ccc
arm_hardfault: BASEPRI: 000000f0 PRIMASK: 00000000 IPSR: 00000003 CONTROL: 00000000
arm_hardfault: CFAULTS: 00020000 HFAULTS: 40000000 DFAULTS: 00000000 BFAULTADDR: e000ed38 AFAULTS: 00000000
arm_hardfault: PANIC!!! Hard fault: 40000000
up_assert: Assertion failed CPU1 at file:armv7-m/arm_hardfault.c line: 135 task: waiter
up_registerdump: R0: 0208ec70 00000000 00000001 00000000 0208ec70 00000000 00000004 02042077
up_registerdump: R8: 00000000 00000000 00000000 00000000 0000000a 02092d18 02046f45 00000000
up_registerdump: xPSR: 20000000 PRIMASK: 00000000 CONTROL: 00000000
up_registerdump: EXC_RETURN: fffffff9
up_dumpstate: sp: 0200cb40
up_dumpstate: IRQ stack:
up_dumpstate: base: 0200c3d0
up_dumpstate: size: 00000800
up_dumpstate: used: 00000190
up_stackdump: 0200cb40: 00000000 00000000 00000000 0000000a 02092d18 02046f45 00000000 02045b55
up_stackdump: 0200cb60: 00000000 e000ed2c 00000000 fffffffe 020838ac 00000000 00000000 02044b01
up_stackdump: 0200cb80: 0205b7a1 02040ebd 40000000 00000000 e000ed38 00000000 02085014 0200cbd4
up_stackdump: 0200cba0: 00000003 02092ccc 00000000 02041ccf 0200cbd4 02041325 020769bc 02092ccc
up_stackdump: 0200cbc0: 00000000 00000004 02042077 02040f61 00000000 00000000 02092ccc 016e3600
up_dumpstate: sp: 02092d18
up_dumpstate: User stack:
up_dumpstate: base: 02090de0
up_dumpstate: size: 00001ff0
up_dumpstate: used: 000001e8
up_stackdump: 02092d00: 00000001 00000000 0000000a 02046f45 00000000 20000000 0200ce45 02092d78
up_stackdump: 02092d20: 00000001 0200b4ea 0208ec70 00000000 02042077 00000000 00000000 00000000
up_stackdump: 02092d40: 00000000 fffffff9 00000002 0208ecfc 020021d4 00000000 0000000a 02042055
up_stackdump: 02092d60: 02046f01 21000000 0200b4ea 0208ec70 00000000 02042055 0200b4e8 0200b4ca
up_stackdump: 02092d80: 0200b4ea 0204207d 0200b4e8 02065a41 00000000 02065911 2aaaaaaa 00000000
up_stackdump: 02092da0: deadbeef 0208ec70 020659ad 00000000 00000000 02044d51 00000000 020425b1
up_stackdump: 02092dc0: 00000000 00000000 00000000 00000000 0200ce40 0200ce40 00002040 80002040
up_dumpstate: CPU1:
up_taskdump: CPU0 IDLE: PID=0 Stack Used=408 of 1024
up_taskdump: CPU1 IDLE: PID=1 Stack Used=192 of 2048
up_taskdump: hpwork: PID=3 Stack Used=248 of 2032
up_taskdump: lpwork: PID=4 Stack Used=376 of 2032
up_taskdump: init: PID=5 Stack Used=968 of 3056
up_taskdump: Telnet daemon: PID=6 Stack Used=456 of 2008
up_taskdump: ostest: PID=17 Stack Used=496 of 2032
up_taskdump: ostest: PID=18 Stack Used=944 of 8136
up_taskdump: waiter: PID=61 Stack Used=488 of 8176
up_taskdump: interfere: PID=62 Stack Used=472 of 8168
```
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your logic is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612109849
##########
File path: include/nuttx/arch.h
##########
@@ -308,9 +308,26 @@ int up_use_stack(FAR struct tcb_s *tcb, FAR void *stack, size_t stack_size);
*
* - adj_stack_size: Stack size after removal of the stack frame from
* the stack
- * - adj_stack_ptr: Adjusted initial stack pointer after the frame has
- * been removed from the stack. This will still be the initial value
- * of the stack pointer when the task is started.
+ * - adj_stack_ptr: Adjusted stack base pointer after the TLS Data and
+ * Arguments has been removed from the stack allocation.
+ *
+ * Here is the diagram after some allocation(tls, arg):
+ *
+ * +-------------+ <-stack_alloc_ptr(lowest)
+ * | TLS Data |
+ * +-------------+
+ * | Arguments |
+ * adj_stack_ptr-> +-------------+\
+ * | Available | +
+ * | Stack | |
+ * | | | |
+ * | | | |
+ * v | | +->adj_stack_size
+ * | | |
+ * | | |
+ * +-------------+ |
+ * | Thread Data | +
+ * +-------------+/
Review comment:
>
>
> Ok, add to here:
> https://github.com/apache/incubator-nuttx/pull/3517/files#diff-b0e4e9a1378b2a5ded4af14faa20cd4fea7cc12ae8d832df45817891718734eaR126-R142
Is that correct? Isn't "Thread Data" at the other end? Like the version I added here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=152115079
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612514004
##########
File path: arch/arm/src/armv6-m/arm_initialstate.c
##########
@@ -74,7 +74,8 @@ void up_initial_state(struct tcb_s *tcb)
/* Save the initial stack pointer */
- xcp->regs[REG_SP] = (uint32_t)tcb->adj_stack_ptr;
+ xcp->regs[REG_SP] = (uint32_t)tcb->adj_stack_ptr +
Review comment:
Ok, I will change to stack_base_ptr because it more similar with stack_alloc_ptr.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817860767
> > Please reread my reply in the previous thread.
>
> Okay. I will wait until you separate this into reviewable PRs. It is impossible to properly review this big mess.
As I reply before, there are five changes in this PR:
0. Rename g_intstackbase to g_intstacktop
1. Allocate the space from the lowest stack(up_stack_frame)
2. The stack dump/check need adjust because adj_stack_ptr point to the low end of stack
3. TLS data need to explicitly be allocated by up_stack_frame to ensure it's address always equals stack_alloc_ptr
4. vfork also need adjust since the implementation of vfork couple with how the stack memory allocate
Item 0 is already separated. Item 1 to 4 is coupled with each other, it's hard to separate them without breaking the runtime. For example, After apply item 1, if we don't change item 3, TLS data will overwrite by argv in the main thread.
--
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] patacongo commented on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817863910
> As I reply before, there are five changes in this PR:
>
> 1. Rename g_intstackbase to g_intstacktop
>
> 2. Allocate the space from the lowest stack(up_stack_frame)
>
> 3. The stack dump/check need adjust because adj_stack_ptr point to the low end of stack
>
> 4. TLS data need to explicitly be allocated by up_stack_frame to ensure it's address always equals stack_alloc_ptr
>
> 5. vfork also need adjust since the implementation of vfork couple with how the stack memory allocate
Too many!!!!!!!!!!!!! And most are completely unrelated and do not belong in the same PR.
Please break up into 5 PRs. This is impossible to review.
--
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 a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612569335
##########
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:
Is it true I have to add adj_stack_ptr and adj_stack_size to see the 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] xiaoxiang781216 edited a comment on pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817810272
> > @davids5 the comment is added.
>
> @xiaoxiang781216 Thank you!
>
> This is still un clear to me. What is the frame in this context?
>
> Can you add a ASCII diagram that show the points that the pointers represent now.
@davids5 here is the diagram:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-b60d398c141520d01499c0154109c89556dc639d5895018f328455f443c229a6R314-R330
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817309644
> You might also look how TLS is implemented in Zephyr.
I will took a look.
> Much nicer than our implementation: https://docs.zephyrproject.org/latest/guides/thread_local_storage.html
>
Zephyr implementation need toolchain support(```__thread``` keyword):
https://github.com/zephyrproject-rtos/zephyr/blame/3c79b565fe7e6afed3687d95dd3e70bb93699a88/doc/guides/thread_local_storage.rst#L10
So, I think it's better to support both approach(explicit and implicit TLS).
> This is good too: https://chao-tic.github.io/blog/2018/12/25/tls#tls-access-in-executables-and-shared-objects
>
> I think if we want to re-architect TLS, we should do a better job.
This patch try to move the most tls action from arch to sched/libc, which is a step to implement the implicit TLS.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-818288101
10.1 was already branched.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612952857
##########
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:
@papatience Here is the patch: https://github.com/apache/incubator-nuttx/pull/3517/commits/d485c3687fc9e25380706f4af09de5157c9c0a79
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611217336
##########
File path: include/nuttx/sched.h
##########
@@ -402,7 +401,7 @@ struct stackinfo_s
FAR void *stack_alloc_ptr; /* Pointer to allocated stack */
/* Needed to deallocate stack */
FAR void *adj_stack_ptr; /* Adjusted stack_alloc_ptr for HW */
- /* The initial stack pointer value */
+ FAR void *tls_info_ptr; /* Thread local storage information */
Review comment:
Ok, I will remove tls_info_ptr
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-820047481
Sure, thanks for helping. Let's wait your result.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
Push Down Push Up
+-------------+ +-------------+
| TLS Data | | TLS Data |
+-------------+ +-------------+
| | | Task Data |
| Available | +-------------+
| Stack | | Arguments |
| | +-------------+
| | | |
| | | Available | |
| | | Stack | v
| | ^ | |
+-------------+ | | |
| Arguments | | |
+-------------+ | |
| Task Data | | |
+-------------+ +-------------+
NOTE: The comment at the referenced location is wrong. That code has nothing to do with push-up or push-down. The code is correct. PR #3519 fixes that bad comment.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612749139
##########
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:
>
>
> Before this change it looked (CONFIG_TLS_ALIGNED not lit) like 300+sizeof(tls) which I got what I asked for and the OS got what it needed. Is that still the case?
I believe you will get 300-sizeof(tls). That could be dangerous for finely tuned, minimal stack sizes.
> Also is it correct that if I ask for an 11K stack it may get truncated by 3K without warning if the TLS_MAXSTACK (log and friends) ends up at 8K?
And CONFIG_TLS_ALIGNED=y. In normal usage, that option is only used with an MMU. In that case, there is no penalty for the alignment because a virtual stack address space is allocated (and backed up by real RAM only on demand). Normally the alignment is around 1Mb. I seem to recall the Linux pthread stacks are aligned to a 2Mb virtual address (but I don't remember for certain).
If you can use CONFIG_TLS_ALIGNED=y, then you get a good performance benefit in the you can avoid a system call. Without CONFIG_TLS_ALIGNED, a system call is required. But with CONFIG_TLS_ALIGNED=y, no system call is needed. The stack base is found simply by:
stack_base = ((builtin_stack_pointer()) & ~(TLS_MAXSTACK - 1)
There is not much motivation to use CONFIG_TLS_ALIGNED=y in a FLAT build since the cost of a system call is very low. There are usually no system calls in the FLAT build (although they can be set up for some improved ELF module support). The PROTECTED build with the MPU might benefit with 8Kb aligned stacks although stack alignment might result in some additional memory fragmentation.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 merged pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
masayuki2009 merged pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817867488
> > As I reply before, there are five changes in this PR:
> > ```
> > 1. Rename g_intstackbase to g_intstacktop
> >
> > 2. Allocate the space from the lowest stack(up_stack_frame)
> >
> > 3. The stack dump/check need adjust because adj_stack_ptr point to the low end of stack
> >
> > 4. TLS data need to explicitly be allocated by up_stack_frame to ensure it's address always equals stack_alloc_ptr
> >
> > 5. vfork also need adjust since the implementation of vfork couple with how the stack memory allocate
> > ```
>
> Too many!!!!!!!!!!!!! And most are completely unrelated and do not belong in the same PR.
>
> Please break up into 5 PRs. This is impossible to review.
If I separate the change into 4 patch, the image mayn't boot at all, because:
1. If we don't change item 2, the stack check and dump report the wrong info.
2. If we don't change item 3, TLS data will overwrite by argv in the main thread.
I can break the change if the temporary runtime error is acceptable.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611645689
##########
File path: include/nuttx/tls.h
##########
@@ -78,10 +78,17 @@ struct tls_info_s
#if CONFIG_TLS_NELEM > 0
uintptr_t tl_elem[CONFIG_TLS_NELEM]; /* TLS elements */
#endif
- FAR struct libvars_s *tl_libvars; /* Task-specific C library data */
int tl_errno; /* Per-thread error number */
};
+struct task_info_s
+{
+ struct tls_info_s ta_tls; /* Must be first field */
+#ifndef CONFIG_BUILD_KERNEL
+ struct getopt_s ta_getopt; /* Globals used by getopt() */
+#endif
Review comment:
This is incorrect. This mean that there will be a copy of the getopt() variables in every thread right? But only the copy in the main thread will used. This seems wrong.
Before your questionable change, there was a single copy of the getopt() variables in the main thread created by up_stack_frame. Each thread pointed to that single copied. But it looks like you have ruined that. Your solution is incorrect.
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. Your change is wrong; the referenced logic is correct. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_stack_pointer()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
NOTE: The comment at the referenced location is wrong. That code has nothing to do with push-up or push-down. The code is correct.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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] gustavonihei commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611906267
##########
File path: arch/mips/src/mips32/mips_vfork.c
##########
@@ -191,32 +168,33 @@ pid_t up_vfork(const struct vfork_s *context)
* effort is overkill.
*/
- newsp = (uint32_t)child->cmn.adj_stack_ptr - stackutil;
+ newtop = (uintptr_t)child->cmn.adj_stack_ptr +
+ child->cmn.adj_stack_size;
+ newsp = newtop - stackutil;
memcpy((void *)newsp, (const void *)context->sp, stackutil);
/* Was there a frame pointer in place before? */
#ifdef CONFIG_MIPS32_FRAMEPOINTER
- if (context->fp <= (uint32_t)parent->adj_stack_ptr &&
- context->fp >= (uint32_t)parent->adj_stack_ptr - stacksize)
+ if (context->fp >= context->sp && context->fp < stacktop)
{
- uint32_t frameutil = (uint32_t)parent->adj_stack_ptr - context->fp;
- newfp = (uint32_t)child->cmn.adj_stack_ptr - frameutil;
+ uint32_t frameutil = stacktop - context->fp;
+ newfp = newtop - frameutil;
}
else
{
newfp = context->fp;
}
- sinfo("Old stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- parent->adj_stack_ptr, context->sp, context->fp);
- sinfo("New stack base:%p SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
- child->cmn.adj_stack_ptr, newsp, newfp);
+ sinfo("Old stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ stacktop, context->sp, context->fp);
+ sinfo("New stack top:%08" PRIx32 "SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
+ newtop, newsp, newfp);
Review comment:
```suggestion
sinfo("Old stack top:%08" PRIx32 " SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
stacktop, context->sp, context->fp);
sinfo("New stack top:%08" PRIx32 " SP:%08" PRIx32 " FP:%08" PRIx32 "\n",
newtop, newsp, newfp);
```
Missing whitespace between printed values.
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r612119684
##########
File path: include/nuttx/arch.h
##########
@@ -308,9 +308,26 @@ int up_use_stack(FAR struct tcb_s *tcb, FAR void *stack, size_t stack_size);
*
* - adj_stack_size: Stack size after removal of the stack frame from
* the stack
- * - adj_stack_ptr: Adjusted initial stack pointer after the frame has
- * been removed from the stack. This will still be the initial value
- * of the stack pointer when the task is started.
+ * - adj_stack_ptr: Adjusted stack base pointer after the TLS Data and
+ * Arguments has been removed from the stack allocation.
+ *
+ * Here is the diagram after some allocation(tls, arg):
+ *
+ * +-------------+ <-stack_alloc_ptr(lowest)
+ * | TLS Data |
+ * +-------------+
+ * | Arguments |
+ * adj_stack_ptr-> +-------------+\
+ * | Available | +
+ * | Stack | |
+ * | | | |
+ * | | | |
+ * v | | +->adj_stack_size
+ * | | |
+ * | | |
+ * +-------------+ |
+ * | Thread Data | +
+ * +-------------+/
Review comment:
Done. I also copy the nice diagram of tls and task from wiki:
https://github.com/apache/incubator-nuttx/pull/3517/files#diff-2c833ffaa18374fab0d57499dc593937167b198a630cb4e9068f654aaebdc42dR74-R93
--
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] patacongo commented on a change in pull request #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#discussion_r611210648
##########
File path: sched/sched/sched_get_stackinfo.c
##########
@@ -101,5 +111,7 @@ int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
stackinfo->adj_stack_size = qtcb->adj_stack_size;
stackinfo->stack_alloc_ptr = qtcb->stack_alloc_ptr;
stackinfo->adj_stack_ptr = qtcb->adj_stack_ptr;
+ stackinfo->tls_info_ptr = qtcb->tls_info_ptr;
Review comment:
> I save this poiner to fix the wrong assumption(push-down stack) you made in #987:
> [bda24f0#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72](https://github.com/apache/incubator-nuttx/commit/bda24f09c2fe0d72b3452686052d075fa7f416ae#diff-dd1838d4b8adff099b4fbcd041c8ec8e7f70cdf1fb9e360781b7298352105be6R68-R72)
This is not a wrong assumption and has nothing to do with a push-down stack. It is correct logic. You logic is wrong. The TLS data must lie at the lowest address of the memory allocation or it cannot be accessed in user space use via:
tlsinfo = (FAR struct tls_info_s *)((builtin_sp()) & TLS_MASK).
On a push-up stack, TLS data must still lie at the lowest allocated address. The only difference is that the memory is reserved and "end" of the stack in one case and at "beginning" in the other. That difference would have to be handled by architecture-specific code if we ever did support an architecture with a push-up stack.
This kind of access (without system calls) is required for efficient, user-space access using aligned stacks.
That is necessary for pure user-space access. You changes are not good and should not be merged (although it is difficult to understand what all is going on in 207 files of mixed changes).
--
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 #3517: arch: Allocate the space from the beginning in up_stack_frame
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3517:
URL: https://github.com/apache/incubator-nuttx/pull/3517#issuecomment-817343549
@patacongo please review again, tls_info_ptr is replaced by DEBUGASSERT.
--
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