Closed
Bug 1099149
Opened 10 years ago
Closed 7 years ago
SIMD: Improve error messages
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: bbouvier, Assigned: pduguet33, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(2 files, 16 obsolete files)
3.75 KB,
patch
|
bbouvier
:
review+
bbouvier
:
checkin+
|
Details | Diff | Splinter Review |
4.79 KB,
patch
|
pduguet33
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
Bug 1088709 introduces load/store operations on SIMD types, which are failing if the read is out of bounds. Consider var arr = new Float32Array(16); print(SIMD.float32x4.load(arr, 13).x); This will throw as the read is out of bounds. Indeed, SIMD.float32x4.load reads 4 float32 from the typed array at the position 13, so the last read value is at position 13 + 4 - 1 == 16, which is out of bounds. Currently, the interpreter displays "TypeError: Invalid arguments" in this case, which is not really precise and could be enhanced. This will need modifications in SIMD.cpp (look how TypedArrayDataPtrFromArgs calls ErrorBadArgs). Error messages are stored in /js/src/js.msg. That could be a first patch. A second good patch would be to change error messages in all functions to be more precise, indicating "expecting a SIMD {float32x4,int32x4} object as argument %d", but that's a bigger task, so let's start by the first one. If you're new to Spidermonkey, feel free to read these links: - https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey - https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation Or you can show up on irc, join #ionmonkey or #jsapi and ask help there.
Assignee | ||
Comment 1•10 years ago
|
||
I'd like to work on this one. Do I need to wait for Bug 1088709 fix to be pushed ?
Comment 2•10 years ago
|
||
(In reply to Paul Duguet from comment #1) > I'd like to work on this one. Great! Do I need to wait for Bug 1088709 fix to be pushed ? No, you can also import the patch from that bug locally and work on top of that. Then again, bbouvier will probably push that patch by tomorrow, so depending on when you'd start working on this bug, the question might be moot.
Assignee | ||
Comment 3•10 years ago
|
||
Okay, just learn that this was possible :) I'm starting right now
Assignee | ||
Comment 4•10 years ago
|
||
Here is a first patch for the specific case of OOB arg. I used the current version of Bug 1088709 patch that will apparently be soon outdated. If it's going in a good direction I can start working on other load/store error messages or even all SIMD messages if needed.
Assignee | ||
Updated•10 years ago
|
Attachment #8524198 -
Attachment description: bug1099149.patch → Arg out of bound patch
Assignee | ||
Comment 5•10 years ago
|
||
I noticed that js.msg documentation about JS_ReportErrorNumber looks outdated, am I right ? http://dxr.mozilla.org/mozilla-central/source/js/src/js.msg?from=js.msg&case=true#37 Would this need a new bug for correction ?
Assignee | ||
Comment 6•10 years ago
|
||
Switched to JSMSG_TYPED_ARRAY_BAD_INDEX which looks more appropriate for this. Load test expects a TypeError in such case. I'm not sure if JSEXN_ERR matches TypeError on the js side. I could have to modify the test if it doesn't. Can someone help me on this point ?
Attachment #8524198 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
Comment on attachment 8524209 [details] [diff] [review] Arg out of bound patch with appropriate error Review of attachment 8524209 [details] [diff] [review]: ----------------------------------------------------------------- This is a great start, thanks! I added a comment inline, but mostly, this is just fine. One thing: since this bug will contain multiple patches, that should be called out in the commit message. It's customary to use the format "Bug NNNNNNN - Part N: [description]". I.e., this one would have the message "Bug 1099149 - Part1: Add a more specific error message when 'load' argument is out of bounds" (Also note the marks around "load" and the "s" at the end.) Oh, and when you upload the next patch, please set the "review" flag to "?" and enter ":bbouvier" in the text input that'll appear beside it. That will cause a review request to be sent to Benjamin, making sure your patch gets attention. ::: js/src/builtin/SIMD.cpp @@ +996,5 @@ > return false; > > int32_t byteStart = index * typedArray->bytesPerElement(); > if (byteStart < 0 || (uint32_t(byteStart) + NumElem * sizeof(VElem)) > typedArray->byteLength()) > + return ErrorArgIndexOutOfRange(cx, 1); Given that this is only called once, I'd recommend just calling JS_ReportErrorNumber inline here.
Attachment #8524209 -
Flags: feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Thank you for your feedback and conventions clarifications. I will correct this patch according to your remark and then start a new one for other methods.
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Paul Duguet from comment #8) > Thank you for your feedback and conventions clarifications. I will correct > this patch according to your remark and then start a new one for other > methods. Thank you! Please don't forget to upload your updated patch, otherwise we won't be able to give you the positive review that will allow it to get checked in.
Assignee: nobody → pduguet33
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•10 years ago
|
||
Switched again to JSMSG_BAD_INDEX error which type is more specific (RangeError) and updated 'load' tests. Removed the ErrorArgIndexOutOfRange method and called JS_ReportErrorNumber method directly.
Attachment #8524209 -
Attachment is obsolete: true
Attachment #8524877 -
Flags: review?(benj)
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8524877 [details] [diff] [review] Arg out of bound patch with appropriate error and tests updated Review of attachment 8524877 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your patch! The polyfill implementation, available at [0], indeed uses RangeError for these out of bounds accesses, so this fits perfectly! I will update the nit below in your patch, and send it to our try-server which will make sure that it runs fine on all platforms. Keep up the good work :) ::: js/src/builtin/SIMD.cpp @@ +989,5 @@ > return false; > > int32_t byteStart = index * typedArray->bytesPerElement(); > if (byteStart < 0 || (uint32_t(byteStart) + NumElem * sizeof(VElem)) > typedArray->byteLength()) > + { nit: { goes on the previous line, right after the closing parenthesis. You might want to give a look at [0] if you want to avoid these in the future :) [0] https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style
Attachment #8524877 -
Flags: review?(benj) → review+
Reporter | ||
Comment 12•10 years ago
|
||
In the future, please make sure to use spaces rather than tabs for indenting code :) I've fixed it for you in this patch. (I actually haven't changed the position of the { in the if conditional, as otherwise it would have been placed after the 100-chars limit in the code.) https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ead2fcfe915f
Attachment #8524877 -
Attachment is obsolete: true
Attachment #8526366 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Thank you for your review ! I already skimmed through coding guidelines but am still getting used to it. I will pay attention to these details for the next patch. Work is in progress for other messages...
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #0) > A second good patch would be to change error messages in all functions to be > more precise, indicating "expecting a SIMD {float32x4,int32x4} object as > argument %d", but that's a bigger task, so let's start by the first one. Is there a way to properly get the type names 'float32x4' and 'int32x4' from the 'load'/'store' methods for use with a parameterized error message? Or can I pass them hardcoded to JS_ErrorReportNumber for use in the format string depending on the type? Otherwise I will add 2 different error messages.
Assignee | ||
Comment 15•9 years ago
|
||
I used TypeDescr to get the expected type and added it as a parameter of TypedArrayDataPtrFromArgs. Please let me know if it is a good way to handle that error.
Attachment #8527211 -
Flags: review?(benj)
Reporter | ||
Comment 16•9 years ago
|
||
Comment on attachment 8527211 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8527211 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to reuse this machinery for other functions as well (in a third patch, if you want to), and would like to avoid the duplication of code between callers. How about having the logic that retrieves the TypeDescr in ErrorWrongArgs, and templatize it on V? ::: js/src/builtin/SIMD.cpp @@ +612,5 @@ > return false; > } > > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, char *expectedType, int32_t argIndex) argIndex is unused in this function @@ +615,5 @@ > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, char *expectedType, int32_t argIndex) > +{ > + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_WRONG_TYPE, > + expectedType, "1"); Last argument should be argIndex converted to a string, right? ::: js/src/js.msg @@ +429,5 @@ > MSG_DEF(JSMSG_TYPED_ARRAY_BAD_OBJECT, 0, JSEXN_TYPEERR, "invalid object argument") > MSG_DEF(JSMSG_TYPED_ARRAY_BAD_INDEX, 0, JSEXN_ERR, "invalid or out-of-range index") > MSG_DEF(JSMSG_TYPED_ARRAY_NEGATIVE_ARG,1, JSEXN_ERR, "argument {0} must be >= 0") > MSG_DEF(JSMSG_TYPED_ARRAY_DETACHED, 0, JSEXN_TYPEERR, "attempting to access detached ArrayBuffer") > +MSG_DEF(JSMSG_TYPED_ARRAY_WRONG_TYPE, 2, JSEXN_TYPEERR, "expecting a SIMD {0} object as argument {1}") There's already a JSMSG_SIMD_NOT_A_VECTOR that it would be nice to change and reuse here.
Attachment #8527211 -
Flags: review?(benj)
Reporter | ||
Comment 17•9 years ago
|
||
Sheriffs, please check-in the r+'ed patch when trees re-open (try run in comment 12).
Keywords: checkin-needed,
leave-open
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #16) > I'd like to reuse this machinery for other functions as well (in a third > patch, if you want to), and would like to avoid the duplication of code > between callers. How about having the logic that retrieves the TypeDescr in > ErrorWrongArgs, and templatize it on V? Ok I'll make a third patch to cover other methods in SIMD. I didn't encapsulate the TypeDescr retrieval because I thought it would save the cost to do it in the method, but it indeed looks cleaner and more reusable. > ::: js/src/builtin/SIMD.cpp > @@ +612,5 @@ > > return false; > > } > > > > +static inline bool > > +ErrorWrongTypeArg(JSContext *cx, char *expectedType, int32_t argIndex) > > argIndex is unused in this function > > @@ +615,5 @@ > > +static inline bool > > +ErrorWrongTypeArg(JSContext *cx, char *expectedType, int32_t argIndex) > > +{ > > + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_WRONG_TYPE, > > + expectedType, "1"); > > Last argument should be argIndex converted to a string, right? My mistake. I'll correct it. > ::: js/src/js.msg > @@ +429,5 @@ > > MSG_DEF(JSMSG_TYPED_ARRAY_BAD_OBJECT, 0, JSEXN_TYPEERR, "invalid object argument") > > MSG_DEF(JSMSG_TYPED_ARRAY_BAD_INDEX, 0, JSEXN_ERR, "invalid or out-of-range index") > > MSG_DEF(JSMSG_TYPED_ARRAY_NEGATIVE_ARG,1, JSEXN_ERR, "argument {0} must be >= 0") > > MSG_DEF(JSMSG_TYPED_ARRAY_DETACHED, 0, JSEXN_TYPEERR, "attempting to access detached ArrayBuffer") > > +MSG_DEF(JSMSG_TYPED_ARRAY_WRONG_TYPE, 2, JSEXN_TYPEERR, "expecting a SIMD {0} object as argument {1}") > > There's already a JSMSG_SIMD_NOT_A_VECTOR that it would be nice to change > and reuse here. I didn't see it. I will adapt and use it instead.
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1fc1a58e9152
Flags: in-testsuite+
Assignee | ||
Comment 21•9 years ago
|
||
I refactored all the places where 'JSMSG_SIMD_NOT_A_VECTOR' was used ('js::ToSimdConstant' and 'SimdTypeDescr::call'). Had to make an overload for 'ErrorWrongTypeArg' to be callable from 'js::ToSimdConstant' as it only seems to provide access to the 'TypeDescr'. Please Let me know if it is a good solution.
Attachment #8527211 -
Attachment is obsolete: true
Attachment #8530686 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
I refactored all the places where 'JSMSG_SIMD_NOT_A_VECTOR' was used ('js::ToSimdConstant' and 'SimdTypeDescr::call'). Had to make an overload for 'ErrorWrongTypeArg' to be callable from 'js::ToSimdConstant' as it only seems to provide access to the 'TypeDescr'. Please Let me know if it is a good solution. (Sorry about last post I mistook with the review flag)
Attachment #8530686 -
Attachment is obsolete: true
Attachment #8530687 -
Flags: review?(benj)
Assignee | ||
Comment 23•9 years ago
|
||
I refactored all the places where 'JSMSG_SIMD_NOT_A_VECTOR' was used ('js::ToSimdConstant' and 'SimdTypeDescr::call'). Had to make an overload for 'ErrorWrongTypeArg' to be callable from 'js::ToSimdConstant' as it only seems to provide access to the 'TypeDescr'. Please Let me know if it is a good solution. (Sorry about last posts I mistook with the review flag and put an outdated patch)
Attachment #8530687 -
Attachment is obsolete: true
Attachment #8530687 -
Flags: review?(benj)
Attachment #8530688 -
Flags: review?(benj)
Reporter | ||
Comment 24•9 years ago
|
||
Paul, I haven't forgotten you. I'll review your patch tomorrow for sure.
Assignee | ||
Comment 25•9 years ago
|
||
Ok, no problem I was told on IRC that last week was busy so I'm not worried :)
Reporter | ||
Comment 26•9 years ago
|
||
Comment on attachment 8530688 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8530688 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Would like to take another look and we should be good to go. Your approach sounds fine. ::: js/src/builtin/SIMD.cpp @@ +40,5 @@ > + return false; > +} > + > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, int32_t argIndex, const HeapSlot &typeNameSlot) What about changing the signature of this function to ErrorWrongTypeArg(JSContext *cx, int32_t argIndex, Handle<TypeDescr*> typeDescr) In this case, both callers (the other ErrorWrongTypeArg and SimdTypeDescr::call) can just give their typeDescr. You'll need to retrieve the typeNameSlot here directly. @@ +46,5 @@ > + char charArgIndex[2]; > + JS_snprintf(charArgIndex, sizeof charArgIndex, "%d", argIndex); > + > + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_SIMD_NOT_A_VECTOR, > + JS_EncodeString(cx, typeNameSlot.toString()), charArgIndex); style nit: please align the JS_EncodeString with the cx argument: JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_SIMD_NOT_A_VECTOR, JS_EncodeString(cx, typeNameSlot.toString()), charArgIndex); @@ +56,5 @@ > +ErrorWrongTypeArg(JSContext *cx, int32_t argIndex) > +{ > + Rooted<TypeDescr*> typeDescr(cx, &V::GetTypeDescr(*cx->global())); > + HeapSlot typeNameSlot = typeDescr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR); > + style nit: can you please delete the blank line here? @@ +98,5 @@ > js::ToSimdConstant(JSContext *cx, HandleValue v, jit::SimdConstant *out) > { > typedef typename V::Elem Elem; > if (!IsVectorObject<V>(v)) { > + return ErrorWrongTypeArg<V>(cx, 1); nit: if bodies that contain only one statement don't need {}: if (!IsVectorObject<V>(v)) return ... @@ +335,5 @@ > Rooted<SimdTypeDescr*> descr(cx, &args.callee().as<SimdTypeDescr>()); > if (args.length() == 1) { > // SIMD type used as a coercion > if (!CheckVectorObject(args[0], descr->type())) { > + return ErrorWrongTypeArg(cx, 1, descr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR)); ditto ::: js/src/js.msg @@ +297,5 @@ > MSG_DEF(JSMSG_STRICT_CODE_LET_EXPR_STMT, 0, JSEXN_ERR, "strict mode code may not contain unparenthesized let expression statements") > MSG_DEF(JSMSG_STRICT_CODE_WITH, 0, JSEXN_SYNTAXERR, "strict mode code may not contain 'with' statements") > MSG_DEF(JSMSG_STRICT_FUNCTION_STATEMENT, 0, JSEXN_SYNTAXERR, "in strict mode code, functions may be declared only at top level or immediately within another function") > MSG_DEF(JSMSG_TEMPLSTR_UNTERM_EXPR, 0, JSEXN_SYNTAXERR, "missing } in template string") > +MSG_DEF(JSMSG_SIMD_NOT_A_VECTOR, 2, JSEXN_TYPEERR, "expecting a SIMD {0} object as argument {1}") hehe, actually {1} will always take the value 1 in this case, but I assume this won't be case in later patches :)
Attachment #8530688 -
Flags: review?(benj) → feedback+
Assignee | ||
Comment 27•9 years ago
|
||
Good design idea passing a 'Handle<TypeDescr*>'! It indeed solves tempate problems. Thanks for the style feedback. Everything is corrected. Of course, the message parameter will be useful in the next patch, I already spotted a function where the argument index is not 1 :)
Attachment #8530688 -
Attachment is obsolete: true
Attachment #8534715 -
Flags: review?(benj)
Reporter | ||
Comment 28•9 years ago
|
||
Comment on attachment 8534715 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8534715 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks! r+ with comments addressed (which means: I r+ your patch, assuming you'll fix the following remarks. For your updated version, you don't have to ask me for review, you can just auto-r+ it (if you have the correct rights) and adding a comment like "carrying forward r+ from comment XXX"). Next step is to ask sheriffs for checking the patch in, once it's r+ and updated; that can be done by adding "checkin-needed" to the Keywords field of the bug. Once again, you might need some special rights for changing the Keywords field, let me know if you don't have them. Thanks! ::: js/src/builtin/SIMD.cpp @@ +40,5 @@ > + return false; > +} > + > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, int32_t argIndex, Handle<TypeDescr*> typeDescr) Actually, let's make argIndex a size_t parameter, to be sure it's positive. @@ +44,5 @@ > +ErrorWrongTypeArg(JSContext *cx, int32_t argIndex, Handle<TypeDescr*> typeDescr) > +{ > + HeapSlot typeNameSlot = typeDescr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR); > + char charArgIndex[2]; > + JS_snprintf(charArgIndex, sizeof charArgIndex, "%d", argIndex); Could you add an assertion that argIndex actually fits in the charArgIndex array? MOZ_ASSERT(argIndex < 10); @@ +92,3 @@ > typedef typename V::Elem Elem; > + if (!IsVectorObject<V>(v)) > + return ErrorWrongTypeArg(cx, 1, typeDescr); I would just inline typeDescr definition here: return ErrorWrongTypeArg(cx, 1, &V::GetTypeDescr(*cx->global()));
Attachment #8534715 -
Flags: review?(benj) → review+
Assignee | ||
Comment 29•9 years ago
|
||
carrying forward r+ from comment 28
Attachment #8534715 -
Attachment is obsolete: true
Attachment #8536090 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Do we need to run tests on the try server? I don't have privileges to do it. Could you grant me the rights or do it for me? Thanks
Reporter | ||
Comment 31•9 years ago
|
||
Paul, can you please file a bug for getting L1 commit access (i.e. try commit access) and CC me on it, so that I can vouch you? More info about the process there: https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/ https://www.mozilla.org/en-US/about/governance/policies/commit/
Assignee | ||
Comment 32•9 years ago
|
||
Merged patch with changeset 221488:f13a3dcc5d0f
Attachment #8536090 -
Attachment is obsolete: true
Attachment #8544271 -
Flags: review+
Reporter | ||
Comment 33•9 years ago
|
||
Cool, thanks for sticking with this bug! Now that you have try commit access, would you like to submit a try build of your patch? There are some notes over there explaining how to do it [0] and this website demystifies the try syntax [1]. [0] https://wiki.mozilla.org/Build:TryServer [1] http://trychooser.pub.build.mozilla.org/
Flags: needinfo?(pduguet33)
Assignee | ||
Comment 34•9 years ago
|
||
I already launched a try build that doesn't look very well but I don't understand why (as build and tests are ok on my local machine). I used the same command as you did for the last checkin. I will look deeper in the doc but maybe you are able to explain this result (or if my try command was incorrect)? Here is the build link : https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a8db19e8b668 Thanks
Flags: needinfo?(pduguet33)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(benj)
Reporter | ||
Comment 35•9 years ago
|
||
Paul, sorry for the late response. There might be issues with unified build in particular (C++ files are concatenated for reducing compile time). Try to create a new build directory, configure with the --disable-unified-compilatio (or use my script spiderbuild [0]), and there should be a few compile errors. Other errors don't seem related to your patch. [0] https://github.com/bnjbvr/.files/blob/master/bin/spiderbuild.py
Flags: needinfo?(benj)
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8544271 [details] [diff] [review] Patch 2 adding error message for wrong type argument Thank you for the tip! I spotted the error in the 'ToSimdConstant' method : > template<typename V> > bool > js::ToSimdConstant(JSContext *cx, HandleValue v, jit::SimdConstant *out) > { > typedef typename V::Elem Elem; >- if (!IsVectorObject<V>(v)) { >- JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_SIMD_NOT_A_VECTOR); >- return false; >- } >+ if (!IsVectorObject<V>(v)) >+ return ErrorWrongTypeArg(cx, 1, Rooted<TypeDescr*>(cx, &V::GetTypeDescr(*cx->global()))); > > Elem *mem = reinterpret_cast<Elem *>(v.toObject().as<TypedObject>().typedMem()); > *out = jit::SimdConstant::CreateX4(mem); > return true; > } It looks like as V is not a vector object, it fails to get the 'TypeDescr'. In this case, would it be acceptable to stay with an 'ErrorBadArgs' call? Or creating/reusing another message for the error?
Flags: needinfo?(benj)
Attachment #8544271 -
Flags: review+
Reporter | ||
Comment 37•9 years ago
|
||
Comment on attachment 8544271 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8544271 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the long time to answer. I think the issue is actually related to something real, see comment below. ::: js/src/builtin/SIMD.cpp @@ +91,5 @@ > js::ToSimdConstant(JSContext *cx, HandleValue v, jit::SimdConstant *out) > { > typedef typename V::Elem Elem; > + if (!IsVectorObject<V>(v)) > + return ErrorWrongTypeArg(cx, 1, Rooted<TypeDescr*>(cx, &V::GetTypeDescr(*cx->global()))); I just found out Rooted are guards, so they need to be explicitly instantiated with a variable name. Can you just do this above and try again? Rooted<TypeDescr*> typeDescr(cx, &V::GetTypeDescr(*cx->global())); if (!IsVectorObject<V>(v)) return ErrorWrongTypeArg(cx, 1, descr);
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(benj)
Assignee | ||
Comment 38•9 years ago
|
||
Sorry for the late answer too... I updated the patch and merged it with incoming changes. I also applied your suggested solution and the try build looks better. There are still some busted builds but I think they are not related : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5e1f8bd502a
Attachment #8544271 -
Attachment is obsolete: true
Attachment #8571425 -
Flags: review?(benj)
Reporter | ||
Comment 39•9 years ago
|
||
Comment on attachment 8571425 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8571425 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for keeping on working! The error message in TypedArrayLoadArgs would be incorrect, see my comment below. Other than that, the patch is looking good, so I'll just wait for the updated patch before setting r+ :) ::: js/src/builtin/SIMD.cpp @@ +45,5 @@ > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, size_t argIndex, Handle<TypeDescr*> typeDescr) > +{ > + MOZ_ASSERT(argIndex < 10); > + HeapSlot typeNameSlot = typeDescr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR); Can you please move this definition right before its first use (that is, directly above JS_ReportErrorNumber)? @@ +1036,2 @@ > if (!args[0].isObject()) > + return ErrorWrongTypeArg(cx, 1, typeDescr); I'm afraid that isn't correct. Load/Store functions expect the first argument to be a typed array (look below: IsAnyTypedArray(&argobj)). So this isn't the right error message here. It can be a third patch for you, if you want, though ;)
Attachment #8571425 -
Flags: review?(benj) → feedback+
Assignee | ||
Comment 40•9 years ago
|
||
Thanks for the feedback. About the definition of 'ErrorWrongTypeArg', it looks like its first use is in 'js::ToSimdConstant'. So should I move it right above this method?
Flags: needinfo?(benj)
Reporter | ||
Comment 41•9 years ago
|
||
(In reply to Paul Duguet from comment #40) > Thanks for the feedback. > About the definition of 'ErrorWrongTypeArg', it looks like its first use is > in 'js::ToSimdConstant'. So should I move it right above this method? I don't have a strong preference here. Having all error methods at a single location in the file looks fine to me.
Flags: needinfo?(benj)
Assignee | ||
Comment 42•9 years ago
|
||
I put the 2 error methods ('ErrorBadArgs' and 'ErrorWrongTypeArg') right above the first use of 'ErrorWrongTypeArg' but grouped together (tell me if it's ok that way). I also corrected the wrongly used 'ErrorWrongTypeArg' to the generic 'ErrorBadArgs' until a suitable method is created in the next patch.
Attachment #8571425 -
Attachment is obsolete: true
Attachment #8578944 -
Flags: review?(benj)
Reporter | ||
Comment 43•9 years ago
|
||
Comment on attachment 8578944 [details] [diff] [review] 1099149_p2.patch Review of attachment 8578944 [details] [diff] [review]: ----------------------------------------------------------------- I think this isn't the right patch :) Maybe forgot a qref or commit --amend? ::: js/src/builtin/SIMD.cpp @@ +78,5 @@ > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, size_t argIndex, Handle<TypeDescr*> typeDescr) > +{ > + MOZ_ASSERT(argIndex < 10); > + HeapSlot typeNameSlot = typeDescr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR); nit: can you move this definition right above the call to JS_ReportErrorNumber, please? @@ +1037,5 @@ > return ErrorBadArgs(cx); > > JSObject &argobj = args[0].toObject(); > if (!IsAnyTypedArray(&argobj)) > + return ErrorWrongTypeArg(cx, 1, typeDescr); Oh?
Attachment #8578944 -
Flags: review?(benj)
Assignee | ||
Comment 44•9 years ago
|
||
Sorry about that. My understanding of the problem was incomplete. I'm not sure I get the place where I should put 'ErrorWrongTypeArg' definition. Its first use is in 'js::ToSimdConstant', so I put it (along with 'ErrorBadArgs') above it:
>+static inline bool
>+ErrorBadArgs(JSContext *cx)
>+{
>+ JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_BAD_ARGS);
>+ return false;
>+}
>+
>+static inline bool
>+ErrorWrongTypeArg(JSContext *cx, size_t argIndex, Handle<TypeDescr*> typeDescr)
>+{
>+ MOZ_ASSERT(argIndex < 10);
>+ HeapSlot typeNameSlot = typeDescr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR);
>+ char charArgIndex[2];
>+ JS_snprintf(charArgIndex, sizeof charArgIndex, "%d", argIndex);
>+
>+ JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_SIMD_NOT_A_VECTOR,
>+ JS_EncodeString(cx, typeNameSlot.toString()), charArgIndex);
>+ return false;
>+}
>+
> template<typename V>
> bool
> js::ToSimdConstant(JSContext* cx, HandleValue v, jit::SimdConstant* out)
> {
Can you tell me exactly above what call of 'JS_ReportErrorNumber' I should put it if 'js::ToSimdConstant' is not the right one?
Flags: needinfo?(benj)
Reporter | ||
Comment 45•9 years ago
|
||
My comment was more about the fact that the latest patch that i've reviewed (comment 43) was the same as the previous review I've made (comment 39), without any modifications. So I was suggesting that the wrong patch had been uploaded. Does this make sense?
Flags: needinfo?(benj)
Assignee | ||
Comment 46•9 years ago
|
||
It was about the same but the location of error methods was different. I think this one is ok.
Attachment #8578944 -
Attachment is obsolete: true
Attachment #8591115 -
Flags: review?(benj)
Reporter | ||
Comment 47•9 years ago
|
||
Comment on attachment 8591115 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8591115 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for sticking! A few things need to be updated, following the changes you've made, but we're on the right track! ::: js/src/builtin/SIMD.cpp @@ +57,5 @@ > { > return CheckVectorObject(v, V::type); > } > > + nit: newly introduced blank line, please remove it @@ +73,5 @@ > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, size_t argIndex, Handle<TypeDescr*> typeDescr) > +{ > + MOZ_ASSERT(argIndex < 10); > + HeapSlot typeNameSlot = typeDescr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR); can you please move this definition (typeNameSlot) just right above JS_ReportErrorNumber? @@ +972,5 @@ > > return StoreResult<V>(cx, args, result); > } > > +template<class V, class VElem, unsigned NumElem> nit: the V template argument isn't used anymore
Attachment #8591115 -
Flags: review?(benj) → feedback+
Assignee | ||
Comment 48•9 years ago
|
||
Sorry about these details. Thank you for the review !
Attachment #8591115 -
Attachment is obsolete: true
Attachment #8593223 -
Flags: review?(benj)
Reporter | ||
Comment 49•9 years ago
|
||
Comment on attachment 8593223 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8593223 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Please do not forget to update your commit message (it's outdated), upload a new version of your patch and we'll be good to go!
Attachment #8593223 -
Flags: review?(benj) → review+
Assignee | ||
Comment 50•9 years ago
|
||
I updated the commit message and launched a build on the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88abee5bb18 It looks good apart from 2 failing tests that don't seem related. Are they?
Attachment #8593223 -
Attachment is obsolete: true
Attachment #8594935 -
Flags: review+
Reporter | ||
Comment 51•9 years ago
|
||
Comment on attachment 8594935 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8594935 [details] [diff] [review]: ----------------------------------------------------------------- Actually it's very probably caused to your patch. Other uses of getReservedSlotRef in dxr show that they all expect a reference. Could you please try this? ::: js/src/builtin/SIMD.cpp @@ +62,5 @@ > template bool js::IsVectorObject<Float32x4>(HandleValue v); > template bool js::IsVectorObject<Float64x2>(HandleValue v); > > +static inline bool > +ErrorBadArgs(JSContext *cx) nit: JSContext* cx @@ +69,5 @@ > + return false; > +} > + > +static inline bool > +ErrorWrongTypeArg(JSContext *cx, size_t argIndex, Handle<TypeDescr*> typeDescr) nit: JSContext* cx (style has changed recently) @@ +75,5 @@ > + MOZ_ASSERT(argIndex < 10); > + char charArgIndex[2]; > + JS_snprintf(charArgIndex, sizeof charArgIndex, "%d", argIndex); > + > + HeapSlot typeNameSlot = typeDescr->getReservedSlotRef(JS_DESCR_SLOT_STRING_REPR); Try using a reference here instead: HeapSlot& typeNameSlot = ...
Assignee | ||
Comment 52•9 years ago
|
||
Indeed, it fixed it ! The build is still busted on Mulet OS X opt though. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0419529b0a8
Attachment #8594935 -
Attachment is obsolete: true
Attachment #8596010 -
Flags: review+
Reporter | ||
Comment 53•9 years ago
|
||
Comment on attachment 8596010 [details] [diff] [review] Patch 2 adding error message for wrong type argument Review of attachment 8596010 [details] [diff] [review]: ----------------------------------------------------------------- Just an intermittent, no worries. Please add "r=bbouvier" at the end of your commit message and you're good for check-in! :)
Assignee | ||
Comment 54•9 years ago
|
||
Done. Question : How did you manage to spot the HeapSlot bug? The test-fail error message was not explicit so I had no clue.
Attachment #8596010 -
Attachment is obsolete: true
Attachment #8596034 -
Flags: review+
Reporter | ||
Updated•9 years ago
|
Attachment #8526366 -
Flags: checkin+
Reporter | ||
Comment 55•9 years ago
|
||
(In reply to Paul Duguet from comment #54) > Created attachment 8596034 [details] [diff] [review] > Patch 2 adding error message for wrong type argument > > Done. Thanks! > Question : How did you manage to spot the HeapSlot bug? The test-fail error > message was not explicit so I had no clue. On treeherder, when you click on a build, you can go to the "results" directory which contains the result of the builds [0]. If you look at the hazards file [1], you'll see this message: Function '_ZL17ErrorWrongTypeArgP9JSContextmN2JS6HandleIPN2js9TypeDescrEEE|SIMD.cpp:uint8 ErrorWrongTypeArg(JSContext*, uint64, class JS::Handle<js::TypeDescr*>)' has unrooted 'typeNameSlot' of type 'js::HeapSlot' live across GC call '_Z20JS_ReportErrorNumberP9JSContextPFPK19JSErrorFormatStringPvjES4_jz|void JS_ReportErrorNumber(JSContext*, (JSErrorFormatString*)(void*,uint32)*, void*, uint32)' at js/src/builtin/SIMD.cpp:81 At this point, I've used DXR [2] to find out how HeapSlot was used, and all uses seem to be either pointers or references [3]. Looking at how getReservedSlotRef is used [4], it confirmed that it expects a reference (as the name suggests). Here we go! [0] https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/pduguet33@hotmail.com-c88abee5bb18/try-linux64-br-haz/ [1] https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/pduguet33@hotmail.com-c88abee5bb18/try-linux64-br-haz/hazards.txt.gz [2] https://dxr.mozilla.org [3] https://dxr.mozilla.org/mozilla-central/search?q=HeapSlot&case=true [4] https://dxr.mozilla.org/mozilla-central/search?q=getReservedSlotRef&case=true
Assignee | ||
Comment 56•9 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #55) > (In reply to Paul Duguet from comment #54) > > Created attachment 8596034 [details] [diff] [review] > > Patch 2 adding error message for wrong type argument > > > > Done. > Thanks! > > > Question : How did you manage to spot the HeapSlot bug? The test-fail error > > message was not explicit so I had no clue. > On treeherder, when you click on a build, you can go to the "results" > directory which contains the result of the builds [0]. If you look at the > hazards file [1], you'll see this message: > > Function > '_ZL17ErrorWrongTypeArgP9JSContextmN2JS6HandleIPN2js9TypeDescrEEE|SIMD.cpp: > uint8 ErrorWrongTypeArg(JSContext*, uint64, class > JS::Handle<js::TypeDescr*>)' has unrooted 'typeNameSlot' of type > 'js::HeapSlot' live across GC call > '_Z20JS_ReportErrorNumberP9JSContextPFPK19JSErrorFormatStringPvjES4_jz|void > JS_ReportErrorNumber(JSContext*, (JSErrorFormatString*)(void*,uint32)*, > void*, uint32)' at js/src/builtin/SIMD.cpp:81 > > At this point, I've used DXR [2] to find out how HeapSlot was used, and all > uses seem to be either pointers or references [3]. Looking at how > getReservedSlotRef is used [4], it confirmed that it expects a reference (as > the name suggests). Here we go! > > [0] > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/ > pduguet33@hotmail.com-c88abee5bb18/try-linux64-br-haz/ > [1] > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/ > pduguet33@hotmail.com-c88abee5bb18/try-linux64-br-haz/hazards.txt.gz > [2] https://dxr.mozilla.org > [3] https://dxr.mozilla.org/mozilla-central/search?q=HeapSlot&case=true > [4] > https://dxr.mozilla.org/mozilla-central/search?q=getReservedSlotRef&case=true Thank you for the explanation ! :)
Reporter | ||
Comment 57•9 years ago
|
||
Comment on attachment 8596034 [details] [diff] [review] Patch 2 adding error message for wrong type argument Sheriffs: please check in only this patch, as the other one already is checked-in.
Attachment #8596034 -
Flags: checkin?
Comment 58•9 years ago
|
||
hey Benjamin, this doesn't apply: adding 1099149 to series file renamed 1099149 -> 1099149_p2.patch applying 1099149_p2.patch patching file js/src/builtin/SIMD.cpp Hunk #2 FAILED at 624 1 out of 2 hunks FAILED -- saving rejects to file js/src/builtin/SIMD.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1099149_p2.patch could you take a look, thanks!
Flags: needinfo?(pduguet33)
Updated•9 years ago
|
Flags: needinfo?(pduguet33)
Updated•9 years ago
|
Attachment #8596034 -
Flags: checkin? → checkin+
Assignee | ||
Comment 60•9 years ago
|
||
Is the problem solved ? Incoming changes were probably committed right after I generated my patch. I am currently generating another patch solving the conflicts.
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 62•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #61) > I did the trivial rebase and pushed it. Thanks. Do I need to update the patch anyway?
Flags: needinfo?(ryanvm)
Comment 64•9 years ago
|
||
Backout: https://hg.mozilla.org/mozilla-central/rev/caf25344f73e
Comment 65•9 years ago
|
||
I had to back this out because something in the push was making the recursion.js jsreftest fail frequently on Android debug with the errors shown in the log below. It wasn't clear what was at fault, so I backed out both possible candidates for the sake of expediency. https://treeherder.mozilla.org/logviewer.html#?job_id=9333091&repo=mozilla-inbound https://hg.mozilla.org/mozilla-central/rev/caf25344f73e I've also started Try runs of each patch individually applied to hopefully determine which was really at fault. I will re-land whatever patch comes out innocent :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e4bc3c9d0c0
That try run looks pretty guilty.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(benj)
Comment 67•9 years ago
|
||
It did indeed. But then, the very next push after the backout had the same failure, as did pretty much every push until you clobbered, and pretty much every push after you clobbered too. I filed bug 1159096, which I presume will end up with us hiding at least jsreftest-2 if not all jsreftest on Android.
Reporter | ||
Comment 68•9 years ago
|
||
It appears the failing jsreftest has a header saying this: // |reftest| skip-if(xulRuntime.OS=="Darwin"&&isDebugBuild) -- this takes too long to over-recurse. So we could add Android debug to this list, although it'd be a bit unfortunate. In the meanwhile, this failing reftest test doesn't appear related to the SIMD patch, in a first glance. The test does relate to recursion, so maybe something changed our way to handle over-recursion (like, we can handle a bigger call stack) and provoked this intermittent failure. However, I can't see a link between the small SIMD patch here and this issue. Can we re-land, please?
Flags: needinfo?(benj)
Keywords: checkin-needed
Reporter | ||
Updated•9 years ago
|
Attachment #8596034 -
Flags: checkin+ → checkin?
Updated•9 years ago
|
Attachment #8596034 -
Flags: checkin? → checkin+
Comment 69•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb9f0ac0d2c
Keywords: checkin-needed
Comment 71•7 years ago
|
||
I was looking into finalizing the patch and found out that required changes in the description are integrated in the source tree one way or another. I was wondering if this bug could be closed, if not is there anything I can do to close it? For reference: Part one in the description (https://bugzilla.mozilla.org/show_bug.cgi?id=1099149#c0) seems to be integrated to the current tree in js\src\tests\ecma_7\SIMD\shell.js:547..554 and around js\src\builtin\SIMD.cpp:1375 (TypedArrayFromArgs method). Part two can be found at SIMD.cpp:135 the ErrorWrongTypeArg method.
Flags: needinfo?(bbouvier)
Reporter | ||
Comment 72•7 years ago
|
||
Hi Kerem! Thanks for reviving this bug. Indeed, part 1 is fixed; part 2 has the tooling necessary (the function you've found; great job for this!). But this tooling is not much used, so we could expand its usage. In any case, this bug should be closed, because it's very old and we should avoid landing stuff in different releases from a single bug. So I'll close it. Moreover, the SIMD.js standard seems to have stalled and the chances it ends up being in JavaScript are getting less and less likely. That being said, if you're interested in working on enhancing this error messaging, even if it's nightly only, I can open a new bug for you. Otherwise, feel free to go to #jsapi or #ionmonkey on irc if you're looking for other JavaScript engine bugs :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(bbouvier)
Resolution: --- → FIXED
Comment 73•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•