Closed
Bug 854400
Opened 12 years ago
Closed 12 years ago
UnsafeSetElement does not handle double conversion
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: nmatsakis, Unassigned)
References
Details
Attachments
(3 files, 1 obsolete file)
7.84 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
4.21 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
1.01 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
It turns out intrinsics can be accessed from within the shell, so it shouldn't be too hard to devise a test.
Comment 2•12 years ago
|
||
Not sure how to write a testcase for this, since we can't test type tags.
Reporter | ||
Comment 3•12 years ago
|
||
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+
Reporter | ||
Comment 4•12 years ago
|
||
Attachment #746963 -
Flags: review?(bhackett1024)
Updated•12 years ago
|
Attachment #746963 -
Flags: review?(bhackett1024) → review+
Reporter | ||
Comment 5•12 years ago
|
||
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)
Reporter | ||
Comment 6•12 years ago
|
||
Try run for these patches (also including patch from bug 865931) looks green (at least for SM tests, didn't run the rest): https://tbpl.mozilla.org/?tree=Try&rev=ec8d7228222b
Updated•12 years ago
|
Attachment #747231 -
Flags: review?(jdemooij) → review+
Comment 7•12 years ago
|
||
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
Reporter | ||
Comment 8•12 years ago
|
||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/378d4fe10c1f
https://hg.mozilla.org/mozilla-central/rev/fee19aa62f5d
https://hg.mozilla.org/mozilla-central/rev/89426b7c3b3b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•