Closed Bug 1261361 Opened 4 years ago Closed 4 years ago

Vector's infallibleGrowByUninitialized should check mReserved instead of mCapacity

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — 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)
(Note that the patch makes growByUninitialized and infallibleGrowByUninitialized work more like append and infallibleAppend.)
Attachment #8737206 - Flags: review?(jwalden+bmo)
Attached patch PatchSplinter Review
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 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+
(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).
https://hg.mozilla.org/mozilla-central/rev/17bc12e98c25
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.