This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Linux: Add gettid system call wrapper [BZ #6399]
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Fri, 14 Dec 2018 16:10:00 -0500
- Subject: Re: [PATCH] Linux: Add gettid system call wrapper [BZ #6399]
- References: <87ftvc2qjo.fsf@oldenburg2.str.redhat.com>
On 12/5/18 5:53 AM, Florian Weimer wrote:
> This commit adds gettid to <unistd.h> on Linux, and not to the
> kernel-independent GNU API.
NEWS:
+* On Linux, the gettid function has been added.
OK.
Overall this looks good to me, regarding unistd_ext.h, that also looks
OK to me and is an internal implementation detail we can refine if we
find commonality among other headers and a different structure appears
that we like better.
This patch needs a manual entry for gettid. Post a v2 with that and I
can go ahead and ACK this, conditional on the other 2 manual patches
going in, then we'll have:
* Documentation for a tid.
* Documentation for sched_* issues.
* Wrapper and documentation for gettid.
That would be all we need and we'd have a new wrapper for gettid().
> 2018-12-05 Florian Weimer <fweimer@redhat.com>
>
> [BZ #6399]
> Linux: Add gettid system call wrapper.
> * posix/Makefile (headers): Add bits/unistd_ext.h.
> * posix/bits/unistd_ext.h: New file.
> * posix/unistd.h: Include it.
> * sysdeps/unix/sysv/linux/Makefile [subdir == misc] (tests): Add
> tst-gettid.
> (tst-gettid): Link with $(shared-thread-library).
> * sysdeps/unix/sysv/linux/Version (GLIBC_2.29): Export gettid.
> * sysdeps/unix/sysv/linux/bits/unistd_ext.h: New file.
> * sysdeps/unix/sysv/linux/bits/syscalls.list (gettid): Add.
> * sysdeps/unix/sysv/linux/bits/tst-gettid.c: New file.
> * sysdeps/unix/sysv/linux/aarch64/libc.abilist (GLIBC_2.29):
> Add gettid.
[snip abilist]
> * sysdeps/unix/sysv/linux/tst-setgetname.c (gettid): Remove.
>
> diff --git a/posix/Makefile b/posix/Makefile
> index a8fb1e1839..49d7a91b77 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -31,7 +31,8 @@ headers := sys/utsname.h sys/times.h sys/wait.h sys/types.h unistd.h \
> bits/local_lim.h tar.h bits/utsname.h bits/confname.h \
> bits/waitflags.h bits/waitstatus.h sys/unistd.h sched.h \
> bits/sched.h bits/cpu-set.h re_comp.h wait.h bits/environments.h \
> - cpio.h spawn.h bits/unistd.h bits/types/struct_sched_param.h
> + cpio.h spawn.h bits/unistd.h bits/types/struct_sched_param.h \
> + bits/unistd_ext.h
OK.
>
> routines := \
> uname \
> diff --git a/posix/bits/unistd_ext.h b/posix/bits/unistd_ext.h
> new file mode 100644
> index 0000000000..8019cfa758
> --- /dev/null
> +++ b/posix/bits/unistd_ext.h
> @@ -0,0 +1,21 @@
> +/* System-specific extensions of <unistd.h>, generic version.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#ifndef _UNISTD_H
> +# error "Never include <bits/unistd_ext.h> directly; use <unistd.h> instead."
> +#endif
OK.
> diff --git a/posix/unistd.h b/posix/unistd.h
> index a8cf28b5e7..127ed494ce 100644
> --- a/posix/unistd.h
> +++ b/posix/unistd.h
> @@ -1166,6 +1166,9 @@ int getentropy (void *__buffer, size_t __length) __wur;
> # include <bits/unistd.h>
> #endif
>
> +/* System-specific extensions. */
> +#include <bits/unistd_ext.h>
OK. I think this idea in general is OK. We have used types/foo.h for type split out
from headers, but have not generalized splitting out extensions like this.
> +
> __END_DECLS
>
> #endif /* unistd.h */
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 362cf3b950..99884799e6 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -48,7 +48,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
> tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
> tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
> test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
> - tst-rlimit-infinity tst-ofdlocks
> + tst-rlimit-infinity tst-ofdlocks tst-gettid
OK.
> tests-internal += tst-ofdlocks-compat
>
>
> @@ -100,6 +100,8 @@ $(objpfx)tst-sysconf-iov_max: $(objpfx)tst-sysconf-iov_max-uapi.o
>
> $(objpfx)tst-pkey: $(shared-thread-library)
>
> +$(objpfx)tst-gettid: $(shared-thread-library)
OK.
> +
> endif # $(subdir) == misc
>
> ifeq ($(subdir),time)
> diff --git a/sysdeps/unix/sysv/linux/Versions b/sysdeps/unix/sysv/linux/Versions
> index 336c13b57d..6b5cd99b51 100644
> --- a/sysdeps/unix/sysv/linux/Versions
> +++ b/sysdeps/unix/sysv/linux/Versions
> @@ -171,6 +171,9 @@ libc {
> mlock2;
> pkey_alloc; pkey_free; pkey_set; pkey_get; pkey_mprotect;
> }
> + GLIBC_2.29 {
> + gettid;
> + }
OK.
> GLIBC_PRIVATE {
> # functions used in other libraries
> __syscall_rt_sigqueueinfo;
[snip ablist diffs]
> diff --git a/sysdeps/unix/sysv/linux/bits/unistd_ext.h b/sysdeps/unix/sysv/linux/bits/unistd_ext.h
> new file mode 100644
> index 0000000000..5f964bca94
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/bits/unistd_ext.h
> @@ -0,0 +1,32 @@
> +/* System-specific extensions of <unistd.h>, Linux version.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#ifndef _UNISTD_H
> +# error "Never include <bits/unistd_ext.h> directly; use <unistd.h> instead."
> +#endif
> +
> +#ifdef __USE_GNU
OK, makse sense to be under USE_GNU.
> +
> +/* Return the kernel thread ID (TID) of the current thread. The
> + returned value is not subject to caching. Note that a TID uniquely
> + identifies a thread only while this thread is running; a TID can be
> + reused once a thread has exited, even if the thread is not detached
> + and has not been joined. */
> +extern __pid_t gettid (void) __THROW;
OK.
> +
> +#endif
[snip abilist diffs]
> diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
> index e24ea29e35..544ab5d54a 100644
> --- a/sysdeps/unix/sysv/linux/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/syscalls.list
> @@ -112,3 +112,4 @@ process_vm_writev EXTRA process_vm_writev i:ipipii process_vm_writev
> memfd_create EXTRA memfd_create i:si memfd_create
> pkey_alloc EXTRA pkey_alloc i:ii pkey_alloc
> pkey_free EXTRA pkey_free i:i pkey_free
> +gettid EXTRA gettid Ei: __gettid gettid
OK.
> diff --git a/sysdeps/unix/sysv/linux/tst-gettid.c b/sysdeps/unix/sysv/linux/tst-gettid.c
> new file mode 100644
> index 0000000000..78bbaee4f3
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-gettid.c
> @@ -0,0 +1,79 @@
> +/* Smoke test for the gettid system call.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <support/check.h>
> +#include <support/namespace.h>
> +#include <support/xthread.h>
> +#include <support/xunistd.h>
> +
> +/* TID of the initial (main) thread. */
> +static pid_t initial_tid;
> +
> +/* Check that PID and TID are the same in a subprocess. */
> +static void
> +subprocess (void *closure)
> +{
> + TEST_COMPARE (getpid (), gettid ());
> + TEST_VERIFY (gettid () != initial_tid);
OK.
> +}
> +
> +/* Check that the TID changes in a new thread. */
> +static void *
> +threadfunc (void *closure)
> +{
> + TEST_VERIFY (getpid () != gettid ());
> + TEST_VERIFY (gettid () != initial_tid);
OK.
> + return NULL;
> +}
> +
> +/* Check for interactions with vfork. */
> +static void
> +test_vfork (void)
> +{
> + pid_t proc = vfork ();
> + if (proc == 0)
> + {
> + if (getpid () != gettid ())
> + _exit (1);
> + if (gettid () == initial_tid)
> + _exit (2);
OK.
> + _exit (0);
> + }
> + int status;
> + xwaitpid (proc, &status, 0);
> + TEST_COMPARE (status, 0);
> +}
> +
> +static int
> +do_test (void)
> +{
> + initial_tid = gettid ();
> +
> + /* The main thread has the same TID as the PID. */
> + TEST_COMPARE (getpid (), gettid ());
> +
> + test_vfork ();
> +
> + support_isolate_in_subprocess (subprocess, NULL);
> +
> + xpthread_join (xpthread_create (NULL, threadfunc, NULL));
OK.
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/tst-setgetname.c b/sysdeps/unix/sysv/linux/tst-setgetname.c
> index b80ff59074..95d8bb9ba4 100644
> --- a/sysdeps/unix/sysv/linux/tst-setgetname.c
> +++ b/sysdeps/unix/sysv/linux/tst-setgetname.c
> @@ -43,12 +43,6 @@
> terminator. */
> #define TASK_COMM_LEN 16
>
> -long
> -gettid (void)
> -{
> - return syscall(__NR_gettid);
> -}
OK.
> -
> /* On Linux we can read this task's name from /proc. */
> int
> get_self_comm (long tid, char *buf, size_t len)
[snip abilist diffs]
--
Cheers,
Carlos.