Closed
Bug 1246109
Opened 7 years ago
Closed 7 years ago
[Static Analysis][Big parameter passed by value] In function IonBuilder::restartLoop
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
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)
1.75 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
jandem
:
review+
|
Details |
The Static Analysis tool Coverity added state is passed as value, because it has around 140bytes we can consider passing it as const reference.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8716230 -
Flags: review?(jdemooij)
Comment 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8716230 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
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
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
Flags: needinfo?(bogdan.postelnicu)
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.
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0505e436ad
Assignee | ||
Comment 9•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/33999/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33999/
Attachment #8716946 -
Flags: review?(jdemooij)
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
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/
Assignee | ||
Comment 13•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
(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•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f70fa6338e
Keywords: checkin-needed
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1f70fa6338e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•