266981 - [PATCH] Fix FreeBSD NSPR support, and add additional FreeBSD platform support

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Joe Marcus Clarke]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.3) Gecko/20041011 Galeon/1.3.18
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.3) Gecko/20041011 Galeon/1.3.18

There have been quite a few local nspr patches sitting in our ports tree, and it
was suggested that they should be merged upstream in a separate bug that was
linked to Bug 264410.

These patches add some additional (alpha, ia64, amd64, and sparc64) platform
support as well as fix some FreeBSD-specific support issues.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Joe Marcus Clarke]

Attachment: Add FreeBSD specific patches to NSPR

Comment 2

[Wan-Teh Chang]

Comment on attachment 164065 [details] [diff] [review]
Add FreeBSD specific patches to NSPR

Thanks for the patch.  I have some review comments.

1. nsprpub/pr/src/md/unix/unix.c

This change is not necessary on the NSPR tip.  We
define HAVE_SOCKLEN_T for FreeBSD in nsprpub/configure
instead.

2. nsprpub/pr/include/md/_freebsd.cfg

Is the __alpha to __alpha__ change necessary?  Why didn't
you make the same change to nsprpub/pr/include/md/_freebsd.h?

I noticed that the existing code defines IS_64 for
the __sparc__ case.  Is that correct?

3. nsprpub/pr/include/md/_freebsd.h

Should we add a case for __sparc64__ too?

Is it possible for __sparc__ and __sparc64__ to be
defined at the same time?  If so, we'll need to
test for __sparc64__ first.  (nsprpub/pr/include/md/_freebsd.cfg
has the same issue.)

4. nsprpub/pr/include/md/_pth.h

>-#define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (EBUSY == pthread_mutex_trylock(&(m)))
>+#define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (0 != pthread_mutex_trylock(&(m)))

Does pthread_mutex_trylock return EDEADLK instead of
EBUSY on FreeBSD?

5. nsprpub/pr/src/io/prprf.c

This change is not necessary on the NSPR tip because
I eliminated the need for the VARARGS_ASSIGN macro.

6. nsprpub/pr/src/pthreads/ptio.c

Is the IPV6_V6ONLY on by default on FreeBSD?  Do you
know that this is opposite to what the RFC specifies?

Comment 3

[Joe Marcus Clarke]

(In reply to comment #2)
> (From update of attachment 164065 [details] [diff] [review])
> Thanks for the patch.  I have some review comments.
> 
> 1. nsprpub/pr/src/md/unix/unix.c
> 
> This change is not necessary on the NSPR tip.  We
> define HAVE_SOCKLEN_T for FreeBSD in nsprpub/configure
> instead.

Cool.

> 
> 2. nsprpub/pr/include/md/_freebsd.cfg
> 
> Is the __alpha to __alpha__ change necessary?  Why didn't
> you make the same change to nsprpub/pr/include/md/_freebsd.h?

Yes, the same should be done for both.

> 
> I noticed that the existing code defines IS_64 for
> the __sparc__ case.  Is that correct?

We only have a sparc64 support, so all FreeBSD on sparc is 64-bit.

> 
> 3. nsprpub/pr/include/md/_freebsd.h
> 
> Should we add a case for __sparc64__ too?

Not necessary,

> 
> Is it possible for __sparc__ and __sparc64__ to be
> defined at the same time?  If so, we'll need to
> test for __sparc64__ first.  (nsprpub/pr/include/md/_freebsd.cfg
> has the same issue.)

On FreeBSD, yes.

> 
> 4. nsprpub/pr/include/md/_pth.h
> 
> >-#define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (EBUSY == pthread_mutex_trylock(&(m)))
> >+#define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (0 != pthread_mutex_trylock(&(m)))
> 
> Does pthread_mutex_trylock return EDEADLK instead of
> EBUSY on FreeBSD?

Older versions do.  Newer versions return EBUSY.  We still need to support both.

> 
> 5. nsprpub/pr/src/io/prprf.c
> 
> This change is not necessary on the NSPR tip because
> I eliminated the need for the VARARGS_ASSIGN macro.

Great.  Thanks!

> 
> 6. nsprpub/pr/src/pthreads/ptio.c
> 
> Is the IPV6_V6ONLY on by default on FreeBSD?  Do you
> know that this is opposite to what the RFC specifies?
> 

Comment 4

[Wan-Teh Chang]

Attachment: FreeBSD patch for NSPR, edited by wtc

This is my version of the patch.  Some of the
changes I made include:

1. I omitted the changes to nsprpub/pr/src/md/unix/unix.c
and nsprpub/pr/src/io/prprf.c because they are no longer
necessary on the NSPR tip.

2. In nsprpub/pr/include/md/_freebsd.cfg, I omitted the
new section for __sparc64__ because that file already has
a section for __sparc__.  Based on what you told me, I
believe __sparc__ and __sparc64__ are equivalent on
FreeBSD.

3. To handle the _PT_PTHREAD_MUTEX_IS_LOCKED problem, I
define it as a function for FreeBSD so I can only allow
pthread_mutex_trylock to return EBUSY or EDEADLK.

4. In added a new feature test macro _PR_IPV6_V6ONLY_PROBE
and define it on Darwin and FreeBSD.

I'd appreciate it if you could test this patch.  Thanks.

Comment 5

[Wan-Teh Chang]

Attachment: FreeBSD patch for NSPR, edited by wtc

I attached the wrong file last time.  This is
the correct patch.

Comment 6

[Joe Marcus Clarke]

This patch looks good to me.  Note: you may also want to take a look at  	207803
which contains all of the sparc64 patches for FreeBSD.  This bug was filed a
while ago before the other architecture patches wer added.

Comment 7

[Wan-Teh Chang]

Comment on attachment 166531 [details] [diff] [review]
FreeBSD patch for NSPR, edited by wtc

I've checked in this patch on the NSPR trunk (NSPR 4.6).
I will check it in on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
when the SeaMonkey trunk re-opens to checkins.

I looked at bug 207803 and verified that all the NSPR
patches in that bug have been checked in.

Comment 8

[Wan-Teh Chang]

I checked in the patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(post Mozilla 1.8 Alpha 5).