libc: make pthread_getname_np and _pthread_getname_np available

[minsoochoo0122_proton.me]

Summary

In order to import jemalloc 5.3.0, _pthread_getname_np should be present in libc. Because the process requires adding new symbol to libc, I created a new revision rather than incorporating this change in the commit that import jemalloc 5.3.0.

Test Plan

make buildworld and make installworld.

[imp]

I wonder why these were not exported... Is there any indication as to why? Was it intentoinal or an oversight?

[jrtc27]

In D41461#944269, @imp wrote:

I wonder why these were not exported... Is there any indication as to why? Was it intentoinal or an oversight?

@kib? It's from https://reviews.freebsd.org/D25117. FWIW there are a lot of pthread_* symbols not exported; a crude script^1 gives me https://termbin.com/u2yb.

^1: Diff'ing awk '/{/{v=$1} /\<pthread_/{gsub(/;$/, "", $1); print $1"@"v}' | sort for the two map files

lib/libc/gen/Symbol.map

443

Should the version match libthr's? All other pthread_* symbols do.

[minsoochoo0122_proton.me]

In D41461#944269, @imp wrote:

I wonder why these were not exported... Is there any indication as to why? Was it intentoinal or an oversight?

I assume that it's because _pthread_getname_np was not used in libc before.
libthr functions that are exported to libc have names thr_* for definition and declaration, __weak_reference macro pthread_* for public functions and _pthread_* for private functions.
For functions that are not exported to libc (because libc does not need it internally), functions are declared as _pthread_* and __weak_reference macro to pthread_*

When a function is exported to libc, both public and private functions are exported, though I don't know the reason behind that. (Maybe DUAL_ENTRY is related? I need to figure out more)

I referred to the implementation of pthread_join and _pthread_join exports in libc.

[minsoochoo0122_proton.me]

pthread_getname_np goes to FBSD_1.6 to match version in pthread.map
pthread_getname_np STUB_FUNC3 not STUB_FUNC2 since it takes 3 parameters

[kib]

lib/libc/gen/Symbol.map

443

Yes it must.

491

I do not think this is needed/useful.

lib/libthr/pthread.map

197 ↗

(On Diff #126026)

Same question, why?

[minsoochoo0122_proton.me]

lib/libc/gen/Symbol.map

491

I'm importing jemalloc 5.3.0 (D41421) which requires _pthread_getname_np in contrib/jemalloc/prof_sys.c. (The original code uses pthread_getname_np but it is patched by contrib/jemalloc/include/jemalloc/jemalloc_FreeBSD.h to use _pthread_getname_np.

lib/libthr/pthread.map

197 ↗

(On Diff #126026)

Same reason as above.

[kib]

lib/libc/gen/Symbol.map

491

If inside libc, you do not need to export the symbol. Also I think it better to stop using the namespacing, it really does not work as intended.

[minsoochoo0122_proton.me]

lib/libc/gen/Symbol.map

491

pthread_getname_np is only used for internal use by libc, so should I remove pthread_getname_np from the symbol map?
And I don't understand which namespacing you are referring to. Can you explain it more in detail please? (sorry, I'm a newbie here)

[kib]

lib/libc/gen/Symbol.map

491

Since you add the jump slot for it, why not export?

Namespacing is the trick where exported functions are renamed in headers by prepending the underscore before the usual name. Idea, as I understand, was that normal symbols like open() are supposed to be interposed and exported as weak symbols, while underscored symbols are non-weak. Then libc using _XXX symbols should get some protection against applications interposing system interfaces and change libc behavior.

Of course this was a failure because 1. weak is not supposed to behave this way in ELF 2. _[a-z] is still app namespace and can be legitimately exported by apps, thus interposing libc symbols 3. use of namespacing is not consistent, and it is hard to decide when we want to allow interposing (like malloc(3)) and when we do not want to.

What you see above are remnants of this experiment. Also see lib/libc/include/{,un-}namespace.h

[minsoochoo0122_proton.me]

Don't export _pthread_getname_np

[kib]

lib/libc/gen/_pthread_stubs.c

294

Don't you need to do something more elaborate than just return success? The caller expects at least the zero-length string saved in the 'name'.

[jrtc27]

lib/libc/gen/_pthread_stubs.c

294

And ESRCH if passed anything other than &main_thread?

[kib]

lib/libc/gen/_pthread_stubs.c

294

There is no other threads than main when this function could be called. OTOH stub for thread_self() effectively returns NULL which is not equal to the value returned after libthr load.

[minsoochoo0122_proton.me]

lib/libc/gen/_pthread_stubs.c

294

Oops. I thought it returns void like pthread_get_name_np. It should return 0 in success otherwise return error code.

[minsoochoo0122_proton.me]

pthread_getname_np returns 0 for success otherwise error code.

[jrtc27]

In D41461#944998, @minsoochoo0122_proton.me wrote:

pthread_getname_np returns 0 for success otherwise error code.

You didn't change the implementation, just the prototype? It's still not writing to the string, and it's reporting success even for garbage input threads.

[minsoochoo0122_proton.me]

In D41461#945001, @jrtc27 wrote:

In D41461#944998, @minsoochoo0122_proton.me wrote:

pthread_getname_np returns 0 for success otherwise error code.

You didn't change the implementation, just the prototype? It's still not writing to the string, and it's reporting success even for garbage input threads.

Sorry, I'm new to contributing base system, and I don't understand which implementation you mean. From the code I read, pthread_getname_np is symbol of _thr_getname_np defined in lib/libthr/thread/thr_info.c, and it exports to libc. Is there any addition work to be done in stub? Thanks.

[jrtc27]

In D41461#945002, @minsoochoo0122_proton.me wrote:

In D41461#945001, @jrtc27 wrote:

In D41461#944998, @minsoochoo0122_proton.me wrote:

pthread_getname_np returns 0 for success otherwise error code.

You didn't change the implementation, just the prototype? It's still not writing to the string, and it's reporting success even for garbage input threads.

Sorry, I'm new to contributing base system, and I don't understand which implementation you mean. From the code I read, pthread_getname_np is symbol of _thr_getname_np defined in lib/libthr/thread/thr_info.c, and it exports to libc. Is there any addition work to be done in stub? Thanks.

The stub needs to be a minimal implementation when there are no threads that satisfies the API documentation in the manpage. stub_zero does not do that, because it (a) doesn't validate the thread (b) doesn't initialise the buffer. This means that (a) if a caller provides junk to the function it returns 0 for success rather than ESRCH for failure (b) if a caller tries to use the buffer even after it gave it a pthread_t of &main_thread it'll be reading uninitialised junk (and may run off the end of the buffer if there doesn't happen to be a 0 byte in there). In short, stub_zero never works properly to implement pthread_getname_np.

[minsoochoo0122_proton.me]

In D41461#945003, @jrtc27 wrote:

In D41461#945002, @minsoochoo0122_proton.me wrote:

In D41461#945001, @jrtc27 wrote:

In D41461#944998, @minsoochoo0122_proton.me wrote:

pthread_getname_np returns 0 for success otherwise error code.

You didn't change the implementation, just the prototype? It's still not writing to the string, and it's reporting success even for garbage input threads.

Sorry, I'm new to contributing base system, and I don't understand which implementation you mean. From the code I read, pthread_getname_np is symbol of _thr_getname_np defined in lib/libthr/thread/thr_info.c, and it exports to libc. Is there any addition work to be done in stub? Thanks.

The stub needs to be a minimal implementation when there are no threads that satisfies the API documentation in the manpage. stub_zero does not do that, because it (a) doesn't validate the thread (b) doesn't initialise the buffer. This means that (a) if a caller provides junk to the function it returns 0 for success rather than ESRCH for failure (b) if a caller tries to use the buffer even after it gave it a pthread_t of &main_thread it'll be reading uninitialised junk (and may run off the end of the buffer if there doesn't happen to be a 0 byte in there). In short, stub_zero never works properly to implement pthread_getname_np.

Thanks you for explaining. So if I understood correctly, would returning ESRCH through stub_esrch be a correct implementation here?

[minsoochoo0122_proton.me]

Using stub_esrch when an invalid thread comes a parameter.

[kib]

This is still wrong IMO. The main case is to return empty name for main thread, and it does not matter much if we do not detect invalid thread handle.

As was said before, stub pthread_getname_np() should store nul string in the target buffer.

[minsoochoo0122_proton.me]

In D41461#945937, @kib wrote:

This is still wrong IMO. The main case is to return empty name for main thread, and it does not matter much if we do not detect invalid thread handle.

As was said before, stub pthread_getname_np() should store nul string in the target buffer.

How can I store null string to the name parameter? Does stub_null works for parameters as well?

[kib]

In D41461#945941, @minsoochoo0122_proton.me wrote:

In D41461#945937, @kib wrote:

This is still wrong IMO. The main case is to return empty name for main thread, and it does not matter much if we do not detect invalid thread handle.

As was said before, stub pthread_getname_np() should store nul string in the target buffer.

How can I store null string to the name parameter? Does stub_null works for parameters as well?

You do if (len >= 1) name[0] = '\0'; as usual. What is the problem? stub_null() cannot be used, of course.

[minsoochoo0122_proton.me]

In D41461#945942, @kib wrote:

In D41461#945941, @minsoochoo0122_proton.me wrote:

In D41461#945937, @kib wrote:

This is still wrong IMO. The main case is to return empty name for main thread, and it does not matter much if we do not detect invalid thread handle.

As was said before, stub pthread_getname_np() should store nul string in the target buffer.

How can I store null string to the name parameter? Does stub_null works for parameters as well?

You do if (len >= 1) name[0] = '\0'; as usual. What is the problem? stub_null() cannot be used, of course.

Sorry, my question was unclear. In `lib/libthr/thread/thr_info.c", I see

if (len > 0)
    buf[0] = '\0';

But I cannot find any file to write this for the stub function. Where can I write this code for stub functions? Like lib/libc/gen/__pthread_mutex_init_calloc_cb_stub.c, should I implement the stub function in a new file?
Example:

/* lib/libc/gen/__pthread_getname_np_stub.c */
#include <sys/cdefs.h>
#include <pthread.h>
#include <errno.h>
#include "libc_private.h"

int
_pthread_getname_np_stub(pthread_t thread, char *buf, size_t len)
{
    if (len > 0)
        buf[0] = '\0';
    return (ESRCH);
}

[kib]

In D41461#945944, @minsoochoo0122_proton.me wrote:

In D41461#945942, @kib wrote:

In D41461#945941, @minsoochoo0122_proton.me wrote:

In D41461#945937, @kib wrote:

This is still wrong IMO. The main case is to return empty name for main thread, and it does not matter much if we do not detect invalid thread handle.

As was said before, stub pthread_getname_np() should store nul string in the target buffer.

How can I store null string to the name parameter? Does stub_null works for parameters as well?

You do if (len >= 1) name[0] = '\0'; as usual. What is the problem? stub_null() cannot be used, of course.

Sorry, my question was unclear. In `lib/libthr/thread/thr_info.c", I see

if (len > 0)
    buf[0] = '\0';

But I cannot find any file to write this for the stub function. Where can I write this code for stub functions? Like lib/libc/gen/__pthread_mutex_init_calloc_cb_stub.c, should I implement the stub function in a new file?

What is wrong with libc/gen/_pthread_stubs.c?

Example:

/* lib/libc/gen/__pthread_getname_np_stub.c */
#include <sys/cdefs.h>
#include <pthread.h>
#include <errno.h>
#include "libc_private.h"

int
_pthread_getname_np_stub(pthread_t thread, char *buf, size_t len)
{
    if (len > 0)
        buf[0] = '\0';
    return (ESRCH);

This should be `return (0);`

}

[minsoochoo0122_proton.me]

In D41461#945957, @kib wrote:

In D41461#945944, @minsoochoo0122_proton.me wrote:

In D41461#945942, @kib wrote:

In D41461#945941, @minsoochoo0122_proton.me wrote:

In D41461#945937, @kib wrote:

This is still wrong IMO. The main case is to return empty name for main thread, and it does not matter much if we do not detect invalid thread handle.

As was said before, stub pthread_getname_np() should store nul string in the target buffer.

How can I store null string to the name parameter? Does stub_null works for parameters as well?

You do if (len >= 1) name[0] = '\0'; as usual. What is the problem? stub_null() cannot be used, of course.

Sorry, my question was unclear. In `lib/libthr/thread/thr_info.c", I see

if (len > 0)
    buf[0] = '\0';

But I cannot find any file to write this for the stub function. Where can I write this code for stub functions? Like lib/libc/gen/__pthread_mutex_init_calloc_cb_stub.c, should I implement the stub function in a new file?

What is wrong with libc/gen/_pthread_stubs.c?

Example:

/* lib/libc/gen/__pthread_getname_np_stub.c */
#include <sys/cdefs.h>
#include <pthread.h>
#include <errno.h>
#include "libc_private.h"

int
_pthread_getname_np_stub(pthread_t thread, char *buf, size_t len)
{
    if (len > 0)
        buf[0] = '\0';
    return (ESRCH);

This should be `return (0);`

}

Is there any possible way to store null string in _pthread_stubs.c? I can't figure out how to store null string using STUB_FUNC3.

[kib]

In D41461#945966, @minsoochoo0122_proton.me wrote:

Is there any possible way to store null string in _pthread_stubs.c? I can't figure out how to store null string using STUB_FUNC3.

I do not understand neither your questions, nor can I guess the possible confusion behind them. Would it be simpler if I provide the compilable (not tested) changes for _pthread_stub.c?

diff --git a/lib/libc/gen/_pthread_stubs.c b/lib/libc/gen/_pthread_stubs.c
index 6741c6a5ec51..3fc0817b8655 100644
--- a/lib/libc/gen/_pthread_stubs.c
+++ b/lib/libc/gen/_pthread_stubs.c
@@ -58,6 +58,7 @@ static int		stub_fail(void);
 static int		stub_true(void);
 static void		stub_exit(void);
 static int		stub_esrch(void);
+static int		stub_getname_np(pthread_t, char *, size_t);
 
 #define	PJT_DUAL_ENTRY(entry)	\
 	(pthread_func_t)entry, (pthread_func_t)entry
@@ -131,6 +132,7 @@ pthread_func_entry_t __thr_jtable[PJT_MAX] = {
 	[PJT_MUTEXATTR_SETROBUST] =	{PJT_DUAL_ENTRY(stub_zero)},
 	[PJT_GETTHREADID_NP] =		{PJT_DUAL_ENTRY(stub_zero)},
 	[PJT_ATTR_GET_NP] =		{PJT_DUAL_ENTRY(stub_esrch)},
+	[PJT_GETNAME_NP] =		{PJT_DUAL_ENTRY(stub_getname_np)},
 };
 
 /*
@@ -289,6 +291,7 @@ STUB_FUNC3(__pthread_cleanup_push_imp, PJT_CLEANUP_PUSH_IMP, void, void *,
 STUB_FUNC1(_pthread_cancel_enter, PJT_CANCEL_ENTER, void, int)
 STUB_FUNC1(_pthread_cancel_leave, PJT_CANCEL_LEAVE, void, int)
 STUB_FUNC2(pthread_attr_get_np, PJT_ATTR_GET_NP, int, pthread_t, pthread_attr_t *)
+STUB_FUNC3(pthread_gettname_np,	PJT_GETNAME_NP, int, pthread_t, char *, size_t)
 
 static int
 stub_zero(void)
@@ -337,3 +340,13 @@ stub_esrch(void)
 {
 	return (ESRCH);
 }
+
+static int
+stub_getname_np(pthread_t thread, char *buf, size_t len)
+{
+	if (thread != &main_thread)
+		return (ESRCH);
+	if (len >= 1)
+		buf[0] = '\0';
+	return (0);
+}

[minsoochoo0122_proton.me]

In D41461#945982, @kib wrote:

In D41461#945966, @minsoochoo0122_proton.me wrote:

Is there any possible way to store null string in _pthread_stubs.c? I can't figure out how to store null string using STUB_FUNC3.

I do not understand neither your questions, nor can I guess the possible confusion behind them. Would it be simpler if I provide the compilable (not tested) changes for _pthread_stub.c?

diff --git a/lib/libc/gen/_pthread_stubs.c b/lib/libc/gen/_pthread_stubs.c
index 6741c6a5ec51..3fc0817b8655 100644
--- a/lib/libc/gen/_pthread_stubs.c
+++ b/lib/libc/gen/_pthread_stubs.c
@@ -58,6 +58,7 @@ static int		stub_fail(void);
 static int		stub_true(void);
 static void		stub_exit(void);
 static int		stub_esrch(void);
+static int		stub_getname_np(pthread_t, char *, size_t);
 
 #define	PJT_DUAL_ENTRY(entry)	\
 	(pthread_func_t)entry, (pthread_func_t)entry
@@ -131,6 +132,7 @@ pthread_func_entry_t __thr_jtable[PJT_MAX] = {
 	[PJT_MUTEXATTR_SETROBUST] =	{PJT_DUAL_ENTRY(stub_zero)},
 	[PJT_GETTHREADID_NP] =		{PJT_DUAL_ENTRY(stub_zero)},
 	[PJT_ATTR_GET_NP] =		{PJT_DUAL_ENTRY(stub_esrch)},
+	[PJT_GETNAME_NP] =		{PJT_DUAL_ENTRY(stub_getname_np)},
 };
 
 /*
@@ -289,6 +291,7 @@ STUB_FUNC3(__pthread_cleanup_push_imp, PJT_CLEANUP_PUSH_IMP, void, void *,
 STUB_FUNC1(_pthread_cancel_enter, PJT_CANCEL_ENTER, void, int)
 STUB_FUNC1(_pthread_cancel_leave, PJT_CANCEL_LEAVE, void, int)
 STUB_FUNC2(pthread_attr_get_np, PJT_ATTR_GET_NP, int, pthread_t, pthread_attr_t *)
+STUB_FUNC3(pthread_gettname_np,	PJT_GETNAME_NP, int, pthread_t, char *, size_t)
 
 static int
 stub_zero(void)
@@ -337,3 +340,13 @@ stub_esrch(void)
 {
 	return (ESRCH);
 }
+
+static int
+stub_getname_np(pthread_t thread, char *buf, size_t len)
+{
+	if (thread != &main_thread)
+		return (ESRCH);
+	if (len >= 1)
+		buf[0] = '\0';
+	return (0);
+}

Thank you for showing me the solution. I was confused since I don't completely understand the mechanism of stub functions, and thus I was unable to clarify my question. The patch builds and works well on reboot. Again, thank you for your help.

[minsoochoo0122_proton.me]

Stores null string in name parameter for stub function.

[kib]

You may send me the complete git patch by mail. Please ensure that the author metadata is correctly filled. Also it would be useful to add tags like Differential revision: etc.