On Thu, May 7, 2026 at 9:48 AM Peter Maydell <[email protected]> wrote:
> 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 > Yea, I was confused by that and opted to 'return negative target errno' notion here, but adding 'errno = ret;' just before the ret = -1 in the existing code would keep get_errno() happy... But in reality do the same thing I submitted... I could also just leave the bug alone as part of my submission. > 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). > Yea, unwinding that looks beyond what I have time for here. > > + 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. > Yea, it is short enough I think inlining isn't terrible, but if more of these cases grow, then refactoring to a routine would make sense. At the moment, I lean more towards inlining... Warner > > } > > pthread_mutex_unlock(&info.mutex); > > pthread_cond_destroy(&info.cond); > > thanks > -- PMM >
