Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
941073002:
Oilpan: improve handling of ASan contiguous container annotations. (Closed)
DescriptionOilpan: improve handling of ASan contiguous container annotations.
Follow up r190347 and support the retirement of contiguous container
annotations for 'non-inline' heap vector backing stores.
In order to handle the tracing of heap vector backing stores identified
conservatively via the stack or other roots without overflowing such
annotations, the tracing of these will implicitly widen the annotation
to apply to the entire backing store allocation.
R=haraken,inferno
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190624
Patch Set 1 #Patch Set 2 : Resize annotation on lazy-trace only #Patch Set 3 : Reinstate retirement of annotations #Patch Set 4 : Support large page vector backings #
Total comments: 11
Patch Set 5 : Tidying adjustments #
Total comments: 2
Messages
Total messages: 24 (5 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look. Various crashes seen on the ASan bots atm, more so on the Oilpan bot (of course.) (Doing a full ASan run atm - nothing untoward encountered with the subset already tested.)
On 2015/02/20 08:02:09, sof wrote: > Please take a look. > > Various crashes seen on the ASan bots atm, more so on the Oilpan bot (of > course.) > > (Doing a full ASan run atm - nothing untoward encountered with the subset > already tested.) Run showed up no issues. Given the current crashiness, would be good to land this one sooner.
On 2015/02/20 13:06:44, sof wrote: > On 2015/02/20 08:02:09, sof wrote: > > Please take a look. > > > > Various crashes seen on the ASan bots atm, more so on the Oilpan bot (of > > course.) > > > > (Doing a full ASan run atm - nothing untoward encountered with the subset > > already tested.) > > Run showed up no issues. Given the current crashiness, would be good to land > this one sooner. Thanks for doing this!
inferno@chromium.org changed reviewers: + ager@chromium.org, tkent@chromium.org
inferno@chromium.org changed reviewers: + haraken@chromium.org
Mostly looks good. Thanks for taking your time in the stabilization. https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:67: #define ASAN_RETIRE_CONTAINER_ANNOTATION(heapIndex, payload, payloadSize) \ You might remove the heapIndex parameter from ASAN_RETIRE_CONTAINER_ANNOTATION. Then I think you can move ASAN_RETIRE_CONTAINER_ANNOTATION(payload, payloadSize) into finalize(). The macro can do something like: BasePage* page = pageFromObject(payload); if (page->heapIndex() == VectorHeapIndex || (page->heapIndex() == LargeObjectHeapIndex && static_cast<LargeObjectPage>(page)->isVectorBackingPage())) { ANNOTATE_DELETE_BUFFER(); } https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1038: if (heapIndex() == VectorHeapIndex) { Can we move this logic to doAllocateLargeObject, which actually does a LargeObject allocation?
https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1038: if (heapIndex() == VectorHeapIndex) { On 2015/02/20 20:57:56, haraken wrote: > > Can we move this logic to doAllocateLargeObject, which actually does a > LargeObject allocation? Don't see how without passing in the heapIndex as an argument, which seems a bit artificial?
inferno@chromium.org changed reviewers: + inferno@chromium.org
https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:41: #if defined(ADDRESS_SANITIZER) can you remove the if defined here. no need of this, better to keep it in side COntainerAnnotations.
https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1038: if (heapIndex() == VectorHeapIndex) { On 2015/02/20 21:06:43, sof wrote: > On 2015/02/20 20:57:56, haraken wrote: > > > > Can we move this logic to doAllocateLargeObject, which actually does a > > LargeObject allocation? > > Don't see how without passing in the heapIndex as an argument, which seems a bit > artificial? You can get heapIndex from payload. payload => page => heapIndex. (Given that this is only for ASAN code and thus not performance-sensitive, I'd prefer encapsulating the complexity into the macro.)
https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:41: #if defined(ADDRESS_SANITIZER) On 2015/02/20 21:07:41, inferno wrote: > can you remove the if defined here. no need of this, better to keep it in side > COntainerAnnotations. I could, but I don't want to incur another #include for a file most likely not used. Heap.h (by way of Handle.h) is included a lot.
On 2015/02/20 21:13:38, sof wrote: > https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Heap.h > File Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... > Source/platform/heap/Heap.h:41: #if defined(ADDRESS_SANITIZER) > On 2015/02/20 21:07:41, inferno wrote: > > can you remove the if defined here. no need of this, better to keep it in side > > COntainerAnnotations. > > I could, but I don't want to incur another #include for a file most likely not > used. Heap.h (by way of Handle.h) is included a lot. ok then it is fine.
https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1038: if (heapIndex() == VectorHeapIndex) { On 2015/02/20 21:11:09, haraken wrote: > On 2015/02/20 21:06:43, sof wrote: > > On 2015/02/20 20:57:56, haraken wrote: > > > > > > Can we move this logic to doAllocateLargeObject, which actually does a > > > LargeObject allocation? > > > > Don't see how without passing in the heapIndex as an argument, which seems a > bit > > artificial? > > You can get heapIndex from payload. payload => page => heapIndex. > > (Given that this is only for ASAN code and thus not performance-sensitive, I'd > prefer encapsulating the complexity into the macro.) I don't get it; payload of what? LargeObjectHeap has a generic&unrelated heap index; this has to be the heap index of the origining NormalPageHeap.
On 2015/02/20 21:15:38, inferno wrote: > On 2015/02/20 21:13:38, sof wrote: > > > https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Heap.h > > File Source/platform/heap/Heap.h (right): > > > > > https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... > > Source/platform/heap/Heap.h:41: #if defined(ADDRESS_SANITIZER) > > On 2015/02/20 21:07:41, inferno wrote: > > > can you remove the if defined here. no need of this, better to keep it in > side > > > COntainerAnnotations. > > > > I could, but I don't want to incur another #include for a file most likely not > > used. Heap.h (by way of Handle.h) is included a lot. > > ok then it is fine. This wish to not add avoidable overhead isn't really valid, I now notice. ThreadState.h includes Vector.h (which again includes ContainerAnnotations.h), so re-including it is no problem. (Will tidy up this CL tomorrow morning.)
https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1038: if (heapIndex() == VectorHeapIndex) { On 2015/02/20 21:45:17, sof wrote: > On 2015/02/20 21:11:09, haraken wrote: > > On 2015/02/20 21:06:43, sof wrote: > > > On 2015/02/20 20:57:56, haraken wrote: > > > > > > > > Can we move this logic to doAllocateLargeObject, which actually does a > > > > LargeObject allocation? > > > > > > Don't see how without passing in the heapIndex as an argument, which seems a > > bit > > > artificial? > > > > You can get heapIndex from payload. payload => page => heapIndex. > > > > (Given that this is only for ASAN code and thus not performance-sensitive, I'd > > prefer encapsulating the complexity into the macro.) > > I don't get it; payload of what? LargeObjectHeap has a generic&unrelated heap > index; this has to be the heap index of the origining NormalPageHeap. As commented above, I think you can get a right heapIndex by: BasePage* page = pageFromObject(payload); if (page->heapIndex() == VectorHeapIndex || (page->heapIndex() == LargeObjectHeapIndex && static_cast<LargeObjectPage>(page)->isVectorBackingPage())) { ANNOTATE_DELETE_BUFFER(); }
On 2015/02/21 06:56:35, haraken wrote: > https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... > Source/platform/heap/Heap.cpp:1038: if (heapIndex() == VectorHeapIndex) { > On 2015/02/20 21:45:17, sof wrote: > > On 2015/02/20 21:11:09, haraken wrote: > > > On 2015/02/20 21:06:43, sof wrote: > > > > On 2015/02/20 20:57:56, haraken wrote: > > > > > > > > > > Can we move this logic to doAllocateLargeObject, which actually does a > > > > > LargeObject allocation? > > > > > > > > Don't see how without passing in the heapIndex as an argument, which seems > a > > > bit > > > > artificial? > > > > > > You can get heapIndex from payload. payload => page => heapIndex. > > > > > > (Given that this is only for ASAN code and thus not performance-sensitive, > I'd > > > prefer encapsulating the complexity into the macro.) > > > > I don't get it; payload of what? LargeObjectHeap has a generic&unrelated heap > > index; this has to be the heap index of the origining NormalPageHeap. > > As commented above, I think you can get a right heapIndex by: > > BasePage* page = pageFromObject(payload); > if (page->heapIndex() == VectorHeapIndex || (page->heapIndex() == > LargeObjectHeapIndex && > static_cast<LargeObjectPage>(page)->isVectorBackingPage())) { > ANNOTATE_DELETE_BUFFER(); > } Sure; the above comment refers to large object handling though. Anyway, CL update incoming :)
https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:67: #define ASAN_RETIRE_CONTAINER_ANNOTATION(heapIndex, payload, payloadSize) \ On 2015/02/20 20:57:56, haraken wrote: > > You might remove the heapIndex parameter from ASAN_RETIRE_CONTAINER_ANNOTATION. > Then I think you can move ASAN_RETIRE_CONTAINER_ANNOTATION(payload, payloadSize) > into finalize(). > > The macro can do something like: > > BasePage* page = pageFromObject(payload); > if (page->heapIndex() == VectorHeapIndex || (page->heapIndex() == > LargeObjectHeapIndex && > static_cast<LargeObjectPage>(page)->isVectorBackingPage())) { > ANNOTATE_DELETE_BUFFER(); > } Done. https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1038: if (heapIndex() == VectorHeapIndex) { On 2015/02/21 06:56:35, haraken wrote: > On 2015/02/20 21:45:17, sof wrote: > > On 2015/02/20 21:11:09, haraken wrote: > > > On 2015/02/20 21:06:43, sof wrote: > > > > On 2015/02/20 20:57:56, haraken wrote: > > > > > > > > > > Can we move this logic to doAllocateLargeObject, which actually does a > > > > > LargeObject allocation? > > > > > > > > Don't see how without passing in the heapIndex as an argument, which seems > a > > > bit > > > > artificial? > > > > > > You can get heapIndex from payload. payload => page => heapIndex. > > > > > > (Given that this is only for ASAN code and thus not performance-sensitive, > I'd > > > prefer encapsulating the complexity into the macro.) > > > > I don't get it; payload of what? LargeObjectHeap has a generic&unrelated heap > > index; this has to be the heap index of the origining NormalPageHeap. > > As commented above, I think you can get a right heapIndex by: > > BasePage* page = pageFromObject(payload); > if (page->heapIndex() == VectorHeapIndex || (page->heapIndex() == > LargeObjectHeapIndex && > static_cast<LargeObjectPage>(page)->isVectorBackingPage())) { > ANNOTATE_DELETE_BUFFER(); > } Addressed in the tidiest manner I could think of. https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/941073002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:41: #if defined(ADDRESS_SANITIZER) On 2015/02/20 21:13:37, sof wrote: > On 2015/02/20 21:07:41, inferno wrote: > > can you remove the if defined here. no need of this, better to keep it in side > > COntainerAnnotations. > > I could, but I don't want to incur another #include for a file most likely not > used. Heap.h (by way of Handle.h) is included a lot. Done.
If anyone has time today to have a look here & it is acceptable, please tick the commit bit. Will be afk for parts of the day (and the asan bots are ailing.)
LGTM https://codereview.chromium.org/941073002/diff/80001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:84: #define ASAN_MARK_LARGE_VECTOR_CONTAINER(heap, largeObject) \ ASAN_ADD_CONTAINER_ANNOTATION ? (for consistency with ASAN_RETIRE_CONTAINER_ANNOTATION)
https://codereview.chromium.org/941073002/diff/80001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/941073002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:84: #define ASAN_MARK_LARGE_VECTOR_CONTAINER(heap, largeObject) \ On 2015/02/21 22:45:41, haraken wrote: > > ASAN_ADD_CONTAINER_ANNOTATION ? (for consistency with > ASAN_RETIRE_CONTAINER_ANNOTATION) That would be imprecise consistency, as it doesn't add the annotation for this large object, merely marks -- Vector.h adds (all) annotations. Hence, I kept the name as-is.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/941073002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190624 |
