Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414453017:
Allocator shims working on VS2015. (Closed)
|
Created:
5 years, 1 month ago by Will Harris Modified:
4 years, 8 months ago CC:
chromium-reviews, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org, Primiano Tucci (use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllocator shims working on VS2015.
VS2015 is happy for us to simply override the CRT symbols since they
seem to be defined weakly by the universal CRT.
This shim is far simpler than the previous one, as the libcmt stripping
technique can be removed.
Allocator shim is also now only be enabled on Release Static builds.
BUG=481611
TEST=base_unittests --gtest_filter=*Memory* in all configurations
(Debug/Release/Static/Component/GN/gyp/x64/x86)
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015
Cr-Commit-Position: refs/heads/master@{#383810}
Committed: https://crrev.com/08a9665ab0bce3813425f8d7ec9bec9178d5faa4
Cr-Commit-Position: refs/heads/master@{#384711}
Patch Set 1 #Patch Set 2 : rebase and fix up #Patch Set 3 : fix IsAllocatorInitialized #Patch Set 4 : fix gn #Patch Set 5 : fix gn. remove some unneeded crt overrides. add aligned mem test. #Patch Set 6 : linux fix #Patch Set 7 : gles2_conform_support should not directly depend on allocator since it does so via egl_native->base #Patch Set 8 : rebase after crrev.com/1835433002 #
Total comments: 13
Patch Set 9 : code review comments. add more aligned tests #
Total comments: 8
Patch Set 10 : no aligned_realloc in POSIX #Patch Set 11 : remove unused generic_cpp_alloc #Patch Set 12 : rebase #
Messages
Total messages: 59 (29 generated)
Message was sent while issue was closed.
Description was changed from ========== Allocator shims working on VS2015. BUG=481611 ========== to ========== Allocator shims working on VS2015. BUG=481611 ==========
Description was changed from ========== Allocator shims working on VS2015. BUG=481611 ========== to ========== Allocator shims working on VS2015. Enable the allocator shim only for Release. Replace the libcmt stripping technique with weak symbol override. BUG=481611 TEST=base_unittests --gtest_filter=Memory* ==========
wfh@chromium.org changed reviewers: + brucedawson@chromium.org, primiano@chromium.org
Description was changed from ========== Allocator shims working on VS2015. Enable the allocator shim only for Release. Replace the libcmt stripping technique with weak symbol override. BUG=481611 TEST=base_unittests --gtest_filter=Memory* ========== to ========== Allocator shims working on VS2015. Enable the allocator shim only for Release. Replace the libcmt stripping technique with weak symbol override. (WIP: working on gyp builds, not yet working on gn builds) BUG=481611 TEST=base_unittests --gtest_filter=Memory* ==========
Description was changed from ========== Allocator shims working on VS2015. Enable the allocator shim only for Release. Replace the libcmt stripping technique with weak symbol override. (WIP: working on gyp builds, not yet working on gn builds) BUG=481611 TEST=base_unittests --gtest_filter=Memory* ========== to ========== Allocator shims working on VS2015. Replace the libcmt stripping technique with weak symbol override. Disable allocator shim on Debug builds. (WIP: working on gyp builds, not yet working on gn builds) BUG=481611 TEST=base_unittests --gtest_filter=Memory* ==========
Description was changed from ========== Allocator shims working on VS2015. Replace the libcmt stripping technique with weak symbol override. Disable allocator shim on Debug builds. (WIP: working on gyp builds, not yet working on gn builds) BUG=481611 TEST=base_unittests --gtest_filter=Memory* ========== to ========== Allocator shims working on VS2015. Replace the libcmt stripping technique with weak symbol override. Disable allocator shim on Debug builds. (WIP: working on gyp builds, not yet working on gn builds) BUG=481611 TEST=base_unittests --gtest_filter=*Memory* ==========
wfh@chromium.org changed reviewers: + thakis@chromium.org - brucedawson@chromium.org, primiano@chromium.org
Description was changed from ========== Allocator shims working on VS2015. Replace the libcmt stripping technique with weak symbol override. Disable allocator shim on Debug builds. (WIP: working on gyp builds, not yet working on gn builds) BUG=481611 TEST=base_unittests --gtest_filter=*Memory* ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly. This shim is far simpler than the previous one, so remove the libcmt stripping technique. Allocator shim will now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations possible (Debug/Release/Static/Component/GN/gyp/x64/x86) ==========
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly. This shim is far simpler than the previous one, so remove the libcmt stripping technique. Allocator shim will now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations possible (Debug/Release/Static/Component/GN/gyp/x64/x86) ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) ==========
PTAL. This does work... no smoke and mirrors. :)
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:26: // See definitions of original functions in ucrt\ucrt_malloc.h Do you mean ucrt\corecrt_malloc.h? From c:\Program Files (x86)\Windows Kits\10\Include? Do we have the source as well? Would be good to give exact locations for the header and source files this is based on. Also, are you overriding all or just some functions from that file? When I've done this previously I found that you had to override every single function or else risk link errors if one of the other ones was referenced.
thakis@chromium.org changed reviewers: + rnk@chromium.org
+rnk since he did something similar (but in a different way) in http://reviews.llvm.org/D18413 Is there reason to believe why """ ucrtbase.dll appears to be built with some kind of cross-module inlining, because there are calls to imported Heap* routines sprinkled throughout the code. This inlining defeats our attempts to hotpatch malloc, _malloc_base, and related functions """ doesn't apply to chrome? https://codereview.chromium.org/1414453017/diff/140001/base/allocator/BUILD.gn File base/allocator/BUILD.gn (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/BUILD.g... base/allocator/BUILD.gn:27: if (is_win && !is_component_build && !is_debug) { since this is repeated below, should it go into some file-scoped var (win_use_allocator, say)?
On 2016/03/28 20:45:48, Nico wrote: > +rnk since he did something similar (but in a different way) in > http://reviews.llvm.org/D18413 > > Is there reason to believe why > > """ > ucrtbase.dll appears to be built with some kind of cross-module > inlining, because there are calls to imported Heap* routines sprinkled > throughout the code. This inlining defeats our attempts to hotpatch > malloc, _malloc_base, and related functions > """ > > doesn't apply to chrome? It just means ucrtbase.dll won't be checked for max allocation size. As long as you aren't redirecting the heap to a new allocator implementation, you're probably fine.
On 2016/03/28 21:06:24, Reid Kleckner wrote: > On 2016/03/28 20:45:48, Nico wrote: > > +rnk since he did something similar (but in a different way) in > > http://reviews.llvm.org/D18413 > > > > Is there reason to believe why > > > > """ > > ucrtbase.dll appears to be built with some kind of cross-module > > inlining, because there are calls to imported Heap* routines sprinkled > > throughout the code. This inlining defeats our attempts to hotpatch > > malloc, _malloc_base, and related functions > > """ > > > > doesn't apply to chrome? > > It just means ucrtbase.dll won't be checked for max allocation size. As long as > you aren't redirecting the heap to a new allocator implementation, you're > probably fine. Yes; the aim of the allocator shim is not to shim all allocations in-process (that might e.g. come from other DLLs loaded in our process) but to just shim any modules that we are building. The specific threat we are trying to mitigate is third party code linked into Chrome that might do unsafe allocations via unsafe type conversions.
On 2016/03/28 20:38:18, brucedawson wrote: > https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... > File base/allocator/allocator_shim_win.cc (right): > > https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... > base/allocator/allocator_shim_win.cc:26: // See definitions of original > functions in ucrt\ucrt_malloc.h > Do you mean ucrt\corecrt_malloc.h? From c:\Program Files (x86)\Windows > Kits\10\Include? > > Do we have the source as well? Would be good to give exact locations for the > header and source files this is based on. Yes, I can certainly do that - the comments were left over from the old CRT version and now should be updated. > > Also, are you overriding all or just some functions from that file? When I've > done this previously I found that you had to override every single function or > else risk link errors if one of the other ones was referenced. yes, previously it did appear that we had to shim a load more functions in order for this to work (see earlier revs of prep_libc.py) but I've done a lot of testing here on which functions need to be shimmed to get OutOfMemoryDeathTest.* (memory_unittest.cc) passing and this is the minimum set. It's possible I could shim _malloc_base/_free_base/_realloc_base etc etc instead of malloc/realloc/free but I found in testing it wasn't necessary. Perhaps if we want to implement this in future for Debug builds it might be necessary to change which functions we override. After looking around the CRT source, I was worried that aligned_malloc might not be covered by the shim because of this, but I added a test specifically for that case in memory_unittest.cc and that seems fine too. if you can think of any other allocation functions I might have missed in the testing that would be useful.
Ok, sounds good. LGTM, let's see how it does :-)
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
wfh@chromium.org changed reviewers: + piman@chromium.org
+piman for gpu/ owners while I fix the nits. I might add a test for aligned_realloc too. https://codereview.chromium.org/1414453017/diff/140001/gpu/gles2_conform_supp... File gpu/gles2_conform_support/gles2_conform_support.gyp (left): https://codereview.chromium.org/1414453017/diff/140001/gpu/gles2_conform_supp... gpu/gles2_conform_support/gles2_conform_support.gyp:99: '../../base/allocator/allocator.gyp:allocator', note to piman: this dependency is a duplicate of the existing egl_native->base->base\allocator dependency but was causing link errors with the new allocator shim.
lgtm
all changes made. https://codereview.chromium.org/1414453017/diff/140001/base/allocator/BUILD.gn File base/allocator/BUILD.gn (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/BUILD.g... base/allocator/BUILD.gn:27: if (is_win && !is_component_build && !is_debug) { On 2016/03/28 20:45:48, Nico wrote: > since this is repeated below, should it go into some file-scoped var > (win_use_allocator, say)? Done. https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:26: // See definitions of original functions in ucrt\ucrt_malloc.h On 2016/03/28 20:38:18, brucedawson wrote: > Do you mean ucrt\corecrt_malloc.h? From c:\Program Files (x86)\Windows > Kits\10\Include? > > Do we have the source as well? Would be good to give exact locations for the > header and source files this is based on. > > Also, are you overriding all or just some functions from that file? When I've > done this previously I found that you had to override every single function or > else risk link errors if one of the other ones was referenced. Yes I updated the comment with the correct header path I updated all the functions below with the ones they are replacing. I am only replacing the ones that need to be replaced and for all the tests to pass.
lgtm
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1414453017/#ps160001 (title: "code review comments. add more aligned tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414453017/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414453017/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
primiano@chromium.org changed reviewers: + primiano@chromium.org
I'll like to see this integrated in the unified allocator shim one day, but that's lower priority and let's not put too much stuff on fire at the same time. I'll take care of switching this to the new shim thingy once this sticks. Some comments below, sorry for the delay. https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (left): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:121: return generic_cpp_alloc(size, false); Out of curiosity, why are you not re-defining these? Is it because you rely on them calling malloc/free under the hoods? Wouldn't it be safer to still redefine them? https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:84: inline void* generic_cpp_alloc(size_t size, bool nothrow) { Looks like nothing is calling this function anymore? https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:177: memset(result, 0, size); I wonder if implementing this as WinHeapAlloc(HEAP_ZERO_MEMORY) would be any faster for larger allocations. On linux calloc(large_size) is usually short-circuited on a mmap(), which gives the guarantee that the memory is zero-initialized (typically the kernel has a pool of alredy laundered pages) avoiding the memset https://codereview.chromium.org/1414453017/diff/160001/base/allocator/allocat... File base/allocator/allocator_check.cc (left): https://codereview.chromium.org/1414453017/diff/160001/base/allocator/allocat... base/allocator/allocator_check.cc:17: // TODO(primiano): replace with an include once base can depend on allocator. thanks, forgot about this :) https://codereview.chromium.org/1414453017/diff/160001/base/process/memory_un... File base/process/memory_unittest.cc (right): https://codereview.chromium.org/1414453017/diff/160001/base/process/memory_un... base/process/memory_unittest.cc:144: TEST(MemoryTest, AllocatorShimWorking) { Not sure how much this will add. It's very easy to end up with different build flags for unittests vs main executable. In the main executable we have a runtime check in place in ContentMainRunner. But I guess this won't harm as a first-level smoke test in the CQ. https://codereview.chromium.org/1414453017/diff/160001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1414453017/diff/160001/build/common.gypi#newc... build/common.gypi:3534: 'ALLOCATOR_SHIM' What is the reason for removing NO_TCMALLOC here? IIRC that drives in build_config the generation of USE_TCMALLOC, which will now become falsely true on windows.
https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:177: memset(result, 0, size); I don't think we can use HeapAlloc(HEAP_ZERO_MEMORY) because then we have an alloc/free mismatch. Or at least we'd have to coordinate with the DrMemory team to avoid having this reported as a mismatch. That is, while the CRT heap does use the Windows heap under the hood in release builds, DrMemory still reports mismatches if you allocate with one family and free with the other.
thanks for review - just in time :) https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (left): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:121: return generic_cpp_alloc(size, false); On 2016/03/29 18:46:06, Primiano (OOO til Mar 31) wrote: > Out of curiosity, why are you not re-defining these? Is it because you rely on > them calling malloc/free under the hoods? Wouldn't it be safer to still redefine > them? yes they call down into malloc. It seems MS put more effort in with the uCRT to support replacing malloc cleanly - see comment on lines 6/7 of ucrt\heap\malloc_base.cpp for evidence of this. https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:84: inline void* generic_cpp_alloc(size_t size, bool nothrow) { On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > Looks like nothing is calling this function anymore? good spot. removed. https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:177: memset(result, 0, size); On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > I wonder if implementing this as WinHeapAlloc(HEAP_ZERO_MEMORY) would be any > faster for larger allocations. > On linux calloc(large_size) is usually short-circuited on a mmap(), which gives > the guarantee that the memory is zero-initialized (typically the kernel has a > pool of alredy laundered pages) avoiding the memset I'm leaving the inners of the functions the same for this CL, but if you have suggestions they can be addressed in future CL. From my testing, the working set doesn't actually get a 2Gb allocation anyways, so I think windows is doing some optimization here anyway... https://codereview.chromium.org/1414453017/diff/160001/base/allocator/allocat... File base/allocator/allocator_check.cc (left): https://codereview.chromium.org/1414453017/diff/160001/base/allocator/allocat... base/allocator/allocator_check.cc:17: // TODO(primiano): replace with an include once base can depend on allocator. On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > thanks, forgot about this :) Acknowledged. https://codereview.chromium.org/1414453017/diff/160001/base/process/memory_un... File base/process/memory_unittest.cc (right): https://codereview.chromium.org/1414453017/diff/160001/base/process/memory_un... base/process/memory_unittest.cc:144: TEST(MemoryTest, AllocatorShimWorking) { On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > Not sure how much this will add. It's very easy to end up with different build > flags for unittests vs main executable. In the main executable we have a runtime > check in place in ContentMainRunner. > But I guess this won't harm as a first-level smoke test in the CQ. Ack. I added it after I tripped it while developing the CL, and wanted an easier way to find out it was working rather than compiling all 20,000 files needed for Chrome. https://codereview.chromium.org/1414453017/diff/160001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1414453017/diff/160001/build/common.gypi#newc... build/common.gypi:3534: 'ALLOCATOR_SHIM' On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > What is the reason for removing NO_TCMALLOC here? > IIRC that drives in build_config the generation of USE_TCMALLOC, which will now > become falsely true on windows. bruce asked the same question in chat. NO_TCMALLOC is defined above on line 2864 so this was always a second duplicate definition. I fixed the glitch when I was adding the Release specific configuration.
Thanks! LGTM https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1414453017/diff/140001/base/allocator/allocat... base/allocator/allocator_shim_win.cc:177: memset(result, 0, size); On 2016/03/29 18:56:20, Will Harris wrote: > On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > > I wonder if implementing this as WinHeapAlloc(HEAP_ZERO_MEMORY) would be any > > faster for larger allocations. > > On linux calloc(large_size) is usually short-circuited on a mmap(), which > gives > > the guarantee that the memory is zero-initialized (typically the kernel has a > > pool of alredy laundered pages) avoiding the memset > > I'm leaving the inners of the functions the same for this CL, but if you have > suggestions they can be addressed in future CL. Looks like brucedawson@ answered here and it seems non trivial. > From my testing, the working set doesn't actually get a 2Gb allocation anyways, > so I think windows is doing some optimization here anyway... Yup, the same happens on linux. The optimization lies in the fact that when you know that the memory is zero-initialized anywyas you can skip the memset. Whatever, on the other side I don't have any evidence on how much do we rely on calloc(large-size) on fast-paths and even if it was the case this is not slower than what it used to be on MSVS2013 shim https://codereview.chromium.org/1414453017/diff/160001/base/process/memory_un... File base/process/memory_unittest.cc (right): https://codereview.chromium.org/1414453017/diff/160001/base/process/memory_un... base/process/memory_unittest.cc:144: TEST(MemoryTest, AllocatorShimWorking) { On 2016/03/29 18:56:20, Will Harris wrote: > On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > > Not sure how much this will add. It's very easy to end up with different build > > flags for unittests vs main executable. In the main executable we have a > runtime > > check in place in ContentMainRunner. > > But I guess this won't harm as a first-level smoke test in the CQ. > > Ack. I added it after I tripped it while developing the CL, and wanted an easier > way to find out it was working rather than compiling all 20,000 files needed for > Chrome. Acknowledged. https://codereview.chromium.org/1414453017/diff/160001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1414453017/diff/160001/build/common.gypi#newc... build/common.gypi:3534: 'ALLOCATOR_SHIM' On 2016/03/29 18:56:21, Will Harris wrote: > On 2016/03/29 18:46:07, Primiano (OOO til Mar 31) wrote: > > What is the reason for removing NO_TCMALLOC here? > > IIRC that drives in build_config the generation of USE_TCMALLOC, which will > now > > become falsely true on windows. > > bruce asked the same question in chat. > > NO_TCMALLOC is defined above on line 2864 so this was always a second duplicate > definition. I fixed the glitch when I was adding the Release specific > configuration. Ahh makes sense, thanks.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, brucedawson@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1414453017/#ps200001 (title: "remove unused generic_cpp_alloc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414453017/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414453017/200001
Message was sent while issue was closed.
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/1839783005/ by sorin@chromium.org. The reason for reverting is: It breaks the link on Windows. FAILED: C:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-with-manifests environment.x86 True views_examples_with_content_exe.exe "C:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper environment.x86 False link.exe /nologo /OUT:views_examples_with_content_exe.exe @views_examples_with_content_exe.exe.rsp" 1 mt.exe rc.exe "obj\ui\views\examples\views_examples_with_content_exe.views_examples_with_content_exe.exe.intermediate.manifest" obj\ui\views\examples\views_examples_with_content_exe.views_examples_with_content_exe.exe.generated.manifest ..\..\ui\views\examples\views_examples.exe.manifest ..\..\build\win\compatibility.manifest allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: __query_new_mode already defined in libucrt.lib(new_mode.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: __set_new_mode already defined in libucrt.lib(new_mode.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: _calloc already defined in libucrt.lib(calloc.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: _free already defined in libucrt.lib(free.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: _malloc already defined in libucrt.lib(malloc.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: _realloc already defined in libucrt.lib(realloc.obj) views_examples_with_content_exe.exe : fatal error LNK1169: one or more multiply defined symbols found .
Message was sent while issue was closed.
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810} ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810} ==========
The two known issues have been fixed in other CLs: 1. Link issue on Windows - https://codereview.chromium.org/1836093004/ 2. Valgrind issue on Linux - https://codereview.chromium.org/1846483002/ so I am going to re-commit this; nothing has changed in this CL.
On 2016/03/31 18:28:28, Will Harris wrote: > The two known issues have been fixed in other CLs: > > 1. Link issue on Windows - https://codereview.chromium.org/1836093004/ > 2. Valgrind issue on Linux - https://codereview.chromium.org/1846483002/ > > so I am going to re-commit this; nothing has changed in this CL. Excellent. Thanks. lgtm.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, thakis@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1414453017/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414453017/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414453017/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414453017/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414453017/220001
win_chromium_x64_rel_ng is a suspected flake see crbug.com/599955
Message was sent while issue was closed.
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810} ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810} ========== to ========== Allocator shims working on VS2015. VS2015 is happy for us to simply override the CRT symbols since they seem to be defined weakly by the universal CRT. This shim is far simpler than the previous one, as the libcmt stripping technique can be removed. Allocator shim is also now only be enabled on Release Static builds. BUG=481611 TEST=base_unittests --gtest_filter=*Memory* in all configurations (Debug/Release/Static/Component/GN/gyp/x64/x86) CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/e20c2e0ab4e5b796e7a1890fb9048b5eb307d015 Cr-Commit-Position: refs/heads/master@{#383810} Committed: https://crrev.com/08a9665ab0bce3813425f8d7ec9bec9178d5faa4 Cr-Commit-Position: refs/heads/master@{#384711} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/08a9665ab0bce3813425f8d7ec9bec9178d5faa4 Cr-Commit-Position: refs/heads/master@{#384711}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/2132963002/ by yukishiino@chromium.org. The reason for reverting is: We're seeing probably the same or similar issue when linking on win again. https://build.chromium.org/p/chromium/builders/Win/builds/45048 FAILED: blacklist_test_main_dll.dll blacklist_test_main_dll.dll.lib blacklist_test_main_dll.dll.pdb C:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-with-manifests environment.x86 True blacklist_test_main_dll.dll "C:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper environment.x86 False link.exe /nologo /IMPLIB:blacklist_test_main_dll.dll.lib /DLL /OUT:blacklist_test_main_dll.dll @blacklist_test_main_dll.dll.rsp" 2 mt.exe rc.exe "obj\chrome_elf\blacklist_test_main_dll.blacklist_test_main_dll.dll.intermediate.manifest" obj\chrome_elf\blacklist_test_main_dll.blacklist_test_main_dll.dll.generated.manifest allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: __query_new_mode already defined in libucrt.lib(new_mode.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: __set_new_mode already defined in libucrt.lib(new_mode.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: _calloc already defined in libucrt.lib(calloc.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: _free already defined in libucrt.lib(free.obj) allocator.lib(allocator.allocator_shim_win.obj) : error LNK2005: _malloc already defined in libucrt.lib(malloc.obj) blacklist_test_main_dll.dll : fatal error LNK1169: one or more multiply defined symbols found Let me revert this CL to see if the revert fixes the build breakage.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
