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/29 03:17:30 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request #3626: libc: Move pthread staff to userspace
no1wudi opened a new pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626
## Summary
See #3182
## Impact
pthread
## Testing
Tested with maix-bit:knsh、maix-bit:kostest and custom boards
--
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] no1wudi removed a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi removed a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852553347
> Not on your list of things was the similar changes to call the on_exit() and atexit() callbacks when a task exits. In order to resolve #1263, we still have to address those security violations as well. Can we add these?
OK, I'll add them to work item.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632754532
##########
File path: include/nuttx/pthread.h
##########
@@ -121,6 +125,72 @@ EXTERN const pthread_attr_t g_default_pthread_attr;
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: nx_pthread_create
+ *
+ * Description:
+ * This function creates and activates a new thread with specified
+ * attributes.
+ *
+ * Input Parameters:
+ * trampoline - The user space startup function
+ * thread - The pthread handle to be used
+ * attr - It points to a pthread_attr_t structure whose contents are
+ * used at thread creation time to determine attributes
+ * for the new thread
+ * entry - The new thread starts execution by invoking entry
+ * arg - It is passed as the sole argument of entry
+ * exit - The user-space pthread exit function
+ *
+ * Returned Value:
+ * OK (0) on success; a (non-negated) errno value on failure. The errno
+ * variable is not set.
+ *
+ ****************************************************************************/
+
+int nx_pthread_create(pthread_trampoline_t trampoline, FAR pthread_t *thread,
+ FAR const pthread_attr_t *attr,
+ pthread_startroutine_t entry, pthread_addr_t arg,
+ pthread_exitroutine_t exit);
+
+/****************************************************************************
+ * Name: nx_pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_value
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
Review comment:
```suggestion
```
If there are no assumptions, the field may be omitted.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623136987
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit(). In the original code before your change there are these:
sched/pthread/pthread_create.c: pthread_exit(exit_status);
sched/signal/sig_default.c: pthread_exit(NULL);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
I think this is an error. We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved.
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829783428
> pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit() from within the OS in supervisor mode. This is a problem.
>
> In the original code before your change there are these:
>
> ```
> sched/pthread/pthread_create.c: pthread_exit(exit_status);
> sched/signal/sig_default.c: pthread_exit(NULL);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
> ```
>
> We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved. See full comments for a suggested solution.
>
> In my point of view, we could probably just open another PR for these remaining issues and merge as is. We are still making a positive step forward.
If replace all pthread_exit() with nx_pthread_exit(), there would be a memory leak that cleanup function (include future ptherad_key_create destructors) not executed correctly.
I'll try this solution:
> Possible solution:
> Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be
> >known in the PROTECTED and KERNEL builds. I think you were not seeing the build failure in the PROTECTED/KERNEL builds > because cancellation points and default signal actions were not enabled.
> Save the pthread_exit() pointer in the TCB
> Replace calls to pthread_exit() in the OS to up_pthread_exit().
> up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
But there is a problem from pthread_cancel.c:88:
```
#ifdef CONFIG_PTHREAD_CLEANUP
/* Perform any stack pthread clean-up callbacks.
*
* REVISIT: In this case, the clean-up callback will execute on the
* thread of the caller of pthread cancel, not on the thread of
* the thread-to-be-canceled. This is a problem when deferred
* cancellation is not supported because, for example, the clean-up
* function will be unable to unlock its own mutexes.
*/
pthread_cleanup_popall(tcb);
#endif
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-854236721
>
>
> > For ELF binary, we have three case:
>
> I believe that the existing solution is superior because there is only a single, unified design that works in all configurations, differing only in the need to drop to user mode in some cases. I hope that we do not change that. That would be a mistake.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642693226
##########
File path: syscall/syscall.csv
##########
@@ -82,15 +83,12 @@
"pread","unistd.h","","ssize_t","int","FAR void *","size_t","off_t"
"pselect","sys/select.h","","int","int","FAR fd_set *","FAR fd_set *","FAR fd_set *","FAR const struct timespec *","FAR const sigset_t *"
"pthread_cancel","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
-"pthread_cleanup_pop","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","int"
-"pthread_cleanup_push","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","pthread_cleanup_t","FAR void *"
"pthread_cond_broadcast","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *"
"pthread_cond_clockwait","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *","FAR pthread_mutex_t *","clockid_t","FAR const struct timespec *"
"pthread_cond_signal","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *"
"pthread_cond_wait","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *","FAR pthread_mutex_t *"
-"pthread_create","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_t *","FAR const pthread_attr_t *","pthread_startroutine_t","pthread_addr_t"
"pthread_detach","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
-"pthread_exit","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
+"nx_pthread_exit","nuttx/pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
Review comment:
tls_get_set() was also out of order. Ordering corrected by #3813
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852143714
> 1. Move pthread_exit & pthread_create to USERSPACE to unify the convention that how to drop to userspace from kernel side like this:
> `regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup`
> `regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_exit`
This part I don't understand. That doesn't unify the designs, it makes them different: There is no USERSPACE in KERNEL mode. I don't understand why this is a good thing.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623136987
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit(). In the original code before your change there are these:
sched/pthread/pthread_create.c: pthread_exit(exit_status);
sched/signal/sig_default.c: pthread_exit(NULL);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
I think this is an error. We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved.
Possible solution:
1. Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be known in the PROTECTED and KERNEL builds
2. Save the pthread_exit() pointer in the TCB
3. Replace calls to pthread_exit() in the OS to up_pthread_exit().
4. up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852173304
userspace_s is only used in PROTECTED mode. It is not used in KERNEL mode and can never be. In KERNEL mode, all tasks are ELF modules and do not expose anything like userspace_s. In KERNEL mode we will have to stick with the existing implementation.
The best solution is to unify the designs and have one solution. The PROTECTED mode can use the KERNEL mode solution which is implemented now. But the KERNEL mode cannot use the PROTECTED mode solution proposed here. I think it is a very bad idea. Best to stay with a unified design.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623910672
##########
File path: arch/arm/src/armv7-a/arm_syscall.c
##########
@@ -290,19 +290,21 @@ uint32_t *arm_syscall(uint32_t *regs)
* R2 = arg
*/
-#if defined(CONFIG_BUILD_KERNEL) && !defined(CONFIG_DISABLE_PTHREAD)
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
Review comment:
```suggestion
/* Set up to enter the user-space pthread start-up function in
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632766289
##########
File path: libs/libc/tls/tls_destruct.c
##########
@@ -0,0 +1,81 @@
+/****************************************************************************
+ * libs/libc/tls/tls_destruct.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdint.h>
+#include <assert.h>
+
+#include <nuttx/arch.h>
+#include <nuttx/tls.h>
+#include <arch/tls.h>
+
+#if CONFIG_TLS_NELEM > 0
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tls_destruct
+ *
+ * Description:
+ * Destruct all TLS data element associated with allocated key
+ *
+ * Input Parameters:
+ *
Review comment:
`None` may be added when there are no parameters
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853854833
> There isn't difference between ELF binary(posix_spawn) and builtin binary(task_spawn) after binfmt finish load and parse ELF file.
In the FLAT build, there could be minor fourth case when libc.a is linked into the ELF module which is often done. There could be a case pthread_start() and pthread_exit() exist only within the ELF module and not in the base FLASH 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] no1wudi edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829779823
> build is failing because of:
>
> ```
> stubs/STUB_nx_pthread_exit.c: In function 'STUB_nx_pthread_exit':
> Error: stubs/STUB_nx_pthread_exit.c:12:1: error: control reaches end of non-void function [-Werror=return-type]
> 12 | }
> | ^
> ```
>
> That should be handled as it is for other no return functions:
>
> ```
> syscall.csv:"_exit","unistd.h","","noreturn","int"
> syscall.csv:"exit","stdlib.h","","noreturn","int"
> syscall.csv:"pthread_exit","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
> ```
>
> But you have:
>
> ```
> "nx_pthread_exit","nuttx/pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
> ```
>
> So I don't understand what the issue is.
Fixed, I forget to add `noreturn_function` attribute to the declaration of nx_pthread_exit
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852553347
> Not on your list of things was the similar changes to call the on_exit() and atexit() callbacks when a task exits. In order to resolve #1263, we still have to address those security violations as well. Can we add these?
OK, I'll add them to work item.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632751276
##########
File path: arch/arm/src/armv8-m/arm_svcall.c
##########
@@ -323,21 +323,21 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
* R2 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
*/
- regs[REG_R0] = regs[REG_R1]; /* pthread entry */
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
regs[REG_R1] = regs[REG_R2]; /* arg */
Review comment:
```suggestion
regs[REG_R1] = regs[REG_R3]; /* arg */
```
The `pthread_addr_t arg` is on R3
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623136987
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit(). In the original code before your change there are these:
sched/pthread/pthread_create.c: pthread_exit(exit_status);
sched/signal/sig_default.c: pthread_exit(NULL);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
I think this is an error. We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved.
Possible solution:
1. Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be known in the PROTECTED and KERNEL builds. I think you were not seeing the build failure in the PROTECTED/KERNEL builds because cancellation points and default signal actions were not enabled.
2. Save the pthread_exit() pointer in the TCB
3. Replace calls to pthread_exit() in the OS to up_pthread_exit().
4. up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
Hmm.. I think that the lm3s6965-ek:qemu-protected should have given an error:
$ find boards/ -name defconfig | xargs grep -l BUILD_PROTECTED | xargs grep CANCELLATION
$ find boards/ -name defconfig | xargs grep -l BUILD_PROTECTED | xargs grep SIG_DEFAULT
boards/arm/tiva/lm3s6965-ek/configs/qemu-protected/defconfig:CONFIG_SIG_DEFAULT=y
$ find boards/ -name defconfig | xargs grep -l BUILD_KERNEL | xargs grep CANCELLATION
$ find boards/ -name defconfig | xargs grep -l BUILD_KERNEL | xargs grep SIG_DEFAULT
It would be worth checking the System.map to assure that pthread_exit() is never referenced in the kernel blob.
--
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] no1wudi edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829783428
> pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit() from within the OS in supervisor mode. This is a problem.
>
> In the original code before your change there are these:
>
> ```
> sched/pthread/pthread_create.c: pthread_exit(exit_status);
> sched/signal/sig_default.c: pthread_exit(NULL);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
> ```
>
> We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved. See full comments for a suggested solution.
>
> In my point of view, we could probably just open another PR for these remaining issues and merge as is. We are still making a positive step forward.
If replace all pthread_exit() with nx_pthread_exit(), there would be a memory leak that cleanup function (include future ptherad_key_create destructors) not executed correctly.
I'll try this solution:
> Possible solution:
> Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be
> >known in the PROTECTED and KERNEL builds. I think you were not seeing the build failure in the PROTECTED/KERNEL builds > because cancellation points and default signal actions were not enabled.
> Save the pthread_exit() pointer in the TCB
> Replace calls to pthread_exit() in the OS to up_pthread_exit().
> up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
or another possible solution:
Replace these pthread_exit with by send SIGABRT, dorp to user space with sig action and call pthread_exit in signal handler ?
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623155477
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
I wonder if any of the calls to pthread_exit() could be replaced with calls to nx_pthread_exit(). That is possible in certain error handling cases where the pthread never actually ran (and so could not have set up any callbacks).
I wonder if we could move the remaining functions into libs/libc/pthread:
sched/signal/sig_default.c
sched/task/task_cancelpt.c
sched/task/task_setcancelstate.c
sched/task/task_setcanceltype.c
If we could that might be a simpler solution.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853854833
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-846350804
@no1wudi Are you interested in finishing this job? That is, fixing on_exit() and atexit() callbacks so that they execute in user mode in all build modes? Logically this would be a very similar job: task_startup() and task exits would have to be moved to libs/libc and the callbacks would have to be re-architected so that the call back function points and parameters lie in TLS and the functions are called in some user-space task exit logic.
This, however, is more complex for a couple of reasons:
1. on_exit() and atexit() processing is more entangled task_exithook(). However, I think that that entanglement is no longer necessary. I think it is an artifact from an older design. It should be okay to move those callbacks to the beginning of the exit sequence in the current design. Hmm.. is there some reason why the exit callbacks should not execute while there could be pthreads running in an SMP configuration? There are a few things to think about.
2. The task_startup() function is the same for the FLAT and PROTECTED builds, but works differently in the KERNEL build. In that case, a crt0.o file it linked at the beginning of th problem. See for example: https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/armv7-a/crt0.c
3. There are several ways to exit: exit(), task_delete(), task_reset(), assert(), abort(), .... others?
_exit() might also be in that group, but since that is reserved for emergency terminations, it probably does not need to honor the on_exit() / atexit() (In fact, it is not really safe to call _exit() at all in an embedded system since it leaves files open).
There is a block diagram of the exit sequence here: https://cwiki.apache.org/confluence/display/NUTTX/Task+Exit+Sequence I am not sure if that is 100% up to date.
--
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] no1wudi commented on a change in pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623096404
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
+#ifdef CONFIG_PTHREAD_CLEANUP
+ int cnt;
+ struct pthread_cleanup_s cleanup[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+ cnt = pthread_cleanup_poplist(cleanup);
+
+ sched_lock();
+ while (cnt-- > 0)
+ {
+ struct pthread_cleanup_s cp = cleanup[cnt];
+ if (cp.pc_cleaner)
+ cp.pc_cleaner(cp.pc_arg);
+ }
+
+ sched_unlock();
+#endif
+
Review comment:
Yes, I had implemented a version of tls data destructors
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-854236295
> For ELF binary, we have three case:
I believe that the existing solution is superior because there is only a single, unified design that works in all configurations. I hope that we do not change that. That would be a mistake.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-854236721
>
>
> > For ELF binary, we have three case:
>
> I believe that the existing solution is superior because there is only a single, unified design that works the same in all configurations, differing only in the need to drop to user mode in some cases. I hope that we do not change that. That would be a mistake.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r640787605
##########
File path: arch/risc-v/src/rv64gc/riscv_swint.c
##########
@@ -282,33 +282,64 @@ int riscv_swint(int irq, FAR void *context, FAR void *arg)
break;
#endif
- /* R0=SYS_pthread_start: This a user pthread start
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
+ /* A0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
- * R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * A0 = SYS_pthread_start
+ * A1 = startup
+ * A2 = entrypt
+ * A3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_EPC] = (uintptr_t)regs[REG_A1] & ~1; /* startup */
Review comment:
RISC-V use the mix encoding(instruction length can be 2bytes or 4bytes), so it don't have something like ARM/THUMB, and the bit0 is always zero. The mask can be removed safely.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623128090
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
+#ifdef CONFIG_PTHREAD_CLEANUP
+ int cnt;
+ struct pthread_cleanup_s cleanup[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+ cnt = pthread_cleanup_poplist(cleanup);
+
+ sched_lock();
+ while (cnt-- > 0)
+ {
+ struct pthread_cleanup_s cp = cleanup[cnt];
+ if (cp.pc_cleaner)
+ cp.pc_cleaner(cp.pc_arg);
+ }
+
+ sched_unlock();
+#endif
+
Review comment:
> Yes, I had implemented a version of tls data destructors
This is not in this PR, however. I was referring to the data destructor in pthread_key_create().
int pthread_key_create(pthread_key_t *key, void (*destructor)(void*));
See https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html :
"An optional destructor function may be associated with each key value. At thread exit, if a key value has a non-NULL destructor pointer, and the thread has a non-NULL value associated with that key, the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument. The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits.
"If, after all the destructors have been called for all non-NULL values with associated destructors, there are still some non-NULL values with associated destructors, then the process is repeated. If, after at least {PTHREAD_DESTRUCTOR_ITERATIONS} iterations of destructor calls for outstanding non-NULL values, there are still some non-NULL values with associated destructors, implementations may stop calling destructors, or they may continue calling destructors until no non-NULL values with associated destructors exist, even though this might result in an infinite loop."
This would effect TLS where the destructors would be stored, pthread_key_create() which would need to save the destructor function pointers, and pthread_exit() that would call the pthread data destructors,
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623914937
##########
File path: arch/arm/src/armv7-a/arm_syscall.c
##########
@@ -290,19 +290,21 @@ uint32_t *arm_syscall(uint32_t *regs)
* R2 = arg
*/
-#if defined(CONFIG_BUILD_KERNEL) && !defined(CONFIG_DISABLE_PTHREAD)
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode. We need:
*
- * R0 = arg
- * PC = entrypt
+ * R0 = entrypt
+ * R1 = arg
+ * PC = startup
* CSPR = user mode
*/
Review comment:
In order to start a pthread in the the KERNEL mode, we will need to make sure that the proper application address space is selected. this should be doable by calling `group_addrenv(tcb)`. Otherwise, we might start the pthread in the address environment of some other process.
I don't see that being done. Hmm... maybe I am wrong. sched/pthread/pthread_create.c does do:
#ifdef CONFIG_ARCH_ADDRENV
/* Share the address environment of the parent task group. */
ret = up_addrenv_attach(ptcb->cmn.group, this_task());
if (ret < 0)
{
errcode = -ret;
goto errout_with_tcb;
}
#endif
Does that do the 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] patacongo commented on a change in pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623176905
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
The long term objective is to move (almost) all of the pthread logic out of the OS and into the C library. That is the standard partitioning for example used in GNU/Linux: Linux implements implements basic native threading but it is GLIBC where all of the pthread interfaces are implemented. NuttX should follow this same partitioning.
A key step to accomplishing that is to move all cancellation point logic into libs/libc/pthread. Many pthread APIs are very simple and just built on top of normal OS interfaces but cannot be moved into libs/libc/pthread because the current cancellation point logic works only in supervisor mode. Moving this to libs/libc/pthread would then enable moving almost all of the other pthread APIs there 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] patacongo commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228182
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -122,6 +115,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
> where SYS_pthread_exit
It is a normal system call. SYS_pthread_start is not. The start function is an internal chip-specific OS interface, SYS_nx_pthread_exit, is part of the common, normal user interface and you will find it in nuttx/syscall. The actual user interface pthread_exit() is a callable function in libs/libc/pthread and not a system call(). The whole point of the change was to get pthread_exit() to run in user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638226397
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
> should we keep the thumb bit?
I don't think that the Thumb bit should be set. This is the saved value of program counter and should not have bit 0 set. Bit 0 works only for call instructions (and a few other places in the ISA). We need to double check this.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642649599
##########
File path: arch/risc-v/src/rv64gc/riscv_swint.c
##########
@@ -282,33 +282,64 @@ int riscv_swint(int irq, FAR void *context, FAR void *arg)
break;
#endif
- /* R0=SYS_pthread_start: This a user pthread start
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
+ /* A0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
- * R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * A0 = SYS_pthread_start
+ * A1 = startup
+ * A2 = entrypt
+ * A3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_EPC] = (uintptr_t)regs[REG_A1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as is, but fails if this recommended change 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] no1wudi edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-842159148
> Looks good to me.
>
> Is this change now complete? What steps do we need to take to get this merged. We need some confidence that this does not break things irreparably. I expect some issues with code turmoil of this magnitude, but I think now is the right to to merge this since we just release 10.1.
The missing **SYS_pthread_exit** case is added to **arm_svcall**.
These patches mainly tested on risc-v (K210) platform, but shoud works on arm platform.
I'm testing them on cortex m4 platform, I will remove the Draft indication when it passed.
@xiaoxiang781216 Could you take a look at these patches please ?
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-854236721
>
>
> > For ELF binary, we have three case:
>
> I believe that the existing solution is superior because there is only a single, unified design that works the same in all configurations, differing only in the need to drop to user mode in some cases. I hope that we do not change that. That would be a mistake.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623001239
##########
File path: libs/libc/pthread/pthread_create.c
##########
@@ -24,70 +24,68 @@
#include <nuttx/config.h>
-#include <pthread.h>
-#include <assert.h>
+#include <debug.h>
-#include <nuttx/userspace.h>
-
-#if !defined(CONFIG_BUILD_FLAT) && !defined(__KERNEL__)
-
-/****************************************************************************
- * Pre-processor Definitions
- ****************************************************************************/
+#include <nuttx/pthread.h>
/****************************************************************************
- * Private Type Declarations
+ * Private Functions
****************************************************************************/
/****************************************************************************
- * Public Data
+ * Name: pthread_startup
+ *
+ * Description:
+ * This function is the user space pthread startup function. Its purpose
+ * is to catch the return from the pthread main function so that
+ * pthread_exit() can be called from user space
+ *
+ * Input Parameters:
+ * entry - The user-space address of the pthread entry point
+ * arg - Standard argument for the pthread entry point
+ *
+ * Returned Value:
+ * None. This function does not return.
+ *
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
+static void pthread_startup(pthread_startroutine_t entry,
+ pthread_addr_t arg)
+{
+ DEBUGASSERT(entry != NULL);
-/****************************************************************************
- * Private Function Prototypes
- ****************************************************************************/
+ /* Pass control to the thread entry point. Handle any returned value. */
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+ pthread_exit(entry(arg));
+}
/****************************************************************************
* Public Functions
****************************************************************************/
/****************************************************************************
- * Name: pthread_startup
+ * Name: pthread_create
Review comment:
```suggestion
* Name: pthread_create
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852169591
All entry point from kernel to userspace before this patch is defined in userspace_s:
https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/userspace.h#L105-L120
it make sense to expose pthread_exit through the same mechanism.
> > ```
> > 1. Move pthread_exit & pthread_create to USERSPACE to unify the convention that how to drop to userspace from kernel side like this:
> > `regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup`
> > `regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_exit`
> > ```
>
> This part I don't understand. That doesn't unify the designs, it makes them different: There is no USERSPACE in KERNEL mode. I don't understand why this is a good thing.
>
Kernel mode should use userspace_s too, the different between protected and kernel mode is how to locate this structure.
> This will also set bit 0 in the EPC. I don't think you want to do that, do you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638428183
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
> > should we keep the thumb bit?
>
> I don't think that the Thumb bit should be set. This is the saved value of program counter and should not have bit 0 set. Bit 0 works only for call instructions (and a few other places in the ISA). We need to double check this.
regs[REG_R1] isn't the saved program counter, but point to start(pthread_trampoline_t) function which should always have bit0 set in Cortex-M context.
>
> This is equivalent to the branch:
>
> ```
> mov Rx, PC
> ```
>
> This just jumps to the address in RX. This is the absolute address version of the relative branch B instruction. The B and BL instructions and BX and BLX instructions differ in that the BX and BLX instructions "interpret" bit 0 as the Thumb indication. That thumb indication is NOT written to the PC. The BX and BLX instructions use bit 0 to set the Thumb bit in the control/status register. See https://topic.alibabacloud.com/a/the-difference-between-the-assembly-jump-instruction-b-bl-bx-blx-and-bxj_8_8_10244895.html . Bit 0 of the PC should never be set and never indicates Thumb mode.
>
Yes, Reading from PC should return an even address, but function pointer or LR(by BL, BLX) will set bit0 for thumb more.
> I believe that setting bit 0 in the PC would cause an unaligned access failure.
No, PC modificaiton instrution move the bit0 to EPSR T-bit:
https://stackoverflow.com/questions/18655916/arm-cortex-m0-m3-m4why-pc-is-always-even-number-in-thumb-state
From the datasheet, we need ensure bit0 of new PC always 1 to avoid LOOKUP.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228077
##########
File path: arch/arm/src/armv6-m/svcall.h
##########
@@ -124,6 +116,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
> where define SYS_pthread_exit?
It is a normal system call. SYS_pthread_start is not. The start function is an internal chip-specific OS interface, SYS_pthread_exit (or is it SYS_nxpthread_exit?). Is part of the normal user interface.
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -122,6 +115,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
> where SYS_pthread_exit
It is a normal system call. SYS_pthread_start is not. The start function is an internal chip-specific OS interface, SYS_pthread_exit (or is it SYS_nxpthread_exit?). Is part of the normal user interface.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638972499
##########
File path: arch/risc-v/src/rv64gc/riscv_swint.c
##########
@@ -282,33 +282,64 @@ int riscv_swint(int irq, FAR void *context, FAR void *arg)
break;
#endif
- /* R0=SYS_pthread_start: This a user pthread start
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
+ /* A0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
- * R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * A0 = SYS_pthread_start
+ * A1 = startup
+ * A2 = entrypt
+ * A3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_EPC] = (uintptr_t)regs[REG_A1] & ~1; /* startup */
Review comment:
Does bit zero have any significance in RISC-V? Or is this just a copy paste error from when the RISC-V port was created?
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632765716
##########
File path: include/nuttx/tls.h
##########
@@ -216,6 +228,80 @@ int tls_set_value(int tlsindex, uintptr_t tlsvalue);
FAR struct tls_info_s *tls_get_info(void);
#endif
+/****************************************************************************
+ * Name: tls_set_dtor
+ *
+ * Description:
+ * Set the TLS element destructor associated with the 'tlsindex' to 'destr'
+ *
+ * Input Parameters:
+ * tlsindex - Index of TLS data destructor to set
+ * destr - The destr of TLS data element
+ *
+ * Returned Value:
+ * Zero is returned on success, a negated errno value is return on
+ * failure:
+ *
+ * EINVAL - tlsindex is not in range.
+ *
+ ****************************************************************************/
+
+#if CONFIG_TLS_NELEM > 0
+int tls_set_dtor(int tlsindex, tls_dtor_t destr);
+#endif
+
+/****************************************************************************
+ * Name: tls_get_dtor
+ *
+ * Description:
+ * Get the TLS element destructor associated with the 'tlsindex' to 'destr'
+ *
+ * Input Parameters:
+ * tlsindex - Index of TLS data destructor to get
+ *
+ * Returned Value:
+ * A non-null destruct function pointer.
+ *
+ ****************************************************************************/
+
+#if CONFIG_TLS_NELEM > 0
+tls_dtor_t tls_get_dtor(int tlsindex);
+#endif
+
+/****************************************************************************
+ * Name: tls_get_set
+ *
+ * Description:
+ * Get the TLS element index set map
+ *
+ * Input Parameters:
+ *
+ * Returned Value:
+ * A set of allocated TLS index
+ *
+ ****************************************************************************/
+
+#if CONFIG_TLS_NELEM > 0
+tls_ndxset_t tls_get_set(void);
+#endif
+
+/****************************************************************************
+ * Name: tls_destruct
+ *
+ * Description:
+ * Destruct all TLS data element associated with allocated key
+ *
+ * Input Parameters:
+ *
Review comment:
`None` may be added when there are no parameters
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228182
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -122,6 +115,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
> where SYS_pthread_exit
It is a normal system call. SYS_pthread_start is not. The start function is an internal chip-specific OS interface, SYS_nx_pthread_exit, is part of the common, normal user interface and you will find it in nuttx/syscall. The actual user interface pthread_exit() is a callable function in libs/libc/pthread and not a system call(). The whole point of the change was to get get pthread_exit() to run in user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642697110
##########
File path: arch/arm/src/armv8-m/arm_svcall.c
##########
@@ -311,34 +311,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as 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 a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642649599
##########
File path: arch/risc-v/src/rv64gc/riscv_swint.c
##########
@@ -282,33 +282,64 @@ int riscv_swint(int irq, FAR void *context, FAR void *arg)
break;
#endif
- /* R0=SYS_pthread_start: This a user pthread start
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
+ /* A0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
- * R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * A0 = SYS_pthread_start
+ * A1 = startup
+ * A2 = entrypt
+ * A3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_EPC] = (uintptr_t)regs[REG_A1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as is, but fails if this recommended change 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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638416602
##########
File path: arch/arm/src/armv6-m/svcall.h
##########
@@ -124,6 +116,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
But I saw all syscall handler e.g.:
https://github.com/apache/incubator-nuttx/pull/3626/files#diff-0f751c458200754b097958f7d3dd7ef9f8ac93d1ad4b4ea22ef61dffff311873R346-R352
add swich/case to handle SYS_pthread_exit. As my unsterstanding, both syscall is required:
1. Userspace issue SYS_nx_pthread_exit to trigger the thread exit process
2. Kernel issue SYS_pthread_exit to switch back to userspace and then repeat step 1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo merged pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-847990871
Has this change been verified on a PROTECTED and a KERNEL build? I think that is essential to know if some of these changes and the comments are correct.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228910
##########
File path: include/nuttx/sched.h
##########
@@ -504,33 +509,35 @@ struct task_group_s
#endif /* CONFIG_SCHED_HAVE_PARENT */
#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
- /* waitpid support ************************************************************/
+ /* waitpid support ********************************************************/
- /* Simple mechanism used only when there is no support for SIGCHLD */
+ /* Simple mechanism used only when there is no support for SIGCHLD */
- uint8_t tg_nwaiters; /* Number of waiters */
- uint8_t tg_waitflags; /* User flags for waitpid behavior */
- sem_t tg_exitsem; /* Support for waitpid */
- FAR int *tg_statloc; /* Location to return exit status */
+ uint8_t tg_nwaiters; /* Number of waiters */
+ uint8_t tg_waitflags; /* User flags for waitpid behavior */
+ sem_t tg_exitsem; /* Support for waitpid */
+ FAR int *tg_statloc; /* Location to return exit status */
#endif
#ifndef CONFIG_DISABLE_PTHREAD
- /* Pthreads *******************************************************************/
+ /* Pthreads ***************************************************************/
- /* Pthread join Info: */
+ /* Pthread join Info: */
- sem_t tg_joinsem; /* Mutually exclusive access to join data */
- FAR struct join_s *tg_joinhead; /* Head of a list of join data */
- FAR struct join_s *tg_jointail; /* Tail of a list of join data */
+ sem_t tg_joinsem; /* Mutually exclusive access to join data */
+ FAR struct join_s *tg_joinhead; /* Head of a list of join data */
+ FAR struct join_s *tg_jointail; /* Tail of a list of join data */
#endif
- /* Thread local storage *******************************************************/
+ /* Thread local storage ***************************************************/
#if CONFIG_TLS_NELEM > 0
- tls_ndxset_t tg_tlsset; /* Set of TLS data indexes allocated */
+ tls_ndxset_t tg_tlsset; /* Set of TLS indexes allocated */
+
+ tls_dtor_t tg_tlsdestr[CONFIG_TLS_NELEM]; /* List of TLS destructors */
Review comment:
> why not save to task_info_s?
Because the destructor is common for all pthreads. It is saved with the pthread data key is created and used on all threads. So it is a group thing. It could go in task_info_s, however (But the all of the group data could go into task_info_s for that matter). I would only move it if it can save system call and if the job can be done with no critical section. Critical sections can only be used within the OS in supervisor mode.
--
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] no1wudi edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829783428
> pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit() from within the OS in supervisor mode. This is a problem.
>
> In the original code before your change there are these:
>
> ```
> sched/pthread/pthread_create.c: pthread_exit(exit_status);
> sched/signal/sig_default.c: pthread_exit(NULL);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
> ```
>
> We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved. See full comments for a suggested solution.
>
> In my point of view, we could probably just open another PR for these remaining issues and merge as is. We are still making a positive step forward.
If replace all pthread_exit() with nx_pthread_exit(), there would be a memory leak that cleanup function (include future ptherad_key_create destructors) not executed correctly.
I'll try this solution:
> Possible solution:
> Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be
> >known in the PROTECTED and KERNEL builds. I think you were not seeing the build failure in the PROTECTED/KERNEL builds > because cancellation points and default signal actions were not enabled.
> Save the pthread_exit() pointer in the TCB
> Replace calls to pthread_exit() in the OS to up_pthread_exit().
> up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
or another possible solution:
Replace these pthread_exit with by send SIG_ABRT, dorp to user space with sig action and call pthread_exit in signal handler ?
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-842152327
@gustavonihei Thanks for your suggestion !
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853531956
> > After more thinking, there is a structure like userspace_s for kernel mode, but name it as addrenv_reserve_s:
> > [incubator-nuttx/include/nuttx/addrenv.h](https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/addrenv.h#L248-L254)
>
> I am unsure if addrenv_reserve_s can be used for this purpose or not. A KERNEL mode process is basically an ELF module with an address environment. addrenv_reserve_s currently only exists if the module has an address environment and it is located only via the fixed virtual address of .data in all processes.
>
> What about other uses of ELF modules that don't have address environments, addrenv_reserve_s will not be avilable for them and even if it were, the OS could not locate them because the physical .data address is not easily known. If the pthread and task startup and exit functions are linked into the ELF module, then there is not way to access them externally. I think there are other complexities and corner cases that you are not addressing.
For ELF binary, we have three case:
1. ELF binary in FLAT build, it simplify to the direct function call
2. ELF binary in PROTECTED build, switch to userspace by userspace_s
3. ELF binary in KERNEL build, switch to userspace by addrenv_reserve_s
There isn't difference from ELF binary(posix_spawn) and builtin binary(task_spawn) after binfmt finish load and parse ELF file.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632752411
##########
File path: arch/or1k/src/common/up_pthread_start.c
##########
@@ -66,7 +67,8 @@ void up_pthread_start(pthread_startroutine_t entrypt, pthread_addr_t arg)
Review comment:
Documentatio on the line above should refer to `sys_call3`
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228910
##########
File path: include/nuttx/sched.h
##########
@@ -504,33 +509,35 @@ struct task_group_s
#endif /* CONFIG_SCHED_HAVE_PARENT */
#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
- /* waitpid support ************************************************************/
+ /* waitpid support ********************************************************/
- /* Simple mechanism used only when there is no support for SIGCHLD */
+ /* Simple mechanism used only when there is no support for SIGCHLD */
- uint8_t tg_nwaiters; /* Number of waiters */
- uint8_t tg_waitflags; /* User flags for waitpid behavior */
- sem_t tg_exitsem; /* Support for waitpid */
- FAR int *tg_statloc; /* Location to return exit status */
+ uint8_t tg_nwaiters; /* Number of waiters */
+ uint8_t tg_waitflags; /* User flags for waitpid behavior */
+ sem_t tg_exitsem; /* Support for waitpid */
+ FAR int *tg_statloc; /* Location to return exit status */
#endif
#ifndef CONFIG_DISABLE_PTHREAD
- /* Pthreads *******************************************************************/
+ /* Pthreads ***************************************************************/
- /* Pthread join Info: */
+ /* Pthread join Info: */
- sem_t tg_joinsem; /* Mutually exclusive access to join data */
- FAR struct join_s *tg_joinhead; /* Head of a list of join data */
- FAR struct join_s *tg_jointail; /* Tail of a list of join data */
+ sem_t tg_joinsem; /* Mutually exclusive access to join data */
+ FAR struct join_s *tg_joinhead; /* Head of a list of join data */
+ FAR struct join_s *tg_jointail; /* Tail of a list of join data */
#endif
- /* Thread local storage *******************************************************/
+ /* Thread local storage ***************************************************/
#if CONFIG_TLS_NELEM > 0
- tls_ndxset_t tg_tlsset; /* Set of TLS data indexes allocated */
+ tls_ndxset_t tg_tlsset; /* Set of TLS indexes allocated */
+
+ tls_dtor_t tg_tlsdestr[CONFIG_TLS_NELEM]; /* List of TLS destructors */
Review comment:
> why not save to task_info_s?
Because the destructor is common for all pthreads. It is saved with the pthread data key is created and used on all threads. So it is a group thing. It could go in task_info_s, however IBut the all of the group data could go into task_info_s for that matter).
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632765892
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,65 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+#include <nuttx/tls.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_value
Review comment:
```suggestion
* exit_value - The pointer of the pthread exit parameter
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638226530
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
> should we keep the thumb bit?
I don't think that the Thumb bit should be set. This is the saved value of program counter and should not have bit 0 set. Bit 0 works only for call instructions (and a few other places in the ISA). We need to double check this.
This is equivalent to the branch:
mov Rx, PC
This just jumps to the address in RX. This is the absolute address version of the relative branch B instruction. The B and BL instructions and BX and BLX instructions differ in that the BX and BLX instructions "interpret" bit 0 as the Thumb indication. That thumb indication is NOT written to the PC. The BX and BLX instructions use to set the Thumb bit in the control/status register. See https://topic.alibabacloud.com/a/the-difference-between-the-assembly-jump-instruction-b-bl-bx-blx-and-bxj_8_8_10244895.html . Bit 0 of the PC should never be set and never indicates Thumb mode.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623089498
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
+#ifdef CONFIG_PTHREAD_CLEANUP
+ int cnt;
+ struct pthread_cleanup_s cleanup[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+ cnt = pthread_cleanup_poplist(cleanup);
+
+ sched_lock();
+ while (cnt-- > 0)
+ {
+ struct pthread_cleanup_s cp = cleanup[cnt];
+ if (cp.pc_cleaner)
+ cp.pc_cleaner(cp.pc_arg);
+ }
+
+ sched_unlock();
+#endif
+
Review comment:
Just a note: Here is where we would need to add support for pthread-specific data destructors as well.
This is beyond the scope of the current PR, however.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852252646
>
>
> Will you be verifying the operation of the KERNEL build? Do you have a configuration and hardware you can use? Currently configurations are only available for the sama5d4-ek board. I have talked to several end-users who are running KERNEL mode on their custom hardware but only sama5d4-ek is supported in the repository. The i.MX6 is another possibility in the repository.
There are others that I forgot about:
1. Allwinner A10 (Cortex-A8) on pcDuino. The A10 is not a well supported architecture.
2. TI AM335x (Cortex-A8) on Beagleboard,
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642697053
##########
File path: arch/arm/src/armv8-m/arm_svcall.c
##########
@@ -311,34 +311,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as 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] gustavonihei commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r626638521
##########
File path: arch/risc-v/src/common/riscv_pthread_start.c
##########
@@ -60,11 +61,13 @@
*
****************************************************************************/
-void up_pthread_start(pthread_startroutine_t entrypt, pthread_addr_t arg)
+void up_pthread_start(pthread_trampoline_t startup,
+ pthread_startroutine_t entrypt, pthread_addr_t arg)
{
/* Let sys_call2() do all of the work */
Review comment:
```suggestion
/* Let sys_call3() do all of the work */
```
--
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] no1wudi commented on a change in pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623106058
##########
File path: libs/libc/pthread/pthread_create.c
##########
@@ -24,70 +24,68 @@
#include <nuttx/config.h>
-#include <pthread.h>
-#include <assert.h>
+#include <debug.h>
-#include <nuttx/userspace.h>
-
-#if !defined(CONFIG_BUILD_FLAT) && !defined(__KERNEL__)
-
-/****************************************************************************
- * Pre-processor Definitions
- ****************************************************************************/
+#include <nuttx/pthread.h>
/****************************************************************************
- * Private Type Declarations
+ * Private Functions
****************************************************************************/
/****************************************************************************
- * Public Data
+ * Name: pthread_startup
+ *
+ * Description:
+ * This function is the user space pthread startup function. Its purpose
+ * is to catch the return from the pthread main function so that
+ * pthread_exit() can be called from user space
+ *
+ * Input Parameters:
+ * entry - The user-space address of the pthread entry point
+ * arg - Standard argument for the pthread entry point
+ *
+ * Returned Value:
+ * None. This function does not return.
+ *
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
+static void pthread_startup(pthread_startroutine_t entry,
+ pthread_addr_t arg)
+{
+ DEBUGASSERT(entry != NULL);
-/****************************************************************************
- * Private Function Prototypes
- ****************************************************************************/
+ /* Pass control to the thread entry point. Handle any returned value. */
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+ pthread_exit(entry(arg));
+}
/****************************************************************************
* Public Functions
****************************************************************************/
/****************************************************************************
- * Name: pthread_startup
+ * Name: pthread_create
Review comment:
I will fix the document issue later
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852225041
> These changes had been verified on stm32 and k210
Were these only FLAT builds? You will need to extend the testing to verify the PROTECTED and KERNEL modes. The changes you and Xiao propose here apply only to PROTECTED and KERNEL modes and require verification in those modes.
The configurations are PROTECTED mode builds:
$ find boards/ -name defconfig | xargs grep -l CONFIG_BUILD_PROTECTED=y
boards/arm/imxrt/imxrt1050-evk/configs/knsh/defconfig
boards/arm/imxrt/imxrt1060-evk/configs/knsh/defconfig
boards/arm/imxrt/imxrt1064-evk/configs/knsh/defconfig
boards/arm/lc823450/lc823450-xgevk/configs/knsh/defconfig
boards/arm/lc823450/lc823450-xgevk/configs/kostest/defconfig
boards/arm/lc823450/lc823450-xgevk/configs/krndis/defconfig
boards/arm/lpc17xx_40xx/lpc4088-devkit/configs/knsh/defconfig
boards/arm/lpc17xx_40xx/lpc4088-quickstart/configs/knsh/defconfig
boards/arm/lpc17xx_40xx/open1788/configs/knsh/defconfig
boards/arm/lpc17xx_40xx/open1788/configs/knxterm/defconfig
boards/arm/lpc17xx_40xx/pnev5180b/configs/knsh/defconfig
boards/arm/lpc43xx/bambino-200e/configs/knsh/defconfig
boards/arm/sam34/sam3u-ek/configs/knsh/defconfig
boards/arm/samv7/samv71-xult/configs/knsh/defconfig
boards/arm/stm32/clicker2-stm32/configs/knsh/defconfig
boards/arm/stm32/mikroe-stm32f4/configs/kostest/defconfig
boards/arm/stm32/olimex-stm32-p407/configs/kelf/defconfig
boards/arm/stm32/olimex-stm32-p407/configs/kmodule/defconfig
boards/arm/stm32/olimex-stm32-p407/configs/knsh/defconfig
boards/arm/stm32/stm3240g-eval/configs/knxwm/defconfig
boards/arm/stm32/stm32f4discovery/configs/kostest/defconfig
boards/arm/stm32l4/stm32l476vg-disco/configs/knsh/defconfig
boards/arm/stm32l4/stm32l4r9ai-disco/configs/knsh/defconfig
boards/arm/tiva/lm3s6965-ek/configs/qemu-protected/defconfig
boards/risc-v/c906/smartl-c906/configs/knsh/defconfig
boards/risc-v/k210/maix-bit/configs/knsh/defconfig
boards/risc-v/k210/maix-bit/configs/knsh_smp/defconfig
boards/risc-v/k210/maix-bit/configs/kostest/defconfig
The kostest configurations are recommended because they really exercise the threading more than any of the others.
For KERNEL mode, as I mentioned above, there is only:
$ find boards/ -name defconfig | xargs grep -l CONFIG_BUILD_KERNEL=y
boards/arm/sama5/sama5d4-ek/configs/knsh/defconfig
In both cases, there are some complexities in building an writing the code to FLASH. This should be described in the README file for each configuration for each board.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852143714
> 1. Move pthread_exit & pthread_create to USERSPACE to unify the convention that how to drop to userspace from kernel side like this:
> `regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup`
> `regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_exit`
This part I don't understand. That doesn't unify the designs, it makes them different: There is no USERSPACE in KERNEL mode. I don't understand why this is a good thing.
This will also set bit 0 in the EPC. I don't think you want to do that, do you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852173304
userspace_s is only used in PROTECTED mode. It is not used in KERNEL mode and can never be. In KERNEL mode, all tasks are ELF models and do not expose anything like userspace_s. In KERNEL mode we will have to stick with the existing implementation.
The best solution is to unify the designs and have one solution. The PROTECTED mode can use the KERNEL mode solution which is implemented now. But the KERNEL mode cannot use the PROTECTED mode solution proposed here. I think it is a very bad idea. Best to stay with a unified design.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623407997
##########
File path: syscall/syscall.csv
##########
@@ -83,14 +84,14 @@
"pselect","sys/select.h","","int","int","FAR fd_set *","FAR fd_set *","FAR fd_set *","FAR const struct timespec *","FAR const sigset_t *"
"pthread_cancel","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
"pthread_cleanup_pop","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","int"
+"pthread_cleanup_poplist","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","int","struct pthread_cleanup_s *"
"pthread_cleanup_push","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","pthread_cleanup_t","FAR void *"
Review comment:
pthread_cleanup_poplist currently uses data in the TCB to save the cleanup info. That is a problem since you have to trap into supervisor mode to access the TCB.
That cleanup data needs to be move to TLS and pthread_cleanup_poplist() needs to be moved to libs/libc/pthread. This should all be done in user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-855882718
> For ELF binary, we have three case:
>
> 1. ELF binary in FLAT build, it simplify to the direct function call
>
> 2. ELF binary in PROTECTED build, switch to userspace by userspace_s
>
> 3. ELF binary in KERNEL build, switch to userspace by addrenv_reserve_s
One thing that I really dislike about using function tables is that they can lead to code bloat. Initialing the function pointers in the function tables draws those functions into the build, whether they are used or not.
This is only a minor problem for PROTECTED mode since there is only a single large blob, but would be a serious problem for KERNEL build where there may be many, small processes. For example, many processes will not use pthreads, but adding the pthread function points to the addrenv_reserve_s structure would cause the entire functionality to be drawn into every process (we do not as yet have shared libraries). So that needs to be avoided.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228182
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -122,6 +115,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
> where SYS_pthread_exit
It is a normal system call. SYS_pthread_start is not. The start function is an internal chip-specific OS interface, SYS_nx_pthread_exit, is part of the common, normal user interface and you will find it in nuttx/syscall.
--
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 pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-841483300
> Review from @gustavonihei has also been requested. I also need to wait for his comments.
I am quite new to this part of the OS, so I am trying to catch up to the development.
Besides some parts of the documentation still not reflecting the latest changes, I believe there is one possible register confusion on the ARMv8-M 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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-851760149
> @no1wudi Has this change been verified on a PROTECTED and a KERNEL build? I think that is essential to know if some of these changes and the comments are correct. If any of your comments are correct, then those builds are currently broken.
These changes had beed verified on stm32 and k210, I'll continue this work according to the latest suggestions from @xiaoxiang781216 and @patacongo :
1. Move pthread_exit & pthread_create to USERSPACE to unify the convention that how to drop to userspace from kernel side like this:
`regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup`
`regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_exit`
2. Move tls destructor to user space (task_info_s)
3. Add SYS_pthread_exit syscall like SYS_pthread_create
4. Move TCB_FLAG_CANCEL_DOING/TCB_FLAG_CANCEL_PENDING to tls_info_s
5. Move task_setcancelstate to libc
6. Minor changes from 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] gustavonihei commented on a change in pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623002061
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
Review comment:
```suggestion
* exit_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] patacongo commented on a change in pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623128090
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
+#ifdef CONFIG_PTHREAD_CLEANUP
+ int cnt;
+ struct pthread_cleanup_s cleanup[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+ cnt = pthread_cleanup_poplist(cleanup);
+
+ sched_lock();
+ while (cnt-- > 0)
+ {
+ struct pthread_cleanup_s cp = cleanup[cnt];
+ if (cp.pc_cleaner)
+ cp.pc_cleaner(cp.pc_arg);
+ }
+
+ sched_unlock();
+#endif
+
Review comment:
> Yes, I had implemented a version of tls data destructors
This is not in this PR, however. I was referring to the data destructor in pthread_key_create(). See https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html :
"An optional destructor function may be associated with each key value. At thread exit, if a key value has a non-NULL destructor pointer, and the thread has a non-NULL value associated with that key, the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument. The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits.
"If, after all the destructors have been called for all non-NULL values with associated destructors, there are still some non-NULL values with associated destructors, then the process is repeated. If, after at least {PTHREAD_DESTRUCTOR_ITERATIONS} iterations of destructor calls for outstanding non-NULL values, there are still some non-NULL values with associated destructors, implementations may stop calling destructors, or they may continue calling destructors until no non-NULL values with associated destructors exist, even though this might result in an infinite loop."
This would effect TLS where the destructors would be stored, pthread_key_create() which would need to save the destructor function pointers, and pthread_exit() that would call the pthread data destructors,
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623407997
##########
File path: syscall/syscall.csv
##########
@@ -83,14 +84,14 @@
"pselect","sys/select.h","","int","int","FAR fd_set *","FAR fd_set *","FAR fd_set *","FAR const struct timespec *","FAR const sigset_t *"
"pthread_cancel","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
"pthread_cleanup_pop","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","int"
+"pthread_cleanup_poplist","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","int","struct pthread_cleanup_s *"
"pthread_cleanup_push","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","pthread_cleanup_t","FAR void *"
Review comment:
pthread_cleanup_poplist currently uses data in the TCB to save the cleanup info. That is a problem since you have to trap into supervisor mode to access the TCB.
That cleanup data needs to be move to TLS and pthread_cleanup_poplist() needs to be moved to libs/libc/pthread. Then this can all be done in user space.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623136987
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit(). In the original code before your change there are these:
sched/pthread/pthread_create.c: pthread_exit(exit_status);
sched/signal/sig_default.c: pthread_exit(NULL);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
I think this is an error. We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved.
Possible solution:
1. Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be known in the PROTECTED and KERNEL builds. I think you were not seeing the build failure in the PROTECTED/KERNEL builds because cancellation points and default signal actions were not enabled.
2. Save the pthread_exit() pointer in the TCB
3. Replace calls to pthread_exit() in the OS to up_pthread_exit().
4. up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829332834
> And, now os call the pthread_cleanup from user space in pthread_exit, but in pthread_cancel they are still called from kernel side, we still need to move cancellation point in syscall to user space.
Our messages are crossing each other in time. I just pointed that out too. Cancellation is a problem and also logic in the default signal actions.
Thanks for working on this. I think this is important for the long term health of the OS but I have not been motivated to work on it very much do to health reasons. I appreciate that you are taking this on. It really is an interesting project. There are a couple of related issues that might also interest you in Issues #1263 and #1265
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852233010
>
>
> > Were these only FLAT builds? You will need to extend the testing to verify the PROTECTED and KERNEL modes. The changes you and Xiao propose here apply only to PROTECTED and KERNEL modes and require verification in those modes.
>
> @patacongo They were verified on both FLAT and PROTECTED mode by:
>
> _boards/risc-v/k210/maix-bit/configs/kostest/defconfig boards/arm/stm32/stm32f4discovery/configs/kostest/defconfig_
>
> And passed the ostest example.
Great! Currently, the kostest is broken on master. There must have been some change to vfork() recently because vfork() now hardfaults on master. See #3812
If it ran for you than I assume this breakage is recent.
We do need to verify KERNEL mode too, especially if we made the change that I don't like to the KERNEL mode address environment.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632750634
##########
File path: arch/arm/src/armv7-r/arm_syscall.c
##########
@@ -285,19 +285,21 @@ uint32_t *arm_syscall(uint32_t *regs)
* R2 = arg
Review comment:
The documentation needs to be update here and on the similar place on other files
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-842159148
> Looks good to me.
>
> Is this change now complete? What steps do we need to take to get this merged. We need some confidence that this does not break things irreparably. I expect some issues with code turmoil of this magnitude, but I think now is the right to to merge this since we just release 10.1.
The missing **SYS_pthread_exit** case is added to **arm_svcall**.
These patches mainly tested on risc-v (K210) platform, but shoud works on arm platform.
I'm testing them on cortex m4 platform, I will remove the Draft indication when it passed.
@xiaoxiang781216 Could you take a look at theses patches please ?
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632738062
##########
File path: arch/arm/src/armv7-a/svcall.h
##########
@@ -105,6 +98,24 @@
#define SYS_signal_handler_return (5)
+#endif /* !CONFIG_BUILD_FLAT */
+
+/* SYS call 3:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (3)
+
+/* SYS call 8:
Review comment:
```suggestion
/* SYS call 6:
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228077
##########
File path: arch/arm/src/armv6-m/svcall.h
##########
@@ -124,6 +116,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
> where define SYS_pthread_exit?
It is a normal system call. SYS_pthread_start is not. The start function is an internal chip-specific OS interface, SYS_nx_pthread_exit, is part of the common, normal user interface and you will find it in nuttx/syscall. The actual user interface pthread_exit() is a callable function in libs/libc/pthread and not a system call(). The whole point of the change was to get get pthread_exit() to run in user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852442374
> These changes had been verified on stm32 and k210, I'll continue this work according to the latest suggestions from @xiaoxiang781216 and @patacongo :
Not on your list of things was the similar changes to call the on_exit() and atexit() callbacks when a task exits. In order to resolve #1263, we still have to address those security violations as well. Can we add these?
The things on your list are not essential, mostly NIH design changes that add nothing new. The on_exit() and atexit() callbacks are important security bugs that need to be fixed.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852216681
Will you be verifying the operation of the KERNEL build? Do you have a configuration and hardware you can use? Currently configurations are only available for the sama5d4-ek board. I have talked to several end-users who are running KERNEL mode on their custom hardware but only sama5d4-ek is supported in the repository. The i.MX6 is another possibility in the repository.
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852230263
> Were these only FLAT builds? You will need to extend the testing to verify the PROTECTED and KERNEL modes. The changes you and Xiao propose here apply only to PROTECTED and KERNEL modes and require verification in those modes.
@patacongo They were verified on both FLAT and PROTECTED mode by:
_boards/risc-v/k210/maix-bit/configs/kostest/defconfig
boards/arm/stm32/stm32f4discovery/configs/kostest/defconfig_
And passed the ostest example.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623914937
##########
File path: arch/arm/src/armv7-a/arm_syscall.c
##########
@@ -290,19 +290,21 @@ uint32_t *arm_syscall(uint32_t *regs)
* R2 = arg
*/
-#if defined(CONFIG_BUILD_KERNEL) && !defined(CONFIG_DISABLE_PTHREAD)
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode. We need:
*
- * R0 = arg
- * PC = entrypt
+ * R0 = entrypt
+ * R1 = arg
+ * PC = startup
* CSPR = user mode
*/
Review comment:
In order to start a pthread in the the KERNEL mode, we will need to make sure that the proper application address space is selected. this should be doable by calling `group_addrenv(tcb)`. Otherwise, we might start the pthread in the address environment of some other process.
I don't see that being done. Hmm... maybe I am wrong. sched/pthread/pthread_create.c does do:
#ifdef CONFIG_ARCH_ADDRENV
/* Share the address environment of the parent task group. */
ret = up_addrenv_attach(ptcb->cmn.group, this_task());
if (ret < 0)
{
errcode = -ret;
goto errout_with_tcb;
}
#endif
Does that do the job? Yes, I think so. Nevermind. group_addrenv() will be called when the pthread runs for the first time.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853230629
How about reuse the signal infrastructure switch from kernel to userspace?
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642694991
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -122,6 +115,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as is, but fails if this recommended change 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] patacongo commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228910
##########
File path: include/nuttx/sched.h
##########
@@ -504,33 +509,35 @@ struct task_group_s
#endif /* CONFIG_SCHED_HAVE_PARENT */
#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
- /* waitpid support ************************************************************/
+ /* waitpid support ********************************************************/
- /* Simple mechanism used only when there is no support for SIGCHLD */
+ /* Simple mechanism used only when there is no support for SIGCHLD */
- uint8_t tg_nwaiters; /* Number of waiters */
- uint8_t tg_waitflags; /* User flags for waitpid behavior */
- sem_t tg_exitsem; /* Support for waitpid */
- FAR int *tg_statloc; /* Location to return exit status */
+ uint8_t tg_nwaiters; /* Number of waiters */
+ uint8_t tg_waitflags; /* User flags for waitpid behavior */
+ sem_t tg_exitsem; /* Support for waitpid */
+ FAR int *tg_statloc; /* Location to return exit status */
#endif
#ifndef CONFIG_DISABLE_PTHREAD
- /* Pthreads *******************************************************************/
+ /* Pthreads ***************************************************************/
- /* Pthread join Info: */
+ /* Pthread join Info: */
- sem_t tg_joinsem; /* Mutually exclusive access to join data */
- FAR struct join_s *tg_joinhead; /* Head of a list of join data */
- FAR struct join_s *tg_jointail; /* Tail of a list of join data */
+ sem_t tg_joinsem; /* Mutually exclusive access to join data */
+ FAR struct join_s *tg_joinhead; /* Head of a list of join data */
+ FAR struct join_s *tg_jointail; /* Tail of a list of join data */
#endif
- /* Thread local storage *******************************************************/
+ /* Thread local storage ***************************************************/
#if CONFIG_TLS_NELEM > 0
- tls_ndxset_t tg_tlsset; /* Set of TLS data indexes allocated */
+ tls_ndxset_t tg_tlsset; /* Set of TLS indexes allocated */
+
+ tls_dtor_t tg_tlsdestr[CONFIG_TLS_NELEM]; /* List of TLS destructors */
Review comment:
> why not save to task_info_s?
Because the destructor is common for all pthreads. It is saved with the pthread data key is created and used on all threads. So it is a group thing. It could go in task_info_s, however.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623001721
##########
File path: include/nuttx/pthread.h
##########
@@ -121,6 +125,65 @@ EXTERN const pthread_attr_t g_default_pthread_attr;
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: nx_pthread_create
+ *
+ * Description:
+ * This function creates and activates a new thread with specified
+ * attributes.
+ *
+ * Input Parameters:
+ * trampoline
+ * thread
+ * attr
+ * start_routine
+ * arg
+ *
+ * Returned Value:
+ * OK (0) on success; a (non-negated) errno value on failure. The errno
+ * variable is not set.
+ *
+ ****************************************************************************/
+
+int nx_pthread_create(pthread_trampoline_t trampoline, FAR pthread_t *thread,
+ FAR const pthread_attr_t *attr,
+ pthread_startroutine_t entry, pthread_addr_t arg);
+
+/****************************************************************************
+ * Name: nx_pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
Review comment:
Missing documentation
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-842152327
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829256289
I think there is an error in the title. Did you mean stuff instead of staff?
There is logic on the branch feature/pthread-user that also implements moving pthread_create and pthread_exit to libs/libc/pthread. See https://github.com/apache/incubator-nuttx/tree/feature/pthread-user and PR #1328.
```
The primary objective here was:
1. To eliminate security problems when pthread_cleanup_popall() is called when the pthread exits: See Issue #1263 and #1328. And
2. Implement pthread thread-specific data destructors (mentioned in #1328)
It looks to me like this PR, takes much of its logic from that feature/pthread-user branch. If/when this PR is merged, I will remove the branch, and close the issue and PR.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623407997
##########
File path: syscall/syscall.csv
##########
@@ -83,14 +84,14 @@
"pselect","sys/select.h","","int","int","FAR fd_set *","FAR fd_set *","FAR fd_set *","FAR const struct timespec *","FAR const sigset_t *"
"pthread_cancel","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
"pthread_cleanup_pop","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","int"
+"pthread_cleanup_poplist","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","int","struct pthread_cleanup_s *"
"pthread_cleanup_push","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","pthread_cleanup_t","FAR void *"
Review comment:
pthread_cleanup_poplist currently uses data in the TCB to save the cleanup info. That is a problem.
That data needs to be move to TLS and pthread_cleanup_poplist() needs to be moved to libs/libc/pthread. This should all be done in user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632749557
##########
File path: arch/arm/src/armv7-m/arm_svcall.c
##########
@@ -324,22 +324,22 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
* R2 = arg
Review comment:
Needs to update the documentation here
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623001475
##########
File path: include/nuttx/pthread.h
##########
@@ -121,6 +125,65 @@ EXTERN const pthread_attr_t g_default_pthread_attr;
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: nx_pthread_create
+ *
+ * Description:
+ * This function creates and activates a new thread with specified
+ * attributes.
+ *
+ * Input Parameters:
+ * trampoline
+ * thread
+ * attr
+ * start_routine
+ * arg
+ *
+ * Returned Value:
+ * OK (0) on success; a (non-negated) errno value on failure. The errno
+ * variable is not set.
+ *
+ ****************************************************************************/
+
+int nx_pthread_create(pthread_trampoline_t trampoline, FAR pthread_t *thread,
+ FAR const pthread_attr_t *attr,
+ pthread_startroutine_t entry, pthread_addr_t arg);
+
+/****************************************************************************
+ * Name: nx_pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
Review comment:
```suggestion
* exit_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] gustavonihei commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632750634
##########
File path: arch/arm/src/armv7-r/arm_syscall.c
##########
@@ -285,19 +285,21 @@ uint32_t *arm_syscall(uint32_t *regs)
* R2 = arg
Review comment:
The documentation needs to be update here
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623914937
##########
File path: arch/arm/src/armv7-a/arm_syscall.c
##########
@@ -290,19 +290,21 @@ uint32_t *arm_syscall(uint32_t *regs)
* R2 = arg
*/
-#if defined(CONFIG_BUILD_KERNEL) && !defined(CONFIG_DISABLE_PTHREAD)
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode. We need:
*
- * R0 = arg
- * PC = entrypt
+ * R0 = entrypt
+ * R1 = arg
+ * PC = startup
* CSPR = user mode
*/
Review comment:
In order to start a pthread in the the KERNEL mode, we will need to make sure that the proper application address space is selected. this should be doable by calling `group_addrenv(tcb)`. Otherwise, we might start the pthread in the address environment of some other process.
I don't see that being done. Hmm... maybe I am wrong. sched/pthread/pthread_create.c does do:
#ifdef CONFIG_ARCH_ADDRENV
/* Share the address environment of the parent task group. */
ret = up_addrenv_attach(ptcb->cmn.group, this_task());
if (ret < 0)
{
errcode = -ret;
goto errout_with_tcb;
}
#endif
Does that do the job? Yes, I think so. Nevermind.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623089498
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
+#ifdef CONFIG_PTHREAD_CLEANUP
+ int cnt;
+ struct pthread_cleanup_s cleanup[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+ cnt = pthread_cleanup_poplist(cleanup);
+
+ sched_lock();
+ while (cnt-- > 0)
+ {
+ struct pthread_cleanup_s cp = cleanup[cnt];
+ if (cp.pc_cleaner)
+ cp.pc_cleaner(cp.pc_arg);
+ }
+
+ sched_unlock();
+#endif
+
Review comment:
Just a note: Here is where we would need to add support for pthread-specific data destructors as well.
##########
File path: libs/libc/pthread/pthread_create.c
##########
@@ -24,70 +24,68 @@
#include <nuttx/config.h>
-#include <pthread.h>
-#include <assert.h>
+#include <debug.h>
-#include <nuttx/userspace.h>
-
-#if !defined(CONFIG_BUILD_FLAT) && !defined(__KERNEL__)
-
-/****************************************************************************
- * Pre-processor Definitions
- ****************************************************************************/
+#include <nuttx/pthread.h>
/****************************************************************************
- * Private Type Declarations
+ * Private Functions
****************************************************************************/
/****************************************************************************
- * Public Data
+ * Name: pthread_startup
+ *
+ * Description:
+ * This function is the user space pthread startup function. Its purpose
+ * is to catch the return from the pthread main function so that
+ * pthread_exit() can be called from user space
+ *
+ * Input Parameters:
+ * entry - The user-space address of the pthread entry point
+ * arg - Standard argument for the pthread entry point
+ *
+ * Returned Value:
+ * None. This function does not return.
+ *
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
+static void pthread_startup(pthread_startroutine_t entry,
+ pthread_addr_t arg)
+{
+ DEBUGASSERT(entry != NULL);
-/****************************************************************************
- * Private Function Prototypes
- ****************************************************************************/
+ /* Pass control to the thread entry point. Handle any returned value. */
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+ pthread_exit(entry(arg));
+}
/****************************************************************************
* Public Functions
****************************************************************************/
/****************************************************************************
- * Name: pthread_startup
+ * Name: pthread_create
*
* Description:
- * This function is the user-space, pthread startup function. It is called
- * from up_pthread_start() in user-mode.
+ * This function creates and activates a new thread with specified
+ * attributes. It is simply a wrapper around the nx_pthread_create system
+ * call.
*
* Input Parameters:
- * entrypt - The user-space address of the pthread entry point
- * arg - Standard argument for the pthread entry point
+ * thread
+ * attr
+ * pthread_entry
+ * arg
Review comment:
It looks like @gustavonihei 's comments about Missing documentation would apply here 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] patacongo commented on pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829601604
build is failing because of:
stubs/STUB_nx_pthread_exit.c: In function 'STUB_nx_pthread_exit':
Error: stubs/STUB_nx_pthread_exit.c:12:1: error: control reaches end of non-void function [-Werror=return-type]
12 | }
| ^
I think that the fix is in syscall.csv as for other no return functions:
syscall.csv:"_exit","unistd.h","","noreturn","int"
syscall.csv:"exit","stdlib.h","","noreturn","int"
syscall.csv:"pthread_exit","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
vs.
"nx_pthread_create","nuttx/pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_trampoline_t","FAR pthread_t *","FAR const pthread_attr_t *","pthread_startroutine_t","pthread_addr_t"
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638226530
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
> should we keep the thumb bit?
I don't think that the Thumb bit should be set. This is the saved value of program counter and should not have bit 0 set. Bit 0 works only for call instructions (and a few other places in the ISA). We need to double check this.
--
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] no1wudi commented on a change in pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623550780
##########
File path: syscall/syscall.csv
##########
@@ -83,14 +84,14 @@
"pselect","sys/select.h","","int","int","FAR fd_set *","FAR fd_set *","FAR fd_set *","FAR const struct timespec *","FAR const sigset_t *"
"pthread_cancel","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
"pthread_cleanup_pop","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","int"
+"pthread_cleanup_poplist","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","int","struct pthread_cleanup_s *"
"pthread_cleanup_push","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","pthread_cleanup_t","FAR void *"
Review comment:
Got 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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r634243332
##########
File path: syscall/syscall.csv
##########
@@ -82,15 +83,12 @@
"pread","unistd.h","","ssize_t","int","FAR void *","size_t","off_t"
"pselect","sys/select.h","","int","int","FAR fd_set *","FAR fd_set *","FAR fd_set *","FAR const struct timespec *","FAR const sigset_t *"
"pthread_cancel","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
-"pthread_cleanup_pop","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","int"
-"pthread_cleanup_push","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","pthread_cleanup_t","FAR void *"
"pthread_cond_broadcast","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *"
"pthread_cond_clockwait","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *","FAR pthread_mutex_t *","clockid_t","FAR const struct timespec *"
"pthread_cond_signal","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *"
"pthread_cond_wait","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t *","FAR pthread_mutex_t *"
-"pthread_create","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_t *","FAR const pthread_attr_t *","pthread_startroutine_t","pthread_addr_t"
"pthread_detach","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t"
-"pthread_exit","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
+"nx_pthread_exit","nuttx/pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
Review comment:
move after line 69
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
should we keep the thumb bit?
##########
File path: arch/arm/src/armv6-m/svcall.h
##########
@@ -124,6 +116,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
where define SYS_pthread_exit?
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
should we keep the thumb bit?
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -122,6 +115,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
where SYS_pthread_exit
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -88,6 +88,7 @@
#define SYS_syscall_return (3)
+#ifndef CONFIG_BUILD_FLAT
Review comment:
remove
##########
File path: arch/arm/src/armv8-m/arm_svcall.c
##########
@@ -311,34 +311,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
remove cast and ~1
##########
File path: arch/arm/src/armv7-r/svcall.h
##########
@@ -108,6 +101,17 @@
#endif /* CONFIG_BUILD_PROTECTED */
+/* SYS call 3:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (3)
Review comment:
where SYS_pthread_exit
##########
File path: arch/arm/src/armv8-m/arm_svcall.c
##########
@@ -311,34 +311,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
remove ~1? and (uint32_t)
##########
File path: arch/arm/src/armv7-a/svcall.h
##########
@@ -65,6 +65,7 @@
#define SYS_syscall_return (0)
+#ifndef CONFIG_BUILD_FLAT
Review comment:
why add CONFIG_BUILD_FLAT? the change look incorrect.
##########
File path: arch/arm/src/armv6-m/svcall.h
##########
@@ -89,6 +89,7 @@
#define SYS_syscall_return (3)
+#ifndef CONFIG_BUILD_FLAT
Review comment:
The change looks wrong
##########
File path: arch/arm/src/armv7-m/arm_svcall.c
##########
@@ -312,34 +312,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
remove the cast and ~1
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
Review comment:
should keep #if defined(CONFIG_BUILD_PROTECTED)
##########
File path: arch/arm/src/armv7-m/arm_svcall.c
##########
@@ -312,34 +312,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
Review comment:
restore to defined(CONFIG_BUILD_PROTECTED)
##########
File path: arch/arm/src/armv7-m/svcall.h
##########
@@ -123,6 +116,24 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start((pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
restore the original location
##########
File path: arch/arm/src/armv7-m/arm_svcall.c
##########
@@ -312,34 +312,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
remove the cast and ~1
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
remove the uint32_t cast
##########
File path: sched/task/task_setcancelstate.c
##########
@@ -112,7 +112,14 @@ int task_setcancelstate(int state, FAR int *oldstate)
if ((tcb->flags & TCB_FLAG_TTYPE_MASK) ==
TCB_FLAG_TTYPE_PTHREAD)
{
+ tcb->flags &= ~TCB_FLAG_CANCEL_PENDING;
+ tcb->flags |= TCB_FLAG_CANCEL_DOING;
Review comment:
why not move TCB_FLAG_CANCEL_DOING/TCB_FLAG_CANCEL_PENDING to tls_info_s? so task_setcancelstate can move libc.
##########
File path: arch/arm/src/armv7-r/svcall.h
##########
@@ -108,6 +101,17 @@
#endif /* CONFIG_BUILD_PROTECTED */
+/* SYS call 3:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (3)
Review comment:
need update CONFIG_SYS_RESERVED
##########
File path: include/nuttx/sched.h
##########
@@ -504,33 +509,35 @@ struct task_group_s
#endif /* CONFIG_SCHED_HAVE_PARENT */
#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
- /* waitpid support ************************************************************/
+ /* waitpid support ********************************************************/
- /* Simple mechanism used only when there is no support for SIGCHLD */
+ /* Simple mechanism used only when there is no support for SIGCHLD */
- uint8_t tg_nwaiters; /* Number of waiters */
- uint8_t tg_waitflags; /* User flags for waitpid behavior */
- sem_t tg_exitsem; /* Support for waitpid */
- FAR int *tg_statloc; /* Location to return exit status */
+ uint8_t tg_nwaiters; /* Number of waiters */
+ uint8_t tg_waitflags; /* User flags for waitpid behavior */
+ sem_t tg_exitsem; /* Support for waitpid */
+ FAR int *tg_statloc; /* Location to return exit status */
#endif
#ifndef CONFIG_DISABLE_PTHREAD
- /* Pthreads *******************************************************************/
+ /* Pthreads ***************************************************************/
- /* Pthread join Info: */
+ /* Pthread join Info: */
- sem_t tg_joinsem; /* Mutually exclusive access to join data */
- FAR struct join_s *tg_joinhead; /* Head of a list of join data */
- FAR struct join_s *tg_jointail; /* Tail of a list of join data */
+ sem_t tg_joinsem; /* Mutually exclusive access to join data */
+ FAR struct join_s *tg_joinhead; /* Head of a list of join data */
+ FAR struct join_s *tg_jointail; /* Tail of a list of join data */
#endif
- /* Thread local storage *******************************************************/
+ /* Thread local storage ***************************************************/
#if CONFIG_TLS_NELEM > 0
- tls_ndxset_t tg_tlsset; /* Set of TLS data indexes allocated */
+ tls_ndxset_t tg_tlsset; /* Set of TLS indexes allocated */
+
+ tls_dtor_t tg_tlsdestr[CONFIG_TLS_NELEM]; /* List of TLS destructors */
Review comment:
why not save to task_info_s?
##########
File path: arch/arm/src/armv8-m/svcall.h
##########
@@ -122,6 +115,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
CONFIG_SYS_RESERVED need update
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
remove the uint32_t cast
##########
File path: arch/arm/src/armv7-m/svcall.h
##########
@@ -81,6 +81,7 @@
#define SYS_switch_context (2)
+#ifndef CONFIG_BUILD_FLAT
Review comment:
remove
##########
File path: sched/task/task_setcanceltype.c
##########
@@ -100,7 +100,14 @@ int task_setcanceltype(int type, FAR int *oldtype)
#ifndef CONFIG_DISABLE_PTHREAD
if ((tcb->flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD)
{
+ tcb->flags &= ~TCB_FLAG_CANCEL_PENDING;
+ tcb->flags |= TCB_FLAG_CANCEL_DOING;
+#if !defined(CONFIG_BUILD_FLAT) && defined(__KERNEL__)
Review comment:
why not move TCB_FLAG_CANCEL_DOING/TCB_FLAG_CANCEL_PENDING to tls_info_s? so task_setcanceltype can move libc.
##########
File path: arch/arm/src/armv7-a/arm_syscall.c
##########
@@ -278,31 +278,48 @@ uint32_t *arm_syscall(uint32_t *regs)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
Review comment:
restore !defined(CONFIG_BUILD_FLAT) to defined(CONFIG_BUILD_KERNEL)
##########
File path: arch/arm/src/armv7-r/arm_syscall.c
##########
@@ -273,31 +273,48 @@ uint32_t *arm_syscall(uint32_t *regs)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
Review comment:
restore to defined(CONFIG_BUILD_PROTECTED)
##########
File path: arch/arm/src/armv7-r/svcall.h
##########
@@ -66,6 +66,7 @@
#define SYS_syscall_return (0)
+#ifndef CONFIG_BUILD_FLAT
Review comment:
remove
##########
File path: arch/arm/src/armv8-m/arm_svcall.c
##########
@@ -311,34 +311,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
Review comment:
restore to defined(CONFIG_BUILD_PROTECTED)
##########
File path: arch/arm/src/armv7-a/svcall.h
##########
@@ -105,6 +98,24 @@
#define SYS_signal_handler_return (5)
+#endif /* !CONFIG_BUILD_FLAT */
+
+/* SYS call 3:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (3)
+
+/* SYS call 8:
Review comment:
I don't see the update here.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623001626
##########
File path: include/nuttx/pthread.h
##########
@@ -121,6 +125,65 @@ EXTERN const pthread_attr_t g_default_pthread_attr;
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: nx_pthread_create
+ *
+ * Description:
+ * This function creates and activates a new thread with specified
+ * attributes.
+ *
+ * Input Parameters:
+ * trampoline
+ * thread
+ * attr
+ * start_routine
+ * arg
Review comment:
Missing documentation
--
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] no1wudi edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-851760149
> @no1wudi Has this change been verified on a PROTECTED and a KERNEL build? I think that is essential to know if some of these changes and the comments are correct. If any of your comments are correct, then those builds are currently broken.
These changes had been verified on stm32 and k210, I'll continue this work according to the latest suggestions from @xiaoxiang781216 and @patacongo :
1. Move pthread_exit & pthread_create to USERSPACE to unify the convention that how to drop to userspace from kernel side like this:
`regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup`
`regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_exit`
2. Move tls destructor to user space (task_info_s)
3. Add SYS_pthread_exit syscall like SYS_pthread_create
4. Move TCB_FLAG_CANCEL_DOING/TCB_FLAG_CANCEL_PENDING to tls_info_s
5. Move task_setcancelstate to libc
6. Minor fixes/changes from 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 edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852444131
> Will you be verifying the operation of the KERNEL build? Do you have a configuration and hardware you can use? Currently configurations are only available for the sama5d4-ek board. I have talked to several end-users who are running KERNEL mode on their custom hardware but only sama5d4-ek is supported in the repository. The i.MX6 is another possibility in the repository.
I seemed to recall that @masayuki2009 was using an i.MX6 emulation under Qemu for testing SMP. Perhaps we could also use that i.MX6 QEMU to verify these changes in KERNEL mode. I know nothing about how to do this, however. @masayuki2009 Is there some information about using the i.MX6 QEMU online or in some README file?
If you want to go this way for your testing, I can create a kostest configuration for the i.MX6 and verify baseline functionality (using real hardware).
--
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] no1wudi edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-851760149
> @no1wudi Has this change been verified on a PROTECTED and a KERNEL build? I think that is essential to know if some of these changes and the comments are correct. If any of your comments are correct, then those builds are currently broken.
These changes had beed verified on stm32 and k210, I'll continue this work according to the latest suggestions from @xiaoxiang781216 and @patacongo :
1. Move pthread_exit & pthread_create to USERSPACE to unify the convention that how to drop to userspace from kernel side like this:
`regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_startup`
`regs[REG_EPC] = (uintptr_t)USERSPACE->pthread_exit`
2. Move tls destructor to user space (task_info_s)
3. Add SYS_pthread_exit syscall like SYS_pthread_create
4. Move TCB_FLAG_CANCEL_DOING/TCB_FLAG_CANCEL_PENDING to tls_info_s
5. Move task_setcancelstate to libc
6. Minor fixes/changes from 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 pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853180821
> After more thinking, there is a structure like userspace_s for kernel mode, but name it as addrenv_reserve_s:
>
> [incubator-nuttx/include/nuttx/addrenv.h](https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/addrenv.h#L248-L254)
I am unsure if addrenv_reserve_s can be used for this purpose or not. A KERNEL mode process is basically an ELF module with an address environment. addrenv_reserve_s currently only exists if the module has an address environment and it is located only via the fixed virtual address of .data in all processes.
What about other uses of ELF modules that don't have address environments, addrenv_reserve_s will not be avilable for them and even if it were, the OS could not locate them because the physical .data address is not easily known. If the pthread and task startup and exit functions are linked into the ELF module, then there is not way to access them externally. I think there are other complexities and corner cases that you are not addressing.
--
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] no1wudi edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-842159148
> Looks good to me.
>
> Is this change now complete? What steps do we need to take to get this merged. We need some confidence that this does not break things irreparably. I expect some issues with code turmoil of this magnitude, but I think now is the right to to merge this since we just release 10.1.
The missing **SYS_pthread_exit** case is added to **arm_svcall**.
These patches mainly tested on risc-v (K210) platform, but shoud works on arm platform.
I'm testing them on cortex m4 platform, I will remove the Draft indication when it passed.
@xiaoxiang781216 Could you take a look at these patches please ?
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-846349445
@no1wudi I just noticed that you unmarked this as draft. I think everyone has had a chance to comment and it has passed all of the checkes... Time to merge!
Everyone: Please let us know if you see any unexpected new behaviors.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r629000307
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
> pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit(). In the original code before your change there are these:
>
> ```
> sched/pthread/pthread_create.c: pthread_exit(exit_status);
> sched/signal/sig_default.c: pthread_exit(NULL);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
> ```
>
> I think this is an error. We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved.
>
> Possible solution:
>
> 1. Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be known in the PROTECTED and KERNEL builds. I think you were not seeing the build failure in the PROTECTED/KERNEL builds because cancellation points and default signal actions were not enabled.
>
> 2. Save the pthread_exit() pointer in the TCB
>
> 3. Replace calls to pthread_exit() in the OS to up_pthread_exit().
>
> 4. up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
I think there is a way to accomplish this without adding a lot of new infrastructure. I think we could re-use up_pthread_start() to call pthread_exit():
1. Call up_pthread_start() with the pthread entry point equal to NULL.
2. up_pthread_start() will switch to user mode and call the user-space pthread_start() function.
3. pthread_start() would detect the NULL pthread entry point and just call pthread_exit().
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829601604
build is failing because of:
stubs/STUB_nx_pthread_exit.c: In function 'STUB_nx_pthread_exit':
Error: stubs/STUB_nx_pthread_exit.c:12:1: error: control reaches end of non-void function [-Werror=return-type]
12 | }
| ^
That should be handled as it is for other no return functions:
syscall.csv:"_exit","unistd.h","","noreturn","int"
syscall.csv:"exit","stdlib.h","","noreturn","int"
syscall.csv:"pthread_exit","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
But you have:
"nx_pthread_exit","nuttx/pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
So I don't understand what the issue 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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-846350804
@no1wudi Are you interested in finishing this job? That is, fixing on_exit() and atexit() callbacks so that they execute in user mode in all build modes? Logically this would be a very similar job: task_startup() and task exits would have to be moved to libs/libc and the callbacks would have to be re-architected so that the call back function points and parameters lie in TLS and the functions are called in some user-space task exit logic.
This, however, is more complex for a couple of reasons:
1. on_exit() and atexit() processing is more entangled task_exithook(). However, I think that that entanglement is no longer necessary. I think it is an artifact from an older design. It should be okay to move those callbacks to the beginning of the exit sequence in the current design. Hmm.. is there some reason why the exit callbacks should not execute while there could be pthreads running in an SMP configuration? There are a few things to think about.
2. There are several ways to exit: exit(), task_delete(), task_reset(), assert(), abort(), .... others?
_exit() might also be in that group, but since that is reserved for emergency terminations, it probably does not need to honor the on_exit() / atexit() (In fact, it is not really safe to call _exit() at all in an embedded system since it leaves files open).
There is a block diagram of the exit sequence here: https://cwiki.apache.org/confluence/display/NUTTX/Task+Exit+Sequence I am not sure if that is 100% up to date.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852182554
After more thinking, there is a structure like userspace_s for kernel mode, but name it as addrenv_reserve_s:
https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/addrenv.h#L248-L254
so I think a more reasonable approach is sync userspace_s and addrenv_reserve_s. Let's one is subset an other.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852205047
I prefer the existing on master solution which is standard and does not introduce either kludgey structure.
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852551838
> Not on your list of things was the similar changes to call the on_exit() and atexit() callbacks when a task exits. In order to resolve #1263, we still have to address those security violations as well. Can we add these?
OK, I'll add them to work item.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632754031
##########
File path: include/nuttx/pthread.h
##########
@@ -121,6 +125,72 @@ EXTERN const pthread_attr_t g_default_pthread_attr;
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: nx_pthread_create
+ *
+ * Description:
+ * This function creates and activates a new thread with specified
+ * attributes.
+ *
+ * Input Parameters:
+ * trampoline - The user space startup function
+ * thread - The pthread handle to be used
+ * attr - It points to a pthread_attr_t structure whose contents are
+ * used at thread creation time to determine attributes
+ * for the new thread
+ * entry - The new thread starts execution by invoking entry
+ * arg - It is passed as the sole argument of entry
+ * exit - The user-space pthread exit function
+ *
+ * Returned Value:
+ * OK (0) on success; a (non-negated) errno value on failure. The errno
+ * variable is not set.
+ *
+ ****************************************************************************/
+
+int nx_pthread_create(pthread_trampoline_t trampoline, FAR pthread_t *thread,
+ FAR const pthread_attr_t *attr,
+ pthread_startroutine_t entry, pthread_addr_t arg,
+ pthread_exitroutine_t exit);
+
+/****************************************************************************
+ * Name: nx_pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_value
Review comment:
```suggestion
* exit_value - The pointer of the pthread exit parameter
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r626641498
##########
File path: include/nuttx/pthread.h
##########
@@ -121,6 +125,68 @@ EXTERN const pthread_attr_t g_default_pthread_attr;
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: nx_pthread_create
+ *
+ * Description:
+ * This function creates and activates a new thread with specified
+ * attributes.
+ *
+ * Input Parameters:
+ * trampoline
+ * thread
+ * attr
+ * start_routine
+ * arg
+ *
+ * Returned Value:
+ * OK (0) on success; a (non-negated) errno value on failure. The errno
+ * variable is not set.
+ *
+ ****************************************************************************/
+
+int nx_pthread_create(pthread_trampoline_t trampoline, FAR pthread_t *thread,
+ FAR const pthread_attr_t *attr,
+ pthread_startroutine_t entry, pthread_addr_t arg);
+
+/****************************************************************************
+ * Name: nx_pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void nx_pthread_exit(FAR void *exit_value) noreturn_function;
+
+/****************************************************************************
+ * Name: pthread_cleanup_popall
+ *
+ * Description:
+ * The pthread_cleanup_popall() is an internal function that will pop and
+ * execute all clean-up functions. This function is only called from
+ * within the pthread_exit() and pthread_cancellation() logic
+ *
+ * Input Parameters:
+ * tcb - The TCB of the pthread that is exiting or being canceled.
Review comment:
Update function documentation, now it is void
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632751806
##########
File path: arch/arm/src/common/arm_pthread_exit.c
##########
@@ -0,0 +1,62 @@
+/****************************************************************************
+ * arch/arm/src/common/arm_pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <pthread.h>
+#include <nuttx/arch.h>
+
+#include "svcall.h"
+#include "arm_internal.h"
+
+#if !defined(CONFIG_BUILD_FLAT) && defined(__KERNEL__) && \
+ !defined(CONFIG_DISABLE_PTHREAD)
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_pthread_exit
+ *
+ * Description:
+ * In this kernel mode build, this function will be called to execute a
+ * pthread in user-space. This kernel-mode stub will then be called
+ * transfer control to the user-mode pthread_exit.
+ *
+ * Input Parameters:
+ * exit - The user-space pthread_exit function
+ * exit_value - The pointer of the pthread exit parameter
+ *
+ * Returned Value:
+ * None
+ ****************************************************************************/
+
+void up_pthread_exit(pthread_exitroutine_t exit, FAR void *exit_value)
+{
+ /* Let sys_call3() do all of the work */
Review comment:
```suggestion
/* Let sys_call2() do all of the work */
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638226530
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
> should we keep the thumb bit?
I don't think that the Thumb bit should be set. This is the saved value of program counter and should not have bit 0 set. Bit 0 works only for call instructions (and a few other places in the ISA). We need to double check this.
This is equivalent to the branch:
mov Rx, PC
This just jumps to the address in RX. This is the absolute address version of the relative branch B instruction. The B and BL instructions and BX and BLX instructions differ in the instruction "interprets" bit 0 as the Thumb indication. That thumb indication is NOT written to the PC. The BX and BLX instructions use to the the Thumb bit in the control/status register. See https://topic.alibabacloud.com/a/the-difference-between-the-assembly-jump-instruction-b-bl-bx-blx-and-bxj_8_8_10244895.html . Bit 0 of the PC should never be set and never indicates Thumb mode.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228910
##########
File path: include/nuttx/sched.h
##########
@@ -504,33 +509,35 @@ struct task_group_s
#endif /* CONFIG_SCHED_HAVE_PARENT */
#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
- /* waitpid support ************************************************************/
+ /* waitpid support ********************************************************/
- /* Simple mechanism used only when there is no support for SIGCHLD */
+ /* Simple mechanism used only when there is no support for SIGCHLD */
- uint8_t tg_nwaiters; /* Number of waiters */
- uint8_t tg_waitflags; /* User flags for waitpid behavior */
- sem_t tg_exitsem; /* Support for waitpid */
- FAR int *tg_statloc; /* Location to return exit status */
+ uint8_t tg_nwaiters; /* Number of waiters */
+ uint8_t tg_waitflags; /* User flags for waitpid behavior */
+ sem_t tg_exitsem; /* Support for waitpid */
+ FAR int *tg_statloc; /* Location to return exit status */
#endif
#ifndef CONFIG_DISABLE_PTHREAD
- /* Pthreads *******************************************************************/
+ /* Pthreads ***************************************************************/
- /* Pthread join Info: */
+ /* Pthread join Info: */
- sem_t tg_joinsem; /* Mutually exclusive access to join data */
- FAR struct join_s *tg_joinhead; /* Head of a list of join data */
- FAR struct join_s *tg_jointail; /* Tail of a list of join data */
+ sem_t tg_joinsem; /* Mutually exclusive access to join data */
+ FAR struct join_s *tg_joinhead; /* Head of a list of join data */
+ FAR struct join_s *tg_jointail; /* Tail of a list of join data */
#endif
- /* Thread local storage *******************************************************/
+ /* Thread local storage ***************************************************/
#if CONFIG_TLS_NELEM > 0
- tls_ndxset_t tg_tlsset; /* Set of TLS data indexes allocated */
+ tls_ndxset_t tg_tlsset; /* Set of TLS indexes allocated */
+
+ tls_dtor_t tg_tlsdestr[CONFIG_TLS_NELEM]; /* List of TLS destructors */
Review comment:
> why not save to task_info_s?
Because the destructor is common for all pthreads. It is saved with the pthread data key is created and used on all threads. So it is a group thing.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623155477
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
I wonder if any of the calls to pthread_exit() could be replaced with calls to nx_pthread_exit(). That is possible in certain error handling cases where the pthread never ran (and so could not have set up any callbacks).
I wonder if we could move the remaining functions into libs/libc/pthread:
sched/signal/sig_default.c
sched/task/task_cancelpt.c
sched/task/task_setcancelstate.c
sched/task/task_setcanceltype.c
If we could that might be a simpler solution.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853531956
> > After more thinking, there is a structure like userspace_s for kernel mode, but name it as addrenv_reserve_s:
> > [incubator-nuttx/include/nuttx/addrenv.h](https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/addrenv.h#L248-L254)
>
> I am unsure if addrenv_reserve_s can be used for this purpose or not. A KERNEL mode process is basically an ELF module with an address environment. addrenv_reserve_s currently only exists if the module has an address environment and it is located only via the fixed virtual address of .data in all processes.
>
> What about other uses of ELF modules that don't have address environments, addrenv_reserve_s will not be avilable for them and even if it were, the OS could not locate them because the physical .data address is not easily known. If the pthread and task startup and exit functions are linked into the ELF module, then there is not way to access them externally. I think there are other complexities and corner cases that you are not addressing.
For ELF binary, we have three case:
1. ELF binary in FLAT build, it simplify to the direct function call
2. ELF binary in PROTECTED build, switch to userspace by userspace_s
3. ELF binary in KERNEL build, switch to userspace by addrenv_reserve_s
There isn't difference between ELF binary(posix_spawn) and builtin binary(task_spawn) after binfmt finish load and parse ELF file.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-847990871
@no1wudi Has this change been verified on a PROTECTED and a KERNEL build? I think that is essential to know if some of these changes and the comments are correct. If any of your comments are correct, then those builds are currently broken.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852175794
Yes, you are right. Please ignore my comment for this point.
--
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] no1wudi edited a comment on pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829281942
> I think there is an error in the title. Did you mean stuff instead of staff?
>
> There is logic on the branch feature/pthread-user that also implements moving pthread_create and pthread_exit to libs/libc/pthread. See https://github.com/apache/incubator-nuttx/tree/feature/pthread-user and PR #1328.
>
> ```
> The primary objective here was:
>
> 1. To eliminate security problems when pthread_cleanup_popall() is called when the pthread exits: See Issue #1263 and #1328. And
> 2. Implement pthread thread-specific data destructors (mentioned in #1328)
>
> It looks to me like this PR, takes much of its logic from that feature/pthread-user branch. If/when this PR is merged, I will remove the branch, and close the issue and PR.
> ```
The main logic in this PR is just picked from feature/pthread_user with a few modification.
And, now os call the pthread_cleanup from user space in pthread_exit, but in pthread_cancel they are still called from kernel side, we still need to move cancellation point in syscall to user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638228077
##########
File path: arch/arm/src/armv6-m/svcall.h
##########
@@ -124,6 +116,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
> where define SYS_pthread_exit?
It is a normal system call. SYS_pthread_start is not. The start function is an internal chip-specific OS interface, SYS_nx_pthread_exit, is part of the common, normal user interface and you will find it in nuttx/syscall. The actual user interface pthread_exit() is a callable function in libs/libc/pthread and not a system call(). The whole point of the change was to get pthread_exit() to run in user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638418101
##########
File path: include/nuttx/sched.h
##########
@@ -504,33 +509,35 @@ struct task_group_s
#endif /* CONFIG_SCHED_HAVE_PARENT */
#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
- /* waitpid support ************************************************************/
+ /* waitpid support ********************************************************/
- /* Simple mechanism used only when there is no support for SIGCHLD */
+ /* Simple mechanism used only when there is no support for SIGCHLD */
- uint8_t tg_nwaiters; /* Number of waiters */
- uint8_t tg_waitflags; /* User flags for waitpid behavior */
- sem_t tg_exitsem; /* Support for waitpid */
- FAR int *tg_statloc; /* Location to return exit status */
+ uint8_t tg_nwaiters; /* Number of waiters */
+ uint8_t tg_waitflags; /* User flags for waitpid behavior */
+ sem_t tg_exitsem; /* Support for waitpid */
+ FAR int *tg_statloc; /* Location to return exit status */
#endif
#ifndef CONFIG_DISABLE_PTHREAD
- /* Pthreads *******************************************************************/
+ /* Pthreads ***************************************************************/
- /* Pthread join Info: */
+ /* Pthread join Info: */
- sem_t tg_joinsem; /* Mutually exclusive access to join data */
- FAR struct join_s *tg_joinhead; /* Head of a list of join data */
- FAR struct join_s *tg_jointail; /* Tail of a list of join data */
+ sem_t tg_joinsem; /* Mutually exclusive access to join data */
+ FAR struct join_s *tg_joinhead; /* Head of a list of join data */
+ FAR struct join_s *tg_jointail; /* Tail of a list of join data */
#endif
- /* Thread local storage *******************************************************/
+ /* Thread local storage ***************************************************/
#if CONFIG_TLS_NELEM > 0
- tls_ndxset_t tg_tlsset; /* Set of TLS data indexes allocated */
+ tls_ndxset_t tg_tlsset; /* Set of TLS indexes allocated */
+
+ tls_dtor_t tg_tlsdestr[CONFIG_TLS_NELEM]; /* List of TLS destructors */
Review comment:
> > why not save to task_info_s?
>
> Because the destructor is common for all pthreads. It is saved with the pthread data key is created and used on all threads. So it is a group thing. It could go in task_info_s, however (But the all of the group data could go into task_info_s for that matter). I would only move it if it can save system call and if the job can be done with no critical section.
Yes, this is a good guide.
> Critical sections can only be used within the OS in supervisor mode.
Yes, that's why I just sugest to move tg_tlsdestr to task_info_s, but leave tg_tlsset in task_group_s, because:
1. tg_tlsset is protected by critical section
2. tg_tlsdestr isn't needed to protect and can remove tls_get_dtor/tls_set_dtor from syscall
We can even change the protection from critical section to semaphere and then move tg_tlsset to task_info_s too.
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829256289
There is logic on the branch feature/pthread-user that also implements moving pthread_create and pthread_exit to libs/libc/pthread. See https://github.com/apache/incubator-nuttx/tree/feature/pthread-user and PR #1328. That logic is incomplete, however, but it is interesting to compare the technical approach.
The primary thing is that:
1. The pthread_create() in sched./pthread was changed to:
```
int nx_pthread_create(pthread_trampoline_t trampoline, FAR pthread_t *thread,
FAR const pthread_attr_t *attr,
pthread_startroutine_t entry, pthread_addr_t arg)
```
And
2. The new pthread_create() in libs/libc/pthread passes the address of the pthread startup function:
```
int pthread_create(FAR pthread_t *thread, FAR const pthread_attr_t *attr,
pthread_startroutine_t pthread_entry, pthread_addr_t arg)
{
return nx_pthread_create(pthread_startup, thread, attr, pthread_entry,
arg);
}
```
That keeps things simple.
The primary objective here was:
1. To eliminate security problems when pthread_cleanup_popall() is called when the pthread exits: See Issue #1263 and #1328. And
2. Implement pthread thread-specific data destructors (mentioned in #1328)
If/when this PR is merged, I will remove the branch, and close the issue and PR.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852444131
> Will you be verifying the operation of the KERNEL build? Do you have a configuration and hardware you can use? Currently configurations are only available for the sama5d4-ek board. I have talked to several end-users who are running KERNEL mode on their custom hardware but only sama5d4-ek is supported in the repository. The i.MX6 is another possibility in the repository.
I seemed to recall that @masayuki2009 was using an i.MX6 emulation under Qemu for testing SMP. Perhaps we could also use that i.MX6 QEMU to verify these changes in KERNEL mode. I know nothing about how to do this, however. @masayuki2009 Is there some information about using the i.MX6 QEMU online or in some README file?
If you want to go this way for your testing, I can create a kostest configuration for the i.MX6 and verify baseline functionality.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852182554
After more thinking, there is a structure like userspace_s for kernel mode, but name it as addrenv_reserve_s:
https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/addrenv.h#L248-L254
so I think a more reasonable approach is sync userspace_s and addrenv_reserve_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] no1wudi edited a comment on pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829281942
> I think there is an error in the title. Did you mean stuff instead of staff?
>
> There is logic on the branch feature/pthread-user that also implements moving pthread_create and pthread_exit to libs/libc/pthread. See https://github.com/apache/incubator-nuttx/tree/feature/pthread-user and PR #1328.
>
> ```
> The primary objective here was:
>
> 1. To eliminate security problems when pthread_cleanup_popall() is called when the pthread exits: See Issue #1263 and #1328. And
> 2. Implement pthread thread-specific data destructors (mentioned in #1328)
>
> It looks to me like this PR, takes much of its logic from that feature/pthread-user branch. If/when this PR is merged, I will remove the branch, and close the issue and PR.
> ```
The main logic in this PR is just picked from feature/pthread_user with few modification
And, now os call the pthread_cleanup from user space in pthread_exit, but in pthread_cancel they are still called from kernel side, we still need to move cancellation to user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-846404118
> @no1wudi Are you interested in finishing this job? That is, fixing on_exit() and atexit() callbacks so that they execute in user mode in all build modes? Logically this would be a very similar job: task_startup() and task exits would have to be moved to libs/libc and the callbacks would have to be re-architected so that the call back function points and parameters lie in TLS and the functions are called in some user-space task exit logic.
Another special case, however, are kernel threads. Currently, they can also have on_exit() and atexit() callbacks -- but unlike tasks, these must run in supervisor mode.
PS: I will remove the old feature/pthread_user branch now. Merging this commit obsoletes that branch.
--
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] no1wudi commented on pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829779823
> build is failing because of:
>
> ```
> stubs/STUB_nx_pthread_exit.c: In function 'STUB_nx_pthread_exit':
> Error: stubs/STUB_nx_pthread_exit.c:12:1: error: control reaches end of non-void function [-Werror=return-type]
> 12 | }
> | ^
> ```
>
> That should be handled as it is for other no return functions:
>
> ```
> syscall.csv:"_exit","unistd.h","","noreturn","int"
> syscall.csv:"exit","stdlib.h","","noreturn","int"
> syscall.csv:"pthread_exit","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
> ```
>
> But you have:
>
> ```
> "nx_pthread_exit","nuttx/pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","noreturn","pthread_addr_t"
> ```
>
> So I don't understand what the issue is.
I forget to add `noreturn_function` attribute to the declaration of nx_pthread_exit
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623128090
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
+#ifdef CONFIG_PTHREAD_CLEANUP
+ int cnt;
+ struct pthread_cleanup_s cleanup[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+ cnt = pthread_cleanup_poplist(cleanup);
+
+ sched_lock();
+ while (cnt-- > 0)
+ {
+ struct pthread_cleanup_s cp = cleanup[cnt];
+ if (cp.pc_cleaner)
+ cp.pc_cleaner(cp.pc_arg);
+ }
+
+ sched_unlock();
+#endif
+
Review comment:
> Yes, I had implemented a version of tls data destructors
This is not in this PR, however. I was referring to the data destructor in pthread_key_create().
int pthread_key_create(pthread_key_t *key, void (*destructor)(void*));
See https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html :
"An optional destructor function may be associated with each key value. At thread exit, if a key value has a non-NULL destructor pointer, and the thread has a non-NULL value associated with that key, the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument. The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits.
"If, after all the destructors have been called for all non-NULL values with associated destructors, there are still some non-NULL values with associated destructors, then the process is repeated. If, after at least {PTHREAD_DESTRUCTOR_ITERATIONS} iterations of destructor calls for outstanding non-NULL values, there are still some non-NULL values with associated destructors, implementations may stop calling destructors, or they may continue calling destructors until no non-NULL values with associated destructors exist, even though this might result in an infinite loop."
This would effect TLS data format where the destructors would be stored, pthread_key_create() which would need to save the destructor function pointers (in TLS), and pthread_exit() that would call the pthread data destructors,
This is separate functionality from the changes of this PR but is part of the longer term motivation for moving pthread_exit into user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638352141
##########
File path: sched/task/task_setcanceltype.c
##########
@@ -100,7 +100,14 @@ int task_setcanceltype(int type, FAR int *oldtype)
#ifndef CONFIG_DISABLE_PTHREAD
if ((tcb->flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD)
{
+ tcb->flags &= ~TCB_FLAG_CANCEL_PENDING;
+ tcb->flags |= TCB_FLAG_CANCEL_DOING;
+#if !defined(CONFIG_BUILD_FLAT) && defined(__KERNEL__)
Review comment:
> why not move TCB_FLAG_CANCEL_DOING/TCB_FLAG_CANCEL_PENDING to tls_info_s? so task_setcanceltype can move libc.
Good idea. But isn't that a different PR?
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642649470
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as 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 a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638226530
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
> should we keep the thumb bit?
I don't think that the Thumb bit should be set. This is the saved value of program counter and should not have bit 0 set. Bit 0 works only for call instructions (and a few other places in the ISA). We need to double check this.
This is equivalent to the branch:
mov Rx, PC
This just jumps to the address in RX. This is the absolute address version of the relative branch B instruction. The B and BL instructions and BX and BLX instructions differ in that the BX and BLX instructions "interpret" bit 0 as the Thumb indication. That thumb indication is NOT written to the PC. The BX and BLX instructions use bit 0 to set the Thumb bit in the control/status register. See https://topic.alibabacloud.com/a/the-difference-between-the-assembly-jump-instruction-b-bl-bx-blx-and-bxj_8_8_10244895.html . Bit 0 of the PC should never be set and never indicates Thumb mode.
I believe that setting bit 0 in the PC would cause an unaligned access failure.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852169591
All entry point from kernel to userspace before this patch is defined in userspace_s:
https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/userspace.h#L105-L120
it make sense to expose pthread_exit through the same mechanism.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642696721
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as 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 edited a comment on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852233010
>
>
> > Were these only FLAT builds? You will need to extend the testing to verify the PROTECTED and KERNEL modes. The changes you and Xiao propose here apply only to PROTECTED and KERNEL modes and require verification in those modes.
>
> @patacongo They were verified on both FLAT and PROTECTED mode by:
>
> _boards/risc-v/k210/maix-bit/configs/kostest/defconfig boards/arm/stm32/stm32f4discovery/configs/kostest/defconfig_
>
> And passed the ostest example.
Great! Currently, the kostest is broken on master. There must have been some change to vfork() recently because vfork() now hardfaults on master. See #3812
If it ran for you than I assume this breakage is recent.
We do need to verify KERNEL mode too, especially if we make the KERNEL mode address environment (which I am hoping that we do not).
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638226530
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
> should we keep the thumb bit?
I don't think that the Thumb bit should be set. This is the saved value of program counter and should not have bit 0 set. Bit 0 works only for call instructions (and a few other places in the ISA). We need to double check this.
This is equivalent to the branch:
mov Rx, PC
This just jumps to the address in RX. This is the absolute address version of the relative branch B instruction. The B and BL instructions and BX and BLX instructions differ in that the BX and BLX instructions "interpret" bit 0 as the Thumb indication. That thumb indication is NOT written to the PC. The BX and BLX instructions use to set the Thumb bit in the control/status register. See https://topic.alibabacloud.com/a/the-difference-between-the-assembly-jump-instruction-b-bl-bx-blx-and-bxj_8_8_10244895.html . Bit 0 of the PC should never be set and never indicates Thumb mode.
I believe that setting bit 0 in the PC would cause an unaligned access failure.
--
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] no1wudi commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852558094
> Great! Currently, the kostest is broken on master. There must have been some change to vfork() recently because vfork() now hardfaults on master. See #3812
Oh, I forget to report that this broken exists before my 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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r629000307
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -0,0 +1,72 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_exit.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <sched.h>
+
+#include <nuttx/pthread.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_exit
+ *
+ * Description:
+ * Terminate execution of a thread started with pthread_create.
+ *
+ * Input Parameters:
+ * exit_valie
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void pthread_exit(FAR void *exit_value)
+{
Review comment:
> pthread_exit() must only be called from user space, especially after this change. However, I still see several calls to pthread_exit(). In the original code before your change there are these:
>
> ```
> sched/pthread/pthread_create.c: pthread_exit(exit_status);
> sched/signal/sig_default.c: pthread_exit(NULL);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_cancelpt.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcancelstate.c: pthread_exit(PTHREAD_CANCELED);
> sched/task/task_setcanceltype.c: pthread_exit(PTHREAD_CANCELED);
> ```
>
> I think this is an error. We must not call pthread_exit() from within the OS without first dropping the privileges to user. Otherwise, the problem that we are trying to solve with this PR is not solved.
>
> Possible solution:
>
> 1. Add a pointer to pthread_exit to nx_pthread_create(). We have to do this because the address of pthread_exit() will not be known in the PROTECTED and KERNEL builds. I think you were not seeing the build failure in the PROTECTED/KERNEL builds because cancellation points and default signal actions were not enabled.
>
> 2. Save the pthread_exit() pointer in the TCB
>
> 3. Replace calls to pthread_exit() in the OS to up_pthread_exit().
>
> 4. up_pthread_exit should issue a trap that drops to user mode and calls to the saved pthread_exit() entry point.
I think there is a way to accomplish this without adding a lot of new infrastructure. I think we could re-use up_pthread_start() to call pthread_exit() indirectly:
1. Call up_pthread_start() with the pthread entry point equal to NULL.
2. up_pthread_start() will switch to user mode and call the user-space pthread_start() function.
3. pthread_start() would detect the NULL pthread entry point and just call pthread_exit().
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853531956
> > After more thinking, there is a structure like userspace_s for kernel mode, but name it as addrenv_reserve_s:
> > [incubator-nuttx/include/nuttx/addrenv.h](https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/addrenv.h#L248-L254)
>
> I am unsure if addrenv_reserve_s can be used for this purpose or not. A KERNEL mode process is basically an ELF module with an address environment. addrenv_reserve_s currently only exists if the module has an address environment and it is located only via the fixed virtual address of .data in all processes.
>
> What about other uses of ELF modules that don't have address environments, addrenv_reserve_s will not be avilable for them and even if it were, the OS could not locate them because the physical .data address is not easily known. If the pthread and task startup and exit functions are linked into the ELF module, then there is not way to access them externally. I think there are other complexities and corner cases that you are not addressing.
For ELF binary, we have three case:
1. ELF binary in FLAT build, it simplify to the direct function call
2. ELF binary in PROTECTED build, switch to userspace by userspace_s
3. ELF binary in KERNEL build, switch to userspace by addrenv_reserve_s
There isn't difference between ELF binary(posix_spawn) and builtin binary(task_spawn) after binfmt finish load and parse ELF file.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632750448
##########
File path: arch/arm/src/armv7-r/arm_syscall.c
##########
@@ -285,19 +285,21 @@ uint32_t *arm_syscall(uint32_t *regs)
* R2 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
- /* Set up to return to the user-space pthread start-up function in
+ /* Set up to enter the user-space pthread start-up function in
* unprivileged mode. We need:
*
- * R0 = arg
+ * R0 = startup
+ * R1 = arg
* PC = entrypt
Review comment:
```suggestion
* R0 = entrypt
* R1 = arg
* PC = startup
```
Since startup is the trampoline function, I believe this should be the correct 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] no1wudi commented on pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829281942
> I think there is an error in the title. Did you mean stuff instead of staff?
>
> There is logic on the branch feature/pthread-user that also implements moving pthread_create and pthread_exit to libs/libc/pthread. See https://github.com/apache/incubator-nuttx/tree/feature/pthread-user and PR #1328.
>
> ```
> The primary objective here was:
>
> 1. To eliminate security problems when pthread_cleanup_popall() is called when the pthread exits: See Issue #1263 and #1328. And
> 2. Implement pthread thread-specific data destructors (mentioned in #1328)
>
> It looks to me like this PR, takes much of its logic from that feature/pthread-user branch. If/when this PR is merged, I will remove the branch, and close the issue and PR.
> ```
The main logic in this PR is just picked from feature/pthread_user with little modification
And, now os call the pthread_cleanup from user space in pthread_exit, but in pthread_cancel they are still called from kernel side, we still need to move cancellation to user space.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852233010
>
>
> > Were these only FLAT builds? You will need to extend the testing to verify the PROTECTED and KERNEL modes. The changes you and Xiao propose here apply only to PROTECTED and KERNEL modes and require verification in those modes.
>
> @patacongo They were verified on both FLAT and PROTECTED mode by:
>
> _boards/risc-v/k210/maix-bit/configs/kostest/defconfig boards/arm/stm32/stm32f4discovery/configs/kostest/defconfig_
>
> And passed the ostest example.
Great! Currently, the kostest is broken on master. There must have been some change to vfork() recently because vfork() now hardfaults on master. See #3812
If it ran for you than I assume this breakage is recent.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853212742
> I think there are other complexities and corner cases that you are not addressing.
Another complexity is the management of the address environment. Before you can call pthread_exit() (or any other user-space function) in KERNEL mode, the OS must select the correct address environment. Accessing any resource of the process from the wrong address environment will fail.
This is not a show stopper, just a complexity in the design that must be handled. The address environment of the process is saved in the group structure (tg_addrenv). It cannot be momentarily selected to access any addrenv_reserve_s data. And it can be selected before pthread_exit() is called.
Is this a flaw in the current 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] no1wudi commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r623785149
##########
File path: sched/pthread/pthread_exit.c
##########
@@ -87,12 +87,6 @@ void pthread_exit(FAR void *exit_value)
tcb->cpcount = 0;
#endif
-#ifdef CONFIG_PTHREAD_CLEANUP
- /* Perform any stack pthread clean-up callbacks */
-
- pthread_cleanup_popall(tcb);
Review comment:
pthread_cleanup should not be called from kernel side, we should resolve it in later 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] gustavonihei commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r632755680
##########
File path: include/sys/syscall_lookup.h
##########
@@ -296,6 +296,9 @@ SYSCALL_LOOKUP(telldir, 1)
#if CONFIG_TLS_NELEM > 0
SYSCALL_LOOKUP(tls_alloc, 0)
SYSCALL_LOOKUP(tls_free, 1)
+ SYSCALL_LOOKUP(tls_get_set, 1)
+ SYSCALL_LOOKUP(tls_get_dtor, 1)
+ SYSCALL_LOOKUP(tls_set_dtor, 2)
Review comment:
```suggestion
SYSCALL_LOOKUP(tls_get_dtor, 1)
SYSCALL_LOOKUP(tls_set_dtor, 2)
```
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638965991
##########
File path: arch/arm/src/armv6-m/svcall.h
##########
@@ -124,6 +116,17 @@
#define SYS_signal_handler_return (7)
#endif /* CONFIG_BUILD_PROTECTED */
+
+/* SYS call 5:
+ *
+ * void up_pthread_start(pthread_startroutine_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
+ * noreturn_function
+ */
+
+#define SYS_pthread_start (5)
Review comment:
Yes, you are 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 commented on a change in pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r638423998
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
The previous code use pthread_startup directly without mask, which work well for long time. There is no reason to mask in the new 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] patacongo commented on pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852444131
> Will you be verifying the operation of the KERNEL build? Do you have a configuration and hardware you can use? Currently configurations are only available for the sama5d4-ek board. I have talked to several end-users who are running KERNEL mode on their custom hardware but only sama5d4-ek is supported in the repository. The i.MX6 is another possibility in the repository.
I seemed to recall that @masayuki2009 was using an i.MX6 emulation under Qemu for testing SMP. Perhaps we could also use that i.MX6 QEMU to verify these changes in KERNEL mode. I know nothing about how to do this, however. @masayuki2009 Is there some information about using the i.MX6 QEMU online or in some README file?
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-852442374
> These changes had been verified on stm32 and k210, I'll continue this work according to the latest suggestions from @xiaoxiang781216 and @patacongo :
Not on your list of things was the similar changes to call the on_exit() and atexit() callbacks when a task exits. In order to resolve #1263, we still have to address those security violations as well. Can we add these?
--
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 #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-829332834
> And, now os call the pthread_cleanup from user space in pthread_exit, but in pthread_cancel they are still called from kernel side, we still need to move cancellation point in syscall to user space.
Our messages are crossing each other in time. I just pointed that out too. Cancellation is a problem and also logic in the default signal actions.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642649470
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -298,34 +298,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * user space pthread_startup:
+ */
+
+ regs[REG_R0] = regs[REG_R2]; /* pthread entry */
+ regs[REG_R1] = regs[REG_R3]; /* arg */
+ }
+ break;
+
+ /* R0=SYS_pthread_exit: This pthread_exit call in user-space
+ *
+ * void up_pthread_exit(pthread_exitroutine_t exit,
+ * FAR void *exit_value)
+ *
+ * At this point, the following values are saved in context:
+ *
+ * R0 = SYS_pthread_exit
+ * R1 = pthread_exit trampoline routine
+ * R2 = exit_value
+ */
+
+ case SYS_pthread_exit:
+ {
+ /* Set up to return to the user-space pthread start-up function in
+ * unprivileged mode.
+ */
+
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as is, but fails if this recommended change 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 pull request #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-853531956
> > After more thinking, there is a structure like userspace_s for kernel mode, but name it as addrenv_reserve_s:
> > [incubator-nuttx/include/nuttx/addrenv.h](https://github.com/apache/incubator-nuttx/blob/a8a1308240b7f631e44d78c3ef1582562c507a8a/include/nuttx/addrenv.h#L248-L254)
>
> I am unsure if addrenv_reserve_s can be used for this purpose or not. A KERNEL mode process is basically an ELF module with an address environment. addrenv_reserve_s currently only exists if the module has an address environment and it is located only via the fixed virtual address of .data in all processes.
>
> What about other uses of ELF modules that don't have address environments, addrenv_reserve_s will not be avilable for them and even if it were, the OS could not locate them because the physical .data address is not easily known. If the pthread and task startup and exit functions are linked into the ELF module, then there is not way to access them externally. I think there are other complexities and corner cases that you are not addressing.
For ELF binary, we have three case:
1. ELF binary in FLAT build, it simplify to the direct function call
2. ELF binary in PROTECTED build, switch to userspace by userspace_s
3. ELF binary in KERNEL build, switch to userspace by addrenv_reserve_s
There isn't difference from ELF binary(posix_spawn) and builtin binary(task_spawn) after binfmt finish load and parse ELF file.
--
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 #3626: libc: Move pthread stuff to userspace
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#discussion_r642697352
##########
File path: arch/arm/src/armv7-m/arm_svcall.c
##########
@@ -312,34 +312,65 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
break;
#endif
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
+
/* R0=SYS_pthread_start: This a user pthread start
*
- * void up_pthread_start(pthread_startroutine_t entrypt,
- * pthread_addr_t arg) noreturn_function;
+ * void up_pthread_start(pthread_trampoline_t startup,
+ * pthread_startroutine_t entrypt, pthread_addr_t arg)
*
* At this point, the following values are saved in context:
*
* R0 = SYS_pthread_start
- * R1 = entrypt
- * R2 = arg
+ * R1 = startup
+ * R2 = entrypt
+ * R3 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup & ~1;
+ regs[REG_PC] = (uint32_t)regs[REG_R1] & ~1; /* startup */
Review comment:
I verified that the code is correct as it is. The PROTECTED build stm32f4discovery:kostest works correctly as 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] masayuki2009 commented on pull request #3626: libc: Move pthread staff to userspace
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #3626:
URL: https://github.com/apache/incubator-nuttx/pull/3626#issuecomment-828954262
https://github.com/apache/incubator-nuttx/pull/3627
--
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