On Thu, 7 May 2026 at 16:20, Warner Losh <[email protected]> wrote:
>
> Fix one of the TODO items when creating a new thread: release the copied
> cpu and free the task state.
>
> Signed-off-by: Warner Losh <[email protected]>
> ---
> Free the new task state and drop references to copied cpu structure when
> pthread_create failes.
> ---
> linux-user/syscall.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d3d9fffb54..7b2e32bcf5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7005,7 +7005,6 @@ static int do_fork(CPUArchState *env, unsigned int
> flags, abi_ulong newsp,
> cpu->random_seed = qemu_guest_random_seed_thread_part1();
>
> ret = pthread_create(&info.thread, &attr, clone_func, &info);
> - /* TODO: Free new CPU state if thread creation failed. */
>
> sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
> pthread_attr_destroy(&attr);
> @@ -7014,7 +7013,10 @@ static int do_fork(CPUArchState *env, unsigned int
> flags, abi_ulong newsp,
> pthread_cond_wait(&info.cond, &info.mutex);
> ret = info.tid;
> } else {
> - ret = -1;
> + ret = -host_to_target_errno(ret);
This is an unrelated bugfix to the one in the commit message.
I found do_fork()'s error handling confusing (the comment at
the top doesn't exactly clarify either), but I think it is:
* might return a negative target errno value
* might return -1 with "errno" set to a host errno
* might succeed and return 0
* might succeed and return some other not-minus-1 value
The callers all do
return get_errno(do_fork(....));
which handles these cases.
It might be less confusing overall if we simplified this to:
* returns negative target errno value on failure
* otherwise success, returning value the guest expects
and dropped the get_errno() at the callsites. The only
cases that don't already follow one of those two options are
the "pthread_create() failed" one (which is buggy currently
since pthread_create() doesn't set errno) and the "fork()
failed" one (which relies on the "return -1 and caller fishes
error out of errno" handling).
> + object_unparent(OBJECT(new_cpu));
> + object_unref(OBJECT(new_cpu));
> + g_free(ts);
For TARGET_AARCH64, the gcs_new_stack() call we did before
attempting the pthread_create() maps memory into the target
address space, which needs to be undone. Compare the bit in
TARGET_NR_exit handling which does:
#ifdef TARGET_AARCH64
if (ts->gcs_base) {
target_munmap(ts->gcs_base, ts->gcs_size);
}
#endif
We could maybe share code between the NR_exit "free stuff
for this TaskState" and this error-exit path, though it's
a bit awkward so I'm 50:50 on that.
> }
> pthread_mutex_unlock(&info.mutex);
> pthread_cond_destroy(&info.cond);
thanks
-- PMM
