Bug 125391

Summary: new Int32Array(new ArrayBuffer(100), 1, 1) shouldn't throw an error that says "RangeError: Byte offset and length out of range of buffer"
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, buildbot, commit-queue, darin, dpino, ggaren, mark.lam, mhahnenberg, msaboff, oliver, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Patch for landing. none

Description Filip Pizlo 2013-12-07 11:44:14 PST
It should tell you that the problem is that byteOffset is not a multiple of 4.
Comment 1 Diego Pino 2014-07-15 04:48:52 PDT
Created attachment 234913 [details]
Patch
Comment 2 Build Bot 2014-07-15 06:47:04 PDT
Comment on attachment 234913 [details]
Patch

Attachment 234913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5657106719965184

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/data-view-test.html
Comment 3 Build Bot 2014-07-15 06:47:07 PDT
Created attachment 234921 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2014-07-15 07:01:51 PDT
Comment on attachment 234913 [details]
Patch

Attachment 234913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4910900712570880

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/data-view-test.html
Comment 5 Build Bot 2014-07-15 07:01:54 PDT
Created attachment 234922 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-07-15 07:59:12 PDT
Comment on attachment 234913 [details]
Patch

Attachment 234913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4860924473114624

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/data-view-test.html
Comment 7 Build Bot 2014-07-15 07:59:15 PDT
Created attachment 234927 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Diego Pino 2014-07-16 02:27:52 PDT
Created attachment 234984 [details]
Patch
Comment 9 Build Bot 2014-07-16 03:58:54 PDT
Comment on attachment 234984 [details]
Patch

Attachment 234984 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6515551765528576

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
fast/canvas/webgl/data-view-test.html
Comment 10 Build Bot 2014-07-16 03:58:57 PDT
Created attachment 234991 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 Build Bot 2014-07-16 05:18:09 PDT
Comment on attachment 234984 [details]
Patch

Attachment 234984 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4573289439559680

New failing tests:
fast/canvas/webgl/data-view-test.html
Comment 12 Build Bot 2014-07-16 05:18:12 PDT
Created attachment 234992 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 13 Build Bot 2014-07-16 06:26:06 PDT
Comment on attachment 234984 [details]
Patch

Attachment 234984 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5325215269650432

New failing tests:
fast/canvas/webgl/data-view-test.html
Comment 14 Build Bot 2014-07-16 06:26:09 PDT
Created attachment 234993 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 15 Diego Pino 2014-07-16 07:42:13 PDT
Created attachment 234994 [details]
Patch
Comment 16 Filip Pizlo 2014-07-16 10:25:07 PDT
Comment on attachment 234994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234994&action=review

> Source/JavaScriptCore/runtime/ArrayBufferView.h:85
> +        return !(sizeof(T) > 1 && byteOffset % sizeof(T));

Why not just: !(byteOffset & (sizeof(T) - 1))
Comment 17 Diego Pino 2014-07-17 10:12:20 PDT
Comment on attachment 234994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234994&action=review

>> Source/JavaScriptCore/runtime/ArrayBufferView.h:85
>> +        return !(sizeof(T) > 1 && byteOffset % sizeof(T));
> 
> Why not just: !(byteOffset & (sizeof(T) - 1))

Nice, I didn't know.
Comment 18 Diego Pino 2014-07-17 10:15:46 PDT
Created attachment 235071 [details]
Patch
Comment 19 Darin Adler 2014-07-19 23:18:34 PDT
Comment on attachment 235071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235071&action=review

Looks good.

But there is no test coverage for alignment problems. The tests only test the string change for out of range problems, not the new code to check for alignment problems.

Also, when there is both an alignment problem and an out of range problem, which error takes precedence? The tests should cover that and the code should be structured to work as expected. I don’t have a preference, but it would be good to have this be well defined.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:82
> +    template <typename T>
> +    static bool verifyByteOffsetAlignment(unsigned byteOffset)

I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:84
> +        return !(byteOffset & (sizeof(T) - 1));

This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?

The old code was optimized/specialized for the size 1, but here that optimization was removed. Was that optimization not important? If it is important, it’s easy to re-create.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:89
>      template <typename T>

I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:91
> +    static bool verifySubRangeLength(PassRefPtr<ArrayBuffer> buffer,
> +        unsigned byteOffset, unsigned numElements)

This should all be on one line.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:-92
> -        if (sizeof(T) > 1 && byteOffset % sizeof(T))
> -            return false;

Why remove the optimization for size 1? Was that optimization not important? If it is important, we could probably write it a lot better.

> Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:67
>          return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSDataView.cpp:52
>          throwVMError(
> -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> +            exec, createRangeError(exec, "Length out of range of buffer"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSDataView.cpp:53
> +        return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSDataView.cpp:57
> +        exec->vm().throwException(
> +            exec, createRangeError(exec, "Byte offset is not aligned"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSDataView.cpp:58
>          return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:88
>          exec->vm().throwException(
> -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> +            exec, createRangeError(exec, "Length out of range of buffer"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:89
> +        return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:93
> +        exec->vm().throwException(
> +            exec, createRangeError(exec, "Byte offset is not aligned"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:94
>          return 0;

Should be nullptr.
Comment 20 Filip Pizlo 2014-07-19 23:35:55 PDT
(In reply to comment #19)
> (From update of attachment 235071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235071&action=review
> 
> Looks good.
> 
> But there is no test coverage for alignment problems. The tests only test the string change for out of range problems, not the new code to check for alignment problems.
> 
> Also, when there is both an alignment problem and an out of range problem, which error takes precedence? The tests should cover that and the code should be structured to work as expected. I don’t have a preference, but it would be good to have this be well defined.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:82
> > +    template <typename T>
> > +    static bool verifyByteOffsetAlignment(unsigned byteOffset)
> 
> I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:84
> > +        return !(byteOffset & (sizeof(T) - 1));
> 
> This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?

The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 

> 
> The old code was optimized/specialized for the size 1, but here that optimization was removed. Was that optimization not important? If it is important, it’s easy to re-create.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:89
> >      template <typename T>
> 
> I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:91
> > +    static bool verifySubRangeLength(PassRefPtr<ArrayBuffer> buffer,
> > +        unsigned byteOffset, unsigned numElements)
> 
> This should all be on one line.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:-92
> > -        if (sizeof(T) > 1 && byteOffset % sizeof(T))
> > -            return false;
> 
> Why remove the optimization for size 1? Was that optimization not important? If it is important, we could probably write it a lot better.
> 
> > Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:67
> >          return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:52
> >          throwVMError(
> > -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> > +            exec, createRangeError(exec, "Length out of range of buffer"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:53
> > +        return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:57
> > +        exec->vm().throwException(
> > +            exec, createRangeError(exec, "Byte offset is not aligned"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:58
> >          return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:88
> >          exec->vm().throwException(
> > -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> > +            exec, createRangeError(exec, "Length out of range of buffer"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:89
> > +        return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:93
> > +        exec->vm().throwException(
> > +            exec, createRangeError(exec, "Byte offset is not aligned"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:94
> >          return 0;
> 
> Should be nullptr.
Comment 21 Oliver Hunt 2014-07-20 00:03:35 PDT
> > This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?
> 
> The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 

Hmmm, however there is a structured type proposal that allows for array-of-struct semantics that probably does not guarantee n^2 semantics, so maybe it's worth considering that as a future proofing thing?
Comment 22 Filip Pizlo 2014-07-20 01:32:59 PDT
(In reply to comment #21)
> > > This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?
> > 
> > The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 
> 
> Hmmm, however there is a structured type proposal that allows for array-of-struct semantics that probably does not guarantee n^2 semantics, so maybe it's worth considering that as a future proofing thing?

That's very far off in the future, so I think that this kind of future-proofing is premature.
Comment 23 Filip Pizlo 2014-07-20 01:37:11 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > > > This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?
> > > 
> > > The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 
> > 
> > Hmmm, however there is a structured type proposal that allows for array-of-struct semantics that probably does not guarantee n^2 semantics, so maybe it's worth considering that as a future proofing thing?
> 
> That's very far off in the future, so I think that this kind of future-proofing is premature.

(Not to mention, that proposal is unlikely to have a requirement that arrays of struct S have a byteOffset that is a multiple of sizeof(S) - likely the alignment requirement will still be in terms of some primitive type and that types size is going to be a power of two.)
Comment 24 Diego Pino 2014-07-20 15:15:19 PDT
Created attachment 235191 [details]
Patch
Comment 25 Diego Pino 2014-07-20 15:20:53 PDT
* Fixed the nits from the previous patch (concatenate lines, return nullptr).
* Removed templates and pass sizeof(T) as a parameter.
* Reintroduced optimization size > 1.
* I left the assumption that size is a 2^n number.
* Added a test case checking a byteoffset exception is triggered.
* Added a test case checking a length exception takes precedence over a byteoffset exception.
Comment 26 Darin Adler 2014-07-20 15:57:15 PDT
(In reply to comment #20)
> I think it's good to take advantage of this and it's not necessary to add more assertions to that effect.

But how is writing code that assumes this rather than other code that will compile to the same thing a helpful change? That’s my point. Was there a problem with the modulo code that led us to change it?
Comment 27 Filip Pizlo 2014-07-20 16:56:57 PDT
(In reply to comment #26)
> (In reply to comment #20)
> > I think it's good to take advantage of this and it's not necessary to add more assertions to that effect.
> 
> But how is writing code that assumes this rather than other code that will compile to the same thing a helpful change? That’s my point. Was there a problem with the modulo code that led us to change it?

Either way, we're already assuming that sizeof(T) is a power of two.  That assumption is already present in this code.  I would find it sloppy to have some places in the typed array code be needlessly generalized for the case that sizeof(T) could be something other than a power of two, given how many places we have where power of two is a hard requirement.

Also, I find that the bitwise-and-based modulo arithmetic is a good way of documenting the invariant that sizeof(T) is a power of two.  In particular, I would *definitely* say "x % sizeof(T)" if I knew that sizeof(T) might not be a power of two.  That's not true here and no upcoming JS proposal would make it true.  Even arrays-of-structs wouldn't require alignment on struct size, but on the size of the largest primitive inside the struct - and primitives always have power-of-two sizes, since otherwise you wouldn't be able to compute their address in an array efficiently.

Using modulo won't be wrong here, but it would be a regression anyway:

- We use the "& (sizeof(T) - 1)" for such alignment checks in a lot of places - including in machine code, where using modulo wouldn't be an option.  It would be weird to be inconsistent.  I find that in tricky code like this, consistency is one way of making the code manageable.  I like getting my mind trained to read "& (x - 1)" as "alignment check".

- We use other bitwise arithmetic for manipulating offsets, addresses, and sizes.  In many places it's because we don't have a good alternative (after all, "(int)pow(2, x)" is pretty much obviously a horrible thing to write).  It's easier to reason about bitwise math if it's used consistently.  It helps remind us of the invariants we rely upon.  In this case: ~(sizeof(T) - 1) is the mask of allowable bits in a well-aligned pointer to T.

- "& (sizeof(T) - 1" doesn't necessarily have the same codegen as "% sizeof(T)" if signed numbers are involved.  So, you wouldn't be able to use % for alignment checks consistently in our codebase, since we do use intptr_t's in some places.

- The modulo operator has undef corner cases while the and operator does not.  Undef corner cases can lead to traps at either the compiler's or hardware's discretion.  For example ,"% 0" is a trap while "& (0 - 1)" is not.
Comment 28 Darin Adler 2014-07-20 17:07:37 PDT
(In reply to comment #27)
> I would find it sloppy to have some places in the typed array code be needlessly generalized for the case that sizeof(T) could be something other than a power of two, given how many places we have where power of two is a hard requirement.

Wow, really long detailed response!

I’m surprised you feel that way. Switching from % to & seemed to me like a gratuitous change in this code that was unmotivated. But it sounds like you think this is much better.
Comment 29 Darin Adler 2014-07-20 17:13:26 PDT
Comment on attachment 235191 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235191&action=review

> Source/JavaScriptCore/runtime/ArrayBufferView.h:83
> +        return !(size > 1 && byteOffset & (size - 1));

I know I was the one who mentioned a special case for a size of 1, but this is not what I had in mind. The code would work fine without the size > 1 expression, so I suggest we omit it. Sorry for leading you astray.
Comment 30 Diego Pino 2014-07-21 02:37:49 PDT
Created attachment 235214 [details]
Patch for landing.
Comment 31 WebKit Commit Bot 2014-07-21 17:26:41 PDT
Comment on attachment 235214 [details]
Patch for landing.

Clearing flags on attachment: 235214

Committed r171323: <http://trac.webkit.org/changeset/171323>
Comment 32 WebKit Commit Bot 2014-07-21 17:26:47 PDT
All reviewed patches have been landed.  Closing bug.