PR_StackPush and PR_StackPop are not implemented on Solaris/x86

RESOLVED FIXED

Status

defect
P3
normal
RESOLVED FIXED
20 years ago
20 years ago

People

(Reporter: wtc, Assigned: wtc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [7.26.1999]waiting for feedback)

Attachments

(3 attachments)

Assignee

Description

20 years ago
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).
Assignee

Updated

20 years ago
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee

Comment 1

20 years ago
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.

Updated

20 years ago
Product: Browser → NSPR

Comment 2

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

Updated

20 years ago
Whiteboard: waiting for feedback

Comment 4

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

Comment 5

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

Updated

20 years ago
Whiteboard: waiting for feedback → [7.26.1999]waiting for feedback

Comment 6

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

Comment 7

20 years ago
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

20 years ago
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

Updated

20 years ago
QA Contact: phillip → gerardok

Comment 9

20 years ago
QA Contact massive update.

Comment 10

20 years ago

Updated

20 years ago
Status: RESOLVED → REOPENED

Comment 11

20 years ago
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
Assignee

Comment 12

20 years ago
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.

Updated

20 years ago
Resolution: FIXED → ---

Comment 13

20 years ago
Clearing Fixed resolution due to reopen of this bug.
Assignee

Comment 15

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

Comment 16

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

Comment 17

20 years ago
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?
Assignee

Comment 18

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

Updated

20 years ago
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Assignee

Comment 19

20 years ago
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.