Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2138173002:
Hookup the generic heap intercept for Windows. (Closed)
|
Created:
4 years, 5 months ago by Sigurður Ásgeirsson Modified:
4 years, 5 months ago CC:
chromium-reviews, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHookup the generic heap intercept for Windows.
With this change, it's possible to set a GN arg "use_experimental_allocator_shim" to true to enable the generic allocator shim on Windows. This only works in non-component builds due to the way the allocator is intercepted on Windows.
BUG=550886
Committed: https://crrev.com/e422cb9901097b09317bc07fd7e1f8f603727ae5
Cr-Commit-Position: refs/heads/master@{#406879}
Patch Set 1 #Patch Set 2 : Address presubmit nits. #Patch Set 3 : Fix broken realloc unittest. #
Total comments: 4
Patch Set 4 : Add missing files. #Patch Set 5 : Fix dangling use of ::g_win_new_mode. #
Total comments: 29
Patch Set 6 : Merge the win-shim refactoring CL. #Patch Set 7 : Address Primiano's comments. #Patch Set 8 : Snip an errant curly. #
Total comments: 14
Patch Set 9 : Address Will's and Primiano's comments. #Patch Set 10 : Scrub code moved to _ucrt header from .cc. #Patch Set 11 : Redirect base::GetPageSize->GetCachedPageSize. #
Total comments: 2
Patch Set 12 : Rephrase comment in hopes that the code is sane as-is. #Patch Set 13 : Merge ToT. #Patch Set 14 : Squash the use_experimental_allocator_shim for NaCL as per Brett's suggestion. #
Dependent Patchsets: Messages
Total messages: 36 (17 generated)
siggi@chromium.org changed reviewers: + primiano@chromium.org
Hey Primiano, this passes all unittests, but as you see it's unbeautiful, and doesn't yet override the CPP allocation functions. Lemme know what you think - I'm open to reorganizing this however, though I think the win-specific shim may need to exist for a bit so that we can enable/disable this on Canary by toggling a flag default. Siggi https://codereview.chromium.org/2138173002/diff/40001/base/allocator/allocato... File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/2138173002/diff/40001/base/allocator/allocato... base/allocator/allocator_shim.cc:248: void* (*malloc_unchecked)(size_t) = &base::allocator::UncheckedAlloc; I'm unhappy about putting this code here for some reason, but maybe this is OK? https://codereview.chromium.org/2138173002/diff/40001/base/allocator/allocato... base/allocator/allocator_shim.cc:284: #if !defined(OS_WIN) this needs a different implementation on Win, as per the libc intercepts, as I don't think the link-time name aliasing trick is available on Win. https://codereview.chromium.org/2138173002/diff/40001/base/allocator/allocato... File base/allocator/allocator_shim_override_libc_symbols.h (right): https://codereview.chromium.org/2138173002/diff/40001/base/allocator/allocato... base/allocator/allocator_shim_override_libc_symbols.h:50: __declspec(restrict) void* malloc(size_t size) { I don't know that the link-time name aliasing used above is available on Windows, nor that it has the desired effect. Come to think of it, the binding of these functions in preference to the CRT's seems precarious, as they rely on the lib containing these functions to precede the CRT on the linker's command line? https://codereview.chromium.org/2138173002/diff/40001/build/config/allocator.gni File build/config/allocator.gni (right): https://codereview.chromium.org/2138173002/diff/40001/build/config/allocator.... build/config/allocator.gni:36: # NaCL doesn't obey any of this configuration. I don't know that this is the right thing to do. Maybe the build flag argument needs to be separated from the flag used to propagate the setting, and the latter set to false for nacl?
Ok this makes great sense to me. I'd just split in two parts, mainly for two reasons: 1. make the CL look less scary 2. I hate to be pessimistic, but by experience these kinds of changes never ever stick at 1st attempt. Would be nice to contain the part being reverted :) Also it's good to isolate change in case of a failure to understand what really caused build breakages. I'd do the following. Do first a pure refactor CL, which doesn't introduce any new feature but just: 1. Introduce a ENABLE_WIN_ALLOCATOR_SHIM_TESTS flag in the existing allocator:features autogenerated header. This flag should be true if (use_experimental_allocator_shim or win_use_allocator_shim) and use that to condition the tests. 2. split out the code from allocator_shim_win.cc to whatever_new_name_impl (see comment below about the name) After this in the next CL you can plumb the calls into the new shim. Recently I've seen weird build breakages just beacuse people shuffled some code around (crbug.com/626539). Splitting into two cls will guarantee that the initial refactor doesn't cause any similar weirdness, nor introduce any unexpected perf regressions. More comments below. Thanks a lot for this. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn File base/allocator/BUILD.gn (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn... base/allocator/BUILD.gn:43: defines = [ "ALLOCATOR_SHIM" ] If I am understanding this correctly you are dropping this if so you can recycle the define for the tests. See my proposal on the top, let's have a different define for the tests, in the allocator features buildflag header. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn... base/allocator/BUILD.gn:82: "allocator_impl_win.cc", I think just for the sake of making all these files less obscure this should have winheap in the name. Maybe: winheap_stubs_win.h/cc ? or something similar https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn... base/allocator/BUILD.gn:314: configs += [ ":allocator_shim_define" ] if you follow my suggestion above about having the ENABLE_WIN_ALLOCATOR_SHIM_TESTS in the features there is not need for this line, as the allocator features are already a dep of base. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_impl_win.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_impl_win.cc:30: void* WinHeapMalloc(size_t size) { I wonder if all these should be inline functions in the .h file, or if the perf hit about this extra call will be noticed. At the end they will be called by just two places until we have both shims, and at the end we'll merge this file into the deault dispatch https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_impl_win.h (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_impl_win.h:12: namespace base { I'd add a TODO to this file saying that once the unified allocator shim sticks this should be deleted (together with win_allocator_shim.cc) and just merged in default_dispatch_to_winheap, so we have less layers to bounce when reading the code. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim.cc:256: // This function behaves similarly to MSVC's _set_new_mode. I think this block is better placed in the impl.cc file right (and remove the other copy in the old shim)? They seem identical. also will keep this file cleaner https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim.cc:284: #if !defined(OS_WIN) maybe add a comment here explaining that on win the default impl already routes them to malloc/free and there is no need to redefine them https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim_default_dispatch_to_winheap.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:14: void* WinHeapMalloc(const AllocatorDispatch*, size_t size) { Can you add some prefix to these functions (DoWinHeap? CallWinHeap? DunnoWinHeap) just so they don't clash with the one in the other file? I know this is a different namespace, but in case they show up in crashes will make the code easier to codesearch. namespaces can be still a bit confusing for humans. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:25: NOTREACHED() << "The windows heap does not support memalign."; I'd make this a CHECK(false), so shows up in production crashes if we do something silly in allocator_shim.cc IIRC NOTREACHED is a noop when building in release (% DCHECK_ALWAYS_ON) https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim_override_libc_symbols.h (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_override_libc_symbols.h:50: __declspec(restrict) void* malloc(size_t size) { maybe let's move these to a allocator_shim_override_ucrt_symbols_win.h at this point? Not very strong honestly https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim_unittest.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_unittest.cc:174: inline size_t GetPageSize() { I think you can use GetPageSize() from base/procesS_metrics.h here https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_unittest.cc:243: ASSERT_GE(reallocs_intercepted_by_addr[Hash(old_realloc_ptr)], 1u); uh?
The CQ bit was checked by siggi@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...
Description was changed from ========== Hookup the generic heap interceptor for Windows. BUG= ========== to ========== Hookup the generic heap interceptor for Windows. BUG=550886 ==========
Please take another look - I've rebased to CL 2143693003, and cleaned up the varia. I've yet to move the extra CRT intercepts, but it's looking less un-beautiful already? https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn File base/allocator/BUILD.gn (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn... base/allocator/BUILD.gn:43: defines = [ "ALLOCATOR_SHIM" ] On 2016/07/12 14:51:04, Primiano Tucci wrote: > If I am understanding this correctly you are dropping this if so you can recycle > the define for the tests. > See my proposal on the top, let's have a different define for the tests, in the > allocator features buildflag header. Done. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn... base/allocator/BUILD.gn:82: "allocator_impl_win.cc", On 2016/07/12 14:51:04, Primiano Tucci wrote: > I think just for the sake of making all these files less obscure this should > have winheap in the name. > Maybe: winheap_stubs_win.h/cc ? or something similar Done. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/BUILD.gn... base/allocator/BUILD.gn:314: configs += [ ":allocator_shim_define" ] On 2016/07/12 14:51:04, Primiano Tucci wrote: > if you follow my suggestion above about having the > ENABLE_WIN_ALLOCATOR_SHIM_TESTS in the features there is not need for this line, > as the allocator features are already a dep of base. Done. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_impl_win.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_impl_win.cc:30: void* WinHeapMalloc(size_t size) { On 2016/07/12 14:51:04, Primiano Tucci wrote: > I wonder if all these should be inline functions in the .h file, or if the perf > hit about this extra call will be noticed. At the end they will be called by > just two places until we have both shims, and at the end we'll merge this file > into the deault dispatch In practice this won't matter for the official builds, as the MSVC compiler will do cross-module inlining with link-time-code generation. I have no preference, happy to move these if you prefer? https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_impl_win.h (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_impl_win.h:12: namespace base { On 2016/07/12 14:51:05, Primiano Tucci wrote: > I'd add a TODO to this file saying that once the unified allocator shim sticks > this should be deleted (together with win_allocator_shim.cc) and just merged in > default_dispatch_to_winheap, so we have less layers to bounce when reading the > code. Done. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim.cc:256: // This function behaves similarly to MSVC's _set_new_mode. On 2016/07/12 14:51:05, Primiano Tucci wrote: > I think this block is better placed in the impl.cc file right (and remove the > other copy in the old shim)? They seem identical. also will keep this file > cleaner There is a small difference in that this one calls SetCallNewHandler. I could ifdef this, I suppose. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim.cc:284: #if !defined(OS_WIN) On 2016/07/12 14:51:05, Primiano Tucci wrote: > maybe add a comment here explaining that on win the default impl already routes > them to malloc/free and there is no need to redefine them Done. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim_default_dispatch_to_winheap.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:14: void* WinHeapMalloc(const AllocatorDispatch*, size_t size) { On 2016/07/12 14:51:05, Primiano Tucci wrote: > Can you add some prefix to these functions (DoWinHeap? CallWinHeap? > DunnoWinHeap) just so they don't clash with the one in the other file? I know > this is a different namespace, but in case they show up in crashes will make the > code easier to codesearch. namespaces can be still a bit confusing for humans. Yeah, naming - I went apeshit with prefix and suffix. Hopefully good enough? https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:25: NOTREACHED() << "The windows heap does not support memalign."; On 2016/07/12 14:51:05, Primiano Tucci wrote: > I'd make this a CHECK(false), so shows up in production crashes if we do > something silly in allocator_shim.cc > IIRC NOTREACHED is a noop when building in release (% DCHECK_ALWAYS_ON) Done. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim_override_libc_symbols.h (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_override_libc_symbols.h:50: __declspec(restrict) void* malloc(size_t size) { On 2016/07/12 14:51:05, Primiano Tucci wrote: > maybe let's move these to a allocator_shim_override_ucrt_symbols_win.h at this > point? Not very strong honestly Yeah, I like that. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim_unittest.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_unittest.cc:174: inline size_t GetPageSize() { On 2016/07/12 14:51:05, Primiano Tucci wrote: > I think you can use GetPageSize() from base/procesS_metrics.h here Done. I wonder if it's safe to use it also in allocator_shim.cc? I'd worry about reentrancy? https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_unittest.cc:243: ASSERT_GE(reallocs_intercepted_by_addr[Hash(old_realloc_ptr)], 1u); On 2016/07/12 14:51:05, Primiano Tucci wrote: > uh? The test fixture records the inbound pointer to the realloc. If the pointer is truly reallocated, then this test fails. This is happening under the WinHeap, presumably because 71->73 jumps a size class in the WinHeap, whereas the other allocators are eating slop in the original allocation. The test is strictly speaking incorrect, as-is.
Looks great, just some small final suggestions. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_impl_win.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_impl_win.cc:30: void* WinHeapMalloc(size_t size) { On 2016/07/14 19:04:27, Sigurður Ásgeirsson wrote: > On 2016/07/12 14:51:04, Primiano Tucci wrote: > > I wonder if all these should be inline functions in the .h file, or if the > perf > > hit about this extra call will be noticed. At the end they will be called by > > just two places until we have both shims, and at the end we'll merge this file > > into the deault dispatch > > In practice this won't matter for the official builds, as the MSVC compiler will > do cross-module inlining with link-time-code generation. I have no preference, > happy to move these if you prefer? SG leaving them as they are. Will see if the perf bots shout and come back here if they do :) https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim.cc:248: void* (*malloc_unchecked)(size_t) = &base::allocator::UncheckedAlloc; To be honest I am not sure I understand all the black magic around malloc_unchecked in memory_win.cc. Is malloc_unchecked part of ucrt and we want to re-define it? If not, can we do a cleaner solution which is in memory_win.cc bool UncheckedMalloc(size_t size, void** result) { #if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) *result = base::allocator::UncheckedAlloc(size) #else *result = malloc_unchecked(size); # return *result != NULL; } But I am really struggling to understand that linker magic there in memory_win.cc https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim.cc:256: // This function behaves similarly to MSVC's _set_new_mode. On 2016/07/14 19:04:27, Sigurður Ásgeirsson wrote: > On 2016/07/12 14:51:05, Primiano Tucci wrote: > > I think this block is better placed in the impl.cc file right (and remove the > > other copy in the old shim)? They seem identical. also will keep this file > > cleaner > > There is a small difference in that this one calls SetCallNewHandler. I could > ifdef this, I suppose. Oh I see. Ok now I follow it. In windows there is an "official" _set_new_mode, so the entry point is that one. SG keeping it separate. Just at this point you can move all this section to allocator_shim_override_ucrt_symbols_win.h. This is really defining the extra windows-specific synmbols required to properly shim allocations on win, so it feels like this entire section belongs to there. In practice there should be no difference as that file is included few lines below here. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... File base/allocator/allocator_shim_unittest.cc (right): https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_unittest.cc:174: inline size_t GetPageSize() { On 2016/07/14 19:04:27, Sigurður Ásgeirsson wrote: > On 2016/07/12 14:51:05, Primiano Tucci wrote: > > I think you can use GetPageSize() from base/procesS_metrics.h here > > Done. > > I wonder if it's safe to use it also in allocator_shim.cc? I'd worry about > reentrancy? the main reason why I didn't use it straight there is because on Linux, it involves doing a sysconf syscall, while my impl does the caching (and its inlined). Not a huge deal for the unittest but can be a perf bummer on Linux. You could probably just rework the one in shim.cc to always do the caching (Even if useless on Windows) and just use GetPageSize(). The current impl look safe to me. https://codereview.chromium.org/2138173002/diff/80001/base/allocator/allocato... base/allocator/allocator_shim_unittest.cc:243: ASSERT_GE(reallocs_intercepted_by_addr[Hash(old_realloc_ptr)], 1u); On 2016/07/14 19:04:27, Sigurður Ásgeirsson wrote: > On 2016/07/12 14:51:05, Primiano Tucci wrote: > > uh? > > The test fixture records the inbound pointer to the realloc. If the pointer is > truly reallocated, then this test fails. This is happening under the WinHeap, > presumably because 71->73 jumps a size class in the WinHeap, whereas the other > allocators are eating slop in the original allocation. > The test is strictly speaking incorrect, as-is. Ahh I see. makes sense. Thanks. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim.cc:299: // Ditto for plain malloc() / calloc() / free() etc. symbols. plz remove this comment line. It was already uncomprehensible before (my fault). But now with the OS_WIN in the middle is very misleading :) https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim_override_ucrt_symbols_win.h (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_override_ucrt_symbols_win.h:5: // Its purpose is to SHIM_ALIAS_SYMBOL the Libc symbols for malloc/new to the I think this comment needs to be reworded, this doens't use SHIM_ALIAS_SYMBOL anymore. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_override_ucrt_symbols_win.h:15: #include "base/allocator/allocator_shim_internals.h" I don't htink you need this header at this point. https://codereview.chromium.org/2138173002/diff/130008/build/config/allocator... File build/config/allocator.gni (right): https://codereview.chromium.org/2138173002/diff/130008/build/config/allocator... build/config/allocator.gni:37: if (!(is_win && is_nacl)) { out of cusiosity why do you need this if? If is_win=true and is_nacl=true, the assert will succeed both when: use_experimental_shim = false then use_experimental_shim = true (by virtue of having is_win == true)
wfh@chromium.org changed reviewers: + wfh@chromium.org
drive by! Thanks for doing this, siggi! in general, the tests we have in base_unittests should be pretty resilient to breakages i.e. they should tell you - As long as the bots are compiling in all the right modes - you probably want to test release/debug in both component/static to be sure. I think the win_chromium_rel_ng compiles in release/static for example... but some waterfall bots are release/component and debug/component. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim.cc:52: return 4096; base::GetPageSize() is the canonical way, but it calls GetSystemInfo each time so should be cached. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_winheap.cc (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:36: CHECK(false) << "The windows heap does not support memalign."; won't this check hit a lot?
Btw none of my comments up are huge deals, so speculative LGTM with comment addressed to kill timezone latency. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_winheap.cc (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:36: CHECK(false) << "The windows heap does not support memalign."; On 2016/07/15 15:49:54, Will Harris wrote: > won't this check hit a lot? my understand is that memalign is a posix invention, so no code which runs on windows should be in the state where it can call memalign today. Now you open an interesting question though. It seems that in windows-land the alternative is _aligned_malloc. That one doesn't seem shimmed (neither in the old nor new shim). Is that intentional? Is that because it calls malloc under the hoods and we intercept it there?
The CQ bit was checked by siggi@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...
siggi@chromium.org changed reviewers: + brettw@chromium.org
K, addressed comments. Brett for owners on allocator.gni please. I'd also love your take on the use_experimental_shim arg. The problem is that setting the use_experimental_shim=true in gn args sets it both on under is_win and under is_nacl. Under is_nacl we don't pay any heed to the setting, so I believe it'll work as-is, just feels unclean. WDYT? Can I have conditional code in args.gn? https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim.cc:52: return 4096; On 2016/07/15 15:49:54, Will Harris wrote: > base::GetPageSize() is the canonical way, but it calls GetSystemInfo each time > so should be cached. > Done. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim.cc:299: // Ditto for plain malloc() / calloc() / free() etc. symbols. On 2016/07/15 14:02:10, Primiano Tucci wrote: > plz remove this comment line. It was already uncomprehensible before (my fault). > But now with the OS_WIN in the middle is very misleading :) Done. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_winheap.cc (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:36: CHECK(false) << "The windows heap does not support memalign."; On 2016/07/15 17:58:09, Primiano Tucci wrote: > On 2016/07/15 15:49:54, Will Harris wrote: > > won't this check hit a lot? > > my understand is that memalign is a posix invention, so no code which runs on > windows should be in the state where it can call memalign today. > > Now you open an interesting question though. It seems that in windows-land the > alternative is _aligned_malloc. That one doesn't seem shimmed (neither in the > old nor new shim). Is that intentional? Is that because it calls malloc under > the hoods and we intercept it there? The Win heap doesn't (AFAIK) support aligned allocations, and so _aligned_malloc requires a matching _aligned_free. Presumably this means these functions over-allocate from malloc, and prepend the returned memory region with the distance to the malloc-ed block. ... time passes ... Yup, it calls through to malloc. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_winheap.cc:36: CHECK(false) << "The windows heap does not support memalign."; On 2016/07/15 15:49:54, Will Harris wrote: > won't this check hit a lot? Nopes, nothing on Windows should call memalign, and there's nothing that dispatches to this function. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... File base/allocator/allocator_shim_override_ucrt_symbols_win.h (right): https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_override_ucrt_symbols_win.h:5: // Its purpose is to SHIM_ALIAS_SYMBOL the Libc symbols for malloc/new to the On 2016/07/15 14:02:10, Primiano Tucci wrote: > I think this comment needs to be reworded, this doens't use SHIM_ALIAS_SYMBOL > anymore. Argh - copy/paste strikes again - sorry. https://codereview.chromium.org/2138173002/diff/130008/base/allocator/allocat... base/allocator/allocator_shim_override_ucrt_symbols_win.h:15: #include "base/allocator/allocator_shim_internals.h" On 2016/07/15 14:02:10, Primiano Tucci wrote: > I don't htink you need this header at this point. Done. https://codereview.chromium.org/2138173002/diff/130008/build/config/allocator... File build/config/allocator.gni (right): https://codereview.chromium.org/2138173002/diff/130008/build/config/allocator... build/config/allocator.gni:37: if (!(is_win && is_nacl)) { On 2016/07/15 14:02:10, Primiano Tucci wrote: > out of cusiosity why do you need this if? > > If is_win=true and is_nacl=true, the assert will succeed both when: > use_experimental_shim = false > then > use_experimental_shim = true (by virtue of having is_win == true) Yeah, this is messed up. I was under the impression that is_nacl was not mutually exclusive to is_win. This is simply not true, however there's a wrinkle when use_experimental_shim is set with an explicit GN argument. In that case the flag is set both for is_nacl, and is_win, which is what confused me. This requires either using a different variables for the argument and modulate it's value depending whether is_win or is_nacl, or else ignoring the flag settings when is_nacl.
https://codereview.chromium.org/2138173002/diff/190001/build/config/allocator... File build/config/allocator.gni (right): https://codereview.chromium.org/2138173002/diff/190001/build/config/allocator... build/config/allocator.gni:36: # TODO(siggi): NaCL doesn't appear to obey this configuration, but this is still You can do this but there's no built-in feature for this. You have to write the code to implement the thing yourself. If somebody sets the flag via "gn args" it will override the default set here in all toolchains (this is how the args work). But after the declare_args block you can always override that value again, in this case checking if we're doing a nacl cross-compile and setting it to false.
Can you also provide a more detailed commit message?
Description was changed from ========== Hookup the generic heap interceptor for Windows. BUG=550886 ========== to ========== Hookup the generic heap intercept for Windows. With this change, it's possible to set a GN arg "use_experimental_allocator_shim" to true to enable the generic allocator shim on Windows. This only works in non-component builds due to the way the allocator is intercepted on Windows. BUG=550886 ==========
Hey Brett, I'm going to need more direction on how to override an arg flag - simply stomping it to a different value doesn't seem to work. I'm guessing this is by design, as it'd be super-confusing if it were possible to e.g. accidentally override these axiomatic configuration values? I added more verbiage to the CL description - good enough? https://codereview.chromium.org/2138173002/diff/190001/build/config/allocator... File build/config/allocator.gni (right): https://codereview.chromium.org/2138173002/diff/190001/build/config/allocator... build/config/allocator.gni:36: # TODO(siggi): NaCL doesn't appear to obey this configuration, but this is still On 2016/07/18 20:11:08, brettw (ping after 24h) wrote: > You can do this but there's no built-in feature for this. You have to write the > code to implement the thing yourself. > > If somebody sets the flag via "gn args" it will override the default set here in > all toolchains (this is how the args work). But after the declare_args block you > can always override that value again, in this case checking if we're doing a > nacl cross-compile and setting it to false. I'm not sure how to do this. Adding this code: --- cut here --- if (is_nacl && use_experimental_allocator_shim) { # When the use_experimental_allocator_shim is set in GN args, it'll take # effect for NaCL, which doesn't obey the configuration. To reduce confusion # the flag is quashed to false for that case here. use_experimental_allocator_shim = false } --- cut here --- causes GN to throw a hissy fit on me: ERROR at //build/config/BUILD.gn:5:1: Value collision. import("//build/config/allocator.gni") ^------------------------------------ This import contains "use_experimental_allocator_shim" See //build/config/allocator.gni:37:37: defined here. use_experimental_allocator_shim = false ^---- Which would clobber the one in your current scope See build arg file (use "gn args <out_dir>" to edit):10:35: defined here. use_experimental_allocator_shim = true ^--- Executing import should not conflict with anything in the current scope unless the values are identical. See //build/config/BUILDCONFIG.gn:461:3: which caused the file to be included. "//build/config:feature_flags",
Brett - gentle nudge
Sorry for the delay, I needed to actually try it.
I'm not seeing that problem with a hacked up example I made (git cl patch is
broken for me, not sure why).
Here's my patch:
diff --git a/build/config/allocator.gni b/build/config/allocator.gni
index 10ac1adf..bf66d63 100644
--- a/build/config/allocator.gni
+++ b/build/config/allocator.gni
@@ -32,6 +32,8 @@ declare_args() {
assert(use_allocator == "none" || use_allocator == "tcmalloc")
assert(!is_win || use_allocator == "none", "Tcmalloc doesn't work on Windows.")
-assert(
- !use_experimental_allocator_shim || is_linux || is_android,
- "use_experimental_allocator_shim supported only on Linux and Android
targets")
+
+if (is_nacl) {
+ use_experimental_allocator_shim = false
+}
+print("allocator_shim = $use_experimental_allocator_shim for
$current_toolchain")
and I set the build arg "use_experimental_allocator_shim = true"
I get the output to the console:
allocator_shim = true for //build/toolchain/win:x64
allocator_shim = false for //build/toolchain/nacl:newlib_pnacl
allocator_shim = false for //build/toolchain/nacl:irt_x64
allocator_shim = true for //native_client/src/trusted/win:nacl_win_as_x64
allocator_shim = false for //build/toolchain/nacl:clang_newlib_x64
allocator_shim = false for //build/toolchain/nacl:glibc_x64
allocator_shim = false for //build/toolchain/nacl:newlib_pnacl_nonsfi
Done. Wrote 5400 targets from 966 files in 1647ms
Interesting, I tried patching this into my workspace with the same failure results. I then synced to ToT, and now it works like a charm - guess something changed in the meantime. On Wed, Jul 20, 2016 at 2:28 PM <brettw@chromium.org> wrote: > Sorry for the delay, I needed to actually try it. > > I'm not seeing that problem with a hacked up example I made (git cl patch > is > broken for me, not sure why). > > Here's my patch: > > diff --git a/build/config/allocator.gni b/build/config/allocator.gni > index 10ac1adf..bf66d63 100644 > --- a/build/config/allocator.gni > +++ b/build/config/allocator.gni > @@ -32,6 +32,8 @@ declare_args() { > assert(use_allocator == "none" || use_allocator == "tcmalloc") > > assert(!is_win || use_allocator == "none", "Tcmalloc doesn't work on > Windows.") > -assert( > - !use_experimental_allocator_shim || is_linux || is_android, > - "use_experimental_allocator_shim supported only on Linux and Android > targets") > + > +if (is_nacl) { > + use_experimental_allocator_shim = false > +} > +print("allocator_shim = $use_experimental_allocator_shim for > $current_toolchain") > > and I set the build arg "use_experimental_allocator_shim = true" > > I get the output to the console: > allocator_shim = true for //build/toolchain/win:x64 > allocator_shim = false for //build/toolchain/nacl:newlib_pnacl > allocator_shim = false for //build/toolchain/nacl:irt_x64 > allocator_shim = true for //native_client/src/trusted/win:nacl_win_as_x64 > allocator_shim = false for //build/toolchain/nacl:clang_newlib_x64 > allocator_shim = false for //build/toolchain/nacl:glibc_x64 > allocator_shim = false for //build/toolchain/nacl:newlib_pnacl_nonsfi > Done. Wrote 5400 targets from 966 files in 1647ms > > > https://codereview.chromium.org/2138173002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Well then, Brett, your suggestion works like a charm at ToT. All good now?
The CQ bit was checked by siggi@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2138173002/#ps250001 (title: "Squash the use_experimental_allocator_shim for NaCL as per Brett's suggestion.")
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 ========== Hookup the generic heap intercept for Windows. With this change, it's possible to set a GN arg "use_experimental_allocator_shim" to true to enable the generic allocator shim on Windows. This only works in non-component builds due to the way the allocator is intercepted on Windows. BUG=550886 ========== to ========== Hookup the generic heap intercept for Windows. With this change, it's possible to set a GN arg "use_experimental_allocator_shim" to true to enable the generic allocator shim on Windows. This only works in non-component builds due to the way the allocator is intercepted on Windows. BUG=550886 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Hookup the generic heap intercept for Windows. With this change, it's possible to set a GN arg "use_experimental_allocator_shim" to true to enable the generic allocator shim on Windows. This only works in non-component builds due to the way the allocator is intercepted on Windows. BUG=550886 ========== to ========== Hookup the generic heap intercept for Windows. With this change, it's possible to set a GN arg "use_experimental_allocator_shim" to true to enable the generic allocator shim on Windows. This only works in non-component builds due to the way the allocator is intercepted on Windows. BUG=550886 Committed: https://crrev.com/e422cb9901097b09317bc07fd7e1f8f603727ae5 Cr-Commit-Position: refs/heads/master@{#406879} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/e422cb9901097b09317bc07fd7e1f8f603727ae5 Cr-Commit-Position: refs/heads/master@{#406879} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
