s390x freeze in javascript interpreter

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jan Horak, Assigned: luke)

Tracking

5 Branch
mozilla8
Other
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js-triage-done])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
We're having problems with s390x builds of Firefox and Thunderbird. They freeze in execution of following javascript code (from Thunderbird code: modules/gloda/log4moz.js):
    for (let i = 0; i < pieces.length - 1; i++) {
      dump("start i = "+i+"\n");
      dump(pieces.length+" pieces.length\n")
      dump(cur+"\n");
      if (cur)
        cur += '.' + pieces[i];
      else
        cur = pieces[i];
      if (cur in this._loggers)
        parent = cur;
      dump("end i = "+i+"\n")
    }
[dumps added by me]
I get following output:
start i = 0
2 pieces.length
undefined
end i = 0
start i = 2.121995791e-314
2 pieces.length
gloda
end i = 2.121995791e-314
start i = 0
3 pieces.length
undefined
end i = 0
start i = 2.121995791e-314
3 pieces.length
gloda
end i = 2.121995791e-314
start i = 1
3 pieces.length
gloda.undefined
end i = 1
start i = 4.2439915824e-314
3 pieces.length
gloda.undefined.ds
end i = 4.2439915824e-314
start i = 1
3 pieces.length
gloda.undefined.ds.undefined
end i = 1
start i = 4.2439915824e-314
3 pieces.length
gloda.undefined.ds.undefined.ds
end i = 4.2439915824e-314
start i = 1
3 pieces.length
gloda.undefined.ds.undefined.ds.undefined
end i = 1
...[etc]...
It seems something wrong is going on with i property (or whole js stack?). Any debugging thoughts? Thanks in advance.
1. I assume this runs in the interpreter only on s390x?

2. Is s390x 32-bit or 64-bit? 2.121995791e-314 is 0x200000000 and 4.2439915824e-314 is 0x200000001. The low 32 bits look like a plausible value for i (modulo its not getting updated propertly), but the high 32 bits are wrong.

3. What happens if you s/let/var ?
Whiteboard: js-triage-needed
(Assignee)

Comment 2

6 years ago
Also, it would be useful to know a regression range.  In particular, has worked since after bug 549143 landed (http://hg.mozilla.org/mozilla-central/rev/9c869e64ee26)?
(Reporter)

Comment 3

6 years ago
. (In reply to comment #1)
> 1. I assume this runs in the interpreter only on s390x?
Yes, problems occures in js::Interpret function

> 2. Is s390x 32-bit or 64-bit? 
s390x is 64 bit 
> 2.121995791e-314 is 0x200000000 and 4.2439915824e-314 is 0x200000001. The low 32 bits look like a plausible
> value for i (modulo its not getting updated propertly), but the high 32 bits
> are wrong.
Hm, I didn't try to convert double to hex. Checking it now cycle starts with:
0                 = 0x0000000000000000
2.121995791e-314  = 0x0000000100000000
0                 = 0x0000000000000000
2.121995791e-314  = 0x0000000100000000
and followed by:
1                 = 0x0000000000000001
4.2439915824e-314 = 0x0000000200000001
forever.


> 3. What happens if you s/let/var ?
Same result - nothing changed.

Could this be related to:
https://bugzilla.mozilla.org/show_bug.cgi?id=673097
or 
https://bugzilla.mozilla.org/show_bug.cgi?id=589735 ?
(Reporter)

Comment 4

6 years ago
(In reply to comment #2)
> Also, it would be useful to know a regression range.  In particular, has
> worked since after bug 549143 landed
> (http://hg.mozilla.org/mozilla-central/rev/9c869e64ee26)?
We were fine with Firefox 3.6.19 (1.9.2) but that's rather old version. I try to revert mentioned changeset an let you know.
(Assignee)

Comment 5

6 years ago
Ah, comment 3 makes this look like an endian-ness thing.  It looks like jsval.h uses #if defined(IS_LITTLE_ENDIAN) to catch little-endian and #else assumes big endian.  Could you check whether IS_LITTLE_ENDIAN is correct for your build?
(Reporter)

Comment 6

6 years ago
First of all thanks for helping me out with this.
S390x should be big endian and it seems that this is detected correctly.

(gdb) ptype jsval_layout
type = union jsval_layout {
    uint64 asBits;
    struct {
        JSValueTag tag : 17;
        uint64 payload47 : 47;
    } debugView;
    struct {
        union {...} payload;
    } s;
    double asDouble;
    void *asPtr;
}
which should be right part of jsval.h

content of jsautocfg.h:

#ifndef js_cpucfg___
#define js_cpucfg___

/* AUTOMATICALLY GENERATED - DO NOT EDIT */

#undef  IS_LITTLE_ENDIAN
#define IS_BIG_ENDIAN 1

#ifdef __hppa
# define JS_STACK_GROWTH_DIRECTION (1)
#else
# define JS_STACK_GROWTH_DIRECTION (-1)
#endif
#endif /* js_cpucfg___ */
(Assignee)

Comment 7

6 years ago
Created attachment 549155 [details] [diff] [review]
fix

Ah ha.  The 64-bit jsval_layout struct is not properly ladding out the 32-bit payload and js::Value::getInt32Ref() (used for the i++) is thus referencing the wrong 32-bit word.  Something tells me bug 618485 didn't actually execute code after getting it to compile :)

Pre-emptive question: does s390 ensure that all your pointers to static, malloced, and mmaped memory has the high 17 bits set to 0?  If you are running code at all it gives me hope that the answer is 'yes'.
(Assignee)

Comment 8

6 years ago
Oh, I forgot to ask: could you test whether the patch in comment 7 compiles and works?
(Reporter)

Comment 9

6 years ago
Thanks for the patch. Following asserts failed during compile:

JS_STATIC_ASSERT(offsetof(jsval_layout, s.payload) == 0);
JS_STATIC_ASSERT(offsetof(JSPropertyDescriptor, shortid) == offsetof(PropertyDescriptor, shortid));
JS_STATIC_ASSERT(sizeof(JSPropertyDescriptor) == sizeof(PropertyDescriptor));
JS_STATIC_ASSERT(sizeof(JSObject) % sizeof(js::Value) == 0);

When ignoring them js engine become unusable (can't call DumpJSStack() in gdb).
(Assignee)

Comment 10

6 years ago
Created attachment 549386 [details] [diff] [review]
take 2

Ah, I see someone has added a word-sized member to the payload union.  That means my last patch was bloating jsval_layout to 12 bytes.  (I'm surprised we don't static assert that somewhere...)  What about this patch?
Attachment #549155 - Attachment is obsolete: true
(Reporter)

Comment 11

6 years ago
Um, still one assert remains:
thunderbird-5.0/comm-miramar/mozilla/js/src/jsvalue.h:294: error: size of array 'js_static_assert6' is negative:
JS_STATIC_ASSERT(offsetof(jsval_layout, s.payload) == 0);

ignoring it by commenting it out leads to the same result as with previous patch.
(Assignee)

Comment 12

6 years ago
Yeah, that static assert seems bogus.

In general, though, you may be the first person executing this code on 64-bit big-endian so to get it working you may have to actually step into the code and debug it a bit.

Btw, do you have an answer to my question in comment 7?
(Reporter)

Comment 13

6 years ago
Created attachment 550344 [details] [diff] [review]
removed unrelevant static assert

I've wrongly linked the libraries and reported this patch as non-working. Actually it is working perfectly. I'm sending a bit modified version with removed no longer necessary static assert. It would be nice to have it in trunk. Thanks.
(Assignee)

Comment 14

6 years ago
Comment on attachment 550344 [details] [diff] [review]
removed unrelevant static assert

Yes, we'll get this landed.  I wrote the patch, so I suppose someone else to review it :)
Attachment #550344 - Flags: review?(jwalden+bmo)
Attachment #550344 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/373e6bd2705d
Whiteboard: js-triage-needed → [js-triage-done][inbound]
(Assignee)

Comment 16

6 years ago
Oops, backed out because of silly jsuword/uint64 incompatibility on 64-bit osx64:
http://hg.mozilla.org/integration/mozilla-inbound/rev/706c47369f83
Relanded:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1d186a5f3a96
http://hg.mozilla.org/mozilla-central/rev/1d186a5f3a96
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Sorry to bring bad news, but it seems that change broke sparc64 builds :

http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/108/steps/build/logs/stdio with rev 5684f06138f39e6c6b95cb076cdbe449875a1c2d was ok (well, busted because of bug 676924 but that's another story) and now with rev be090ee1747a378bef88e392164ad01548d912ed including that commit, it fails with :

/var/buildslave/mozilla-central-sparc64/build/js/src/jsvalue.h:298: error: size of array 'arg' is negative

See http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/109/steps/build/logs/stdio for full log.

Should i reopen or file another bug ?
Of course, commenting out that offending JS_ASSERT makes the build go further, but i doubt its the way to go.

JS_STATIC_ASSERT(offsetof(jsval_layout, s.payload) == 0);
(Assignee)

Comment 20

6 years ago
D'oh, I forgot to remove that.  I think that static assert should be bogus.  comment 13 seems to confirm this.
(Assignee)

Comment 21

6 years ago
Removed, and fix strict-aliasing warning:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6f2c0dbb88d3
http://hg.mozilla.org/mozilla-central/rev/6f2c0dbb88d3
Whiteboard: [js-triage-done][inbound] → [js-triage-done]

Updated

6 years ago
Assignee: general → luke
Duplicate of this bug: 680652
You need to log in before you can comment on or make changes to this bug.