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: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2013-12-07 11:44:14 PST
Created attachment 234913 [details]
Patch
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 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 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 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 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 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
Created attachment 234984 [details]
Patch
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 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 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 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 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 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
Created attachment 234994 [details]
Patch
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 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. Created attachment 235071 [details]
Patch
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. (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.
> > 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?
(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. (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.) Created attachment 235191 [details]
Patch
* 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. (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? (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. (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 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. Created attachment 235214 [details]
Patch for landing.
Comment on attachment 235214 [details] Patch for landing. Clearing flags on attachment: 235214 Committed r171323: <http://trac.webkit.org/changeset/171323> All reviewed patches have been landed. Closing bug. |