Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1883603002:
OptimizeFunctionOnNextCall and DeoptimizeFunction ignore calls on non-JSFunction objects. (Closed)
DescriptionOptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects.
Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that
the argument is a JSFunction object.These are used by fuzzers to get coverage
of optimizations in compiler. Having an assert causes a fuzzer test to fail
when OptimizeFunctionOnNextCall is called on objects that are not functions.
We can instead, silently return on such calls.
BUG=chromium:601391
LOG=N
Committed: https://crrev.com/62801ee3a11d77b95b9ea110875a78e7afc11f7d
Cr-Commit-Position: refs/heads/master@{#35539}
Patch Set 1 #Patch Set 2 : Also fixes DeoptimizeFunction. #
Total comments: 4
Patch Set 3 : fixes nits. #Messages
Total messages: 36 (19 generated)
mythria@chromium.org changed reviewers: + bmeurer@chromium.org
This cl is to fix a clusterfuzz bug: https://bugs.chromium.org/p/chromium/issues/detail?id=601391. %OptimizeFunctionOnNextCall is called on a variable that is not function. And that causes problems. This cl just silently ignores such calls. If this fix looks ok, there are also couple of other places (Runtime_DeoptimizeFunction, Runtime_NeverOptimizeFunction) the same check might be required. PTAL. Thanks, Mythri
bmeurer@chromium.org changed reviewers: + mstarzinger@chromium.org
bmeurer@chromium.org changed required reviewers: + mstarzinger@chromium.org
Hey Mythri, Sorry missed this one yesterday. I think Michi is the better reviewer here. Personally I'd rather blacklist %OptimizeFunctionOnNextCall and friends for clusterfuzz, but I'm not fully sure about the implications. -- Benedikt
mstarzinger@chromium.org changed reviewers: + dehrenberg@chromium.org
LGTM. Yes, you are absolutely right, there are more runtime functions that suffer from the same problem. This has been introduced by a recent change that made all CONVERT_ARG_HANDLE_CHECKED failures appear on ClusterFuzz. Adding Dan who switched the checking. Dan, do you have a plan for reducing the noise-level on ClusterFuzz?
mstarzinger@chromium.org changed reviewers: + littledan@chromium.org - dehrenberg@chromium.org
Ooops, wrong username, adding Dan for realz this time.
I'm not sure why we're going in this direction. My suggestion would be to not check for runtime errors on ClusterFuzz tests which include direct calls to % functions.
On 2016/04/13 17:18:28, Dan Ehrenberg wrote: > I'm not sure why we're going in this direction. My suggestion would be to not > check for runtime errors on ClusterFuzz tests which include direct calls to % > functions. The %OptimizeFunctionOnNextCall needs to be called by ClusterFuzz so that we get coverage for the optimzing compiler. This is also the reason why is is explicitly listed on the whitelist of allowed runtime functions. I don't see how we would get this coverage if we were to forbid ClusterFuzz to call %OptimizeFunctionOnNextCall.
On 2016/04/13 at 17:26:20, mstarzinger wrote: > On 2016/04/13 17:18:28, Dan Ehrenberg wrote: > > I'm not sure why we're going in this direction. My suggestion would be to not > > check for runtime errors on ClusterFuzz tests which include direct calls to % > > functions. > > The %OptimizeFunctionOnNextCall needs to be called by ClusterFuzz so that we get coverage for the optimzing compiler. This is also the reason why is is explicitly listed on the whitelist of allowed runtime functions. I don't see how we would get this coverage if we were to forbid ClusterFuzz to call %OptimizeFunctionOnNextCall. Oh, OK, in that case, this is fine. Could you put a comment in the code explaining that this function is special in that way?
Description was changed from ========== Runtime_OptimizeFunctionOnNextCall ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall expects a JSFunction as an argument. It asserts that the argument is a JSFunction object. It causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction expects a JSFunction as an argument. It asserts that the argument is a JSFunction object. These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ==========
Description was changed from ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction expects a JSFunction as an argument. It asserts that the argument is a JSFunction object. These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object. These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ==========
Description was changed from ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object. These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ==========
Description was changed from ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ==========
Description was changed from ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ==========
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883603002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883603002/20001
Michi, PTAL I also added the check for DeoptimizeFunction.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yep. Still LGTM with nits. https://codereview.chromium.org/1883603002/diff/20001/src/runtime/runtime-tes... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/1883603002/diff/20001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:20: // This function is used by clusterFuzz to get coverage in compiler. nit: Can we instead of mentioning "ClusterFuzz" specifically just talk about "fuzzers" in general instead within this comment? This would not tie this comment in V8's codebase to a specific fuzzer platform that we happen to be using at the moment. https://codereview.chromium.org/1883603002/diff/20001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:97: // This function is used by clusterFuzz to get coverage for optimizations nit: Likewise.
Description was changed from ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by clusterFuzz to get coverage of optimizations in compiler. Having an assert causes a clusterfuzz test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by fuzzers to get coverage of optimizations in compiler. Having an assert causes a fuzzer test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ==========
Thanks, updated the comment. I will land it once the bots are happy. https://codereview.chromium.org/1883603002/diff/20001/src/runtime/runtime-tes... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/1883603002/diff/20001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:20: // This function is used by clusterFuzz to get coverage in compiler. On 2016/04/15 13:51:35, Michael Starzinger wrote: > nit: Can we instead of mentioning "ClusterFuzz" specifically just talk about > "fuzzers" in general instead within this comment? This would not tie this > comment in V8's codebase to a specific fuzzer platform that we happen to be > using at the moment. Thanks, Done. https://codereview.chromium.org/1883603002/diff/20001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:97: // This function is used by clusterFuzz to get coverage for optimizations On 2016/04/15 13:51:35, Michael Starzinger wrote: > nit: Likewise. Done.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883603002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1883603002/#ps40001 (title: "fixes nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883603002/40001
Message was sent while issue was closed.
Description was changed from ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by fuzzers to get coverage of optimizations in compiler. Having an assert causes a fuzzer test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by fuzzers to get coverage of optimizations in compiler. Having an assert causes a fuzzer test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by fuzzers to get coverage of optimizations in compiler. Having an assert causes a fuzzer test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N ========== to ========== OptimizeFunctionOnNextCall and DeoptimizeFunction ignores calls on non-JSFunction objects. Runtime_OptimizeFunctionOnNextCall and Runtime_DeoptimizeFunction asserts that the argument is a JSFunction object.These are used by fuzzers to get coverage of optimizations in compiler. Having an assert causes a fuzzer test to fail when OptimizeFunctionOnNextCall is called on objects that are not functions. We can instead, silently return on such calls. BUG=chromium:601391 LOG=N Committed: https://crrev.com/62801ee3a11d77b95b9ea110875a78e7afc11f7d Cr-Commit-Position: refs/heads/master@{#35539} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/62801ee3a11d77b95b9ea110875a78e7afc11f7d Cr-Commit-Position: refs/heads/master@{#35539} |
