Closed Bug 5358 Opened 22 years ago Closed 21 years ago

PR_StackPush and PR_StackPop are not implemented on Solaris/x86

Categories

(NSPR :: NSPR, defect, P3)

Sun
Solaris
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

References

Details

(Whiteboard: [7.26.1999]waiting for feedback)

Attachments

(3 files)

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).
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Product: Browser → NSPR
NSPR now has its own Bugzilla product.  Moving this bug to the NSPR product.
Whiteboard: waiting for feedback
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.
Whiteboard: waiting for feedback → [7.26.1999]waiting for feedback
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: phillip → gerardok
QA Contact massive update.
Attached patch a context diffSplinter Review
Status: RESOLVED → REOPENED
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.
Resolution: FIXED → ---
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'.
Status: REOPENED → RESOLVED
Closed: 22 years ago21 years ago
Resolution: --- → FIXED
Attachment 2600 [details] [diff] is checked in to the tip.  It is
also checked into the internal repository.

Marked the bug fixed.
You need to log in before you can comment on or make changes to this bug.