Bug 163955 – Explore increasing max JSString::m_length to UINT_MAX.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

If there's no perf implications, we should increase max JSString::m_length to UINT_MAX to match the max StringImpl length.

[Keith Miller]

Created attachment 319083 [details]
Patch

[Keith Miller]

Created attachment 319085 [details]
Patch

[Keith Miller]

rdar://problem/32001499

[Keith Miller]

I'll watch the bots for perf changes. I doubt this should make a difference to perf

[JF Bastien]

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 :)

[JF Bastien]

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?

[WebKit Commit Bot]

Comment on attachment 319085 [details]
Patch

Clearing flags on attachment: 319085

Committed r221192: <http://trac.webkit.org/changeset/221192>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

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));

    ...

[Keith Miller]

(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.

[Saam Barati]

(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?