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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added state is passed as value, because it has around 140bytes we can consider passing it as const reference.

Comment 1

[Andi [:andi]]

Attachment: pass state as reference instead of pass by value

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Andi [:andi]]

Attachment: Bug 1246109.diff

Comment 4

[Andi [:andi]]

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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c7b979be014

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/acfaae9ed379 for being a possible cause of build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=21169623&repo=mozilla-inbound

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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.

Comment 8

[Andi [:andi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0505e436ad

Comment 9

[Andi [:andi]]

Attachment: MozReview Request: Bug 1246109 - pass state as reference instead of pass by value. r?jandem

Review commit: https://reviewboard.mozilla.org/r/33999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33999/

Comment 10

[Andi [:andi]]

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;

Comment 11

[Jan de Mooij [:jandem]]

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.

Comment 12

[Andi [:andi]]

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/

Comment 13

[Andi [:andi]]

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.

Comment 14

[Jan de Mooij [:jandem]]

(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!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f70fa6338e

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1f70fa6338e