624218 - Intermittent js1_5/Regress/regress-366122.js or js1_5/extensions/regress-347306-02.js | Exited with code 1 from "Assertion failure: abs(dst - src) >= ptrdiff_t(nelem)"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Ringnalda (:philor)]

Attachment: Full log + minidump

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1294544604.1294545658.15917.gz
Rev3 Fedora 12x64 tracemonkey debug test jsreftest on 2011/01/08 19:43:24
s: talos-r3-fed64-043

REFTEST TEST-START | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-366122.js
++DOMWINDOW == 15 (0x267c468) [serial = 4240] [outer = 0x2969600]
BUGNUMBER: 366122
STATUS: Compile large script
Assertion failure: abs(dst - src) >= ptrdiff_t(nelem), at /builds/slave/tm-lnx64-dbg/build/js/src/jsutil.h:362
TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-366122.js | Exited with code 1 during test run

Thread 0 (crashed)
 0  libpthread-2.11.so + 0xee6b
    rbx = 0x150b4120   r12 = 0xec1e1760   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0x00000000   rip = 0xd360ee6b   rsp = 0x9f774658   rbp = 0x9f774680
    Found by: given as instruction pointer in context
 1  libxul.so!JS_Assert [jsutil.cpp:558d826c33bf : 83 + 0x9]
    rip = 0x2871a391   rsp = 0x9f774660
    Found by: stack scanning
 2  libxul.so!js::PodCopy<jschar> [jsutil.h:558d826c33bf : 362 + 0x40]
    rip = 0x286f6e78   rsp = 0x9f774690
    Found by: stack scanning
 3  libxul.so!JSString::flatten [jsstr.cpp:558d826c33bf : 207 + 0x14]
    rip = 0x286f70e0   rsp = 0x9f7746d0
    Found by: stack scanning

Comment 1

[Luke Wagner [:luke]]

Attachment: use word-sized abs() in assert

I'd have to see the values of src/dst to be sure, but I think this is the same abs()-isn't-word-sized assert botch that Jason pointed out yesterday.  Apparently there is no single portable word-sized abs(); GNU has llabs(), msvc has _ab64().  I can only imagine what S/390 has.  So I'll just do expand it.

Comment 2

[Mike Shaver (:shaver emeritus)]

You, sir, are philor-approved:

01:15 < philor> I think the last time someone fixed an orange before the
                second time I starred it was... never

Comment 3

[Luke Wagner [:luke]]

Anything I can do to give back to someone who has given so much peace of mind to me.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 502315 [details] [diff] [review]
use word-sized abs() in assert

>+    JS_ASSERT_IF(dst >= src, dst - src >= ptrdiff_t(nelem));
>+    JS_ASSERT_IF(src >= dst, src - dst >= ptrdiff_t(nelem));

I think it's more correct to do an unsigned comparison:

      JS_ASSERT_IF(dst >= src, size_t(dst - src) >= nelem);
      JS_ASSERT_IF(src >= dst, size_t(src - dst) >= nelem);

since the difference between two char pointers can be negative:

    char *src = (char *) 0x00123456;
    char *dst = (char *) 0x80123456;
    dst >= src                 ---> true
    dst - src                  ---> ptrdiff_t(0x80000000), which is negative
    dst - src >= ptrdiff_t(12) ---> false
    size_t(dst - src) >= nelem ---> true

An academic concern, perhaps? I'm not sure. On Windows, it could bite you if you managed to build JS with /LARGEADDRESSAWARE and ran it on a system booted with /3GB. (Very unlikely.)

Comment 5

[Luke Wagner [:luke]]

Heh, thanks, will do (although nelem's unsigned-ness I think will imply unsigned comparison ;-).

Comment 6

[Jeff Walden [:Waldo]]

Seems to me that "best" would be to avoid pointer subtraction madness altogether:

  JS_ASSERT_IF(dst >= src, src + nelem <= dst);
  JS_ASSERT_IF(src >= dst, dst + nelem <= src);

But I'm not going to really care regardless.

Comment 7

[Brendan Eich [:brendan]]

Point subtraction is not madness, it's specified sanely by C and C++ standards.

Anyway, no need for two loose relationals (one can be > not >=).

/be

Comment 8

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294732292.1294732994.31956.gz
Rev3 Fedora 12x64 mozilla-central debug test jsreftest on 2011/01/10 23:51:32
s: talos-r3-fed64-040

Comment 9

[Luke Wagner [:luke]]

(In reply to comment #7)
> Anyway, no need for two loose relationals (one can be > not >=).

No need, but its prettier when they are aligned :)

Comment 10

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/cb17d7d6a831

Comment 11

[Jonathan Kew [:jfkthame]]

This assertion also occurred on mozilla-central during layout/reftests/svg/as-image/img-height-slice-2.html - different test, but I assume the underlying problem (and fix) is the same as here.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294915524.1294916717.4259.gz

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/cb17d7d6a831