[TraceMonkey] Multiple cases in trace-test failed on Sparc platform.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Leon Sha, Assigned: Leon Sha)

Tracking

unspecified
All
Solaris
Points:
---

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

465 bytes, patch
brendan
: review+
Robert Sayre
: approval1.9.2+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
js/src/trace-test/tests/sunspider/check-3d-morph.js
js/src/trace-test/tests/sunspider/check-3d-raytrace.js
js/src/trace-test/tests/sunspider/check-access-binary-trees.js
js/src/trace-test/tests/sunspider/check-access-fannkuch.js
js/src/trace-test/tests/sunspider/check-access-nbody.js
.........
The root cause is JSString::unitStringTable in jsstr.* is not 8 bytes aligned compiled with Sun Studio.
There are some discussions in bug 515252 about this problem.

Stack Trace:
-----------------------
 000b8f40 js_LookupPropertyWithFlags (20edc8, fec61000, 1a4c9c, 20edc8, fec61000, ffff) + 54
 000b8ed4 js_LookupProperty (20edc8, fec61000, 1a4c9c, ffbfda0c, ffbfda08, f6b28) + 18
 000a7d88 js_CheckRedeclaration (20edc8, fec61000, 1a4c9c, 5, ffbfe610, ffbfe60c) + 34
 0017ee1c js_Interpret (0, 20edc8, ffbfe6a8, 21990c, 0, 3) + a974
 000a7c9c js_Execute (20edc8, fec61000, 2198d0, 0, fec61000, 0) + 544
 000576d0 JS_ExecuteScript (20edc8, fec61000, 2198d0, 0, 20edc8, 2000) + 1c
 0004460c void Process(JSContext*,JSObject*,char*,int) (20edc8, ff35793c, ffbfeb9c, 19efc8, ffea5b08, fffea3bf) + 20c
 00045604 int ProcessArgs(JSContext*,JSObject*,char**,int) (19eff0, 19f000, 20edc8, 7, fec61000, 19e880) + 714
 0004af44 main     (8, ffbfec04, ffbfec28, 1aba60, fec61000, 1) + 3ec
 00043f58 _start   (0, 0, 0, 0, 0, 0) + 108
(Assignee)

Comment 1

9 years ago
Created attachment 400424 [details] [diff] [review]
hacking.

With this patch unitStringTable will be aligned to 8 bytes by lucky.
Attachment #400424 - Flags: review?(brendan)
Comment on attachment 400424 [details] [diff] [review]
hacking.

I prefer ugly hacks to call out the lack of compiler support ;-). Thanks,

/be
Attachment #400424 - Flags: review?(brendan) → review+
(Assignee)

Comment 3

9 years ago
http://hg.mozilla.org/tracemonkey/rev/2b49d5b1cb6d
Assignee: general → leon.sha
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 4

9 years ago
This hacking don't resolve the problem. unitStringTable is aliened to 8 only by lucky. Seems it only works for release build. The test cases are still failed on debug builds. So back it out.
 
http://hg.mozilla.org/tracemonkey/rev/91833e14fc3f
(Assignee)

Comment 5

9 years ago
Created attachment 402050 [details] [diff] [review]
patch v2

Fortunately jsstr.h is surrounded by EXTERN_C. That means the name of unitStringTable will not be mangled. We can use #pragma align here.
Attachment #400424 - Attachment is obsolete: true
Attachment #402050 - Flags: review?(brendan)
(Assignee)

Updated

9 years ago
Whiteboard: fixed-in-tracemonkey
Attachment #402050 - Flags: review?(brendan) → review+
Comment on attachment 402050 [details] [diff] [review]
patch v2

Glad this got sorted out without a hack.

/be
The dependency on JS_{BEGIN,END}_EXTERN_C being used in jsstr.h is obscure, though. And we have been removing extern "C" {...} macros from .h files now that we are committed to C++ outside of the public API headers. So a comment on how the SUNPRO code in the patch depends on extern "C" would be good. Cc'ing Jim B. in case he has thoughts.

/be

Comment 8

9 years ago
If the surrounding 'extern "C"' were to go away, then we'll just have to put a SunPro-specific mangled name inside a SunPro-specific pragma.  It doesn't seem that bad to me.

It would be nice to have an assert at initialization that checked the alignment, if we're going to be running into this repeatedly.
(Assignee)

Comment 9

9 years ago
Comment on attachment 402050 [details] [diff] [review]
patch v2

I am so sorry. The patch is not correct. In my work space, I got the aligned unitStringTable. It is still a lucky ball. According to this document http://docs.sun.com/app/docs/doc/819-5267/6n7c46fnt?a=view When #pragma align is used inside a namespace, mangled names must be used. Even jsstr.h is surrounded bu extern "C" {...}, unitStringTable is still inside a namespace. I wrote a similar testcase to confirm that. I have no idea now.
Maybe jim's suggestion to use mangled name is acceptable. As I know the mangled name is not fixed. It may changes on multiple conditions like platform, compile options. But for this particular  case, seems the mangled name for unitStringTable is always the same. I'll ask our compiler guys to check that.
Attachment #402050 - Attachment is obsolete: true

Comment 10

9 years ago
What is the linker symbol name ("mangled" name) your compiler generates for unitStringTable?

If an object is not static or in an anonymous namespace, the mangled name must always be the same for a given ABI.  Otherwise, you wouldn't be able to define it in one .o file, refer to it in another, and have the linker pair up the reference and definition.

Different compilers may choose different mangling conventions, but in practice, the authors of different compilers usually collaborate enough to agree on which mangling rules to use for a given architecture, so that object files produced by different compilers can be linked together.  In other words, the mangling rules are part of the ABI.

Since the pragma you want to use only makes sense to the Sun Studio compiler anyway, there's no harm in assuming that the Sun Studio ABI is in use; and that determines the mangled name unambiguously.

Comment 11

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2b49d5b1cb6d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 12

9 years ago
oops, this isn't fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 13

9 years ago
Ah, we're having crashes with mochitest since it is on mozilla-central now.

I'll update the patch soon.
Hardware: Sun → All

Comment 14

9 years ago
Created attachment 406467 [details] [diff] [review]
patch

Use mangled name, align for 8 bytes.
Attachment #406467 - Flags: review?(brendan)

Comment 15

9 years ago
(In reply to comment #13)
> Ah, we're having crashes with mochitest since it is on mozilla-central now.
> 
> I'll update the patch soon.

Actually the crash was not caused by this bug.
It seems it is a bug of the compiler.
I'll file another bug to follow up.
Attachment #406467 - Flags: review?(brendan) → review+
(Assignee)

Comment 16

9 years ago
Not #pragma align 8. It's #pragma align 64.
(In reply to comment #14)
> Created an attachment (id=406467) [details]
> patch
> 
> Use mangled name, align for 8 bytes.

Comment 17

9 years ago
(In reply to comment #16)
> Not #pragma align 8. It's #pragma align 64.

Nope, the measure for #pragma align is byte.

Pushed:
http://hg.mozilla.org/tracemonkey/rev/15384f3da4b1
http://hg.mozilla.org/mozilla-central/rev/326f615aaabd

Updated

9 years ago
Attachment #406467 - Flags: approval1.9.2?

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Attachment #406467 - Flags: approval1.9.2? → approval1.9.2+
You need to log in before you can comment on or make changes to this bug.