5358 - PR_StackPush and PR_StackPop are not implemented on Solaris/x86

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Wan-Teh Chang]

This bug is reported by Justin A. Kolodziej
<[e-mail]> 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).

Comment 1

[Wan-Teh Chang]

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.

Comment 2

[Terry Weissman]

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

Comment 3

[Philip Pokorny]

Attachment: Here is code that implements the "real fix" for this bug.

Comment 4

[phillip]

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

Comment 5

[Wan-Teh Chang]

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

Comment 6

[phillip]

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

Comment 7

[Philip Pokorny]

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...

Comment 8

[srinivas]

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

Comment 9

[gerardok]

QA Contact massive update.

Comment 10

[Roy Jones]

Attachment: a context diff

Comment 11

[Roy Jones]

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

Comment 12

[Wan-Teh Chang]

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.

Comment 13

[leger]

Clearing Fixed resolution due to reopen of this bug.

Comment 14

[Wan-Teh Chang]

Attachment: Proposed patch to remove #ifdef _PR_HAVE_ATOMIC_CAS from os_SunOS_x86.s and related makefile changes.

Comment 15

[Wan-Teh Chang]

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

Comment 16

[Roy Jones]

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

Comment 17

[Roy Jones]

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?

Comment 18

[Wan-Teh Chang]

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

Comment 19

[Wan-Teh Chang]

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

Marked the bug fixed.