Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650293003:
[wasm] Interpreter: Don't pause on invalid position (Closed)
|
Created:
3 years, 11 months ago by Clemens Hammacher Modified:
3 years, 11 months ago Reviewers:
titzer CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Interpreter: Don't pause on invalid position
Always execute the implicit return if we fell off the function bytes.
This is not considered an additional "step" as it is not executing a
wasm instruction.
Otherwise, we might pause at an invalid position (one after the
function bytes).
R=titzer@chromium.org
BUG=v8:5822
Review-Url: https://codereview.chromium.org/2650293003
Cr-Commit-Position: refs/heads/master@{#42730}
Committed: https://chromium.googlesource.com/v8/v8/+/e29a2cd5296650efc29cfe9f0241628b9865ecb9
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move code to better location #Patch Set 3 : Rebase #
Messages
Total messages: 26 (18 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.cc File src/wasm/wasm-interpreter.cc (right): https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.c... src/wasm/wasm-interpreter.cc:1182: while (pc >= limit || --max >= 0) { Hmm, that's weird. Why are we checking that the pc is out of bounds? (>=)?
https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.cc File src/wasm/wasm-interpreter.cc (right): https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.c... src/wasm/wasm-interpreter.cc:1182: while (pc >= limit || --max >= 0) { On 2017/01/25 at 12:25:06, titzer wrote: > Hmm, that's weird. Why are we checking that the pc is out of bounds? (>=)? To run into the if 4 lines below. I can also make it "pc == limit", if that's more readable. My first attempt was to move the "pc >= limit" check into the kExprEnd case, but it also happens after a kExprBr or BrIf.
On 2017/01/25 at 12:33:33, Clemens Hammacher wrote: > https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.cc > File src/wasm/wasm-interpreter.cc (right): > > https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.c... > src/wasm/wasm-interpreter.cc:1182: while (pc >= limit || --max >= 0) { > On 2017/01/25 at 12:25:06, titzer wrote: > > Hmm, that's weird. Why are we checking that the pc is out of bounds? (>=)? > > To run into the if 4 lines below. I can also make it "pc == limit", if that's more readable. > My first attempt was to move the "pc >= limit" check into the kExprEnd case, but it also happens after a kExprBr or BrIf. Oh, wait, we can also move it to the end of the loop. That probably makes more sense. Wait for the next patch :)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/25 at 12:34:30, Clemens Hammacher wrote: > On 2017/01/25 at 12:33:33, Clemens Hammacher wrote: > > https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.cc > > File src/wasm/wasm-interpreter.cc (right): > > > > https://codereview.chromium.org/2650293003/diff/1/src/wasm/wasm-interpreter.c... > > src/wasm/wasm-interpreter.cc:1182: while (pc >= limit || --max >= 0) { > > On 2017/01/25 at 12:25:06, titzer wrote: > > > Hmm, that's weird. Why are we checking that the pc is out of bounds? (>=)? > > > > To run into the if 4 lines below. I can also make it "pc == limit", if that's more readable. > > My first attempt was to move the "pc >= limit" check into the kExprEnd case, but it also happens after a kExprBr or BrIf. > > Oh, wait, we can also move it to the end of the loop. That probably makes more sense. Wait for the next patch :) PTAL
lgtm
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 clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20029) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2650293003/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485510381820770,
"parent_rev": "b7947f8cd73ca254ea086c441dd75dd239a19b58", "commit_rev":
"e29a2cd5296650efc29cfe9f0241628b9865ecb9"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Interpreter: Don't pause on invalid position Always execute the implicit return if we fell off the function bytes. This is not considered an additional "step" as it is not executing a wasm instruction. Otherwise, we might pause at an invalid position (one after the function bytes). R=titzer@chromium.org BUG=v8:5822 ========== to ========== [wasm] Interpreter: Don't pause on invalid position Always execute the implicit return if we fell off the function bytes. This is not considered an additional "step" as it is not executing a wasm instruction. Otherwise, we might pause at an invalid position (one after the function bytes). R=titzer@chromium.org BUG=v8:5822 Review-Url: https://codereview.chromium.org/2650293003 Cr-Commit-Position: refs/heads/master@{#42730} Committed: https://chromium.googlesource.com/v8/v8/+/e29a2cd5296650efc29cfe9f0241628b986... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/e29a2cd5296650efc29cfe9f0241628b986... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
