Closed Bug 917800 Opened 6 years ago Closed 6 years ago

Odinmonkey: correct rounding up of the heap length.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: dougc, Assigned: dougc)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Sorry, there was a typo in the function RoundUpToNextValidAsmJSHeapLength
added in bug 911254 that prevents the use of a 4M quanta for heap lengths
less than 1024M.
The error message reported when the heap buffer size is
not large enough for the heap length constraint has also
been modified to report the actual conflicting lengths.
Attachment #806625 - Flags: review?(luke)
Blocks: 911254
Comment on attachment 806625 [details] [diff] [review]
Fix the rounding up of the heap length, add some test cases

Review of attachment 806625 [details] [diff] [review]:
-----------------------------------------------------------------

D'oh!  Good catch

::: js/src/jit/AsmJSLink.cpp
@@ +225,5 @@
>          // loads and stores start on an aligned boundary and the heap byteLength has larger alignment.
>          JS_ASSERT((module.minHeapLength() - 1) <= INT32_MAX);
>          if (heap->byteLength() < module.minHeapLength())
> +            return LinkFail(cx, JS_smprintf("ArrayBuffer byteLength of 0x%x is less than the largest source code heap length constraint of 0x%x.",
> +                                            heap->byteLength(), module.minHeapLength()));

This line now needs braces.  Also, for the wording, how about: "ArrayBuffer byteLength of 0x%x is less than 0x%x (which is the largest constant heap access offset rounded up to the next valid heap size)"
Attachment #806625 - Flags: review?(luke) → review+
Carrying forward r+
Attachment #806625 - Attachment is obsolete: true
Attachment #806652 - Flags: review+
(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 806625 [details] [diff] [review]
...
> ::: js/src/jit/AsmJSLink.cpp
> @@ +225,5 @@
> >          // loads and stores start on an aligned boundary and the heap byteLength has larger alignment.
> >          JS_ASSERT((module.minHeapLength() - 1) <= INT32_MAX);
> >          if (heap->byteLength() < module.minHeapLength())
> > +            return LinkFail(cx, JS_smprintf("ArrayBuffer byteLength of 0x%x is less than the largest source code heap length constraint of 0x%x.",
> > +                                            heap->byteLength(), module.minHeapLength()));
> 
> This line now needs braces.  Also, for the wording, how about: "ArrayBuffer
> byteLength of 0x%x is less than 0x%x (which is the largest constant heap
> access offset rounded up to the next valid heap size)"

Done. Thank you for the quick review and feedback.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/58e8f3ac3986
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 806652 [details] [diff] [review]
Fix the rounding up of the heap length, add some test cases, address reviewer feedback.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 911254
User impact if declined: Broken feature would cause backwards compatibility problems. The feature has already been uplifted in bug 911254, but sorry there was a typo in the table for rounding the heap lengths and it would seem best to correct this.
Testing completed (on m-c, etc.): new tests were added
Risk to taking this patch (and alternatives if risky): very low 
String or IDL/UUID changes made by this patch:
Attachment #806652 - Flags: approval-mozilla-aurora?
Attachment #806652 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.