Closed
Bug 1261361
Opened 8 years ago
Closed 8 years ago
Vector's infallibleGrowByUninitialized should check mReserved instead of mCapacity
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
Details
Attachments
(1 file, 1 obsolete file)
11.76 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
infallibleGrowByUninitialized should check the new length <= mReserved, instead of checking it's <= mCapacity. The attached patch makes this change and also fixes up a bunch of minor things I noticed.
Attachment #8737206 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 1•8 years ago
|
||
(Note that the patch makes growByUninitialized and infallibleGrowByUninitialized work more like append and infallibleAppend.)
Assignee | ||
Updated•8 years ago
|
Attachment #8737206 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•8 years ago
|
||
I'm struggling a bit with the MPhi::addInput use case. Callers of addInput currently don't call reserve(), but that seems like a bug (it's not how infallibleAppend et al work). Let me know if you can think of a nicer fix.
Attachment #8737206 -
Attachment is obsolete: true
Attachment #8737237 -
Flags: review?(jwalden+bmo)
Comment 3•8 years ago
|
||
Comment on attachment 8737237 [details] [diff] [review] Patch Review of attachment 8737237 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +7243,5 @@ > : public MDefinition, > public InlineListNode<MPhi>, > public NoTypePolicy::Data > { > + typedef js::Vector<MUse, 2, JitAllocPolicy> InputVector; Mild preference for |using InputVector = ...;|. ::: js/src/jit/MIRGraph.cpp @@ +1347,5 @@ > exitDef = entryDef->getOperand(0); > } > > + // Phis always have room for 2 operands, so this can't fail. > + entryDef->addInlineInput(exitDef); If the comment's also claiming the phi has one current operand, and that this adds the second, it seems like we should assert the only-one-current operand thing here. But I don't know what this code is doing well enough to verify this change is correct, except on the comment's blind say-so, so you decide whether someone else should look at this.
Attachment #8737237 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > If the comment's also claiming the phi has one current operand, and that > this adds the second, it seems like we should assert the only-one-current > operand thing here. But I don't know what this code is doing well enough to > verify this change is correct, except on the comment's blind say-so, so you > decide whether someone else should look at this. I added the numOperands == 1 assert. These are loop phis so they have one operand (the value before the loop) and here we're adding the second operand (the value at the loop backedge).
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/17bc12e98c25
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•