Opened 6 years ago

Closed 5 years ago

#9495 closed defect (fixed)

Must we still disable threads on *-*-solaris*?

Reported by: nickm Owned by:
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay solaris 026-triaged-1 regression
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Back in 2005, in 8753e7ef6530c14a6d35c477a11ff203008bde50 (svn:r4383), we disabled threading on Solaris, in order to prevent some lockup bug or other. Unfortunately, back in 2005 we weren't so good at tracking bugs, so I can't easily find who reported it or how we diagnosed it.

But this is eight years later. If there was really a platform bug, surely it's gotten better by now?

We could contact one of the two or three operators whose nodes report being "on SunOS", and ask them if their nodes still work after an explicit --enable-threads , I guess.

Child Tickets

Change History (17)

comment:1 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:2 Changed 6 years ago by nickm

Keywords: 026-triaged-1 added

comment:3 Changed 6 years ago by nickm

Implemented in 58f4200789d0cc47ebd88f3091207cf4dd493573

comment:4 Changed 6 years ago by nickm

Resolution: fixed
Status: newclosed

comment:5 Changed 5 years ago by ruebezahl

Resolution: fixed
Status: closedreopened

I just ran into this while testing tor-alpha releases on SunOS 5.11 joyent_20150219T102159Z (Illumos).

I bisect-ed it down to commit 5b4ee475aa00bb30675e819a0096209e5e427668 - all builds after run without errors, but the binaries and tests coredump immediately.

I'm quite confident Illumos and/or the Joyent Smartmachine Environment support Multithreading :->

Edit: If you contact me on IRC, I can provide a testing-Environment if needed.

Last edited 5 years ago by ruebezahl (previous) (diff)

comment:6 Changed 5 years ago by nickm

Weird! Have you been able to get a stack trace on that? I don't see how that commit could be responsible for anything SunOS-related; it's only supposed to be removing WinCE stuff.

comment:7 Changed 5 years ago by ruebezahl

Hi,

5b4ee475aa00bb30675e819a0096209e5e427668 is the last commit on which tor doesn't coredump.
(as I wrote: all builds _after_)

thx!

comment:8 Changed 5 years ago by nickm

So, 58f4200789d0cc47ebd88f3091207cf4dd493573 is the first that fails?

I wonder, is this one of those platforms that requires us to pass additional CFLAGS to the compiler to build with threads correctly?

comment:9 Changed 5 years ago by nickm

Keywords: regression added

comment:10 Changed 5 years ago by ruebezahl

yep, 58f4200789d0cc47ebd88f3091207cf4dd493573 is the first failure-commit. I guess nobody tested this?

comment:11 Changed 5 years ago by nickm

I wonder, is this one of those platforms that requires us to pass additional CFLAGS to the compiler to build with threads correctly?

I'm guessing it could be this. My guess is that we need to include "-pthreads pthread -mt -pthread" or something like that in our CFLAGS.

In 0.2.7, we should use acx_pthread.m4. Opening a ticket for that. Do we have an easy workaround we can do in 0.2.6?

comment:12 Changed 5 years ago by nickm

Whoops, I misread the ticket. I think just the "-pthreads" flag has a good chance of working. Let me know if that's right?

comment:13 Changed 5 years ago by nickm

Possible patch:

diff --git a/configure.ac b/configure.ac
index e5e36d5..b28c893 100644
--- a/configure.ac
+++ b/configure.ac
@@ -366,6 +366,12 @@ if test "$LIBS" != "$saved_LIBS"; then
    have_rt=yes
 fi
 
+case $host in
+   *-*-solaris* )
+     CFLAGS="$CFLAGS -pthreads"
+     ;;
+esac
+
 AC_SEARCH_LIBS(pthread_create, [pthread])
 AC_SEARCH_LIBS(pthread_detach, [pthread])

comment:14 Changed 5 years ago by nickm

Oh man.

I think I have it.

diff --git a/src/common/compat_pthreads.c b/src/common/compat_pthreads.c
index f4a6cad..984e4c8 100644
--- a/src/common/compat_pthreads.c
+++ b/src/common/compat_pthreads.c
@@ -279,7 +279,7 @@ tor_threads_init(void)
     pthread_mutexattr_init(&attr_recursive);
     pthread_mutexattr_settype(&attr_recursive, PTHREAD_MUTEX_RECURSIVE);
     tor_assert(0==pthread_attr_init(&attr_detached));
-    tor_assert(0==pthread_attr_setdetachstate(&attr_detached, 1));
+    tor_assert(0==pthread_attr_setdetachstate(&attr_detached, PTHREAD_CREATE_DETACHED));
     threads_initialized = 1;
     set_main_thread();
   }

Now it doesn't crash for me.

comment:15 Changed 5 years ago by nickm

Status: reopenedneeds_review

Fix in branch bug9495_redux in my public repository.

comment:16 Changed 5 years ago by Sebastian

looks trivially correct, except the commit message ends without some :)

comment:17 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Fixed commit message and merged to 0.2.6 and forward!

13:57 <nickm> it's a little embarassing
13:57 <nickm> I tried commenting out that line, and it worked.
13:58 <nickm> I tried forcing the alignment of the attribute, and it didn't 
              help.
13:58 <nickm> so I read the manpage.
13:58 <nickm> And saw that I was supposed to use a constant.
13:58 <nickm> So I tried using the constant, and it worked :)
Note: See TracTickets for help on using tickets.