Details

Reviewers

Commits

rS350483: Avoid conflicts with libc symbols in libthr jump table.
rS350481: Avoid conflicts with libc symbols in libthr jump table.

Summary

In some corner cases of static linking and unexpected libraries order on the linker command line, libc symbol might be preferred over the same libthr symbol, in which case libthr jump table points back to libc causing either infinite recursion or loop. Handle several of such symbols by using libthr names for them, ensuring that the right pointers are installed into the table.

Reported by: arichardson
PR: 239475

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 25591

Event Timeline

kib created this revision.Jul 27 2019, 4:24 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 27 2019, 4:24 PM

Harbormaster completed remote builds in B25540: Diff 60197.Jul 27 2019, 4:24 PM

Thanks! I can confirm that it works for the testcase from the PR if I apply the suggested fix.

lib/libthr/thread/thr_clean.c

I think this should be __weak_reference(__pthread_cleanup_pop_imp, __pthread_cleanup_pop_imp1);

I get the following build error with the current patch:

ld: error: /home/alr48/cheri/build/freebsd-default-options-native-build/exports/users/alr48/sources/freebsd-universe/amd64.amd64/tmp/usr/lib/libpthread.so: undefined reference to __pthread_cleanup_pop_imp1
cc: error: linker command failed with exit code 1 (use -v to see invocation)
--- camdd.full ---
*** [camdd.full] Error code 1
make[4]: stopped in /exports/users/alr48/sources/freebsd-universe/usr.sbin/camdd

This revision is now accepted and ready to land.Jul 27 2019, 6:13 PM

However, I just noticed that the following test case still results in an infinite loop/recursion even with this patch. I prints the address of pthread_create, but then freezes. Guess we need to do the same trick for even more functions.

#include <stdio.h>
#include <pthread.h>

void* thread_func(void* arg) {
    return (void*)0x1234;
}

int main() {
  printf("pthread_create = %p\n", (void*)&pthread_create);
  pthread_t thr;
  pthread_create(&thr, NULL, &thread_func, NULL);
  void* result;
  pthread_join(thr, &result);
  printf("result = %p\n", result);
  return 0;
}

root@qemu-native-alr48:~ # clang -static -o test-thr-first -nodefaultlibs -lthr -lc test.c -fuse-ld=lld && ./test-thr-first
pthread_create = 0x269d70
result = 0x1234
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && ./test-c-first
pthread_create = 0x2a58a0
^C

Looks like the same problem, just this time with pthread_join:

root@qemu-native-alr48:~ # lldb -- ./test-c-first
(lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) r
Process 801 launching
Process 801 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 801 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
    frame #0: 0x00000000002a5570 test-c-first`pthread_join_int(p0=0x00000008002ce500, p1=0x00007fffffffeb58) at _pthread_stubs.c:281
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
  * frame #0: 0x00000000002a5570 test-c-first`pthread_join_int(p0=0x00000008002ce500, p1=0x00007fffffffeb58) at _pthread_stubs.c:281
    frame #1: 0x00000000002a56ef test-c-first`main + 95
    frame #2: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7

Fix typo.
Handle pthread_join(3).

This revision now requires review to proceed.Jul 27 2019, 7:55 PM

Harbormaster completed remote builds in B25544: Diff 60202.Jul 27 2019, 7:55 PM

Seems like it will still break for all other functions that aren't updated?

lib/libthr/thread/thr_join.c
44	I'm not sure this will compile, there is no `_thr_join` function

In D21088#457716, @arichardson wrote:

Seems like it will still break for all other functions that aren't updated?

Yes, but I think I will do them all later. For now lets finish this piece that demonstrates the the approach is viable.

lib/libthr/thread/thr_join.c
44	It did compiled and linked, strange as is. I did not tested it at runtime.

Do provide thr_join.

Harbormaster completed remote builds in B25547: Diff 60205.Jul 27 2019, 9:27 PM

Approach is definitely viable. However, if we need to update all functions a macro is probably a good idea.

The extended test case now fails in pthread_self:

Process 712 launching
Process 712 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 712 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
    frame #0: 0x00000000002a51b0 test-c-first`pthread_self_int at _pthread_stubs.c:251
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
  * frame #0: 0x00000000002a51b0 test-c-first`pthread_self_int at _pthread_stubs.c:251
    frame #1: 0x00000000002a3d5f test-c-first`_flockfile(fp=0x00000000002b5ff8) at _flock_stub.c:66:24
    frame #2: 0x0000000000281e58 test-c-first`vfprintf_l(fp=0x00000000002b5ff8, locale=0x00000000002b4bf8, fmt0="r2
    frame #3: 0x00000000002a57ba test-c-first`printf(fmt=<unavailable>) at printf.c:57:8
    frame #4: 0x00000000002a5707 test-c-first`main + 119
    frame #5: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7

This revision is now accepted and ready to land.Jul 27 2019, 9:41 PM

Handle the rest of the jump table. Not tested.

This revision now requires review to proceed.Jul 28 2019, 5:10 PM

Harbormaster completed remote builds in B25552: Diff 60214.Jul 28 2019, 5:10 PM

I am now getting a very strange error:

root@qemu-native-alr48:~ # clang -static -o test-thr-first -nodefaultlibs -lthr -lc test.c -fuse-ld=lld && ./test-thr-first
pthread_create = 0x2693d0
result = 0x1234
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && ./test-c-first
pthread_create = 0x2a58a0
Segmentation fault (core dumped)
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && lldb -- ./test-c-first
(lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) r
Process 746 launching
Process 746 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 746 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
    frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
  * frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
    frame #1: 0x00000000002a2b53 test-c-first`__sfvwrite(fp=<unavailable>, uio=0x00007fffffffe888) at fvwrite.c:188:5
    frame #2: 0x0000000000285442 test-c-first`__vfprintf [inlined] __sprint(fp=<unavailable>, uio=<unavailable>, locale=<unavailable>) at vfprintf.c:166:8
    frame #3: 0x0000000000285436 test-c-first`__vfprintf [inlined] io_flush(iop=<unavailable>, locale=<unavailable>) at printfcommon.h:157
    frame #4: 0x000000000028541f test-c-first`__vfprintf(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0=<unavailable>, ap=0x00007fffffffeb10) at vfprintf.c:1033
    frame #5: 0x0000000000281ebd test-c-first`vfprintf_l(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprintf.c:285:9
    frame #6: 0x00000000002a57ba test-c-first`printf(fmt=<unavailable>) at printf.c:57:8
    frame #7: 0x00000000002a5707 test-c-first`main + 119
    frame #8: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7

lib/libthr/thread/thr_private.h
1026	Is there a reason that some of these start with `_Tthr` instead of `_thr`?

In D21088#457869, @arichardson wrote:

I am now getting a very strange error:

root@qemu-native-alr48:~ # clang -static -o test-thr-first -nodefaultlibs -lthr -lc test.c -fuse-ld=lld && ./test-thr-first
pthread_create = 0x2693d0
result = 0x1234
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && ./test-c-first
pthread_create = 0x2a58a0
Segmentation fault (core dumped)
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && lldb -- ./test-c-first
(lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) r
Process 746 launching
Process 746 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 746 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
    frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
  * frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
    frame #1: 0x00000000002a2b53 test-c-first`__sfvwrite(fp=<unavailable>, uio=0x00007fffffffe888) at fvwrite.c:188:5
    frame #2: 0x0000000000285442 test-c-first`__vfprintf [inlined] __sprint(fp=<unavailable>, uio=<unavailable>, locale=<unavailable>) at vfprintf.c:166:8
    frame #3: 0x0000000000285436 test-c-first`__vfprintf [inlined] io_flush(iop=<unavailable>, locale=<unavailable>) at printfcommon.h:157
    frame #4: 0x000000000028541f test-c-first`__vfprintf(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0=<unavailable>, ap=0x00007fffffffeb10) at vfprintf.c:1033
    frame #5: 0x0000000000281ebd test-c-first`vfprintf_l(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprintf.c:285:9
    frame #6: 0x00000000002a57ba test-c-first`printf(fmt=<unavailable>) at printf.c:57:8
    frame #7: 0x00000000002a5707 test-c-first`main + 119
    frame #8: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7

Indeed strange. I cannot reproduce it locally. For me it is just

x% ./test-c-first                     /usr/home/pooma/build/bsd/DEV/stuff/tests
pthread_create = 0x2a58b0
x%

Can you build libc with debugging info and lower optimization level (DEBUG_FLAGS=-g should be enough), and then print out the stdout FILE state ? I suspect there is some problem there.

lib/libthr/thread/thr_private.h
1026	Yes, there is _thr_testcancel() already, I I risk completely blow my head if I would also start renaming unrelated functions. I could use _Tthr prefix for all new symbols, but this was too late in the patch, and _thr is more traditional for libthr, so I decided to not go too deep.

In D21088#458174, @kib wrote:

In D21088#457869, @arichardson wrote:

I am now getting a very strange error:

root@qemu-native-alr48:~ # clang -static -o test-thr-first -nodefaultlibs -lthr -lc test.c -fuse-ld=lld && ./test-thr-first
pthread_create = 0x2693d0
result = 0x1234
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && ./test-c-first
pthread_create = 0x2a58a0
Segmentation fault (core dumped)
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && lldb -- ./test-c-first
(lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) r
Process 746 launching
Process 746 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 746 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
    frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
  * frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
    frame #1: 0x00000000002a2b53 test-c-first`__sfvwrite(fp=<unavailable>, uio=0x00007fffffffe888) at fvwrite.c:188:5
    frame #2: 0x0000000000285442 test-c-first`__vfprintf [inlined] __sprint(fp=<unavailable>, uio=<unavailable>, locale=<unavailable>) at vfprintf.c:166:8
    frame #3: 0x0000000000285436 test-c-first`__vfprintf [inlined] io_flush(iop=<unavailable>, locale=<unavailable>) at printfcommon.h:157
    frame #4: 0x000000000028541f test-c-first`__vfprintf(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0=<unavailable>, ap=0x00007fffffffeb10) at vfprintf.c:1033
    frame #5: 0x0000000000281ebd test-c-first`vfprintf_l(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprintf.c:285:9
    frame #6: 0x00000000002a57ba test-c-first`printf(fmt=<unavailable>) at printf.c:57:8
    frame #7: 0x00000000002a5707 test-c-first`main + 119
    frame #8: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7

Indeed strange. I cannot reproduce it locally. For me it is just

x% ./test-c-first                     /usr/home/pooma/build/bsd/DEV/stuff/tests
pthread_create = 0x2a58b0
x%

Can you build libc with debugging info and lower optimization level (DEBUG_FLAGS=-g should be enough), and then print out the stdout FILE state ? I suspect there is some problem there.

I'll try an O0 build of libthr and libc later. I tried applying this patch to cheribsd and fp->_p ended up being an invalid capability without a tag so memcpy fails. Not sure how it got corrupted. Guess the same thing is happening on x86 and causing the segfault. I'll investigate further...

Just to be clear this only happens if I actually start a thread and call pthread_join (using the program I posted here). The simple case from the PR works fine.

With -O0 at the time of the crash:

(lldb) p *__stdoutp
(FILE) $1 = {
  _p = 0x00000000000186e8 ""
  _r = 0
  _w = 0
  _flags = 2185
  _file = 1
  _bf = (_base = "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5)
  _lbfsize = -4096
  _cookie = 0x000000000031d2e8
  _close = 0x00000000002fe580 (test-c-first`__sclose at stdio.c:80)
  _read = 0x00000000002fe4c0 (test-c-first`__sread at stdio.c:56)
  _seek = 0x00000000002fe540 (test-c-first`__sseek at stdio.c:72)
  _write = 0x00000000002fe500 (test-c-first`__swrite at stdio.c:64)
  _ub = (_base = 0x0000000000000000, _size = 0)
  _up = 0x0000000000000000
  _ur = 0
  _ubuf = ""
  _nbuf = ""
  _lb = (_base = 0x0000000000000000, _size = 0)
  _blksize = 4096
  _offset = 0
  _fl_mutex = 0x0000000000000000
  _fl_owner = 0x0000000000000000
  _fl_count = 1
  _orientation = 0
  _mbstate = (__mbstate8 = "", _mbstateL = 0)
  _flags2 = 0
}
(lldb) p __stdoutp->_bf._base
(unsigned char *) $4 = 0x000000080033b000 "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa.
(lldb) p __stdoutp->_bf._size
(int) $5 = 4096

This is extremly odd. After lots of adding breakpoints and single-stepping (watchpoints didn't work, I guess because it's a syscall) it turns out the syscall in __sys_thr_self is causing the corruption.

(lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) b __sys_thr_self
Breakpoint 1: where = test-c-first`__sys_thr_self + 5, address = 0x00000000002fd4b5
(lldb) r
Process 771 launching
Process 771 launched: '/root/test-c-first' (x86_64)
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
    frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
  * frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
    frame #1: 0x00000000003041f5 test-c-first`init_main_thread(thread=0x0000000800335000) at thr_init.c:379:2
    frame #2: 0x0000000000303dac test-c-first`_libpthread_init(curthread=0x0000000800335000) at thr_init.c:339:3
    frame #3: 0x0000000000303cfd test-c-first`_thread_init_hack at thr_init.c:295:2
    frame #4: 0x00000000002361e9 test-c-first`handle_static_init(argc=1, argv=0x00007fffffffebd8, env=0x00007fffffffebe8) at ignore_init.c:124:4
    frame #5: 0x0000000000236102 test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:75:2
(lldb) c
Process 771 resuming
pthread_create = 0x301e10
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
    frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
  * frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
    frame #1: 0x0000000000300787 test-c-first`pthread_self_int at _pthread_stubs.c:251:1
    frame #2: 0x00000000002fea01 test-c-first`_flockfile(fp=0x000000000031d2e8) at _flock_stub.c:66:24
    frame #3: 0x00000000002cb85a test-c-first`vfprintf_l(fp=0x000000000031d2e8, locale=0x000000000031bec0, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprint2
    frame #4: 0x00000000002d0875 test-c-first`vfprintf(fp=0x000000000031d2e8, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprintf.c:292:9
    frame #5: 0x0000000000301c1f test-c-first`printf(fmt="result = %p\n") at printf.c:57:8
    frame #6: 0x0000000000301a97 test-c-first`main at test.c:14:2
    frame #7: 0x000000000023610f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7
(lldb) p __stdoutp->_p
(unsigned char *) $0 = 0x000000080033b000 "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5.
(lldb) si
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = instruction step into
    frame #0: 0x00000000002fd4b8 test-c-first`__sys_thr_self at thr_self.S:3
(lldb) disassemble
test-c-first`__sys_thr_self:
    0x2fd4b0 <+0>:  movl   $0x1b0, %eax              ; imm = 0x1B0
    0x2fd4b5 <+5>:  movq   %rcx, %r10
->  0x2fd4b8 <+8>:  syscall
    0x2fd4ba <+10>: jb     0x2ff904                  ; .cerror
    0x2fd4c0 <+16>: retq
(lldb) p __stdoutp->_p
(unsigned char *) $1 = 0x000000080033b000 "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5.
(lldb) si
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = instruction step into
    frame #0: 0x00000000002fd4ba test-c-first`__sys_thr_self at thr_self.S:3
(lldb) p __stdoutp->_p
(unsigned char *) $2 = 0x00000000000186e3 ""

Some more information:

(lldb) register read
General Purpose Registers:
       rax = 0x00000000000001b0
       rbx = 0x0000000000000001
       rcx = 0x00007fffffffeb10
       rdx = 0x00000000002050fd
       rdi = 0x000000000031d2e8  __sF + 312
       rsi = 0x000000000031bec0  __xlocale_global_locale
       rbp = 0x00007fffffffe890
       rsp = 0x00007fffffffe878
        r8 = 0x00007fffffffeb10
        r9 = 0x00007fffffffeb10
       r10 = 0x00007fffffffeb10
       r11 = 0x0000000000000201
       r12 = 0x00000000002002e0
       r13 = 0x00000000002002e0
       r14 = 0x00007fffffffebd8
       r15 = 0x00007fffffffebe8
       rip = 0x00000000002fd4b8  test-c-first`__sys_thr_self + 8
    rflags = 0x0000000000000202
        cs = 0x0000000000000043


        ss = 0x000000000000003b

rdi contains the address of __stdoutp->_p 0x000000000031d2e8. I'm not familiar with the x86 syscall convention but I guess the syscall is writing the return value to *rdi?

arichardson added inline comments.Jul 30 2019, 8:40 AM

lib/libthr/thread/thr_self.c
45	Aha, seems like this needs to be _Tthr_self, since now the jumptable resolves to the __sys_thr_self syscall....

I can confirm that it works when I apply the following change on top of your patch:

diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
index d9bde8a37c9d..78146c2d6dcd 100644
--- a/lib/libthr/thread/thr_init.c
+++ b/lib/libthr/thread/thr_init.c
@@ -257,7 +257,7 @@ static pthread_func_t jmp_table[][2] = {
        [PJT_RWLOCK_TRYWRLOCK] = {DUAL_ENTRY(_Tthr_rwlock_trywrlock)},
        [PJT_RWLOCK_UNLOCK] = {DUAL_ENTRY(_Tthr_rwlock_unlock)},
        [PJT_RWLOCK_WRLOCK] = {DUAL_ENTRY(_Tthr_rwlock_wrlock)},
-       [PJT_SELF] = {DUAL_ENTRY(_thr_self)},
+       [PJT_SELF] = {DUAL_ENTRY(_Tthr_self)},
        [PJT_SETCANCELSTATE] = {DUAL_ENTRY(_thr_setcancelstate)},
        [PJT_SETCANCELTYPE] = {DUAL_ENTRY(_thr_setcanceltype)},
        [PJT_SETSPECIFIC] = {DUAL_ENTRY(_thr_setspecific)},
diff --git a/lib/libthr/thread/thr_private.h b/lib/libthr/thread/thr_private.h
index 90385d1ab31c..47b82009765d 100644
--- a/lib/libthr/thread/thr_private.h
+++ b/lib/libthr/thread/thr_private.h
@@ -1069,7 +1069,7 @@ int _thr_setspecific(pthread_key_t, const void *);
 void *_thr_getspecific(pthread_key_t);
 int _thr_setcancelstate(int, int *);
 int _thr_setcanceltype(int, int *);
-pthread_t _thr_self(void);
+pthread_t _Tthr_self(void);
 int _thr_rwlock_init(pthread_rwlock_t *, const pthread_rwlockattr_t *);
 int _thr_rwlock_destroy(pthread_rwlock_t *);
 int _Tthr_rwlock_rdlock(pthread_rwlock_t *);
diff --git a/lib/libthr/thread/thr_self.c b/lib/libthr/thread/thr_self.c
index 168d91721e0b..68bf5bc39312 100644
--- a/lib/libthr/thread/thr_self.c
+++ b/lib/libthr/thread/thr_self.c
@@ -38,11 +38,11 @@ __FBSDID("$FreeBSD$");

 #include "thr_private.h"

-__weak_reference(_thr_self, pthread_self);
-__weak_reference(_thr_self, _pthread_self);
+__weak_reference(_Tthr_self, pthread_self);
+__weak_reference(_Tthr_self, _pthread_self);

 pthread_t
-_thr_self(void)
+_Tthr_self(void)
 {
        _thr_check_init();

It might make sense to include a test that we can link libthr statically?

In D21088#458329, @arichardson wrote:

I can confirm that it works when I apply the following change on top of your patch:

This totally makes sense and is obvious after the diagnostic. Thank you very much for tracking it down.

It might make sense to include a test that we can link libthr statically?

Perhaps.

I wil ask pho to do some sanity check of the combined patch later today.

Fix pthread_self().

Harbormaster completed remote builds in B25586: Diff 60270.Jul 30 2019, 11:16 AM

Just found one more issue but otherwise LGTM. Maybe better to consistently use the _Tthr prefix.

lib/libthr/thread/thr_init.c
242	Actually we might get the same problem as for `_thr_self` here since there is a `thr_kill` syscall?

kib added inline comments.Jul 30 2019, 1:05 PM

lib/libthr/thread/thr_init.c
242	And thr_exit.

thr_kill and thr_exit

Harbormaster completed remote builds in B25591: Diff 60283.Jul 30 2019, 1:06 PM

arichardson accepted this revision.Jul 30 2019, 1:07 PM

This revision is now accepted and ready to land.Jul 30 2019, 1:07 PM

I ran all of the threaded tests from stress2 on amd64 with D21088.60283.diff.
No problems seen.

Closed by commit rS350481: Avoid conflicts with libc symbols in libthr jump table. (authored by kib). · Explain WhyJul 31 2019, 7:27 PM

This revision was automatically updated to reflect the committed changes.

kib added a commit: rS350481: Avoid conflicts with libc symbols in libthr jump table..

kib added a commit: rS350483: Avoid conflicts with libc symbols in libthr jump table..Jul 31 2019, 8:04 PM

andrew_tao173.riddles.org.uk added a subscriber: andrew_tao173.riddles.org.uk.Aug 21 2019, 6:15 PM

andrew_tao173.riddles.org.uk added inline comments.

head/lib/libthr/thread/thr_init.c
269 ↗	(On Diff #60332)	This is clearly wrong? _pthread_cancel_enter(int) vs. _thr_cancel_enter(struct pthread *) This causes segfaults in sem_wait / sem_timedwait if linked with libthr, breaking postgresql

Avoid conflicts with libc symbols in libthr jump table.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 60283

lib/libthr/thread/thr_attr.c

lib/libthr/thread/thr_cancel.c

lib/libthr/thread/thr_clean.c

lib/libthr/thread/thr_cond.c

lib/libthr/thread/thr_detach.c

lib/libthr/thread/thr_equal.c

lib/libthr/thread/thr_exit.c

lib/libthr/thread/thr_fork.c

lib/libthr/thread/thr_getthreadid_np.c

lib/libthr/thread/thr_init.c

lib/libthr/thread/thr_join.c

lib/libthr/thread/thr_kill.c

lib/libthr/thread/thr_main_np.c

lib/libthr/thread/thr_mutex.c

lib/libthr/thread/thr_mutexattr.c

lib/libthr/thread/thr_once.c

lib/libthr/thread/thr_private.h

lib/libthr/thread/thr_rwlock.c

lib/libthr/thread/thr_self.c

lib/libthr/thread/thr_sig.c

lib/libthr/thread/thr_spec.c

Avoid conflicts with libc symbols in libthr jump table.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 60283

lib/libthr/thread/thr_attr.c

lib/libthr/thread/thr_cancel.c

lib/libthr/thread/thr_clean.c

lib/libthr/thread/thr_cond.c

lib/libthr/thread/thr_detach.c

lib/libthr/thread/thr_equal.c

lib/libthr/thread/thr_exit.c

lib/libthr/thread/thr_fork.c

lib/libthr/thread/thr_getthreadid_np.c

lib/libthr/thread/thr_init.c

lib/libthr/thread/thr_join.c

lib/libthr/thread/thr_kill.c

lib/libthr/thread/thr_main_np.c

lib/libthr/thread/thr_mutex.c

lib/libthr/thread/thr_mutexattr.c

lib/libthr/thread/thr_once.c

lib/libthr/thread/thr_private.h

lib/libthr/thread/thr_rwlock.c

lib/libthr/thread/thr_self.c

lib/libthr/thread/thr_sig.c

lib/libthr/thread/thr_spec.c

Avoid conflicts with libc symbols in libthr jump table.
ClosedPublic
Actions

Revision Contents
Changeset List