Closed Bug 1246109 Opened 4 years ago Closed 4 years ago

[Static Analysis][Big parameter passed by value] In function IonBuilder::restartLoop

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1123207)

Attachments

(2 files, 1 obsolete file)

The Static Analysis tool Coverity added state is passed as value, because it has around 140bytes we can consider passing it as const reference.
Attachment #8716230 - Flags: review?(jdemooij)
Comment on attachment 8716230 [details] [diff] [review]
pass state as reference instead of pass by value

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

Good find, thanks!

Ideally we'd also delete CFGState's assignment operator and copy constructor, so the compiler will complain next time this happens:

  CFGState(const CFGState& other) = delete;
  void operator=(const CFGState& other) = delete;

But that will probably fail to compile without other changes (I think CFGState::IfElse and friends rely on C++ Return Value Optimization..)

This patch looks good either way.

::: js/src/jit/IonBuilder.h
@@ +316,5 @@
>      bool maybeAddOsrTypeBarriers();
>  
>      // Restarts processing of a loop if the type information at its header was
>      // incomplete.
> +    ControlStatus restartLoop(const CFGState &state);

Nit: & goes after the type:

const CFGState& state

Also in the cpp file.
Attachment #8716230 - Flags: review?(jdemooij) → review+
Attached patch Bug 1246109.diffSplinter Review
Attachment #8716230 - Attachment is obsolete: true
Thx Jan for the fast review, this is why i didn't delete the copy constructor:

>>IonBuilder::CFGState
>>IonBuilder::CFGState::If(jsbytecode* join, MTest* test)
>>{
>>    CFGState state;
>>    state.state = IF_TRUE;
>>    state.stopAt = join;
>>    state.branch.ifFalse = test->ifFalse();
>>    state.branch.test = test;
>>    return state;
>>}

similar to every function that returns CFGState,  just as you said.
Keywords: checkin-needed
With more failures coming in like https://treeherder.mozilla.org/logviewer.html#?job_id=21176431&repo=mozilla-inbound I'm beginning to think this was the patch at fault for everything.
Hello Jan,

I did some changes to my patch since it busted the build. state is a reference to cfgStack_.back so basically when this gets called:

>>    if (!pushLoop(state.loop.initialState, state.loop.initialStopAt, header, state.loop.osr,
>>                  state.loop.loopHead, state.loop.initialPc,
>>                  state.loop.bodyStart, state.loop.bodyEnd,
>>                  state.loop.exitpc, state.loop.continuepc))
>>    {
>>        return ControlStatus_Error;
>>    }

state will reference the newly created object in pushLoop so basically the following would have happened:

    nstate.loop.condpc == state.loop.condpc
    nstate.loop.updatepc == state.loop.updatepc
    nstate.loop.updateEnd == state.loop.updateEnd

having only garbage.

I think the idea here is to copy the original values from state.loop.condpc, state.loop.updatepc and state.loop.updateEnd to it's counterpart from nstate;
Flags: needinfo?(bogdan.postelnicu)
Comment on attachment 8716946 [details]
MozReview Request: Bug 1246109 - pass state as reference instead of pass by value. r?jandem

https://reviewboard.mozilla.org/r/33999/#review31031

::: js/src/jit/IonBuilder.cpp:2461
(Diff revision 1)
> -IonBuilder::restartLoop(CFGState state)
> +IonBuilder::restartLoop(const CFGState& state)

I suppose we could make this a pointer: `const CFGState* state`, and then we can do `state = nullptr;` after we pull everything we need out of it. What do you think? Patch looks good either way.

::: js/src/jit/IonBuilder.cpp:2489
(Diff revision 1)
> +    // keep a local copy for these pointers since state will be overwritten in 

Nit: s/keep/Keep/, and there's some whitespace at the end of this line.
Attachment #8716946 - Flags: review?(jdemooij) → review+
Comment on attachment 8716946 [details]
MozReview Request: Bug 1246109 - pass state as reference instead of pass by value. r?jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33999/diff/1-2/
Thanks for the review. If it's ok with you i prefer to leave it as it is since i think it's more readable.
Keywords: checkin-needed
(In reply to Bogdan Postelnicu from comment #13)
> Thanks for the review. If it's ok with you i prefer to leave it as it is
> since i think it's more readable.

Sure!
https://hg.mozilla.org/mozilla-central/rev/f1f70fa6338e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.