While jsval is 64 bits on 64-bit systems, JSVAL_INTs are still 31-bit integers. Since, on 64-bit systems, Strings can be > 32 bits, assignment of match indices to jsvals can overflow. Two instances I've found are in str_search and js_ExecuteRegExp in jsregexp.cpp, but I suspect there are others.

This is a problem for the x64 JIT too because we want indexes to be 32-bit integers. As a compromise to solve this and that situation, this patch limits string lengths to 30 bits, since an unsigned 30-bit integer can still fit in a 31-bit jsval. I would have liked to make this based on JSVAL_INT_BITS but I couldn't use that from jsstr.h so I made a static assert instead.

I just talked to David about this, and there is an argument for changing the type returned by JSString::length() (and taken by getCharsAndLength) to reflect the fact that the length can necessarily fit in a JSVAL_INT. Perhaps jsuint? This would probably require a rather large patch, so maybe something to do later.

Comment on attachment 398062 [details] [diff] [review] proposed patch >+JS_STATIC_ASSERT(size_t(JSString::MAX_LENGTH) <= UINT_MAX && >+ INT_FITS_IN_JSVAL(JSString::MAX_LENGTH)); Two separate static assertions in a row beats one using && so you can tell what botched. I don't see the connection with UINT_MAX, which depends on the target C++ type model. Are we using unsigned/uintN/uint anywhere relevant? If you mean jsuint, then $ grep jsuint *.h | grep typedef jspubtd.h:typedef uint32 jsuint; suggests using JSUINT_MAX and defining that appropriately in jspubtd.h. ILP64 targets will have an overlarge UINT_MAX but jsuint will still be uint32. r=me once I understand the <limits.h>/UINT_MAX dependency. /be

Removed limits.h dependency. Also lowered bits to 29 from 30, so waiting for OOM in some tests doesn't take forever and slow your system to a crawl. Alternately I could raise it back up to 30, and shortcut in ConcatStrings etc that the resulting string will fit in a JSString.

Comment on attachment 401066 [details] [diff] [review] v2 >+#if JS_BITS_PER_WORD > 32 >+ LENGTH_BITS = 29, >+#else > LENGTH_BITS = JS_BITS_PER_WORD - 4, >+#endif In the interest of inter-operation, why not use 28 for both 32- and 64-bit targets? r=me with that. /be

pushed with change from comment #5 http://hg.mozilla.org/tracemonkey/rev/126a52305332

Might be better to avoid the ifdef and always use 28, elsewhere static-asserting that JS_BITS_PER_WORD - 28 >= 4 (the number of bits reserved for flags). But the static assert should live near the reserved flags' definitions, hrm. Maybe this is the way to go. /be

(In reply to comment #7) > Maybe this is the way to go. In case this was confusing, I mean let's go with what landed ;-). /be