This bug is reported by Justin A. Kolodziej <4wg7kolodzie@marquette.edu> in the mozilla-nspr newsgroup. NSPR does not build on Solaris/x86 right now because the libnspr3 library is missing the symbols PR_StackPush and PR_StackPop. PR_StackPush and PR_StackPop are implemented in the SPARC assembly in os_SunOS.s, however, there are no corresponding definitions for the x86 in os_SunOS_x86.s. The real fix is to write the x86 assembly for PR_StackPush and PR_StackPop in os_SunOS_x86.s. A quick fix is to define _PR_HAVE_ATOMIC_CAS only for SPARC in nsprpub/pr/include/md/_solaris.h. Then _PR_HAVE_ATOMIC_CAS will be undefined for x86, and hence x86 will use the default atomic stack implementation (which uses a lock).

The quick fix is checked into the tip. /cvsroot/mozilla/nsprpub/pr/include/md/_solaris.h, revision 3.5. Also checked into NSPRPUB_RELEASE_3_1_BRANCH. /cvsroot/mozilla/nsprpub/pr/include/md/_solaris.h, revision 3.4.34.1. Marked the bug fixed. To verify, log into a Solaris/x86 machine (e.g., solar.mcom.com) and do this: % cd mozilla/nsprpub % gmake NS_USE_GCC=1 export % cd pr/tests % gmake NS_USE_GCC=1 export % cd SunOS5.6_x86_PTH_DBG.OBJ % ./stack The 'stack' test program should report pass.

NSPR now has its own Bugzilla product. Moving this bug to the NSPR product.

has the real fix been checked in yet? is it going to happen? (should i verify this?)

I need to find someone who understands x86 assembly to review this patch.

ppokorny, has this been checked into the tree? can you verify that it works?

I can verify that this code passes the STACK.C test included with NSPR. I added some debug code to my version of stack.c so that I could test the code, but I didn't materially change the code logic. In fact the first few implementations were wrong and having the stack.c code helped me find which registers were reserved and needed to be saved and which could be modified without saving. I don't have a working Mozilla build, so I can't say how it affects that. As for checking it in, I'd love to, but I don't have CVS. Sorry...

The assembly language implementation in the attachment (id=559) has been checked in with one modification. The 'lock' prefix is not needed for the 'xchg' instruction on x86; the processor automatically asserts the LOCK signal for the xchg instruction. Files modified: nsprpub/pr/src/md/unix/os_SunOS_x86.s nsprpub/pr/include/md/_solaris.h

QA Contact massive update.

unfortunately this bug persists, the new functions are not defined correctly. Even with the attached patch the ifdef PR_HAVE_ATOMIC_CAS does not work as it needs md/_solaris.h and for some reason gcc is not passing the include directories to cpp

We should just remove "#ifdef _PR_HAVE_ATOMIC_CAS" and "#endif / _PR_HAVE_ATOMIC_CAS" from os_SunOS_x86.s. It is tricky to make gcc invoke the C preprocessor on an assembly file. The gcc man page suggests that we can either use the .S (capital S) file name suffix or use the "-x assembler-with-cpp" compiler option to tell gcc to preprocess the assembly file before assembling it. I remember we tried "-x assembler-with-cpp" and ran into some strange problem. You might want to give .S and "-x assembler-with-cpp" a try, but I would just remove the ifdef.

Clearing Fixed resolution due to reopen of this bug.

*** Bug 17935 has been marked as a duplicate of this bug. ***

Have tested propossed patch (id=2600) and nspr passes stack test, mozilla also running correctly. Looks good.

The proposed patch removes the ifdef from the asm file which also removes the need to run cpp over it. This enables us to remove -Wa,-P from SunOS5.mk and so allow people to use gas or as to build mozilla. The -s flag is still needed for debug builds but as newer versions of gas ignore it and older ones print stats this is not a problem, unfortunately this would not work for sparcs as their asm code still needs cpp. Do you think it worth splitting the ASFLAGS in SunOS5.mk for this?

Yes, since -Wa,-P is only needed by the sparc assembly file, we can conditionalize it on $(CPU_ARCH) being 'sparc'.

Attachment 2600 [details] [diff] is checked in to the tip. It is also checked into the internal repository. Marked the bug fixed.