Closed Bug 261649 Opened 15 years ago Closed 14 years ago

[PATCH] GNU/k*BSD support

Categories

(NSPR :: NSPR, defect)

All
Other
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rmh, Assigned: wtc)

Details

Attachments

(3 files, 9 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3

This patch fixes NSPR to build and pass the testsuite on GNU/k*BSD.  It also
includes preliminar fixes to support GNU/Hurd.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Attached patch GNU/k*BSD support (obsolete) — Splinter Review
Thanks for the patch.  I will review it.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch new patch (obsolete) — Splinter Review
I noticed a pair of errors in my previous patch (the findstring checks were not
correct).  I'm adding an update.

Will you review this soon?
Robert,

Could you tell me a little about GNU/k*BSD
and its relation to Hurd, Linux, and FreeBSD?
Your patch seems to identify GNU/k*BSD by
the "GLIBC" macro.  So I suspect GNU/k*BSD
is glibc on top of FreeBSD kernel?

I am also wondering why you don't just use
the __GLIBC__ macro.
Excuse me for the delay.  I've been busy working on the other parts of mozilla.

(In reply to comment #4)
> Robert,
> 
> Could you tell me a little about GNU/k*BSD
> and its relation to Hurd, Linux, and FreeBSD?

Sure!  GNU/kFreeBSD is a system with GNU userland and libc on kernel of
FreeBSD.  You can learn more about it in http://www.debian.org/ports/kfreebsd-gnu

My patch puts it together with other systems that are known to
use Glibc.  The exception to that is Linux, for which we only know the
kernel and can't assume Glibc.

> Your patch seems to identify GNU/k*BSD by
> the "GLIBC" macro.  So I suspect GNU/k*BSD
> is glibc on top of FreeBSD kernel?

Yes, that is the essential.

> I am also wondering why you don't just use
> the __GLIBC__ macro.

The problem with __GLIBC__ is you can't assume it's defined for
all Glibc systems in all situations.  Usualy __GLIBC__ is defined
in features.h, so if that header hasn't been included (either directly
or indirectly) the macro is not defined.

On GNU/kFreeBSD, however, __GLIBC__ is a preprocessor macro.  The
following check defines GLIBC reliing on preprocessor macros exclussively:

+#if !defined(GLIBC) && (defined(__GNU__) || defined(__GLIBC__))
+#define GLIBC
+#endif
ping
Attachment #171766 - Flags: review?(wtchang)
Robert, you wrote:

  My patch puts [GNU/kFreeBSD] together with other systems
  that are known to use Glibc.  The exception to that is Linux,
  for which we only know the kernel and can't assume Glibc.

I don't understand why we can't assume Linux uses Glibc.

If we can assume Linux uses Glibc, all the instances of
    defined(LINUX) || defined(GLIBC)
in your patch can be replaced by
    defined(GLIBC)

Is "GLIBC" the standard feature macro to indicate a Glibc
system?

If every Glibc system has <features.h>, can we just include
<features.h> so we can test the __GLIBC__ macro?
Comment on attachment 171766 [details] [diff] [review]
new patch

>+    case "$target_os" in
>+      linux*) AC_DEFINE(LINUX) ;;
>+      gnu* | k*bsd*-gnu) AC_DEFINE(GLIBC) ;;
>+    esac

How do we determine if a Linux system uses Glibc?

>diff -ur nsprpub.old/pr/include/md/_linux.cfg nsprpub/pr/include/md/_linux.cfg
>--- nsprpub.old/pr/include/md/_linux.cfg	2004-09-24 20:29:02.000000000 +0200
>+++ nsprpub/pr/include/md/_linux.cfg	2004-09-25 13:43:44.000000000 +0200
>@@ -42,10 +42,14 @@
... 
>-#ifndef LINUX
>+#if !defined(LINUX) && defined(__linux__)
> #define LINUX
> #endif

This definitely needs a comment to explain that _linux.cfg
is now used by not only Linux but also other Glibc systems.

>+#if !defined(GLIBC) && (defined(__GNU__) || defined(__GLIBC__))
>+#define GLIBC
>+#endif

This should removed.  _linux.cfg is a public header file used
by not only NSPR itself but also NSPR clients.	I don't want to
define GLIBC in a public header if I can avoid it.  When compiling
NSPR, we already have -DGLIBC on the compiler command line, so
NSPR itself doesn't need this.

Does __GNU__ mean GNU/Hurd?

>diff -ur nsprpub.old/pr/include/md/_pth.h nsprpub/pr/include/md/_pth.h
>--- nsprpub.old/pr/include/md/_pth.h	2004-09-24 20:29:02.000000000 +0200
>+++ nsprpub/pr/include/md/_pth.h	2004-09-25 17:44:15.000000000 +0200
>@@ -139,10 +139,14 @@
...
> #elif defined(IRIX) || defined(OSF1) || defined(AIX) || defined(SOLARIS) \
>-	|| defined(HPUX) || defined(LINUX) || defined(FREEBSD) \
>+	|| defined(HPUX) || defined(LINUX) || defined(GLIBC) || defined(FREEBSD) \
...)
>+#ifdef __GNU__
>+# error Using Hurd pthreads?
>+/* Hurd pthreads don't have an invalid value for pthread_t. -- rmh */
>+#endif

I am a little worried about this #error statement.

>diff -ur nsprpub.old/pr/src/md/unix/unix.c nsprpub/pr/src/md/unix/unix.c
>--- nsprpub.old/pr/src/md/unix/unix.c	2004-09-24 20:29:02.000000000 +0200
>+++ nsprpub/pr/src/md/unix/unix.c	2004-09-24 20:24:02.000000000 +0200
>@@ -65,7 +65,7 @@
>  * PRInt32* pointer to a _PRSockLen_t* pointer.
>  */
> #if defined(HAVE_SOCKLEN_T) \
>-    || (defined(LINUX) && defined(__GLIBC__) && __GLIBC__ >= 2)
>+    || (defined(__GLIBC__) && __GLIBC__ >= 2)

Why can we omit defined(LINUX) here and not replace it
by defined(GLIBC)?

>diff -ur nsprpub.old/pr/src/md/unix/uxrng.c nsprpub/pr/src/md/unix/uxrng.c
>--- nsprpub.old/pr/src/md/unix/uxrng.c	2004-09-24 20:29:02.000000000 +0200
>+++ nsprpub/pr/src/md/unix/uxrng.c	2004-09-25 17:34:05.000000000 +0200
>@@ -138,7 +138,7 @@
...
>-#elif (defined(LINUX) || defined(FREEBSD) || defined(NETBSD) || defined(OPENBSD))
>+#elif (defined(LINUX) || defined(__FreeBSD_kernel__) || defined(__NetBSD_kernel__) || defined(OPENBSD))

In other places, you handle this by adding defined(GLIBC).
This avoids the need to define __FreeBSD_kernel__ and
__NetBSD_kernel__ on FreeBSD and NetBSD.

Why the difference?

>diff -ur nsprpub.old/pr/tests/Makefile.in nsprpub/pr/tests/Makefile.in
>--- nsprpub.old/pr/tests/Makefile.in	2004-09-24 20:29:02.000000000 +0200
>+++ nsprpub/pr/tests/Makefile.in	2004-09-24 20:24:02.000000000 +0200
>@@ -376,8 +376,8 @@
>     EXTRA_LIBS = -lsocket -lnsl -lgen -lresolv
> endif
> 
>-ifeq ($(OS_ARCH), Linux)
>-    ifeq ($(OS_RELEASE), 1.2)
>+ifneq (,$(filter $(OS_ARCH),Linux GNU GNU_kFreeBSD GNU_kNetBSD))
>+    ifeq ($(OS_ARCH)-$(OS_RELEASE), Linux-1.2)
>         EXTRA_LIBS = -ldl
>     else
>         LDOPTS += -Xlinker -rpath $(ABSOLUTE_LIB_DIR)

Does GNU mean GNU/Hurd?
> I don't understand why we can't assume Linux uses Glibc.

Well I don't really mind.  If you prefer to assume it, then we do.

> If we can assume Linux uses Glibc, all the instances of
>     defined(LINUX) || defined(GLIBC)
> in your patch can be replaced by
>     defined(GLIBC)

Ok.

> Is "GLIBC" the standard feature macro to indicate a Glibc
> system?

No, __GLIBC__ is the standard.

> If every Glibc system has <features.h>, can we just include
> <features.h> so we can test the __GLIBC__ macro?

This is a chicken-and-egg problem.  This header is Glibc-specific so including
it to check wether we're on Glibc will cause error for non-Glibc systems before
we can even check.

This is why, on GNU/kFreeBSD, __GLIBC__ is a C preprocessor macro and can be
checked at any time.  So if you want a generic check for Glibc, and assuming
Linux uses Glibc:

#if defined(__linux__) || defined(__GNU__) || defined(__GLIBC__)
/* this catches all Glibc-based systems that exist currently, and probably
future Glibc-based systems if they're wise and define __GLIBC__ too */
#endif

A sub-optimal alternative is including a standard system header like stdio.h,
which in turn includes features.h when we're using Glibc.

> (From update of attachment 171766 [details] [diff] [review] [edit])
> >+    case "$target_os" in
> >+      linux*) AC_DEFINE(LINUX) ;;
> >+      gnu* | k*bsd*-gnu) AC_DEFINE(GLIBC) ;;
> >+    esac
> 
> How do we determine if a Linux system uses Glibc?

Earlier you said we'd rather assume it?  It can be checked in many ways but all
boil down to the __GLIBC__ macro in features.h.

> >diff -ur nsprpub.old/pr/include/md/_linux.cfg nsprpub/pr/include/md/_linux.cfg
> >--- nsprpub.old/pr/include/md/_linux.cfg	2004-09-24 20:29:02.000000000 +0200
> >+++ nsprpub/pr/include/md/_linux.cfg	2004-09-25 13:43:44.000000000 +0200
> >@@ -42,10 +42,14 @@
> ... 
> >-#ifndef LINUX
> >+#if !defined(LINUX) && defined(__linux__)
> > #define LINUX
> > #endif
> 
> This definitely needs a comment to explain that _linux.cfg
> is now used by not only Linux but also other Glibc systems.

Ok.

> >+#if !defined(GLIBC) && (defined(__GNU__) || defined(__GLIBC__))
> >+#define GLIBC
> >+#endif
> 
> This should removed.  _linux.cfg is a public header file used
> by not only NSPR itself but also NSPR clients.	I don't want to
> define GLIBC in a public header if I can avoid it.  When compiling
> NSPR, we already have -DGLIBC on the compiler command line, so
> NSPR itself doesn't need this.

It's been long since I wrote this, but I recall this redundancy was added
because -DGLIBC didn't apply to this file in some situations.  I'll check again
later.

> Does __GNU__ mean GNU/Hurd?

Yes.

> > #elif defined(IRIX) || defined(OSF1) || defined(AIX) || defined(SOLARIS) \
> >-	|| defined(HPUX) || defined(LINUX) || defined(FREEBSD) \
> >+	|| defined(HPUX) || defined(LINUX) || defined(GLIBC) || defined(FREEBSD) \
> ...)
> >+#ifdef __GNU__
> >+# error Using Hurd pthreads?
> >+/* Hurd pthreads don't have an invalid value for pthread_t. -- rmh */
> >+#endif
> 
> I am a little worried about this #error statement.

Would you feel better with a #warning?  My intention was merely for the problem
not to remain unnoticed when this library is ported to GNU/Hurd.

> >diff -ur nsprpub.old/pr/src/md/unix/unix.c nsprpub/pr/src/md/unix/unix.c
> >--- nsprpub.old/pr/src/md/unix/unix.c	2004-09-24 20:29:02.000000000 +0200
> >+++ nsprpub/pr/src/md/unix/unix.c	2004-09-24 20:24:02.000000000 +0200
> >@@ -65,7 +65,7 @@
> >  * PRInt32* pointer to a _PRSockLen_t* pointer.
> >  */
> > #if defined(HAVE_SOCKLEN_T) \
> >-    || (defined(LINUX) && defined(__GLIBC__) && __GLIBC__ >= 2)
> >+    || (defined(__GLIBC__) && __GLIBC__ >= 2)
> 
> Why can we omit defined(LINUX) here and not replace it
> by defined(GLIBC)?

It doesn't really matter.  In this context either GLIBC or __GLIBC__ imply we're
using Glibc which is what the "__GLIBC__ >= 2" test is about.

> >diff -ur nsprpub.old/pr/src/md/unix/uxrng.c nsprpub/pr/src/md/unix/uxrng.c
> >--- nsprpub.old/pr/src/md/unix/uxrng.c	2004-09-24 20:29:02.000000000 +0200
> >+++ nsprpub/pr/src/md/unix/uxrng.c	2004-09-25 17:34:05.000000000 +0200
> >@@ -138,7 +138,7 @@
> ...
> >-#elif (defined(LINUX) || defined(FREEBSD) || defined(NETBSD) ||
defined(OPENBSD))
> >+#elif (defined(LINUX) || defined(__FreeBSD_kernel__) ||
defined(__NetBSD_kernel__) || defined(OPENBSD))
> 
> In other places, you handle this by adding defined(GLIBC).
> This avoids the need to define __FreeBSD_kernel__ and
> __NetBSD_kernel__ on FreeBSD and NetBSD.
> 
> Why the difference?

Because the code that follows this check is kernel-specific but not
libc-specific.  We can't assume it'll work for any Glibc system.

I don't remember what the code in question was.  Maybe something related to
errno macro values (which are defined by kernel) or /proc filesystem?

> >-ifeq ($(OS_ARCH), Linux)
> >-    ifeq ($(OS_RELEASE), 1.2)
> >+ifneq (,$(filter $(OS_ARCH),Linux GNU GNU_kFreeBSD GNU_kNetBSD))
> >+    ifeq ($(OS_ARCH)-$(OS_RELEASE), Linux-1.2)
> >         EXTRA_LIBS = -ldl
> >     else
> >         LDOPTS += -Xlinker -rpath $(ABSOLUTE_LIB_DIR)
> 
> Does GNU mean GNU/Hurd?

Yes.

You're exploring quite far the different possibilities of improving this patch.
 Not that I don't like it, but I'm a bit confused at what you want.  Basicaly if
you want me to supply a new patch I'd need to know:

  - Can we assume Linux uses Glibc?
  - If so, are you satisfied by a "defined(__linux__) || defined(__GNU__) ||
defined(__GLIBC__)" check?
  - Otherwise, which of these methods do you prefer for detecting Glibc on Linux?:
      - autoconf check for features.h followed by __GLIBC__ check in code.
      - include a dummy header (e.g. stdio.h) then check for __GLIBC__
      - check for $host_os values and define GLIBC on linux*|k*bsd*-gnu|gnu*
Attached patch wtchang's patch (obsolete) — Splinter Review
It'll probably be easier if I ask you to test my edited
version of your patch.	Here it is.  I will next describe
what changes in your patch I omitted and why.
Comment on attachment 171766 [details] [diff] [review]
new patch

Here is a summary of the changes in your patch that I omitted.
For this bug please use the tip of the NSPR source tree
(cvs -q co -A mozilla/nsprpub).

In nsprpub/config/nsinstall.c, you have:

>-#ifdef LINUX
>+#if defined(LINUX) || defined(GLIBC)
> #include <getopt.h>
> #endif

The standard header that declares getopt(3) is <unistd.h>.  In
older versions of glibc, getopt(3) is only declared in <getopt.h>.
I believe that with the versions of glibc you are using you can
just include <unistd.h>.

In nsprpub/configure.in, I don't define __FreeBSD_kernel__ and
__NetBSD_kernel__.  This means in the C files I need to say
    defined(FREEBSD) || defined(__FreeBSD_kernel__)
    defined(NETBSD) || defined(__NetBSD_kernel__)

Similarly, I don't define GLIBC, so in the C files I need to say
    defined(LINUX) || defined(__GNU__) || defined(__GLIBC__)

In nsprpub/pr/include/md/_linux.cfg, I don't define GLIBC.

In nsprpub/pr/src/md/prosdep.c, I use defined(__GNU__) || defined(__GLIBC__)
because your patch uses defined(GLIBC).  Since getpagesize() is a system
call, it may be more correct to use defined(__FreeBSD_kernel__) ||
defined(__NetBSD_kernel__) instead.  In this case it doesn't matter
which one we use because Linux, FreeBSD, and NetBSD all use getpagesize().

In nsprpub/pr/src/md/unix/uxrng.c, I now understand why you use
defined(__FreeBSD_kernel__) || defined(__NetBSD_kernel__); the code in
question tries to open /dev/random, which is a kernel feature.	But in
this case it doesn't matter which macros we use either because Linux,
FreeBSD, and NetBSD all use the same code.

In nsprpub/pr/src/pthreads/ptio.c, I omitted your change related to
sendfile for two reasons.  1. sendfile is a system call, so it's probably
more correct to use defined(__FreeBSD_kernel__) || defined(__NetBSD_kernel__)
than defined(GLIBC).  2. We'd need to make similar changes to
pt_linux_sendfile_cont
and pt_LinuxSendFile.  I omitted your change related to PRSockOptVal_t
because that code has been removed on the NSPR tip.

In nsprpub/pr/tests/Makefile.in, I omitted your change related to ECHO
because the "runtests" makefile target is obsolete.  You should instead
use the shell script nsprpub/pr/tests/runtests.sh to run our test suite.
Attachment #171766 - Flags: review?(wtchang)
Comment on attachment 171766 [details] [diff] [review]
new patch

Another change I made to nsprpub/pr/tests/Makefile.in
is to drop support for Linux 1.2 kernel.
(In reply to comment #11)
> (From update of attachment 171766 [details] [diff] [review] [edit])
> Here is a summary of the changes in your patch that I omitted.

Your patch didn't apply cleanly.  I'm attaching an update of the patch you just
added with the minor changes to apply to HEAD.

In addition, I'm attaching a fix for the getopt problem (see below).

After appliing both of these, we build and pass testsuite succesfuly.

> In nsprpub/config/nsinstall.c, you have:
> 
> >-#ifdef LINUX
> >+#if defined(LINUX) || defined(GLIBC)
> > #include <getopt.h>
> > #endif
> 
> The standard header that declares getopt(3) is <unistd.h>.  In
> older versions of glibc, getopt(3) is only declared in <getopt.h>.
> I believe that with the versions of glibc you are using you can
> just include <unistd.h>.

This is correct, except <unistd.h> only declares getopt(3) when _GNU_SOURCE has
been enabled.  My attached patch does just that.

> In nsprpub/pr/src/md/prosdep.c, I use defined(__GNU__) || defined(__GLIBC__)
> because your patch uses defined(GLIBC).  Since getpagesize() is a system
> call, it may be more correct to use defined(__FreeBSD_kernel__) ||
> defined(__NetBSD_kernel__) instead.  In this case it doesn't matter
> which one we use because Linux, FreeBSD, and NetBSD all use getpagesize().

But maybe GNU/Hurd doesn't.  I think it should be defined(__FreeBSD_kernel__) ||
defined(__NetBSD_kernel__) as you say.

> In nsprpub/pr/src/pthreads/ptio.c, I omitted your change related to
> sendfile for two reasons.  1. sendfile is a system call, so it's probably
> more correct to use defined(__FreeBSD_kernel__) || defined(__NetBSD_kernel__)
> than defined(GLIBC).

Actualy we don't have this syscall (it's disabled in glibc to avoid trouble), so
I assume no change is needed.
Attachment #160149 - Attachment is obsolete: true
Attachment #171766 - Attachment is obsolete: true
Attached patch getopt fix (obsolete) — Splinter Review
I forgot to mention that config.guess and config.sub need to be updated to a
more recent version in order to detect GNU/kFreeBSD.

Thanks!
Comment on attachment 188688 [details] [diff] [review]
getopt fix

In nsprpub/pr/src/linking/Makefile.in, you have:

> # For Dl_info and dladdr.
>-ifeq ($(OS_TARGET),Linux)
>+ifeq (,$(filter-out Linux GNU GNU_kFreeBSD GNU_kNetBSD,$(OS_TARGET)))
> DEFINES    += -D_GNU_SOURCE
> endif

This change is useless without a corresponding change
to nsprpub/pr/src/linking/prlink.c (around dladdr).
Ok.  Wrt dladdr thing, a __GLIBC__ check will do it.

diff -x configure -ur nsprpub.old/pr/src/linking/prlink.c
nsprpub/pr/src/linking/prlink.c
--- nsprpub.old/pr/src/linking/prlink.c 2005-07-02 00:26:36.000000000 +0200
+++ nsprpub/pr/src/linking/prlink.c     2005-07-09 13:46:24.000000000 +0200
@@ -1689,7 +1689,7 @@
 PR_IMPLEMENT(char *)
 PR_GetLibraryFilePathname(const char *name, PRFuncPtr addr)
 {
-#if defined(SOLARIS) || defined(LINUX) || defined(FREEBSD)
+#if defined(SOLARIS) || defined(LINUX) || defined(FREEBSD) || defined(__GLIBC__)
     Dl_info dli;
     char *result;
 
Attached patch new patch (obsolete) — Splinter Review
complete patch with wtchang's approved parts plus _GNU_SOURCE definitions (in
configure.in and pr/src/linking/Makefile.in) for getopt and dladdr.

diffed against current HEAD.

Please remember to update config.{guess,sub}!
Attachment #188687 - Attachment is obsolete: true
Attachment #188688 - Attachment is obsolete: true
Hi!

This bug is almost a year old.  I think everything is sorted out now, is the
patch ready for commit?
Hi! 
 
I'm very interested in this bug.  Is there something I can do to get the patch 
applied?  It seems to have been stalled. 
 
Ping..
Attached patch new patch v2 (obsolete) — Splinter Review
Thanks a lot for the new patch.  It looks good.
I only made some minor changes, most of which are
formatting changes.

1. mozilla/nsprpub/configure.in

I removed AC_DEFINE(_GNU_SOURCE).  mozilla/nsprpub/pr/src/linking/prlink.c
is the only file that needs to be compiled with
-D_GNU_SOURCE, and we take care of that in
mozilla/nsprpub/pr/src/linking/Makefile.in.

2. mozilla/nsprpub/pr/include/md/_pth.h

I'm wondering why you didn't add __GNU__ and __GLIBC__
to the test for PT_NO_SIGTIMEDWAIT.  Is it because
sigtimedwait is a kernel feature (system call)?

3. mozilla/nsprpub/pr/src/linking/prlink.c

Why didn't you add defined(__GNU__) ?

4. In the Makefile.in's, you can use

    GNU_%

  in place of

    GNU_kFreeBSD GNU_kNetBSD

Our makefiles change / to _ in the uname -s output.
Your PORTING guide suggests we use GNU/* in a uname
check.  The equivalent in our makefiles is GNU_%.
(% is GNU Make's wildcard in pattern matching.)

I've checked in this patch on the NSPR trunk (NSPR 4.7).
I will check it in on the NSPRPUB_PRE_4_2_CLIENT_BRANCH,
which is used by the Mozilla trunk, tomorrow.

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.208; previous revision: 1.207
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.210; previous revision: 1.209
done
Checking in pr/include/md/_linux.cfg;
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.cfg,v  <--  _linux.cfg
new revision: 3.19; previous revision: 3.18
done
Checking in pr/include/md/_pth.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_pth.h,v  <--  _pth.h
new revision: 3.31; previous revision: 3.30
done
Checking in pr/include/md/_unixos.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_unixos.h,v  <--  _unixos.h
new revision: 3.36; previous revision: 3.35
done
Checking in pr/include/md/prosdep.h;
/cvsroot/mozilla/nsprpub/pr/include/md/prosdep.h,v  <--  prosdep.h
new revision: 3.17; previous revision: 3.16
done
Checking in pr/src/linking/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/linking/Makefile.in,v  <--  Makefile.in
new revision: 1.16; previous revision: 1.15
done
Checking in pr/src/linking/prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.83; previous revision: 3.82
done
Checking in pr/src/md/prosdep.c;
/cvsroot/mozilla/nsprpub/pr/src/md/prosdep.c,v  <--  prosdep.c
new revision: 3.12; previous revision: 3.11
done
Checking in pr/src/md/unix/unix.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/unix.c,v  <--  unix.c
new revision: 3.52; previous revision: 3.51
done
Checking in pr/src/md/unix/uxproces.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/uxproces.c,v  <--  uxproces.c
new revision: 3.20; previous revision: 3.19
done
Checking in pr/src/md/unix/uxrng.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/uxrng.c,v  <--  uxrng.c
new revision: 1.19; previous revision: 1.18
done
Checking in pr/src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.48; previous revision: 3.47
done
Checking in pr/src/pthreads/ptio.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptio.c,v  <--  ptio.c
new revision: 3.103; previous revision: 3.102
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.48; previous revision: 1.47
done
Attachment #188966 - Attachment is obsolete: true
Attachment #188966 - Flags: review?(wtchang)
Hi!

I just tried current CVS.  It needed a minor fix to build (in nsinstall.c), other than this it builds and passes the testsuite.

I'm attaching a new patch with that, and the fixes you request below.

(after these changes, testsuite also passes ok)

(In reply to comment #23)
> 2. mozilla/nsprpub/pr/include/md/_pth.h
> 
> I'm wondering why you didn't add __GNU__ and __GLIBC__
> to the test for PT_NO_SIGTIMEDWAIT.  Is it because
> sigtimedwait is a kernel feature (system call)?

My oversight.  Yes, I think this can be added.

> 3. mozilla/nsprpub/pr/src/linking/prlink.c
> 
> Why didn't you add defined(__GNU__) ?

Oversight again.  I added it in new patch too.

> 4. In the Makefile.in's, you can use
> 
>     GNU_%
> 
>   in place of
> 
>     GNU_kFreeBSD GNU_kNetBSD
> 
> Our makefiles change / to _ in the uname -s output.
> Your PORTING guide suggests we use GNU/* in a uname
> check.  The equivalent in our makefiles is GNU_%.
> (% is GNU Make's wildcard in pattern matching.)

Ah, I didn't know this feature.  I'm adding it too.

That's all.  See attached new patch.

Thanks a lot!
I can't send a new attachment due to Bugzilla internal error, so I'll paste it here instead.  Sorry for the inconvenience.

Index: nsprpub/config/nsinstall.c
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/config/nsinstall.c,v
retrieving revision 3.20
diff -u -r3.20 nsinstall.c
--- nsprpub/config/nsinstall.c 25 Apr 2004 15:00:34 -0000 3.20
+++ nsprpub/config/nsinstall.c 24 Dec 2005 11:39:07 -0000
@@ -98,7 +98,7 @@
 }
 #endif /* NEXTSTEP */
 
-#ifdef LINUX
+#if defined(LINUX) || defined(__GLIBC__) || defined(__GNU__)
 #include <getopt.h>
 #endif
 
Index: nsprpub/pr/include/md/_pth.h
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/include/md/_pth.h,v
retrieving revision 3.31
diff -u -r3.31 _pth.h
--- nsprpub/pr/include/md/_pth.h 24 Dec 2005 08:25:23 -0000 3.31
+++ nsprpub/pr/include/md/_pth.h 24 Dec 2005 11:39:07 -0000
@@ -201,6 +201,7 @@
  * These platforms don't have sigtimedwait()
  */
 #if (defined(AIX) && !defined(AIX4_3_PLUS)) || defined(LINUX) \
+ || defined(__GLIBC__) || defined(__GNU__) \
  || defined(FREEBSD) || defined(NETBSD) || defined(OPENBSD) \
  || defined(BSDI) || defined(VMS) || defined(UNIXWARE) \
  || defined(DARWIN)
Index: nsprpub/pr/src/linking/Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/src/linking/Makefile.in,v
retrieving revision 1.16
diff -u -r1.16 Makefile.in
--- nsprpub/pr/src/linking/Makefile.in 24 Dec 2005 08:25:28 -0000 1.16
+++ nsprpub/pr/src/linking/Makefile.in 24 Dec 2005 11:39:07 -0000
@@ -63,7 +63,7 @@
 INCLUDES = -I$(dist_includedir) -I$(topsrcdir)/pr/include -I$(topsrcdir)/pr/include/private
 
 # For Dl_info and dladdr.
-ifeq (,$(filter-out Linux GNU GNU_kFreeBSD GNU_kNetBSD,$(OS_TARGET)))
+ifeq (,$(filter-out Linux GNU GNU_%,$(OS_TARGET)))
 DEFINES    += -D_GNU_SOURCE
 endif
 
Index: nsprpub/pr/src/linking/prlink.c
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v
retrieving revision 3.83
diff -u -r3.83 prlink.c
--- nsprpub/pr/src/linking/prlink.c 24 Dec 2005 08:25:28 -0000 3.83
+++ nsprpub/pr/src/linking/prlink.c 24 Dec 2005 11:39:08 -0000
@@ -1389,7 +1389,7 @@
 PR_GetLibraryFilePathname(const char *name, PRFuncPtr addr)
 {
 #if defined(SOLARIS) || defined(FREEBSD) \
-        || defined(LINUX) || defined(__GLIBC__)
+        || defined(LINUX) || defined(__GLIBC__) || defined(__GNU__)
     Dl_info dli;
     char *result;
 
Index: nsprpub/pr/tests/Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v
retrieving revision 1.48
diff -u -r1.48 Makefile.in
--- nsprpub/pr/tests/Makefile.in 24 Dec 2005 08:25:31 -0000 1.48
+++ nsprpub/pr/tests/Makefile.in 24 Dec 2005 11:39:08 -0000
@@ -376,7 +376,7 @@
     EXTRA_LIBS = -lsocket -lnsl -lgen -lresolv
 endif
 
-ifeq (,$(filter-out Linux GNU GNU_kFreeBSD GNU_kNetBSD,$(OS_ARCH)))
+ifeq (,$(filter-out Linux GNU GNU_%,$(OS_ARCH)))
     LDOPTS += -Xlinker -rpath $(ABSOLUTE_LIB_DIR)
     ifeq ($(USE_PTHREADS),1)
         EXTRA_LIBS = -lpthread
Attached patch new patch v3 (obsolete) — Splinter Review
I incorporated the new changes into this patch.
I've checked in the new changes on the NSPR trunk (NSPR 4.7)
and this patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla/Gecko trunk/1.9 alpha).  Here is the cvs commit
output for the NSPRPUB_PRE_4_2_CLIENT_BRANCH:

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.78.2.126; previous revision: 1.78.2.125
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.83.2.124; previous revision: 1.83.2.123
done
Checking in config/nsinstall.c;
/cvsroot/mozilla/nsprpub/config/nsinstall.c,v  <--  nsinstall.c
new revision: 3.17.2.4; previous revision: 3.17.2.3
done
Checking in pr/include/md/_linux.cfg;
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.cfg,v  <--  _linux.cfg
new revision: 3.12.4.9; previous revision: 3.12.4.8
done
Checking in pr/include/md/_pth.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_pth.h,v  <--  _pth.h
new revision: 3.19.2.14; previous revision: 3.19.2.13
done
Checking in pr/include/md/_unixos.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_unixos.h,v  <--  _unixos.h
new revision: 3.31.4.5; previous revision: 3.31.4.4
done
Checking in pr/include/md/prosdep.h;
/cvsroot/mozilla/nsprpub/pr/include/md/prosdep.h,v  <--  prosdep.h
new revision: 3.13.4.4; previous revision: 3.13.4.3
done
Checking in pr/src/linking/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/linking/Makefile.in,v  <--  Makefile.in
new revision: 1.10.2.6; previous revision: 1.10.2.5
done
Checking in pr/src/linking/prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.51.2.29; previous revision: 3.51.2.28
done
Checking in pr/src/md/prosdep.c;
/cvsroot/mozilla/nsprpub/pr/src/md/prosdep.c,v  <--  prosdep.c
new revision: 3.9.4.3; previous revision: 3.9.4.2
done
Checking in pr/src/md/unix/unix.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/unix.c,v  <--  unix.c
new revision: 3.43.2.9; previous revision: 3.43.2.8
done
Checking in pr/src/md/unix/uxproces.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/uxproces.c,v  <--  uxproces.c
new revision: 3.14.2.6; previous revision: 3.14.2.5
done
Checking in pr/src/md/unix/uxrng.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/uxrng.c,v  <--  uxrng.c
new revision: 1.11.4.6; previous revision: 1.11.4.5
done
Checking in pr/src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.21.2.27; previous revision: 3.21.2.26
done
Checking in pr/src/pthreads/ptio.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptio.c,v  <--  ptio.c
new revision: 3.71.2.24; previous revision: 3.71.2.23
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.34.2.14; previous revision: 1.34.2.13
done
Attachment #188364 - Attachment is obsolete: true
Attachment #206751 - Attachment is obsolete: true
I just tested your last commit and it builds and passes testsuite.

Thank you very much!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #206762 - Attachment is obsolete: true
I just realized you may have added AC_DEFINE(_GNU_SOURCE)
so that you could build mozilla/nsprpub/config/nsinstall.c
without the change to include <getopt.h>.  I think it may
be a good idea to define _GNU_SOURCE (or just remove the
-ansi compiler flag), but we don't need to do that in this
bug.

Now NSPR works on GNU/kFreeBSD and GNU/kNetBSD, but still
doesn't work on GNU/Hurd (because of the lack of an invalid
value for pthread_t), right?

I'm sorry about my long delay in reviewing and checking in
your patch.  Thanks a lot for your contribution.  Merry
Christmas! :-)
Whiteboard: [4.7]
> Now NSPR works on GNU/kFreeBSD and GNU/kNetBSD, but still
> doesn't work on GNU/Hurd (because of the lack of an invalid
> value for pthread_t), right?

Yes.  You'll have to check with the GNU/Hurd porters to provide another solution -- this is out of my reach.

> I'm sorry about my long delay in reviewing and checking in
> your patch.  Thanks a lot for your contribution.

No problem.  It was worth the wait! :)

Btw, I might propose this to be merged into 1.8 or even 1.8.0 later (along with my other patches for trunk that aren't yet submitted).  Do I have to check with you, or just send the proposal?

Thank you.
You need to get your patches checked in on the trunk
first.  The Mozilla 1.8 branch is more likely to accept
your patches.  The Mozilla 1.8.0.1 branch is mainly for
security vulnerability fixes, so the GNU/k*BSD porting
patches may not be accepted.
it looks like this patch may have caused a regression in BeOS.  See https://bugzilla.mozilla.org/show_bug.cgi?id=321579 
In bug 321579 we learned that BeOS also uses
glibc, but either that glibc doesn't have the
dlxxx functions declared in <dlfcn.h>, or we
(NSPR) choose not to use them.

I've asked the BeOS Mozilla developers to review
our patch v3 (attachment 206765 [details] [diff] [review]) in this bug.

This patch fixes the BeOS build error caused by
that patch.
Whiteboard: [4.7]
Target Milestone: --- → 4.7
I found that most of the patch (attachment 206765 [details] [diff] [review]) is
in Unix-only code.  I collected the changes that affect
BeOS in this patch.  Please review and make sure they
don't break BeOS.

1. mozilla/nsprpub/config/nsinstall.c: you just need
to verify that BeOS has the <getopt.h> header.

2. mozilla/nsprpub/pr/src/linking/prlink.c: I know
this patch works, so I'm just curious as to whether
BeOS doesn't have the dlxxx functions (declared in
<dlfcn.h>), or we choose not to use them.

3. mozilla/nsprpub/pr/src/misc/prnetdb.c: you need
to verify that BeOS has the following two functions
(declared in <netdb.h>) and it's ok to use them:

extern int getprotobyname_r (__const char *__restrict __name,
                             struct protoent *__restrict __result_buf,
                             char *__restrict __buf, size_t __buflen,
                             struct protoent **__restrict __result);

extern int getprotobynumber_r (int __proto,
                               struct protoent *__restrict __result_buf,
                               char *__restrict __buf, size_t __buflen,
                               struct protoent **__restrict __result);
Attachment #207454 - Flags: superreview?(sergei_d)
Attachment #207454 - Flags: review?(cbiesinger)
Comment on attachment 207454 [details] [diff] [review]
Subset of patch that affects BeOS

hm... I only have Zeta here, which is a pretty new version of beos. so, take this with a grain of salt.

I do have getopt.h.

I'm pretty sure not all beos systems have libdl, which is why we prefer not to use it, and instead use native beos functions. there's also the issue of the .stub libraries we use...

The getproto* functions are declared and can be called, though they seem to just fail... (tested with "tcp" as name, or 6 as number)
They have four arguments:
- the name/number (const char* or int)
- struct protoent*
- char*
- int

(NOTE: I haven't checked whether the non-reentrant versions return meaningful data)

So at least the last part here is not ok - there is no 5-arg version of getproto*.
Attachment #207454 - Flags: review?(cbiesinger) → review-
Christian: do you always define BONE_VERSION when building
Mozilla clients on BeOS now?

Are getprotobyname_r and getprotobynumber_r declared like
the following (ignoring parameter names) on BeOS?

  struct protoent *getprotobyname_r(const char  *name,  struct
  protoent *result, char *buffer, int buflen);

  struct protoent *getprotobynumber_r(int proto,  struct
  protoent *result, char *buffer, int buflen);

Since you already provided the parameters in comment 35, I
just need to know if these two functions return struct protoent *.
sorry, I don't know the answer to the BONE_VERSION question.

Yes, the functions do return struct protoent*.
No, BONE is unfortunatly not supported by plain BeOS R5 which a lot of users depend upon. When building on Zeta BONE should be default, but I've never built from Zeta.
Wan-Teh,
is patch
https://bugzilla.mozilla.org/attachment.cgi?id=207454
checked in already?
I got strange problem building seamonkey on "classical" BeOS R5 (no BONE), which looks seomewhat related to nsinstall in NSS:
http://community.livejournal.com/bezilla/167654.html
Sergei, the NSPR patch (attachment 207454 [details] [diff] [review]) has been checked in
on the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla trunk) and
NSPR trunk only.  It hasn't been checked in on any other
Mozilla branch.

Your build errors are in NSS, so are unlikely to be related
to this bug.  We landed a new NSS (3.11) on the Mozilla
trunk in January and on the MOZILLA_1_8_BRANCH last Friday
(bug 317620).  Even though that's a massive change, it shouldn't
cause the strange build problem you are seeing.  Let's continue
the discussion by email.
As Christian reported in comment 35, BeOS has the getprotobyxxx_r
functions, but they aren't glibc's 5-argument version.
Attachment #207454 - Attachment is obsolete: true
Attachment #213385 - Flags: review?(cbiesinger)
Attachment #207454 - Flags: superreview?(sergei_d)
Attachment #213385 - Flags: review?(cbiesinger) → review+
You need to log in before you can comment on or make changes to this bug.