263952 - mmap64, shm_open and shm_unlink compiler warnings

Status: NEW

(NSPR :: NSPR, defect, P3)

Description

[Julien Pierre]

mmap64 on Solaris is defined to return and take char*, rather than void* .
This causes a compiler warning in 64-bit builds, both Sparc and AMD64, in
pr/src/md/unix.c:2790 

    _md_iovector._mmap64 = mmap64;

        pointer to function(pointer to void, unsigned int, int, int, int, long
long) returning pointer to void "=" pointer to function(pointer to char,
unsigned int, int, int, int, long long) returning pointer to char

The prototype for the function mmap64 does not agree with the definition of
_MD_Mmap64 .

I would rather not cast mmap64 to _MD_Mmap64 in unix.c, because this could hide
real problems in the future. The proper solution is probably to ifdef the
_MD_Mmap64 definition for Solaris . On other unix platforms (Linux), the mmap64
definition uses void* .

Comment 1

[Wan-Teh Chang]

The proper declaration of mmap and mmap64 should use void *.
See http://www.opengroup.org/onlinepubs/009695399/functions/mmap.html.

I think the solution is to compile NSPR with the appropriate
macros so that we get the right declaration of mmap and mmap64
in <sys/mman.h>.  By looking at the standards(5) man page on
Solaris 8, I believe we should use -D_XOPEN_SOURCE=500 -D__EXTENSIONS__.
These in turn require that we use -DHAVE_SOCKLEN_T.

You might want to ask someone in the Solaris or compiler team
about the right macros to define.

You should also test the above changes under gcc.

Comment 2

[Julien Pierre]

Wan-Teh,

Thank you ! Those defines worked. I did ask somebody in the Solaris group, but
you answered earlier ;).

Comment 3

[Julien Pierre]

Hmm. Actually, these defines fixed the warning on Solaris 9 / sparc, but on
S10/AMD64, I got :
"/usr/include/sys/feature_tests.h", line 332: #error: "Compiler or options
invalid for pre-UNIX 03 X/Open applications  and pre-2001 POSIX applications"

I had to set the version to 600 to make it work.

This also fixes another problem with implicit definitions for shm_open and
shm_unlink .
"../../../../../pr/src/md/unix/uxshm.c", line 358: warning: implicit function
declaration: shm_open
"../../../../../pr/src/md/unix/uxshm.c", line 465: warning: implicit function
declaration: shm_unlink

Comment 4

[Julien Pierre]

It looks like the patch will require checking specific minor versions of Solaris
in order to define the correct XOPEN version. We can't define a higher version.
Defining version 600 and compiling on Solaris 9 results in failure. We have to
define version 500 exactly to build on that platform. In addition, the gcc
compiler behaves differently with the various defines. This bug does not have to
be fixed short term, so targetting NSPR 4.6.

Comment 5

[Julien Pierre]

Attachment: working patch

Gets rid of warnings on Solaris for mmap64, shm_open and shm_unlink
Tested on x86 / AMD64 / sparcv8 / sparcv9

Comment 6

[Wan-Teh Chang]

Comment on attachment 193773 [details] [diff] [review]
working patch

The __EXTENSIONS__ and _XPG4_2 macros should be
defined in configure.in so that they are only
used when compiling NSPR itself.  _solaris.cfg
will be included by NSPR customers.  We don't want
to force our customers to define those two macros.

Could you explain how you determined that these
two macros are the right ones to define?

Have you tested this on Solaris 8-10 and with GCC?

Comment 7

[Julien Pierre]

I tested with gcc. I tried Solaris 9 and 10, I didn't check with 8.
I'll make a new patch for configure / configure.in so that this change doesn't
affect customers.

I found that these are the right macros by examining the tests in the Solaris
header files, mostly /usr/include/sys/mman.h . See the following section in
particular :

#ifdef  __STDC__
#if (_POSIX_C_SOURCE > 2) || defined(_XPG4_2)
extern void *mmap(void *, size_t, int, int, int, off_t);
extern int munmap(void *, size_t);
extern int mprotect(void *, size_t, int);
extern int msync(void *, size_t, int);
#if (!defined(_XPG4_2) || (_POSIX_C_SOURCE > 2)) || defined(__EXTENSIONS__)
extern int mlock(const void *, size_t);
extern int munlock(const void *, size_t);
extern int shm_open(const char *, int, mode_t);
extern int shm_unlink(const char *);

We don't set POSIX_C_SOURCE, and I had no luck playing with that macro. Short of
that, the only possibility that allows shm_open and shm_unlink to be defined is
for both _XPG4_2 and __EXTENSIONS__ to be set .

This happened to also fix mmap64 at the same time, and didn't change any other
warnings for my NSPR build.

Comment 8

[Julien Pierre]

Attachment: make all the changes in configure / configure.in

Comment 9

[Julien Pierre]

Thanks for the quick review, Wan-Teh. Could you please check the patch in to the
trunk and mark the bug fixed ?

Comment 10

[Wan-Teh Chang]

Comment on attachment 193776 [details] [diff] [review]
make all the changes in configure / configure.in

Julien: sorry, I found that _XPG4_2 is a private feature
test macro and applications must not use it.  See the
comments in <sys/feature_tests.h>.

I looked at Solaris 10's <sys/feature_tests.h> and couldn't
figure out a solution without using autoconf tests.

Comment 11

[Julien Pierre]

Wan-Teh,

FYI, the comments about _XPG4_2 sys/feature_tests.h Solaris 10 are different
than for 8 and 9, and say nothing about it being an internal Sun macro ...

In fact, the s10 header says :

 * _XPG4_2  X/Open CAE Specification, Issue 4, Version 2 (XPG4v2/UNIX 95/SUS)

Comment 12

[Wan-Teh Chang]

See lines 247-264 in <sys/feature_tests.h>, which I quote below:
(http://cvs.opensolaris.org/source/xref/usr/src/uts/common/sys/feature_tests.h)

    247  * In order to simplify the guards within the headers, the following
    248  * implementation private test macros have been created. Applications
    249  * must NOT use these private test macros as unexpected results will
    250  * occur.
    251  *
    252  * Note that in general, the use of these private macros is cumulative.
    253  * For example, the use of _XPG3 with no other restrictions on the X/Open
    254  * namespace will make the symbols visible for XPG3 through XPG6
    255  * compilation environments. The use of _XPG4_2 with no other X/Open
    256  * namespace restrictions indicates that the symbols were introduced in
    257  * XPG4v2 and are therefore visible for XPG4v2 through XPG6 compilation
    258  * environments, but not for XPG3 or XPG4 compilation environments.
    259  *
    260  * _XPG3    X/Open Portability Guide, Issue 3 (XPG3)
    261  * _XPG4    X/Open CAE Specification, Issue 4 (XPG4)
    262  * _XPG4_2  X/Open CAE Specification, Issue 4, Version 2 (XPG4v2/UNIX
95/SUS)
    263  * _XPG5    X/Open CAE Specification, Issue 5 (XPG5/UNIX 98/SUSv2)
    264  * _XPG6    Open Group Technical Standard, Issue 6 (XPG6/UNIX 03/SUSv3)

So _XPG4_2 is one of the "private test macros" the comments at lines
247-250 refer to.  Also, do "man -s 5 standards" on Solaris to look
at the standards(5) man page.  These private test macros are not
documented in that man page.

Comment 13

[Julien Pierre]

Attachment: use public macros

This patch only uses public, blessed macros, and works on s9 and s10, with both
studio and gcc .

Comment 14

[Wan-Teh Chang]

Comment on attachment 193876 [details] [diff] [review]
use public macros

r=wtc.

The only thing in this patch I'm worried about
is socklen_t.  I don't know in which Solaris release
socklen_t was added.  Let's at least compile the
code on Solaris 8.

The standards(5) man page seems to say that XPG4v2
support was added in Solaris 2.6, which means the
_XOPEN_SOURCE and _XOPEN_SOURCE_EXTENDED defines
are definitely okay.

Comment 15

[Julien Pierre]

Wan-Teh,

I just compiled NSPR with this patch on Solaris 8 x86, and it built
successfully, as well as without the warnings reported in this bug.

Comment 16

[Wan-Teh Chang]

Comment on attachment 193876 [details] [diff] [review]
use public macros

I checked in this patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla 1.9 development) for some real world testing.	I
suspect the HAVE_SOCKLEN_T define will break builds on old
Solaris releases (such as Solaris 2.6).  I will check this
patch on the NSPR trunk when things settle down.

Comment 17

[Julien Pierre]

This patch does break the build on Solaris 2.6, but not because of HAVE_SOCKLEN_T :

cc -o prfdcach.o -c      -xstrconst -xs -g -KPIC  -UNDEBUG -DDEBUG_jp96085 
-DDEBUG=1 -DXP_UNIX=1 -DSVR4=1 -DSYSV=1 -D__svr4=1 -D__svr4__=1 -DSOLARIS=1
-DHAVE_FCNTL_FILE_LOCKING=1 -DHAVE_SOCKLEN_T=1 -D__EXTENSIONS__=1
-D_XOPEN_SOURCE=1 -D_XOPEN_SOURCE_EXTENDED=1 -D_PR_HAVE_OFF64_T=1
-DHAVE_LCHOWN=1 -DHAVE_STRERROR=1 -D_REENTRANT=1 -DHAVE_POINTER_LOCALTIME_R=1 
-DFORCE_PR_LOG -D_PR_PTHREADS -UHAVE_CVAR_BUILT_ON_SEM -D_NSPR_BUILD_
-I/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include
-I../../../../pr/include -I../../../../pr/include/private 
../../../../pr/src/io/prfdcach.c
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 600: syntax error before or at: _MDOff64_t
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 600: warning: old-style declaration or incorrect type for: _MDOff64_t
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 618: syntax error before or at: *
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 618: syntax error before or at: _MDOff64_t
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OB
J/include/md/_unixos.h", line 618: warning: undefined or missing type for:
_MDOff64_t
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 618: warning: undefined or missing type for: PRIntn
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 618: function cannot return function or array
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 618: identifier redeclared: _MDOff64_t
        current : function() returning int
        previous: int :
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 600
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 628: syntax error before or at: _MD_Lseek64
"/h/monstre/export/home/nss/perf/mozilla/security/nss/../../dist/SunOS5.6_DBG.OBJ/include/md/_unixos.h",
line 628: cannot recover from previous errors
cc: acomp failed for ../../../../pr/src/io/prfdcach.c
gmake[4]: *** [prfdcach.o] Error 2
gmake[4]: Leaving directory
`/h/monstre/export/home/nss/perf/mozilla/nsprpub/SunOS5.6_DBG.OBJ/pr/src/io'
gmake[3]: *** [export] Error 2
gmake[3]: Leaving directory
`/h/monstre/export/home/nss/perf/mozilla/nsprpub/SunOS5.6_DBG.OBJ/pr/src'
gmake[2]: *** [export] Error 2
gmake[2]: Leaving directory
`/h/monstre/export/home/nss/perf/mozilla/nsprpub/SunOS5.6_DBG.OBJ/pr'
gmake[1]: *** [export] Error 2
gmake[1]: Leaving directory
`/h/monstre/export/home/nss/perf/mozilla/nsprpub/SunOS5.6_DBG.OBJ'

Comment 18

[Wan-Teh Chang]

The build would fail still later with socklen_t if it could
continue.

I don't have any Solaris 2.6 machines.  Julien, do you know
how to fix the off64_t problem?  If not, we will need to update
the configure script to only define those macros on
Solaris 8 or higher.

Could you email me the Solaris 2.6 header that defines off64_t?

Comment 19

[Saul Edwards]

Patch 193876, which was checked in to the trunk, caused a large performance
regression (~2.5%) in specweb performance using the current NSS trunk build.  I
am raising the priority of this bug because this regression must be fixed by NSS
3.11 RTM.

Judging by the difficulty of authoring another patch which both fixes the
warnings and does not cause a performance regression, I propose we revert the
change for now.

Comment 20

[Wan-Teh Chang]

NSS 3.11 should be built against the NSPR_4_6_BRANCH.
Julien's patch is only on the NSPR trunk; it is not
on the NSPR_4_6_BRANCH.  So the performance regression
does not need to be fixed before NSS 3.11 RTM.

Comment 21

[Julien Pierre]

Unfortunately, I don't have the time to do the investigation of the performance
regression caused by this patch at the moment. I'm unsetting the target for this
fix since there is no 4.7 target yet, and the fix isn't on the NSPR_4_6_BRANCH. 

Comment 22

[Wan-Teh Chang]

Attachment: Define the _MDAddr_t type for an address

This is an alternate way to fix the compiler
warning for mmap64.  I can't reproduce the
compiler warnings for shm_open and shm_unlink,
so this patch may not address that issue.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

This can't be P1 if it has no target milestone.
P1 means we'd hold a release for it.  Which release would we hold?

Comment 24

[Julien Pierre]

Nelson,

The release we would hold for this is NSPR 4.7 . The reason the target is not
set on this bug is that the 4.7 version number is not currently in the available
targets in bugzilla.

Comment 25

[Leon Sha]

Now I can not build firefox trunk on solaris 10.
The error message is below.

cd config; gmake -j1 export
gmake[1]: Entering directory
`/export/home/leon/work/mozilla/firefox/src/build1009/mozilla/nsprpub/config'
/opt/SUNWspro/bin/cc -o nsinstall.o -c     -xlibmil -xstrconst -xs -g -KPIC 
-UNDEBUG -DDEBUG_leon  -DDEBUG=1 -DXP_UNIX=1 -DSVR4=1 -DSYSV=1 -D__svr4=1
-D__svr4__=1 -DSOLARIS=1 -DHAVE_FCNTL_FILE_LOCKING=1 -D_XOPEN_SOURCE=1
-D_XOPEN_SOURCE_EXTENDED=1 -D__EXTENSIONS__=1 -DHAVE_SOCKLEN_T=1
-D_PR_HAVE_OFF64_T=1 -D_PR_INET6=1 -DHAVE_LCHOWN=1 -DHAVE_STRERROR=1
-D_REENTRANT=1 -DHAVE_POINTER_LOCALTIME_R=1  -DFORCE_PR_LOG -D_PR_PTHREADS
-UHAVE_CVAR_BUILT_ON_SEM   nsinstall.c
"/usr/include/sys/feature_tests.h", line 332: #error: "Compiler or options
invalid for pre-UNIX 03 X/Open applications  and pre-2001 POSIX applications"
cc: acomp failed for nsinstall.c
gmake[1]: *** [nsinstall.o] Error 2
gmake[1]: Leaving directory
`/export/home/leon/work/mozilla/firefox/src/build1009/mozilla/nsprpub/config'
gmake: *** [export] Error 2

Comment 26

[Wan-Teh Chang]

Leon:

There may be a conflict between the -xlibmil flag that you use
use and the -D_XOPEN_SOURCE=1 -D_XOPEN_SOURCE_EXTENDED=1 feature
test macros we added.  Could you try configuring Firefox without
-xlibmil in your CFLAGS?

In any case, I backed out the checkin to define the Solaris feature
test macros for SUS (XPG4v2) with extensions.  I'll try the other
patch (attachment 196874 [details] [diff] [review]) to define a type for an address and use
that type in the prototype of the function type _MD_Mmap64.

Comment 27

[Leon Sha]

(In reply to comment #26)
> Leon:
> 
> There may be a conflict between the -xlibmil flag that you use
> use and the -D_XOPEN_SOURCE=1 -D_XOPEN_SOURCE_EXTENDED=1 feature
> test macros we added.  Could you try configuring Firefox without
> -xlibmil in your CFLAGS?

This does not help.
Still the same error.

> 
> In any case, I backed out the checkin to define the Solaris feature
> test macros for SUS (XPG4v2) with extensions.  I'll try the other
> patch (attachment 196874 [details] [diff] [review] [edit]) to define a type for an address and use
> that type in the prototype of the function type _MD_Mmap64.

Comment 28

[Julien Pierre]

Leon,

I tried the exact same compiler command you used in comment #25 on my tree,
prior to Wan-Teh backing out the change. It compiled just fine, both on Sparc
and x86 on Solaris 10 . I was using the Studio 10 compiler. I also tried with
Forte 6 update 2 on Sparc.

Which compiler were you using that got you the failure ?

Comment 29

[Leon Sha]

I have tried Sun stadio 8 and Sun stadio 9.
Both of them have problem to build this.
And I have also tried Sun Stadio 10 and forte 6.
There is no problem to build this.

Since there are some problems when build firefox 1.0.x using Sun Stadio 10. I am
always using Sun Stadio 8 to build it. Maybe I should use Sun Stadio 10 to build
firefox 1.5 later.

(In reply to comment #28)
> Leon,
> 
> I tried the exact same compiler command you used in comment #25 on my tree,
> prior to Wan-Teh backing out the change. It compiled just fine, both on Sparc
> and x86 on Solaris 10 . I was using the Studio 10 compiler. I also tried with
> Forte 6 update 2 on Sparc.
> 
> Which compiler were you using that got you the failure ?
> 

Comment 30

[Julien Pierre]

Reassigning to Nelson who is Sun's main NSPR contact now.

This is a minor bug . Unfortunately, we were not able to find a single combination of macros that works accross all of Sun's compilers and platforms to resolve these compiler warnings. This would need to be passed to a Solaris expert, if it needs to be resolved. I'm not sure if it does - this is really an aesthetic thing - warnings show up in build logs when looking for warnings and errors, and that makes us look bad since these warnings are legit.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Reassigning to Alexei, since I am no longer at Sun and no longer have access
to Sun systems with which to test any possible fixes.