If there's no perf implications, we should increase max JSString::m_length to UINT_MAX to match the max StringImpl length.
Created attachment 319083 [details] Patch
Created attachment 319085 [details] Patch
rdar://problem/32001499
I'll watch the bots for perf changes. I doubt this should make a difference to perf
Comment on attachment 319085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319085&action=review What prevents size overflow without this check? Don't you want to check UINT_MAX instead of INT_MAX? > Source/JavaScriptCore/ChangeLog:3 > + Explore increasing max JSString::m_length to UINT_MAX. Weird to still be "exploring" here :)
Comment on attachment 319085 [details] Patch OK Keith pointed out that size_t is misleading because string's .length returns unsigned. The assumption I have now is that tryMakeString will fail on its own, so this patch is OK. Please confirm that's the case before committing. r=me if so. Maybe needs a test?
Comment on attachment 319085 [details] Patch Clearing flags on attachment: 319085 Committed r221192: <http://trac.webkit.org/changeset/221192>
All reviewed patches have been landed. Closing bug.
I don't think this patch can be right. It deletes an ASSERT and a comment explaining a problem, but it doesn't fix the problem. Consider ThunkGenerators.cpp::stringCharLoad and similar functions: // load index jit.loadInt32Argument(0, SpecializedThunkJIT::regT1); // regT1 contains the index // Do an unsigned compare to simultaneously filter negative indices as well as indices that are too large jit.appendFailure(jit.branch32(MacroAssembler::AboveOrEqual, SpecializedThunkJIT::regT1, SpecializedThunkJIT::regT2)); ...
(In reply to Geoffrey Garen from comment #9) > I don't think this patch can be right. It deletes an ASSERT and a comment > explaining a problem, but it doesn't fix the problem. > > Consider ThunkGenerators.cpp::stringCharLoad and similar functions: > > // load index > jit.loadInt32Argument(0, SpecializedThunkJIT::regT1); // regT1 contains > the index > > // Do an unsigned compare to simultaneously filter negative indices as > well as indices that are too large > jit.appendFailure(jit.branch32(MacroAssembler::AboveOrEqual, > SpecializedThunkJIT::regT1, SpecializedThunkJIT::regT2)); > > ... I think this is fine. That thunk calls the slow path (the C++ code) if it fails that check. The C++ code does the right thing for positive offsets. It will only fail if the unsigned offset they are looking for is > 2GB, which is also exceedingly unlikely. It also doesn't seem like any of the other code does the wrong thing.
(In reply to Keith Miller from comment #10) > (In reply to Geoffrey Garen from comment #9) > > I don't think this patch can be right. It deletes an ASSERT and a comment > > explaining a problem, but it doesn't fix the problem. > > > > Consider ThunkGenerators.cpp::stringCharLoad and similar functions: > > > > // load index > > jit.loadInt32Argument(0, SpecializedThunkJIT::regT1); // regT1 contains > > the index > > > > // Do an unsigned compare to simultaneously filter negative indices as > > well as indices that are too large > > jit.appendFailure(jit.branch32(MacroAssembler::AboveOrEqual, > > SpecializedThunkJIT::regT1, SpecializedThunkJIT::regT2)); > > > > ... > > I think this is fine. That thunk calls the slow path (the C++ code) if it > fails that check. The C++ code does the right thing for positive offsets. It > will only fail if the unsigned offset they are looking for is > 2GB, which > is also exceedingly unlikely. It also doesn't seem like any of the other > code does the wrong thing. What's "any of the other code" in this context?