Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565713002:
[heap] Initialize the owner on each page after lospace allocation (Closed)
|
Created:
4 years ago by jgruber Modified:
4 years ago Reviewers:
Michael Lippautz CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Initialize the owner on each page after lospace allocation
The least two bits of the owner field of a Page are used to determine
whether the Page is part of a large object. If these bits are not equal
to 0x11, the page is part of a large object and needs special handling
e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it
belongs to.
This CL fixes an issue in which the store buffer overflows after
a large object space allocation but before the object has been fully
initialized. Store buffer overflow handling attempts to look up the
chunk of a page, but fails to do so correctly since the page's owner
field has not yet been initialized.
This CL ensures that the owner field of all pages belonging to a large
object allocation are initialized to a value that is interpreted
correctly.
BUG=chromium:672041
Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4
Committed: https://crrev.com/bbf3c697aee6bcde1bf4e110c8aec5cdae26ebcf
Cr-Original-Commit-Position: refs/heads/master@{#41641}
Cr-Commit-Position: refs/heads/master@{#41687}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comments #Patch Set 3 : Fix msan marking and page range for owner initialization #Patch Set 4 : Simplify msan marking #
Messages
Total messages: 40 (27 generated)
The CQ bit was checked by jgruber@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.
jgruber@chromium.org changed reviewers: + mlippautz@chromium.org
lgtm % comment Hannes: FYI: CodeStubAssembler supports LO allocation of filler objects through a runtime call. This now fixes the remaining issue with the owner field. https://codereview.chromium.org/2565713002/diff/1/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/2565713002/diff/1/src/heap/spaces-inl.h#newco... src/heap/spaces-inl.h:601: owner_addr = new_owner_value; Please just write // Clear out kPageHeaderTag. owner_addr = 0; here. This value will be overwritten by the mutator with an actual payload value anyways. The only important thing here is that we write a value without the 2nd bit set.
The CQ bit was checked by jgruber@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/2565713002/diff/1/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/2565713002/diff/1/src/heap/spaces-inl.h#newco... src/heap/spaces-inl.h:601: owner_addr = new_owner_value; On 2016/12/09 20:09:40, Michael Lippautz wrote: > Please just write > // Clear out kPageHeaderTag. > owner_addr = 0; > here. > > This value will be overwritten by the mutator with an actual payload value > anyways. The only important thing here is that we write a value without the 2nd > bit set. Done.
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 jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlippautz@chromium.org Link to the patchset: https://codereview.chromium.org/2565713002/#ps20001 (title: "Address comments")
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": 20001, "attempt_start_ts": 1481548598109160,
"parent_rev": "465cba7c42bbb33de10f28c133e8b4c188fd802a", "commit_rev":
"5642a564182cc1b4757af9a4fbdcc3f1bc2bed58"}
Message was sent while issue was closed.
Description was changed from ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 ========== to ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Review-Url: https://codereview.chromium.org/2565713002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Review-Url: https://codereview.chromium.org/2565713002 ========== to ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Cr-Commit-Position: refs/heads/master@{#41641} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Cr-Commit-Position: refs/heads/master@{#41641}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2562273004/ by hablich@chromium.org. The reason for reverting is: Tree closer: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20....
Message was sent while issue was closed.
See also: https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%...
Message was sent while issue was closed.
Description was changed from ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Cr-Commit-Position: refs/heads/master@{#41641} ========== to ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Cr-Commit-Position: refs/heads/master@{#41641} ==========
The CQ bit was checked by jgruber@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...)
The CQ bit was checked by jgruber@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 newest patchset fixes two issues: * Only initialize the owner field within the object area (= allocated space - minus chunk header and trailing space). * On msan builds, mark the object area as uninitialized before writing owner fields (not after). PTAL.
lgtm
The CQ bit was checked by jgruber@chromium.org
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": 60001, "attempt_start_ts": 1481697807156110,
"parent_rev": "cae3c5ea44e4b15ebbb3e274ef415950cc854b89", "commit_rev":
"9b40d4bcbefe088d6b136d48c3237c73e435abbf"}
Message was sent while issue was closed.
Description was changed from ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Cr-Commit-Position: refs/heads/master@{#41641} ========== to ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Cr-Commit-Position: refs/heads/master@{#41641} Review-Url: https://codereview.chromium.org/2565713002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Cr-Commit-Position: refs/heads/master@{#41641} Review-Url: https://codereview.chromium.org/2565713002 ========== to ========== [heap] Initialize the owner on each page after lospace allocation The least two bits of the owner field of a Page are used to determine whether the Page is part of a large object. If these bits are not equal to 0x11, the page is part of a large object and needs special handling e.g. in MemoryChunk::FromAnyPointerAddress to determine which chunk it belongs to. This CL fixes an issue in which the store buffer overflows after a large object space allocation but before the object has been fully initialized. Store buffer overflow handling attempts to look up the chunk of a page, but fails to do so correctly since the page's owner field has not yet been initialized. This CL ensures that the owner field of all pages belonging to a large object allocation are initialized to a value that is interpreted correctly. BUG=chromium:672041 Committed: https://crrev.com/9b6808bfb5366beebe3af30a06f9851edb2039d4 Committed: https://crrev.com/bbf3c697aee6bcde1bf4e110c8aec5cdae26ebcf Cr-Original-Commit-Position: refs/heads/master@{#41641} Cr-Commit-Position: refs/heads/master@{#41687} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bbf3c697aee6bcde1bf4e110c8aec5cdae26ebcf Cr-Commit-Position: refs/heads/master@{#41687} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
