Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201663003:
Heap::CopyCode handling of references into NEW_SPACE (Closed)
|
Created:
4 years, 4 months ago by Mircea Trofin Modified:
4 years, 4 months ago CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan, ahaas, ddchen Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionHeap::CopyCode does not correctly handle references into NEW_SPACE.
A fix would be to walk the reloc info and RecordWriteIntoCode. Doing
so, however, upsets a scavenger DCHECK.
We stumbled upon this issue because we were placing wasm objects
(fixed arrays) in NEW_SPACE, rather than OLD_SPACE. These fixed
arrays were subsequently referenced from Code objects, which were
then cloned.
The current CL ensures wasm constructs are allocated in OLD_SPACE,
by pre-tenuring them (consistent with other wasm allocations). In
addition, it adds a DCHECK for CopyCode clarifying its lack of support
for references to NEW_SPACE.
We can investigate in a subsequent CL making CopyCode more robust,
pending understanding of the Scavenger's assumptions.
BUG=
Committed: https://crrev.com/c0dfc8d8f93d892c86193b3d70f85bc9a8d3ae84
Cr-Commit-Position: refs/heads/master@{#38263}
Patch Set 1 #Patch Set 2 : CopyCode #Patch Set 3 : CopyCode #Patch Set 4 : Just wasm-module changes. #
Total comments: 2
Messages
Total messages: 40 (29 generated)
The CQ bit was checked by mtrofin@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_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/11582) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by mtrofin@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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by mtrofin@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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10066) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by mtrofin@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...
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Fix CopyCode BUG= ========== to ========== Heap::CopyCode does not correctly handle references into NEW_SPACE. A fix would be to walk the reloc info and RecordWriteIntoCode. Doing so, however, upsets a scavenger DCHECK. We stumbled upon this issue because we were placing wasm objects (fixed arrays) in NEW_SPACE, rather than OLD_SPACE. These fixed arrays were subsequently referenced from Code objects, which were then cloned. The current CL ensures wasm constructs are allocated in OLD_SPACE, by pre-tenuring them (consistent with other wasm allocations). In addition, it adds a DCHECK for CopyCode clarifying its lack of support for references to NEW_SPACE. We can investigate in a subsequent CL making CopyCode more robust, pending understanding of the Scavenger's assumptions. BUG= ==========
mtrofin@chromium.org changed reviewers: + mlippautz@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for hinting us to the actual problem! We are missing a generational barrier for old to new typed slots. Those slots have only been added recently (ahaas@ knows more). You can follow the progress at crbug.com/633539 You can mitigate the issues by allocating them TENURED (separate CL if you require a short-term fix), but let's not leave a TODO or DCHECK in the CopyCode function as this needs to be fixed asap.
The CQ bit was checked by mtrofin@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 2016/08/02 09:30:53, Michael Lippautz wrote: > Thanks for hinting us to the actual problem! > > We are missing a generational barrier for old to new typed slots. Those slots > have only been added recently (ahaas@ knows more). You can follow the progress > at crbug.com/633539 > > You can mitigate the issues by allocating them TENURED (separate CL if you > require a short-term fix), but let's not leave a TODO or DCHECK in the CopyCode > function as this needs to be fixed asap. PTAL, thanks! ahaas@ hinted at what the underlying cause may be while he was in MTV, and we actually tried out calling RelocWriteIntoCode when cloning the code. Unfortunately, we were mislead into believing we had a fix, because we didn't see the subsequent issue I pointed out (Scavenger invalidated assumptions) because I was building Release - since that's how I was able to repro the issue reliably. I'm happy to hear he's looking into the underlying issue!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/02 15:40:50, Mircea Trofin wrote: > On 2016/08/02 09:30:53, Michael Lippautz wrote: > > Thanks for hinting us to the actual problem! > > > > We are missing a generational barrier for old to new typed slots. Those slots > > have only been added recently (ahaas@ knows more). You can follow the progress > > at crbug.com/633539 > > > > You can mitigate the issues by allocating them TENURED (separate CL if you > > require a short-term fix), but let's not leave a TODO or DCHECK in the > CopyCode > > function as this needs to be fixed asap. > > PTAL, thanks! > > ahaas@ hinted at what the underlying cause may be while he was in MTV, and we > actually tried out calling RelocWriteIntoCode when cloning the code. > Unfortunately, > we were mislead into believing we had a fix, because we didn't see the > subsequent > issue I pointed out (Scavenger invalidated assumptions) because I was building > Release - since that's how I was able to repro the issue reliably. > > I'm happy to hear he's looking into the underlying issue! lgtm on the TENURED mitigation of Code to new references. No clue about the what's going on in wasm modules ;) You might need another wasm/ OWNER as reviewer.
On 2016/08/02 19:14:36, Michael Lippautz wrote: > On 2016/08/02 15:40:50, Mircea Trofin wrote: > > On 2016/08/02 09:30:53, Michael Lippautz wrote: > > > Thanks for hinting us to the actual problem! > > > > > > We are missing a generational barrier for old to new typed slots. Those > slots > > > have only been added recently (ahaas@ knows more). You can follow the > progress > > > at crbug.com/633539 > > > > > > You can mitigate the issues by allocating them TENURED (separate CL if you > > > require a short-term fix), but let's not leave a TODO or DCHECK in the > > CopyCode > > > function as this needs to be fixed asap. > > > > PTAL, thanks! > > > > ahaas@ hinted at what the underlying cause may be while he was in MTV, and we > > actually tried out calling RelocWriteIntoCode when cloning the code. > > Unfortunately, > > we were mislead into believing we had a fix, because we didn't see the > > subsequent > > issue I pointed out (Scavenger invalidated assumptions) because I was > building > > Release - since that's how I was able to repro the issue reliably. > > > > I'm happy to hear he's looking into the underlying issue! > > lgtm on the TENURED mitigation of Code to new references. > > No clue about the what's going on in wasm modules ;) You might need another > wasm/ OWNER as reviewer. Oh - yes. Easily solved :)
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org
mtrofin@chromium.org changed reviewers: + ddchen@chromium.org - bradnelson@chromium.org
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org - ddchen@chromium.org
+brad for the wasm part +ddchen FYI, since I'm shuffling his code back to "how it was" before we detected the GC issue and I recommended the alternative.
https://codereview.chromium.org/2201663003/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2201663003/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1207: for (int j = 0; j < clone_indirect_tables->length(); ++j) { I think you want to keep this.
lgtm https://codereview.chromium.org/2201663003/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2201663003/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1207: for (int j = 0; j < clone_indirect_tables->length(); ++j) { On 2016/08/02 22:47:14, bradnelson wrote: > I think you want to keep this. Nm, this got pulled into the loop above.
The CQ bit was checked by mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Heap::CopyCode does not correctly handle references into NEW_SPACE. A fix would be to walk the reloc info and RecordWriteIntoCode. Doing so, however, upsets a scavenger DCHECK. We stumbled upon this issue because we were placing wasm objects (fixed arrays) in NEW_SPACE, rather than OLD_SPACE. These fixed arrays were subsequently referenced from Code objects, which were then cloned. The current CL ensures wasm constructs are allocated in OLD_SPACE, by pre-tenuring them (consistent with other wasm allocations). In addition, it adds a DCHECK for CopyCode clarifying its lack of support for references to NEW_SPACE. We can investigate in a subsequent CL making CopyCode more robust, pending understanding of the Scavenger's assumptions. BUG= ========== to ========== Heap::CopyCode does not correctly handle references into NEW_SPACE. A fix would be to walk the reloc info and RecordWriteIntoCode. Doing so, however, upsets a scavenger DCHECK. We stumbled upon this issue because we were placing wasm objects (fixed arrays) in NEW_SPACE, rather than OLD_SPACE. These fixed arrays were subsequently referenced from Code objects, which were then cloned. The current CL ensures wasm constructs are allocated in OLD_SPACE, by pre-tenuring them (consistent with other wasm allocations). In addition, it adds a DCHECK for CopyCode clarifying its lack of support for references to NEW_SPACE. We can investigate in a subsequent CL making CopyCode more robust, pending understanding of the Scavenger's assumptions. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Heap::CopyCode does not correctly handle references into NEW_SPACE. A fix would be to walk the reloc info and RecordWriteIntoCode. Doing so, however, upsets a scavenger DCHECK. We stumbled upon this issue because we were placing wasm objects (fixed arrays) in NEW_SPACE, rather than OLD_SPACE. These fixed arrays were subsequently referenced from Code objects, which were then cloned. The current CL ensures wasm constructs are allocated in OLD_SPACE, by pre-tenuring them (consistent with other wasm allocations). In addition, it adds a DCHECK for CopyCode clarifying its lack of support for references to NEW_SPACE. We can investigate in a subsequent CL making CopyCode more robust, pending understanding of the Scavenger's assumptions. BUG= ========== to ========== Heap::CopyCode does not correctly handle references into NEW_SPACE. A fix would be to walk the reloc info and RecordWriteIntoCode. Doing so, however, upsets a scavenger DCHECK. We stumbled upon this issue because we were placing wasm objects (fixed arrays) in NEW_SPACE, rather than OLD_SPACE. These fixed arrays were subsequently referenced from Code objects, which were then cloned. The current CL ensures wasm constructs are allocated in OLD_SPACE, by pre-tenuring them (consistent with other wasm allocations). In addition, it adds a DCHECK for CopyCode clarifying its lack of support for references to NEW_SPACE. We can investigate in a subsequent CL making CopyCode more robust, pending understanding of the Scavenger's assumptions. BUG= Committed: https://crrev.com/c0dfc8d8f93d892c86193b3d70f85bc9a8d3ae84 Cr-Commit-Position: refs/heads/master@{#38263} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c0dfc8d8f93d892c86193b3d70f85bc9a8d3ae84 Cr-Commit-Position: refs/heads/master@{#38263} |
