Closed Bug 1099149 Opened 10 years ago Closed 7 years ago

SIMD: Improve error messages

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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.
I'd like to work on this one. Do I need to wait for Bug 1088709 fix to be pushed ?
(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.
Okay, just learn that this was possible :) I'm starting right now
Attached patch Arg out of bound patch (obsolete) — Splinter Review
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.
Attachment #8524198 - Attachment description: bug1099149.patch → Arg out of bound patch
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 ?
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 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+
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.
(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
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)
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+
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+
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...
(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.
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)
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)
Sheriffs, please check-in the r+'ed patch when trees re-open (try run in comment 12).
(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.
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+
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)
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)
Paul, I haven't forgotten you. I'll review your patch tomorrow for sure.
Ok, no problem I was told on IRC that last week was busy so I'm not worried :)
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+
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)
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+
carrying forward r+ from comment 28
Attachment #8534715 - Attachment is obsolete: true
Attachment #8536090 - Flags: review+
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
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/
Merged patch with changeset 221488:f13a3dcc5d0f
Attachment #8536090 - Attachment is obsolete: true
Attachment #8544271 - Flags: review+
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)
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)
Flags: needinfo?(benj)
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)
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+
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);
Flags: needinfo?(benj)
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)
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+
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)
(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)
Attached patch 1099149_p2.patch (obsolete) — Splinter Review
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)
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)
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)
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)
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)
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+
Sorry about these details. Thank you for the review !
Attachment #8591115 - Attachment is obsolete: true
Attachment #8593223 - Flags: review?(benj)
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+
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+
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 = ...
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+
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! :)
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+
Attachment #8526366 - Flags: checkin+
(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
(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 ! :)
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?
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)
Flags: needinfo?(pduguet33)
Attachment #8596034 - Flags: checkin? → checkin+
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)
I did the trivial rebase and pushed it.
Flags: needinfo?(ryanvm)
(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)
no
Flags: needinfo?(ryanvm)
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
Flags: needinfo?(benj)
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.
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
Attachment #8596034 - Flags: checkin+ → checkin?
Attachment #8596034 - Flags: checkin? → checkin+
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)
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
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.