From: Érico Rolim <ericonr@disroot.org> add general helper __proctidcomm to assemble the path to where the thread name is stored, and take the opportunity to add O_CLOEXEC flag to open() in pthread_setname_np. --- I added the proctidcomm helper so information wouldn't be duplicated in multiple places; same with the THREAD_NAME_PATH_SIZE macro. I could turn proctidcomm into a macro, if you want. Tested with the following C program: #define _GNU_SOURCE #include <pthread.h> #include <stdio.h> #include <errno.h> #include <unistd.h> #include <string.h> void *me(void *p) { pause(); } int main() { char n[16]; pthread_t t; printf("pid: %ld\n", (long)getpid()); pthread_setname_np(pthread_self(), "hello"); errno = pthread_getname_np(pthread_self(), n, sizeof n); perror("getname"); puts(n); pthread_create(&t, 0, me, 0); errno = pthread_setname_np(t, "long name oh boooooy!"); perror("setname other"); errno = pthread_setname_np(t, "value"); perror("setname other 2"); /* check that the string is cut off at the right size */ strcpy(n, "value431"); errno = pthread_getname_np(t, n, sizeof n); perror("getname other"); puts(n); pause(); } include/pthread.h | 1 + src/internal/proctidcomm.c | 8 ++++++++ src/internal/pthread_impl.h | 3 +++ src/thread/pthread_getname_np.c | 26 ++++++++++++++++++++++++++ src/thread/pthread_setname_np.c | 6 +++--- 5 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 src/internal/proctidcomm.c create mode 100644 src/thread/pthread_getname_np.c diff --git a/include/pthread.h b/include/pthread.h index 0492f26a..89fd9ff7 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -221,6 +221,7 @@ int pthread_getaffinity_np(pthread_t, size_t, struct cpu_set_t *); int pthread_setaffinity_np(pthread_t, size_t, const struct cpu_set_t *); int pthread_getattr_np(pthread_t, pthread_attr_t *); int pthread_setname_np(pthread_t, const char *); +int pthread_getname_np(pthread_t, char *, size_t); int pthread_getattr_default_np(pthread_attr_t *); int pthread_setattr_default_np(const pthread_attr_t *); int pthread_tryjoin_np(pthread_t, void **); diff --git a/src/internal/proctidcomm.c b/src/internal/proctidcomm.c new file mode 100644 index 00000000..91e81e16 --- /dev/null +++ b/src/internal/proctidcomm.c @@ -0,0 +1,8 @@ +#include <stdio.h> + +#include "pthread_impl.h" + +void __proctidcomm(char *buf, int tid) +{ + snprintf(buf, THREAD_NAME_PATH_SIZE, "/proc/self/task/%d/comm", tid); +} diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h index de2b9d8b..5cb3b74a 100644 --- a/src/internal/pthread_impl.h +++ b/src/internal/pthread_impl.h @@ -194,6 +194,9 @@ extern hidden volatile int __abort_lock[1]; extern hidden unsigned __default_stacksize; extern hidden unsigned __default_guardsize; +#define THREAD_NAME_PATH_SIZE (sizeof "/proc/self/task//comm" + 3*sizeof(int)) +hidden void __proctidcomm(char *, int); + #define DEFAULT_STACK_SIZE 131072 #define DEFAULT_GUARD_SIZE 8192 diff --git a/src/thread/pthread_getname_np.c b/src/thread/pthread_getname_np.c new file mode 100644 index 00000000..60e6fd4e --- /dev/null +++ b/src/thread/pthread_getname_np.c @@ -0,0 +1,26 @@ +#define _GNU_SOURCE +#include <fcntl.h> +#include <unistd.h> +#include <sys/prctl.h> + +#include "pthread_impl.h" + +int pthread_getname_np(pthread_t thread, char *name, size_t len) +{ + int fd, cs, status = 0; + char f[THREAD_NAME_PATH_SIZE]; + + if (len < 16) return ERANGE; + + if (thread == pthread_self()) + return prctl(PR_GET_NAME, (unsigned long)name, 0UL, 0UL, 0UL) ? errno : 0; + + __proctidcomm(f, thread->tid); + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); + if ((fd = open(f, O_RDONLY|O_CLOEXEC)) < 0 || (len = read(fd, name, len)) < 0) status = errno; + if (fd >= 0) close(fd); + pthread_setcancelstate(cs, 0); + /* remove trailing new line */ + name[len-1] = 0; + return status; +} diff --git a/src/thread/pthread_setname_np.c b/src/thread/pthread_setname_np.c index 82d35e17..6f53f408 100644 --- a/src/thread/pthread_setname_np.c +++ b/src/thread/pthread_setname_np.c @@ -9,7 +9,7 @@ int pthread_setname_np(pthread_t thread, const char *name) { int fd, cs, status = 0; - char f[sizeof "/proc/self/task//comm" + 3*sizeof(int)]; + char f[THREAD_NAME_PATH_SIZE]; size_t len; if ((len = strnlen(name, 16)) > 15) return ERANGE; @@ -17,9 +17,9 @@ int pthread_setname_np(pthread_t thread, const char *name) if (thread == pthread_self()) return prctl(PR_SET_NAME, (unsigned long)name, 0UL, 0UL, 0UL) ? errno : 0; - snprintf(f, sizeof f, "/proc/self/task/%d/comm", thread->tid); + __proctidcomm(f, thread->tid); pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); - if ((fd = open(f, O_WRONLY)) < 0 || write(fd, name, len) < 0) status = errno; + if ((fd = open(f, O_WRONLY|O_CLOEXEC)) < 0 || write(fd, name, len) < 0) status = errno; if (fd >= 0) close(fd); pthread_setcancelstate(cs, 0); return status; -- 2.29.2
From: Érico Rolim <ericonr@disroot.org> --- include/pthread.h | 1 + src/thread/pthread_getname_np.c | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 src/thread/pthread_getname_np.c diff --git a/include/pthread.h b/include/pthread.h index 0492f26a..89fd9ff7 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -221,6 +221,7 @@ int pthread_getaffinity_np(pthread_t, size_t, struct cpu_set_t *); int pthread_setaffinity_np(pthread_t, size_t, const struct cpu_set_t *); int pthread_getattr_np(pthread_t, pthread_attr_t *); int pthread_setname_np(pthread_t, const char *); +int pthread_getname_np(pthread_t, char *, size_t); int pthread_getattr_default_np(pthread_attr_t *); int pthread_setattr_default_np(const pthread_attr_t *); int pthread_tryjoin_np(pthread_t, void **); diff --git a/src/thread/pthread_getname_np.c b/src/thread/pthread_getname_np.c new file mode 100644 index 00000000..9eecc17e --- /dev/null +++ b/src/thread/pthread_getname_np.c @@ -0,0 +1,25 @@ +#define _GNU_SOURCE +#include <fcntl.h> +#include <unistd.h> +#include <sys/prctl.h> + +#include "pthread_impl.h" + +int pthread_getname_np(pthread_t thread, char *name, size_t len) +{ + int fd, cs, status = 0; + char f[sizeof "/proc/self/task//comm" + 3*sizeof(int)]; + + if (len < 16) return ERANGE; + + if (thread == pthread_self()) + return prctl(PR_GET_NAME, (unsigned long)name, 0UL, 0UL, 0UL) ? errno : 0; + + snprintf(f, sizeof f, "/proc/self/task/%d/comm", thread->tid); + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); + if ((fd = open(f, O_RDONLY|O_CLOEXEC)) < 0 || (len = read(fd, name, len)) < 0) status = errno; + else name[len-1] = 0; /* remove trailing new line */ + if (fd >= 0) close(fd); + pthread_setcancelstate(cs, 0); + return status; +} -- 2.30.2
From: Érico Rolim <ericonr@disroot.org> based on the pthread_setname_np implementation --- I might have forgotten to send this final version here include/pthread.h | 1 + src/thread/pthread_getname_np.c | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 src/thread/pthread_getname_np.c diff --git a/include/pthread.h b/include/pthread.h index 0492f26a..89fd9ff7 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -221,6 +221,7 @@ int pthread_getaffinity_np(pthread_t, size_t, struct cpu_set_t *); int pthread_setaffinity_np(pthread_t, size_t, const struct cpu_set_t *); int pthread_getattr_np(pthread_t, pthread_attr_t *); int pthread_setname_np(pthread_t, const char *); +int pthread_getname_np(pthread_t, char *, size_t); int pthread_getattr_default_np(pthread_attr_t *); int pthread_setattr_default_np(const pthread_attr_t *); int pthread_tryjoin_np(pthread_t, void **); diff --git a/src/thread/pthread_getname_np.c b/src/thread/pthread_getname_np.c new file mode 100644 index 00000000..48d1a294 --- /dev/null +++ b/src/thread/pthread_getname_np.c @@ -0,0 +1,25 @@ +#define _GNU_SOURCE +#include <fcntl.h> +#include <unistd.h> +#include <sys/prctl.h> + +#include "pthread_impl.h" + +int pthread_getname_np(pthread_t thread, char *name, size_t len) +{ + int fd, cs, status = 0; + char f[sizeof "/proc/self/task//comm" + 3*sizeof(int)]; + + if (len < 16) return ERANGE; + + if (thread == pthread_self()) + return prctl(PR_GET_NAME, (unsigned long)name, 0UL, 0UL, 0UL) ? errno : 0; + + snprintf(f, sizeof f, "/proc/self/task/%d/comm", thread->tid); + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); + if ((fd = open(f, O_RDONLY|O_CLOEXEC)) < 0 || (len = read(fd, name, len)) < 0) status = errno; + else name[len-1] = 0; /* remove trailing new line only if successful */ + if (fd >= 0) close(fd); + pthread_setcancelstate(cs, 0); + return status; +} -- 2.31.1
these macros are used to indicate that the implementation uses, respectively, utf-16 and utf-32 encoding for char16_t and char32_t. --- don't change features.h to include stdc-predef.h anymore include/stdc-predef.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/stdc-predef.h b/include/stdc-predef.h index f8cd4b89..af1a2799 100644 --- a/include/stdc-predef.h +++ b/include/stdc-predef.h @@ -7,4 +7,7 @@ #define __STDC_IEC_559__ 1 #endif +#define __STDC_UTF_16__ 1 +#define __STDC_UTF_32__ 1 + #endif -- 2.31.1
From: Érico Rolim <erico.erc@gmail.com> Implement part of the glibc behavior, where the 32-bit identifier stored in /etc/hostid, if the file exists, is returned. If this file doesn't contain at least 32 bits or can't be opened for some reason, return 0. --- src/misc/gethostid.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/misc/gethostid.c b/src/misc/gethostid.c index 25bb35db..d529de9c 100644 --- a/src/misc/gethostid.c +++ b/src/misc/gethostid.c @@ -1,6 +1,19 @@ #include <unistd.h> +#include <stdio.h> +#include <stdint.h> long gethostid() { - return 0; + FILE *f; + int32_t rv = 0; + + f = fopen("/etc/hostid", "reb"); + if (f) { + if (fread(&rv, sizeof(rv), 1, f) == 0) { + rv = 0; + } + fclose(f); + } + + return rv; } -- 2.31.1
the function already returns (void *) --- ldso/dynlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ldso/dynlink.c b/ldso/dynlink.c index 8b67ef59..5b9c8be4 100644 --- a/ldso/dynlink.c +++ b/ldso/dynlink.c @@ -1831,7 +1831,7 @@ void __dls3(size_t *sp, size_t *auxv) dprintf(2, "%s: cannot load %s: %s\n", ldname, argv[0], strerror(errno)); _exit(1); } - Ehdr *ehdr = (void *)map_library(fd, &app); + Ehdr *ehdr = map_library(fd, &app); if (!ehdr) { dprintf(2, "%s: %s: Not a valid dynamic program\n", ldname, argv[0]); _exit(1); -- 2.31.1
when building for armhf, this makes libc.so text smaller by 4 bytes: 606619 to 606615 --- src/string/arm/__aeabi_memset.s | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/string/arm/__aeabi_memset.s b/src/string/arm/__aeabi_memset.s index f9f60583..980774e8 100644 --- a/src/string/arm/__aeabi_memset.s +++ b/src/string/arm/__aeabi_memset.s @@ -24,8 +24,7 @@ __aeabi_memset: cmp r1, #0 beq 2f adds r1, r0, r1 -1: strb r2, [r0] - adds r0, r0, #1 +1: strb r2, [r0], #1 cmp r1, r0 bne 1b 2: bx lr -- 2.31.1
* Érico Nogueira <ericonr@disroot.org> [2021-04-20 16:15:19 -0300]: > when building for armhf, this makes libc.so text smaller by 4 bytes: > 606619 to 606615 > --- > src/string/arm/__aeabi_memset.s | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/src/string/arm/__aeabi_memset.s b/src/string/arm/__aeabi_memset.s > index f9f60583..980774e8 100644 > --- a/src/string/arm/__aeabi_memset.s > +++ b/src/string/arm/__aeabi_memset.s > @@ -24,8 +24,7 @@ __aeabi_memset: > cmp r1, #0 > beq 2f > adds r1, r0, r1 > -1: strb r2, [r0] > - adds r0, r0, #1 > +1: strb r2, [r0], #1 this is not available before armv7 as thumb instruction (and it has 32bit thumb encoding, so you replace two 16bit instructions with a 32bit one.) normally this asm is compiled in arm mode even if your toolchain defaults to thumb (i'm not sure why), but if you select a cpu or arch that only supports thumb then the assembler will try to use thumb and fail e.g. on -march=armv6-m (but i'm not sure if musl supports that compilation mode throughout) > cmp r1, r0 > bne 1b > 2: bx lr > -- > 2.31.1
On Wed, Apr 21, 2021 at 4:25 AM Szabolcs Nagy <nsz@port70.net> wrote: > > * Érico Nogueira <ericonr@disroot.org> [2021-04-20 16:15:19 -0300]: > > when building for armhf, this makes libc.so text smaller by 4 bytes: > > 606619 to 606615 > > --- > > src/string/arm/__aeabi_memset.s | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/src/string/arm/__aeabi_memset.s b/src/string/arm/__aeabi_memset.s > > index f9f60583..980774e8 100644 > > --- a/src/string/arm/__aeabi_memset.s > > +++ b/src/string/arm/__aeabi_memset.s > > @@ -24,8 +24,7 @@ __aeabi_memset: > > cmp r1, #0 > > beq 2f > > adds r1, r0, r1 > > -1: strb r2, [r0] > > - adds r0, r0, #1 > > +1: strb r2, [r0], #1 > > this is not available before armv7 as thumb instruction (and it > has 32bit thumb encoding, so you replace two 16bit instructions > with a 32bit one.) > > normally this asm is compiled in arm mode even if your toolchain > defaults to thumb (i'm not sure why), but if you select a cpu or > arch that only supports thumb then the assembler will try to use > thumb and fail e.g. on -march=armv6-m (but i'm not sure if musl > supports that compilation mode throughout) > Musl currently does not build on Thumb1-only devices (Armv6-m and Armv8-m.base). I submitted a patch some time ago to allow it to build, though life and other obstacles have made me much too slow in actually getting a functioning toolchain together to test it properly. > > cmp r1, r0 > > bne 1b > > 2: bx lr > > -- > > 2.31.1
On Wed, Apr 21, 2021 at 10:24:58AM +0200, Szabolcs Nagy wrote:
> * Érico Nogueira <ericonr@disroot.org> [2021-04-20 16:15:19 -0300]:
> > when building for armhf, this makes libc.so text smaller by 4 bytes:
> > 606619 to 606615
> > ---
> > src/string/arm/__aeabi_memset.s | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/src/string/arm/__aeabi_memset.s b/src/string/arm/__aeabi_memset.s
> > index f9f60583..980774e8 100644
> > --- a/src/string/arm/__aeabi_memset.s
> > +++ b/src/string/arm/__aeabi_memset.s
> > @@ -24,8 +24,7 @@ __aeabi_memset:
> > cmp r1, #0
> > beq 2f
> > adds r1, r0, r1
> > -1: strb r2, [r0]
> > - adds r0, r0, #1
> > +1: strb r2, [r0], #1
>
> this is not available before armv7 as thumb instruction (and it
> has 32bit thumb encoding, so you replace two 16bit instructions
> with a 32bit one.)
>
> normally this asm is compiled in arm mode even if your toolchain
> defaults to thumb (i'm not sure why), but if you select a cpu or
> arch that only supports thumb then the assembler will try to use
> thumb and fail e.g. on -march=armv6-m (but i'm not sure if musl
> supports that compilation mode throughout)
Should we hold off on doing anything about this for now then? I'd
rather avoid making more work for future, and this is pure *junk* code
that we do not expect to be called from anywhere (it's extremely slow)
and only there to satisfy broken tooling generating calls to it rather
than to the standard functions.
Rich
Em 21/04/2021 14:38, Rich Felker escreveu: > On Wed, Apr 21, 2021 at 10:24:58AM +0200, Szabolcs Nagy wrote: >> * Érico Nogueira <ericonr@disroot.org> [2021-04-20 16:15:19 -0300]: >>> when building for armhf, this makes libc.so text smaller by 4 bytes: >>> 606619 to 606615 >>> --- >>> src/string/arm/__aeabi_memset.s | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/src/string/arm/__aeabi_memset.s b/src/string/arm/__aeabi_memset.s >>> index f9f60583..980774e8 100644 >>> --- a/src/string/arm/__aeabi_memset.s >>> +++ b/src/string/arm/__aeabi_memset.s >>> @@ -24,8 +24,7 @@ __aeabi_memset: >>> cmp r1, #0 >>> beq 2f >>> adds r1, r0, r1 >>> -1: strb r2, [r0] >>> - adds r0, r0, #1 >>> +1: strb r2, [r0], #1 >> >> this is not available before armv7 as thumb instruction (and it >> has 32bit thumb encoding, so you replace two 16bit instructions >> with a 32bit one.) >> >> normally this asm is compiled in arm mode even if your toolchain >> defaults to thumb (i'm not sure why), but if you select a cpu or >> arch that only supports thumb then the assembler will try to use >> thumb and fail e.g. on -march=armv6-m (but i'm not sure if musl >> supports that compilation mode throughout) > > Should we hold off on doing anything about this for now then? I'd > rather avoid making more work for future, and this is pure *junk* code > that we do not expect to be called from anywhere (it's extremely slow) > and only there to satisfy broken tooling generating calls to it rather > than to the standard functions. That's ok for me. I was just browsing this file for some reason and noted the potential to "simplify" it. That said, src/string/arm/memcpy.S also uses this addressing mode, so it is probably relevant to watch out for it for an eventual port: /* align source to 32 bits. We need to insert 2 instructions between * a ldr[b|h] and str[b|h] because byte and half-word instructions * stall 2 cycles. */ movs r12, r3, lsl #31 sub r2, r2, r3 /* we know that r3 <= r2 because r2 >= 4 */ ldrbmi r3, [r1], #1 ldrbcs r4, [r1], #1 ldrbcs r12,[r1], #1 strbmi r3, [r0], #1 strbcs r4, [r0], #1 strbcs r12,[r0], #1 > > Rich
Hi, On 2021-04-20 21:15, Érico Nogueira wrote: > From: Érico Rolim <ericonr@disroot.org> > > based on the pthread_setname_np implementation > --- > > I might have forgotten to send this final version here > > include/pthread.h | 1 + > src/thread/pthread_getname_np.c | 25 +++++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > create mode 100644 src/thread/pthread_getname_np.c > > diff --git a/include/pthread.h b/include/pthread.h > index 0492f26a..89fd9ff7 100644 > --- a/include/pthread.h > +++ b/include/pthread.h > @@ -221,6 +221,7 @@ int pthread_getaffinity_np(pthread_t, size_t, > struct cpu_set_t *); > int pthread_setaffinity_np(pthread_t, size_t, const struct cpu_set_t *); > int pthread_getattr_np(pthread_t, pthread_attr_t *); > int pthread_setname_np(pthread_t, const char *); > +int pthread_getname_np(pthread_t, char *, size_t); > int pthread_getattr_default_np(pthread_attr_t *); > int pthread_setattr_default_np(const pthread_attr_t *); > int pthread_tryjoin_np(pthread_t, void **); > diff --git a/src/thread/pthread_getname_np.c b/src/thread/pthread_getname_np.c > new file mode 100644 > index 00000000..48d1a294 > --- /dev/null > +++ b/src/thread/pthread_getname_np.c > @@ -0,0 +1,25 @@ > +#define _GNU_SOURCE > +#include <fcntl.h> > +#include <unistd.h> > +#include <sys/prctl.h> > + > +#include "pthread_impl.h" > + > +int pthread_getname_np(pthread_t thread, char *name, size_t len) > +{ > + int fd, cs, status = 0; > + char f[sizeof "/proc/self/task//comm" + 3*sizeof(int)]; > + > + if (len < 16) return ERANGE; > + > + if (thread == pthread_self()) > + return prctl(PR_GET_NAME, (unsigned long)name, 0UL, 0UL, 0UL) ? errno : 0; > + > + snprintf(f, sizeof f, "/proc/self/task/%d/comm", thread->tid); > + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); > + if ((fd = open(f, O_RDONLY|O_CLOEXEC)) < 0 || (len = read(fd, name, > len)) < 0) status = errno; > + else name[len-1] = 0; /* remove trailing new line only if successful */ It seems that Linux indeed always adds a newline, so removing it unconditionally seems fine. > + if (fd >= 0) close(fd); > + pthread_setcancelstate(cs, 0); > + return status; > +} Looks good to me: Reviewed-by: Stefan Agner <stefan@agner.ch> FWIW, We carry basically the same patch for musl to build TensorFlow and I was about to post it. -- Stefan
On 2021-04-20, Érico Nogueira <ericonr@disroot.org> wrote:
> + if ((fd = open(f, O_RDONLY|O_CLOEXEC)) < 0 || (len = read(fd, name, len)) < 0) status = errno;
This read error check isn't quite right. len has type size_t (which is
unsigned), so (len = ...) < 0 is always false. I think you probably
need to use a separate variable here.