Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690523003:
[wasm] Use PC after EnsureSpace in RecordProtectedInstruction (Closed)
Description[wasm] Use PC after EnsureSpace in RecordProtectedInstruction
Previously we captured the PC before calling EnsureSpace in
RecordProtectedInstruction. Sometimes EnsureSpace would resize and move
the buffer, which would invalidate the previously captured PC and trip an
assert when writing RelocInfo. With this change, we do not capture the PC
until after we've ensured there's enough space, which ensures the PC will
be valid.
BUG=
Review-Url: https://codereview.chromium.org/2690523003
Cr-Commit-Position: refs/heads/master@{#43202}
Committed: https://chromium.googlesource.com/v8/v8/+/c9e83ebc3947aca22d024129963e1e7d25d804ac
Patch Set 1 #Patch Set 2 : Rebasing #Patch Set 3 : Rebasing #
Messages
Total messages: 31 (20 generated)
eholk@chromium.org changed reviewers: + ahaas@chromium.org
lgtm
The CQ bit was checked by eholk@chromium.org
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
Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17350)
The CQ bit was checked by eholk@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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17356)
The CQ bit was checked by eholk@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 eholk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2690523003/#ps20001 (title: "Rebasing")
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
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34688)
eholk@chromium.org changed reviewers: + bradnelson@chromium.org, mtrofin@chromium.org
On 2017/02/14 00:50:30, Eric Holk wrote: could you expand a motivation in the CL description? LGTM otherwise
Description was changed from ========== [wasm] Use PC after EnsureSpace in RecordProtectedInstruction BUG= ========== to ========== [wasm] Use PC after EnsureSpace in RecordProtectedInstruction Previously we captured the PC before calling EnsureSpace in RecordProtectedInstruction. Sometimes EnsureSpace would resize and move the buffer, which would invalidate the previously captured PC and trip an assert when writing RelocInfo. With this change, we do not capture the PC until after we've ensured there's enough space, which ensures the PC will be valid. BUG= ==========
On 2017/02/14 04:52:56, Mircea Trofin wrote: > On 2017/02/14 00:50:30, Eric Holk wrote: > > could you expand a motivation in the CL description? LGTM otherwise Done.
The CQ bit was checked by eholk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org, ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2690523003/#ps40001 (title: "Rebasing")
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": 1487096909758780,
"parent_rev": "c7eabee42207adc614324441fa2b0d8309bf8e47", "commit_rev":
"c9e83ebc3947aca22d024129963e1e7d25d804ac"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Use PC after EnsureSpace in RecordProtectedInstruction Previously we captured the PC before calling EnsureSpace in RecordProtectedInstruction. Sometimes EnsureSpace would resize and move the buffer, which would invalidate the previously captured PC and trip an assert when writing RelocInfo. With this change, we do not capture the PC until after we've ensured there's enough space, which ensures the PC will be valid. BUG= ========== to ========== [wasm] Use PC after EnsureSpace in RecordProtectedInstruction Previously we captured the PC before calling EnsureSpace in RecordProtectedInstruction. Sometimes EnsureSpace would resize and move the buffer, which would invalidate the previously captured PC and trip an assert when writing RelocInfo. With this change, we do not capture the PC until after we've ensured there's enough space, which ensures the PC will be valid. BUG= Review-Url: https://codereview.chromium.org/2690523003 Cr-Commit-Position: refs/heads/master@{#43202} Committed: https://chromium.googlesource.com/v8/v8/+/c9e83ebc3947aca22d024129963e1e7d25d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/c9e83ebc3947aca22d024129963e1e7d25d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
