Closed Bug 854400 Opened 10 years ago Closed 10 years ago

UnsafeSetElement does not handle double conversion


(Core :: JavaScript Engine, defect)

22 Branch
Not set





(Reporter: nmatsakis, Unassigned)




(3 files, 1 obsolete file)

I believe that the inlining for UnsafeSetElement() is not properly handling double conversion.

On IRC, jandem writes:

[09:59:31] <jandem> nmatsakis: yes you have to ensure int32 values written to the array are converted to double if needed
[10:00:32] <jandem> nmatsakis: something like TypeOracle::elementWriteNeedsDoubleConversion, if that's |true| then insert an MToDouble instruction, like IonBuilder::jsop_setelem_dense

One tricky part is that creating a simple testcase for intrinsics is difficult since they cannot be directly invoked.
Blocks: PJS
It turns out intrinsics can be accessed from within the shell, so it shouldn't be too hard to devise a test.
Attached patch patch (obsolete) — Splinter Review
Not sure how to write a testcase for this, since we can't test type tags.
Carrying over r+ from bhackett, which was given on attachment 746701 [details] [diff] [review] (accidentally added to bug 833898)
Attachment #729359 - Attachment is obsolete: true
Attachment #746961 - Flags: review+
Attachment #746963 - Flags: review?(bhackett1024) → review+
I am not 100% sure how to create a targeted test case for this, but I do know that double conversion was blocking the test that I will be adding in bug 865931.
Attachment #747231 - Flags: review?(jdemooij)
Blocks: 865931
Try run for these patches (also including patch from bug 865931) looks green (at least for SM tests, didn't run the rest):
Attachment #747231 - Flags: review?(jdemooij) → review+
Comment on attachment 747231 [details] [diff] [review]
Disable double conversion during parallel exec

Review of attachment 747231 [details] [diff] [review]:

::: js/src/ion/IonBuilder.cpp
@@ +6226,5 @@
>      // If we can load the element as a definite double, make sure to check that
>      // the array has been converted to homogenous doubles first.
> +    //
> +    // NB: We disable this optimization in parallel execution mode
> +    // because it inherently not threadsafe (how do you convert the

Nit: s/it/it's
You need to log in before you can comment on or make changes to this bug.