430884 - remove non-pthreads code for Solaris

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Julien Pierre]

I believe everybody should be using the pthreads version of NSPR on Solaris nowadays. It is the default, and we don't build or test the non-pthreads version. I propose that we clean up the code and remove support for the old pre-POSIX Solaris threads. We had an incident where somebody internally at Sun tried to build this way. Let's not give them the chance to mess this up again.

Comment 1

[Julien Pierre]

pthread has been in Solaris since version 2.5 . The non-pthreads code is only there to support Solaris 2.4 . That's long dead and EOL'ed. It's time we expunge it. I will create a patch for it after you review my patch for bug 430883 .

Comment 2

[Wan-Teh Chang]

Remember to remove the --with-native-threads option from configure.in, too.  Thanks.

Comment 3

[Wan-Teh Chang]

It is a lot of work to remove the old Solaris threads code cleanly.
I suggest that as a stopgap measure we first improve the configure
--help message.  Right now configure --help has these threading
options:

  --with-pthreads         Use system pthreads library as thread subsystem
  --enable-user-pthreads  Build using userland pthreads
  --enable-nspr-threads   Build using classic nspr threads
  --with-bthreads         Use system bthreads library as thread subsystem
                          (BeOS only)
  --with-native-threads   Use native system threads as thread subsystem
                          (Solaris only)

It makes --with-native-threads very tempting for a Solaris developer.
I suggest the following wording:

  --with-native-threads   Use native system threads as thread subsystem
                          (Solaris only).  This option is deprecated.
                          --with-pthreads (the default) is recommended.

If "deprecated" is not strong enough, use "obsolete" or "DEPRECATED".

Comment 4

[Wan-Teh Chang]

Attachment: Improve configure --help message

Comment 5

[Julien Pierre]

wan-Teh,

I am already in the process of removing the native thread code. I know it's a bit of work, but I think it's worth it to expunge what we don't need. I will attach a patch when I am done.

Comment 6

[Julien Pierre]

Attachment: fix configure and Makefiles only

Wan-Teh,

I believe this does what we need.
I will attach a separate patch to expunge the actual code from the source. I believe anything under _PR_GLOBAL_THREADS_ONLY_ and Solaris just needs to go.

Comment 7

[Julien Pierre]

Attachment: Also expunge source code

Comment 8

[Julien Pierre]

Note that the 2nd patch is inclusive of the first one . But the only change is the souce and header files. All other files are unchanged and don't need to be reviewed again.

Comment 9

[Julien Pierre]

Comment on attachment 317820 [details] [diff] [review]
Improve configure --help message

This would be acceptable if I hadn't already written the other 2 patches to completely remove the pre-POSIX Solaris native thread support from NSPR.

Comment 10

[Wan-Teh Chang]

Attachment: Proposed patch v2

Comment 11

[Wan-Teh Chang]

Comment on attachment 319117 [details] [diff] [review]
Proposed patch v2

Julien, I reviewed your patch "Also expunge source code"
(attachment 317826 [details] [diff] [review]) and made the following changes.

1. configure is a generated file.  By convention we
don't include it in patches.

2. configure.in: I took the opportunity to fix two typos
in the original code -- USE_USER_THREADS should be
USE_USER_PTHREADS.  But USE_USER_PTHREADS isn't available
on Solaris, so I changed one instance to USE_NSPR_THREADS.

3. Makefile.in in various "tests" directories:  I
removed
  EXTRA_LIBS = -lpthread
because that line was originally ifdef'd for SunOS
5.5.  I assume you intended to remove all that "SunOS
5.4 and 5.5 need to link with -lthread or -lpthread"
stuff.

4. _solaris.h: I updated the comment to not mention the
code you removed.

5. solaris.c: the atomic code using mutex_t should also
be removed.

5. pr/tests/{foreign.c,provider.c}: the "uithread" code
can all be removed.  UI thread refers to the Solaris
threads thr_xxx API.

Comment 12

[Wan-Teh Chang]

Comment on attachment 319117 [details] [diff] [review]
Proposed patch v2

I checked in this patch on the NSPR trunk (NSPR 4.7.2).

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.232; previous revision: 1.231
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.236; previous revision: 1.235
done
Checking in lib/msgc/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/msgc/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.15; previous revision: 1.14
done
Checking in lib/prstreams/tests/testprstrm/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/prstreams/tests/testprstrm/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in lib/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
Checking in pr/include/md/_solaris.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_solaris.h,v  <--  _solaris.h
new revision: 3.29; previous revision: 3.28
done
Checking in pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.49; previous revision: 1.48
done
Checking in pr/src/cplus/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/cplus/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Removing pr/src/md/unix/os_SunOS.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_SunOS.s,v  <--  os_SunOS.s
new revision: delete; previous revision: 3.7
done
Checking in pr/src/md/unix/solaris.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/solaris.c,v  <--  solaris.c
new revision: 3.17; previous revision: 3.16
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.54; previous revision: 1.53
done
Checking in pr/tests/foreign.c;
/cvsroot/mozilla/nsprpub/pr/tests/foreign.c,v  <--  foreign.c
new revision: 3.13; previous revision: 3.12
done
Checking in pr/tests/provider.c;
/cvsroot/mozilla/nsprpub/pr/tests/provider.c,v  <--  provider.c
new revision: 3.16; previous revision: 3.15
done
Checking in tools/Makefile.in;
/cvsroot/mozilla/nsprpub/tools/Makefile.in,v  <--  Makefile.in
new revision: 1.15; previous revision: 1.14
done

Comment 13

[Wan-Teh Chang]

Attachment: Supplemental patch

Julien,

These review requests are just FYI.  What I checked in is
your patch with minor tweaks, some of which may only make
sense after you know the history of old NSPR code.

I resurrected the -lpthread linker options (along with the
associated comments) in those makefiles.  Those makefiles
build dead code.  I thought it's better to remove them in
their entirety later rather than in parts now.

I also removed the LOCAL_THREADS_ONLY build variable.  That
variable was only used on Solaris, because Solaris had two
"classic NSPR" implementations ("Solaris threads" and "local
threads only").  Now that the Solaris threads implementation
is gone, Solaris has only one classic NSPR implementation,
and can just use the CLASSIC_NSPR build variable.

I checked in this patch on the NSPR trunk (NSPR 4.7.2).

Checking in lib/msgc/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/msgc/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.16; previous revision: 1.15
done
Checking in lib/prstreams/tests/testprstrm/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/prstreams/tests/testprstrm/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
Checking in lib/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.21; previous revision: 1.20
done
Checking in pr/include/md/_solaris.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_solaris.h,v  <--  _solaris.h
new revision: 3.30; previous revision: 3.29
done
Checking in pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.50; previous revision: 1.49
done
Checking in pr/src/cplus/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/cplus/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.55; previous revision: 1.54
done
Checking in tools/Makefile.in;
/cvsroot/mozilla/nsprpub/tools/Makefile.in,v  <--  Makefile.in
new revision: 1.16; previous revision: 1.15
done

Comment 14

[Julien Pierre]

Wan-Teh,

Thanks for checking this in.

Re: comment 11,

1. I don't have the proper autoconf so I included it in case I was going to perform the check-in

3. My intent was to remove all Solaris 2.4 and 2.5 support for the NSPR build, and default to link with libpthread

5. I wasn't sure that this was the case for all 4 builds of Solaris we have (x84, amd64, sparc32, sparc64) which is why I had left this in. It's good that we can delete it.

5 bis. same as 5.

Re: comment 13,

How would we go about removing these Makefiles altogether ? Don't they build some sources that are not dead yet ?

I wasn't familiar with the two flavors of "classic NSPR" builds. I don't think I ever did a build of this kind. Thanks for clearing this up.

Comment 15

[Wan-Teh Chang]

The dead code I was referring to is:
- mozilla/nsprpub/lib/msgc
- mozilla/nsprpub/lib/prstreams
- mozilla/nsprpub/pr/src/cplus

We would remove those makefiles along with the sources
they build.

Note that mozilla/nsprpub/lib/tests is *not* dead code.
mozilla/nsprpub/tools is borderline dead code.

Re: those -lpthread linker options ifdef'd for SunOS 5.5 only

I believe when I added them, the test programs would
have unresolved symbols without -lpthread.  This is
why I thought we should just delete them.

Then, on second thought, I remember that even if we don't
need to link with -lpthread to resolve symbols, it is still
necessary to link with -lpthread to enforce the correct
library linking order (libpthread.so and libthread.so must
be before libc.so).  So I resurrected the -lpthread.  But
I still left them ifdef'd for SunOS 5.5 only to preserve history.
Since those makefiles aren't being used, I don't want to
speculate how they should be updated, which isn't the
goal of this bug.

Comment 16

[Julien Pierre]

Comment on attachment 319213 [details] [diff] [review]
Supplemental patch

Wan-Teh,

I would prefer backing out the changes for SunOS 5.5 . That OS is 13 years old, and has been EOL for a long time. We don't need to support it anymore.

Comment 17

[Wan-Teh Chang]

It's a reminder for myself and others to check if
-lpthread should also be used on recent Solaris versions
to get the correct linking order (libpthread.so before
libc.so).  Sometimes a program with the wrong linking
order (libc.so before libpthread.so and libthread.so)
doesn't have any linking error but have strange runtime
errors.