Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2584123002:
PartitionAllocTest: increase address space limit to 6 GB. (Closed)
DescriptionPartitionAllocTest: increase address space limit to 6 GB.
In some cases, such as sanitizer_coverage_flags=edge,
more than 5 GB of address space is reserved at a startup.
Raising the address space limit to be higher than that.
Unfortunately, Linux has a broken implementation of getrusage,
and the proper way of fixing this (by setting the current usage + 4 GB)
is not trivial / impossible under a sandbox. This is the riskless
change I could make up: increase the address limit, while still
not increasing the amount of memory allocated to avoid issues on
Android.
BUG=674665
Committed: https://crrev.com/57d9a8022c40f1612774d7eb6a2554cf4a0cefe2
Cr-Commit-Position: refs/heads/master@{#439372}
Patch Set 1 #
Messages
Total messages: 19 (11 generated)
krasin@chromium.org changed reviewers: + haraken@chromium.org, palmer@chromium.org
krasin@chromium.org changed reviewers: + haraken@chromium.org, palmer@chromium.org
The CQ bit was checked by krasin@chromium.org to run a CQ dry run
Description was changed from ========== ParitionAllocTest: increase address space limit to 6 GB. In some cases, such as sanitizer_coverage_flags=edge, more than 5 GB of address space is reserved at a startup. Raising the address space limit to be higher than that. Unfortunately, Linux has a broken implementation of getrusage, and the proper way of fixing this (by setting the current usage + 4 GB) is not trivial / impossible under a sandbox. This is the riskless change I could make up: increase the address limit, while still not increasing the amount of memory allocated to avoid issues on Android. BUG=674665 ========== to ========== PartitionAllocTest: increase address space limit to 6 GB. In some cases, such as sanitizer_coverage_flags=edge, more than 5 GB of address space is reserved at a startup. Raising the address space limit to be higher than that. Unfortunately, Linux has a broken implementation of getrusage, and the proper way of fixing this (by setting the current usage + 4 GB) is not trivial / impossible under a sandbox. This is the riskless change I could make up: increase the address limit, while still not increasing the amount of memory allocated to avoid issues on Android. BUG=674665 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm not opposed to this change but why is the sanitizer reserving so many address spaces? That sounds crazy to me...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/17 02:15:08, haraken wrote: > I'm not opposed to this change but why is the sanitizer reserving so many > address spaces? That sounds crazy to me... The largest alloc comes from this code: https://cs.chromium.org/chromium/src/third_party/llvm/compiler-rt/lib/sanitiz... // Allocate tr_event_array with a guard page at the end. tr_event_array = reinterpret_cast<u32 *>(MmapNoReserveOrDie( sizeof(tr_event_array[0]) * kTrEventArrayMaxSize + GetMmapGranularity(), "CovInit::tr_event_array")) Where kTrEventArrayMaxSize is defined as // Tracing event array, size and current pointer. // We record all events (basic block entries) in a global buffer of u32 // values. Each such value is the index in pc_array. // So far the tracing is highly experimental: // - not thread-safe; // - does not support long traces; // - not tuned for performance. static const uptr kTrEventArrayMaxSize = FIRST_32_SECOND_64(1 << 22, 1 << 30); In other words, it's the coverage log where a value is added whenever a basic block is visited. The length of the array denotes the maximum log size. Since the actual reservation will only happen if the program runs long enough, it's not a problem per se. And mapping enough address space in advance does not sound too crazy. Interestingly, that the same problem would have appeared with asan / lsan / msan / tsan, but this test is not compiled if MEMORY_TOOL_REPLACES_ALLOCATOR is defined. Otherwise, there would not be a simple fix like that: AddressSanitizer maps 16+ Terabytes at startup for shadow memory, guards, etc. See http://clang.llvm.org/docs/AddressSanitizer.html#limitations
On 2016/12/17 03:33:50, krasin1 wrote: > On 2016/12/17 02:15:08, haraken wrote: > > I'm not opposed to this change but why is the sanitizer reserving so many > > address spaces? That sounds crazy to me... > > The largest alloc comes from this code: > https://cs.chromium.org/chromium/src/third_party/llvm/compiler-rt/lib/sanitiz... > > // Allocate tr_event_array with a guard page at the end. > tr_event_array = reinterpret_cast<u32 *>(MmapNoReserveOrDie( > sizeof(tr_event_array[0]) * kTrEventArrayMaxSize + GetMmapGranularity(), > "CovInit::tr_event_array")) > > Where kTrEventArrayMaxSize is defined as > > // Tracing event array, size and current pointer. > // We record all events (basic block entries) in a global buffer of u32 > // values. Each such value is the index in pc_array. > // So far the tracing is highly experimental: > // - not thread-safe; > // - does not support long traces; > // - not tuned for performance. > static const uptr kTrEventArrayMaxSize = FIRST_32_SECOND_64(1 << 22, 1 << 30); > > In other words, it's the coverage log where a value is added whenever a basic > block is visited. The length of the array denotes the maximum log size. Since > the actual reservation will only happen if the program runs long enough, it's > not a problem per se. And mapping enough address space in advance does not sound > too crazy. > > Interestingly, that the same problem would have appeared with asan / lsan / msan > / tsan, but this test is not compiled if MEMORY_TOOL_REPLACES_ALLOCATOR is > defined. Otherwise, there would not be a simple fix like that: AddressSanitizer > maps 16+ Terabytes at startup for shadow memory, guards, etc. See > http://clang.llvm.org/docs/AddressSanitizer.html#limitations Thanks for the details -- makes sense. LGTM.
The CQ bit was checked by krasin@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": 1, "attempt_start_ts": 1482077971518500, "parent_rev":
"a3f03884c9c80d0e4307e0006ef99270dd5d7580", "commit_rev":
"46765d910cf8d6bac5b78a487719006e525eed0c"}
Message was sent while issue was closed.
Description was changed from ========== PartitionAllocTest: increase address space limit to 6 GB. In some cases, such as sanitizer_coverage_flags=edge, more than 5 GB of address space is reserved at a startup. Raising the address space limit to be higher than that. Unfortunately, Linux has a broken implementation of getrusage, and the proper way of fixing this (by setting the current usage + 4 GB) is not trivial / impossible under a sandbox. This is the riskless change I could make up: increase the address limit, while still not increasing the amount of memory allocated to avoid issues on Android. BUG=674665 ========== to ========== PartitionAllocTest: increase address space limit to 6 GB. In some cases, such as sanitizer_coverage_flags=edge, more than 5 GB of address space is reserved at a startup. Raising the address space limit to be higher than that. Unfortunately, Linux has a broken implementation of getrusage, and the proper way of fixing this (by setting the current usage + 4 GB) is not trivial / impossible under a sandbox. This is the riskless change I could make up: increase the address limit, while still not increasing the amount of memory allocated to avoid issues on Android. BUG=674665 Review-Url: https://codereview.chromium.org/2584123002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== PartitionAllocTest: increase address space limit to 6 GB. In some cases, such as sanitizer_coverage_flags=edge, more than 5 GB of address space is reserved at a startup. Raising the address space limit to be higher than that. Unfortunately, Linux has a broken implementation of getrusage, and the proper way of fixing this (by setting the current usage + 4 GB) is not trivial / impossible under a sandbox. This is the riskless change I could make up: increase the address limit, while still not increasing the amount of memory allocated to avoid issues on Android. BUG=674665 Review-Url: https://codereview.chromium.org/2584123002 ========== to ========== PartitionAllocTest: increase address space limit to 6 GB. In some cases, such as sanitizer_coverage_flags=edge, more than 5 GB of address space is reserved at a startup. Raising the address space limit to be higher than that. Unfortunately, Linux has a broken implementation of getrusage, and the proper way of fixing this (by setting the current usage + 4 GB) is not trivial / impossible under a sandbox. This is the riskless change I could make up: increase the address limit, while still not increasing the amount of memory allocated to avoid issues on Android. BUG=674665 Committed: https://crrev.com/57d9a8022c40f1612774d7eb6a2554cf4a0cefe2 Cr-Commit-Position: refs/heads/master@{#439372} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/57d9a8022c40f1612774d7eb6a2554cf4a0cefe2 Cr-Commit-Position: refs/heads/master@{#439372} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
