Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2143693003:
Start refactoring Windows allocator shim in prep for hooking in generic allocator shim on 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. |
DescriptionStart refactoring Windows allocator shim in prep for hooking in generic allocator shim on Windows.
BUG=550886
Committed: https://crrev.com/25da2dbf685abbc6ee87f1bd527ddf93e298f3c4
Cr-Commit-Position: refs/heads/master@{#405984}
Patch Set 1 #Patch Set 2 : Add missing build dependencies. #Patch Set 3 : Clean up includes, add comments and a missing include guard. #Patch Set 4 : Add back necessary includes. #
Total comments: 18
Patch Set 5 : Address comments. #
Total comments: 2
Patch Set 6 : Address Nico's comments. #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 34 (8 generated)
Description was changed from ========== Start refactoring Windows allocator shim in prep for hooking in generic allocator shim on Windows. BUG= ========== to ========== Start refactoring Windows allocator shim in prep for hooking in generic allocator shim on Windows. BUG=550886 ==========
siggi@chromium.org changed reviewers: + primiano@chromium.org
Hey Primiano, here's the refactoring CL you suggested in https://codereview.chromium.org/2138173002/. I like the feature flag, but I wonder if it'd make sense to change the name to something relating to the allocator shim itself, like maybe: "ENABLE_WIN_ALLOCATOR_SHIM", and gate the tests on that? Siggi
LGTM with minor comments. The only major comment is that GYP || condition, which seems a bit odd to me (See comments inline). Did you check that the tests are actually running on the trybots (do we still have bots on gyp)? Thanks a lot for splitting the CL, this makes it way easier to review. Adding: +thakis to cover the rest of base (specifically for the GYP questions) +wfh as he wrote the original shim to see if he has any concern or can spot something I missed. The context here is: siggi@ has volunteered to port the win_shim to the unified allocator shim (and drop the old code once everything sticks), and I am very enthusiastically supporting that. >I like the feature flag, but I wonder if it'd make sense to change the name to something relating to the allocator shim itself, like maybe: "ENABLE_WIN_ALLOCATOR_SHIM", and gate the tests on that? So I think that once everything sticks we can just use the existing USE_EXPERIMENTAL_ALLOCATOR_SHIM define (at some point we should drop the EXPERIMENTAL part of the name). In the meantime I think this makes easier to codesearch and to understand what the define is about. (Not too strong on this though) https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn#newcode1988 base/BUILD.gn:1988: "//base/allocator:features", Not sure this is required. tests depend on base which depends on allocator:features. You should get this automatically by virtue of depending on base. I suppose you are doing this not because it's required but more in the spirit of Include What You Use. No idea what is tha pettern in GN, some base owner will comment, I'm indifferent. https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... base/allocator/allocator.gyp:391: 'ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', uhm not sure about this part. At very least this should be s/||/or/ But then I am not sure if the right way of spelling this is: 1) 'FLAG=<(a) or <(b)' 2) 'FLAG=<(a or b)' 3) you cannot do this and need to create first a variable two lines above. You need a gyp-ninja for this :) https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... base/allocator/allocator_shim_win.cc:17: #include "base/allocator/allocator_shim_win.h" maybe add a TODO saying that this file will be deleted once the unified allocator sticks. https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... File base/allocator/winheap_stubs_win.cc (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.cc:54: void* WinHeapCalloc(size_t n, size_t elem_size) { Not sure you need this here. This function is not used by the old shim (and I think what you are doing is correct). But this suggests that in the next CL you can just put this body inside the dispatch_to_winheap.cc https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... File base/allocator/winheap_stubs_win.h (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: drop your wayback machine :P (also the new copyright header doesn't have the (c) https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.h:9: #ifndef BASE_ALLOCATOR_ALLOCATOR_WINHEAP_STUBS_H_ nit: s/ALLOCATOR_ALLOCATOR_/ALLOCATOR_/ https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.h:12: #include <windows.h> not sure you need this windows.h here. Looks like you really need stdint.h https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp#newcode625 base/base.gyp:625: 'allocator/allocator.gyp:allocator_features#target', ditto about the doubt about IWYU. defer to thakis.
logic looks good to me, this looks just like a refactor, but I'd like primiano's comments to be addressed before committing. modulo that, lgtm from me.
wfh@chromium.org changed reviewers: + wfh@chromium.org
https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... File base/allocator/winheap_stubs_win.cc (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.cc:69: bool WinCallNewHandler(size_t size) { this function might need to be added to the "functions to ignore when trying to work out correct bucket in crash" go/link_for_2143693003
siggi@chromium.org changed reviewers: + thakis@chromium.org
Thanks guys addressed comments - another look? https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn#newcode1988 base/BUILD.gn:1988: "//base/allocator:features", On 2016/07/13 17:42:49, Primiano Tucci wrote: > Not sure this is required. It does seem it is. >tests depend on base which depends on > allocator:features. Mmm, tests->base is a link-time dependency, you'd still want to be able to compile all test and base file concurrently. > You should get this automatically by virtue of depending on > base. I suppose you are doing this not because it's required but more in the > spirit of Include What You Use. > No idea what is tha pettern in GN, some base owner will comment, I'm > indifferent. I actually get trybot errors without this, complaining that the OOM tests in /process are including the results of a non-dependency. GN looks to be pretty awesome that way :). The trick to repro seems to gn gen <output_dir> --check https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... base/allocator/allocator.gyp:391: 'ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', On 2016/07/13 17:42:49, Primiano Tucci wrote: > uhm not sure about this part. At very least this should be s/||/or/ > But then I am not sure if the right way of spelling this is: > 1) 'FLAG=<(a) or <(b)' > 2) 'FLAG=<(a or b)' > 3) you cannot do this and need to create first a variable two lines above. > > You need a gyp-ninja for this :) I tested the GYP build locally, and the tests run under this condition. Unfortunately '<(a or b)' does not work, as GYP tries to resolve the variable "a or b" with poor success. Any whitespace in the expression turns into a newline in the macro definition. Leave as-is, grit our teeth and wait for GYP to die? https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... base/allocator/allocator_shim_win.cc:17: #include "base/allocator/allocator_shim_win.h" On 2016/07/13 17:42:49, Primiano Tucci wrote: > maybe add a TODO saying that this file will be deleted once the unified > allocator sticks. Done. https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... File base/allocator/winheap_stubs_win.cc (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.cc:54: void* WinHeapCalloc(size_t n, size_t elem_size) { On 2016/07/13 17:42:49, Primiano Tucci wrote: > Not sure you need this here. This function is not used by the old shim (and I > think what you are doing is correct). But this suggests that in the next CL you > can just put this body inside the dispatch_to_winheap.cc Removed, thanks. https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.cc:69: bool WinCallNewHandler(size_t size) { On 2016/07/13 18:10:37, Will Harris wrote: > this function might need to be added to the "functions to ignore when trying to > work out correct bucket in crash" > > go/link_for_2143693003 Acknowledged. https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... File base/allocator/winheap_stubs_win.h (right): https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/07/13 17:42:49, Primiano Tucci wrote: > nit: drop your wayback machine :P (also the new copyright header doesn't have > the (c) Copy-paste FTW. https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.h:9: #ifndef BASE_ALLOCATOR_ALLOCATOR_WINHEAP_STUBS_H_ On 2016/07/13 17:42:49, Primiano Tucci wrote: > nit: s/ALLOCATOR_ALLOCATOR_/ALLOCATOR_/ and again :/ https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... base/allocator/winheap_stubs_win.h:12: #include <windows.h> On 2016/07/13 17:42:49, Primiano Tucci wrote: > not sure you need this windows.h here. Looks like you really need stdint.h Done. https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp#newcode625 base/base.gyp:625: 'allocator/allocator.gyp:allocator_features#target', On 2016/07/13 17:42:49, Primiano Tucci wrote: > ditto about the doubt about IWYU. defer to thakis. Yeah I dunno - logic is that if the allocator.h file is generated at build time, then the tests could/should be compiled concurrently with base code. Thakis?
Nico - gentle nudge. There's a couple of Qs regarding build config, plus owners if you'd please?
I had no idea you wanted me to look at this. Looking at the thread again, I still have no idea. How was I supposed to know? (Before now, that is)
On 2016/07/13 17:42:49, Primiano Tucci wrote: > LGTM with minor comments. The only major comment is that GYP || condition, which > seems a bit odd to me (See comments inline). Did you check that the tests are > actually running on the trybots (do we still have bots on gyp)? > Thanks a lot for splitting the CL, this makes it way easier to review. > > Adding: > +thakis to cover the rest of base (specifically for the GYP questions) > +wfh as he wrote the original shim to see if he has any concern or can spot > something I missed. > > The context here is: siggi@ has volunteered to port the win_shim to the unified > allocator shim (and drop the old code once everything sticks), and I am very > enthusiastically supporting that. > > > >I like the feature flag, but I wonder if it'd make sense to change the name to > something relating to the allocator shim itself, like maybe: > "ENABLE_WIN_ALLOCATOR_SHIM", and gate the > tests on that? > So I think that once everything sticks we can just use the existing > USE_EXPERIMENTAL_ALLOCATOR_SHIM define (at some point we should drop the > EXPERIMENTAL part of the name). > In the meantime I think this makes easier to codesearch and to understand what > the define is about. (Not too strong on this though) > > https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn#newcode1988 > base/BUILD.gn:1988: "//base/allocator:features", > Not sure this is required. tests depend on base which depends on > allocator:features. You should get this automatically by virtue of depending on > base. I suppose you are doing this not because it's required but more in the > spirit of Include What You Use. > No idea what is tha pettern in GN, some base owner will comment, I'm > indifferent. > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > File base/allocator/allocator.gyp (right): > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > base/allocator/allocator.gyp:391: > 'ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', > uhm not sure about this part. At very least this should be s/||/or/ > But then I am not sure if the right way of spelling this is: > 1) 'FLAG=<(a) or <(b)' > 2) 'FLAG=<(a or b)' > 3) you cannot do this and need to create first a variable two lines above. > > You need a gyp-ninja for this :) > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > File base/allocator/allocator_shim_win.cc (right): > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > base/allocator/allocator_shim_win.cc:17: #include > "base/allocator/allocator_shim_win.h" > maybe add a TODO saying that this file will be deleted once the unified > allocator sticks. > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > File base/allocator/winheap_stubs_win.cc (right): > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > base/allocator/winheap_stubs_win.cc:54: void* WinHeapCalloc(size_t n, size_t > elem_size) { > Not sure you need this here. This function is not used by the old shim (and I > think what you are doing is correct). But this suggests that in the next CL you > can just put this body inside the dispatch_to_winheap.cc > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > File base/allocator/winheap_stubs_win.h (right): > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > base/allocator/winheap_stubs_win.h:1: // Copyright (c) 2012 The Chromium > Authors. All rights reserved. > nit: drop your wayback machine :P (also the new copyright header doesn't have > the (c) > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > base/allocator/winheap_stubs_win.h:9: #ifndef > BASE_ALLOCATOR_ALLOCATOR_WINHEAP_STUBS_H_ > nit: s/ALLOCATOR_ALLOCATOR_/ALLOCATOR_/ > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > base/allocator/winheap_stubs_win.h:12: #include <windows.h> > not sure you need this windows.h here. Looks like you really need stdint.h > > https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp > File base/base.gyp (right): > > https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp#newcode625 > base/base.gyp:625: 'allocator/allocator.gyp:allocator_features#target', > ditto about the doubt about IWYU. defer to thakis.
lgtm https://codereview.chromium.org/2143693003/diff/80001/base/allocator/winheap_... File base/allocator/winheap_stubs_win.cc (right): https://codereview.chromium.org/2143693003/diff/80001/base/allocator/winheap_... base/allocator/winheap_stubs_win.cc:66: #error "Exceptions in allocator shim are not supported!" nit: I'd do #if !defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS #error ... #end then you don't need a second branch
On Thu, Jul 14, 2016 at 3:13 PM, <primiano@chromium.org> wrote: > On 2016/07/13 17:42:49, Primiano Tucci wrote: > > LGTM with minor comments. The only major comment is that GYP || > condition, > which > > seems a bit odd to me (See comments inline). Did you check that the > tests are > > actually running on the trybots (do we still have bots on gyp)? > > Thanks a lot for splitting the CL, this makes it way easier to review. > > > > Adding: > > +thakis to cover the rest of base (specifically for the GYP questions) > That line isn't in my gmail thread. Maybe it was written before I got added? > > +wfh as he wrote the original shim to see if he has any concern or can > spot > > something I missed. > > > > The context here is: siggi@ has volunteered to port the win_shim to the > unified > > allocator shim (and drop the old code once everything sticks), and I am > very > > enthusiastically supporting that. > > > > > > >I like the feature flag, but I wonder if it'd make sense to change the > name > to > > something relating to the allocator shim itself, like maybe: > > "ENABLE_WIN_ALLOCATOR_SHIM", and gate the > > tests on that? > > So I think that once everything sticks we can just use the existing > > USE_EXPERIMENTAL_ALLOCATOR_SHIM define (at some point we should drop the > > EXPERIMENTAL part of the name). > > In the meantime I think this makes easier to codesearch and to > understand what > > the define is about. (Not too strong on this though) > > > > https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn > > File base/BUILD.gn (right): > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/BUILD.gn#newcode1988 > > base/BUILD.gn:1988: "//base/allocator:features", > > Not sure this is required. tests depend on base which depends on > > allocator:features. You should get this automatically by virtue of > depending > on > > base. I suppose you are doing this not because it's required but more in > the > > spirit of Include What You Use. > > No idea what is tha pettern in GN, some base owner will comment, I'm > > indifferent. > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > > File base/allocator/allocator.gyp (right): > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > > base/allocator/allocator.gyp:391: > > > > 'ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', > > uhm not sure about this part. At very least this should be s/||/or/ > > But then I am not sure if the right way of spelling this is: > > 1) 'FLAG=<(a) or <(b)' > > 2) 'FLAG=<(a or b)' > > 3) you cannot do this and need to create first a variable two lines > above. > > > > You need a gyp-ninja for this :) > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > > File base/allocator/allocator_shim_win.cc (right): > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/allocato... > > base/allocator/allocator_shim_win.cc:17: #include > > "base/allocator/allocator_shim_win.h" > > maybe add a TODO saying that this file will be deleted once the unified > > allocator sticks. > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > > File base/allocator/winheap_stubs_win.cc (right): > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > > base/allocator/winheap_stubs_win.cc:54: void* WinHeapCalloc(size_t n, > size_t > > elem_size) { > > Not sure you need this here. This function is not used by the old shim > (and I > > think what you are doing is correct). But this suggests that in the next > CL > you > > can just put this body inside the dispatch_to_winheap.cc > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > > File base/allocator/winheap_stubs_win.h (right): > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > > base/allocator/winheap_stubs_win.h:1: // Copyright (c) 2012 The Chromium > > Authors. All rights reserved. > > nit: drop your wayback machine :P (also the new copyright header doesn't > have > > the (c) > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > > base/allocator/winheap_stubs_win.h:9: #ifndef > > BASE_ALLOCATOR_ALLOCATOR_WINHEAP_STUBS_H_ > > nit: s/ALLOCATOR_ALLOCATOR_/ALLOCATOR_/ > > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/allocator/winheap_... > > base/allocator/winheap_stubs_win.h:12: #include <windows.h> > > not sure you need this windows.h here. Looks like you really need > stdint.h > > > > https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp > > File base/base.gyp (right): > > > > > https://codereview.chromium.org/2143693003/diff/60001/base/base.gyp#newcode625 > > base/base.gyp:625: 'allocator/allocator.gyp:allocator_features#target', > > ditto about the doubt about IWYU. defer to thakis. > > > > https://codereview.chromium.org/2143693003/ > -- 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.
Quoting fail, lemme try again: On 2016/07/14 19:11:41, Nico wrote: > I had no idea you wanted me to look at this. Looking at the thread again, I > still have no idea. How was I supposed to know? (Before now, that is) My #4 was: """ ...SNIP... Adding: +thakis to cover the rest of base (specifically for the GYP questions) """ I suppose my "SNIP" part was too long and you didn't read the "+thakis" part?
On Thu, Jul 14, 2016 at 3:14 PM, <primiano@chromium.org> wrote: > Quoting fail, lemme try again: > > On 2016/07/14 19:11:41, Nico wrote: > > I had no idea you wanted me to look at this. Looking at the thread > again, I > > still have no idea. How was I supposed to know? (Before now, that is) > > > My #4 was: > """ > ...SNIP... > Adding: > +thakis to cover the rest of base (specifically for the GYP questions) > """ > > I suppose my "SNIP" part was too long and you didn't read the "+thakis" > part? > No, that's not in my gmail. The thread starts with siggi's "Thanks guys addressed comments - another look?" email for me. -- 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.
Demo: http://imgur.com/dZuvfXt Please repeat the questions you wanted me to answer, probably easier than me trying to hunt them down in the review thread. On Thu, Jul 14, 2016 at 3:15 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, Jul 14, 2016 at 3:14 PM, <primiano@chromium.org> wrote: > >> Quoting fail, lemme try again: >> >> On 2016/07/14 19:11:41, Nico wrote: >> > I had no idea you wanted me to look at this. Looking at the thread >> again, I >> > still have no idea. How was I supposed to know? (Before now, that is) >> >> >> My #4 was: >> """ >> ...SNIP... >> Adding: >> +thakis to cover the rest of base (specifically for the GYP questions) >> """ >> >> I suppose my "SNIP" part was too long and you didn't read the "+thakis" >> part? >> > > No, that's not in my gmail. The thread starts with siggi's "Thanks guys > addressed comments - another look?" email for me. > > -- 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.
On 2016/07/14 19:15:49, Nico wrote: > No, that's not in my gmail. The thread starts with siggi's "Thanks guys > addressed comments - another look?" email for me. Ok by looking at the hidden generated messages it was my fail. Apparently I wrote +thakis but actually forgot to add you, and siggi added you only later in #9. Can't you just watch chromium-reviews. It would just make our lives easier. I am pretty sure delta betwen that and your inbox is marginal :P
still lgtm from me but I'd like some assurance that you've tested (or changed) the crash bucketing so all our OOMs still report as [Out Of Memory] in crash/
Sorry, my bad. Figured something had borked as we're way outside your normal ETA :). I'm leaving early, so can't dig from here (on phone) but this related to deps for build feature flags in the .gyp and .gn. I added deps for use of the generated (allocator_features.h?) header in the base_unittests target. The q is where that's necessary and/or a good/bad/indifferent idea. On Thu, Jul 14, 2016, 15:11 <thakis@chromium.org> wrote: > I had no idea you wanted me to look at this. Looking at the thread again, I > still have no idea. How was I supposed to know? (Before now, that is) > > https://codereview.chromium.org/2143693003/ > -- 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.
I think that's cool. On Thu, Jul 14, 2016 at 3:42 PM, Sigurður Ásgeirsson <siggi@chromium.org> wrote: > Sorry, my bad. Figured something had borked as we're way outside your > normal ETA :). > I'm leaving early, so can't dig from here (on phone) but this related to > deps for build feature flags in the .gyp and .gn. > I added deps for use of the generated (allocator_features.h?) header in > the base_unittests target. The q is where that's necessary and/or a > good/bad/indifferent idea. > > > On Thu, Jul 14, 2016, 15:11 <thakis@chromium.org> wrote: > >> I had no idea you wanted me to look at this. Looking at the thread again, >> I >> still have no idea. How was I supposed to know? (Before now, that is) >> >> https://codereview.chromium.org/2143693003/ >> > -- 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.
Ah also my other question was, in allocator.gyp ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', Not sure about that ||, but siggi@ confirmed it seems to work.
On 2016/07/14 20:06:01, Primiano Tucci wrote: > Ah also my other question was, in allocator.gyp > > ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', > > Not sure about that ||, but siggi@ confirmed it seems to work. Haha :-) What I'm guessing is happening here is that gyp replaces the two <(...) with 0 or 1, and then the symbol gets defined as "0||1", and then the C preprocessor evaluates that at compile time. That's a bit questionable, I suppose (using a gyp conditional might be a bit clearer)
Yup, that's what comes out the other end. So define another gyp variable is the way to go? On Thu, Jul 14, 2016, 16:09 <thakis@chromium.org> wrote: > On 2016/07/14 20:06:01, Primiano Tucci wrote: > > Ah also my other question was, in allocator.gyp > > > > > > ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', > > > > Not sure about that ||, but siggi@ confirmed it seems to work. > > Haha :-) What I'm guessing is happening here is that gyp replaces the two > <(...) > with 0 or 1, and then the symbol gets defined as "0||1", and then the C > preprocessor evaluates that at compile time. That's a bit questionable, I > suppose (using a gyp conditional might be a bit clearer) > > https://codereview.chromium.org/2143693003/ > -- 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.
I think you don't need another variable, you can do something like
'buildflag_flags': [
...existing flag...
],
'conditions': {
['<(use_experimental_allocator_shim) or (win_use_allocator_shim)', {
'buildflag_flags': [ 'ENABLE_WIN_ALLOCATOR_SHIM_TESTS=1' ],
}, {
'buildflag_flags': [ 'ENABLE_WIN_ALLOCATOR_SHIM_TESTS=0' ],
}],
},
(untested) But if that doesn't work with light massaging, I think what you
have is fine too, especially given that gyp's not long for this world.
On Thu, Jul 14, 2016 at 4:11 PM, Sigurður Ásgeirsson <siggi@chromium.org>
wrote:
> Yup, that's what comes out the other end. So define another gyp variable
> is the way to go?
>
> On Thu, Jul 14, 2016, 16:09 <thakis@chromium.org> wrote:
>
>> On 2016/07/14 20:06:01, Primiano Tucci wrote:
>> > Ah also my other question was, in allocator.gyp
>> >
>> >
>>
>>
ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)',
>> >
>> > Not sure about that ||, but siggi@ confirmed it seems to work.
>>
>> Haha :-) What I'm guessing is happening here is that gyp replaces the two
>> <(...)
>> with 0 or 1, and then the symbol gets defined as "0||1", and then the C
>> preprocessor evaluates that at compile time. That's a bit questionable, I
>> suppose (using a gyp conditional might be a bit clearer)
>>
>> https://codereview.chromium.org/2143693003/
>>
>
--
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.
On Thu, Jul 14, 2016, 21:09 <thakis@chromium.org> wrote: > On 2016/07/14 20:06:01, Primiano Tucci wrote: > > Ah also my other question was, in allocator.gyp > > > > > > ENABLE_WIN_ALLOCATOR_SHIM_TESTS=<(use_experimental_allocator_shim)||<(win_use_allocator_shim)', > > > > Not sure about that ||, but siggi@ confirmed it seems to work. > > Haha :-) What I'm guessing is happening here is that gyp replaces the two > <(...) > with 0 or 1, and then the symbol gets defined as "0||1", and then the C > preprocessor evaluates that at compile time. > ^@@^ all these stages of preprocessing, expansion and evaluation frighten and confuse me. but now I think to how many monstrous hacks like this I could have achieved. I am discovering all this too late. shame. :D That's a bit questionable, > I > suppose (using a gyp conditional might be a bit clearer) > > https://codereview.chromium.org/2143693003/ > -- 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.
Thanks, guys. I think I've tested this one as far as I can locally. https://codereview.chromium.org/2143693003/diff/80001/base/allocator/winheap_... File base/allocator/winheap_stubs_win.cc (right): https://codereview.chromium.org/2143693003/diff/80001/base/allocator/winheap_... base/allocator/winheap_stubs_win.cc:66: #error "Exceptions in allocator shim are not supported!" On 2016/07/14 19:13:33, Nico wrote: > nit: I'd do > > #if !defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS > #error ... > #end > > then you don't need a second branch Done. https://codereview.chromium.org/2143693003/diff/100001/base/allocator/allocat... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/2143693003/diff/100001/base/allocator/allocat... base/allocator/allocator.gyp:392: 'conditions': [ This is a little less eye-bleed ugly - thanks! I've verified that this turns on the OOM unittests under GYP on non-component builds.
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, thakis@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2143693003/#ps100001 (title: "Address Nico's comments.")
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 ========== Start refactoring Windows allocator shim in prep for hooking in generic allocator shim on Windows. BUG=550886 ========== to ========== Start refactoring Windows allocator shim in prep for hooking in generic allocator shim on Windows. BUG=550886 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Start refactoring Windows allocator shim in prep for hooking in generic allocator shim on Windows. BUG=550886 ========== to ========== Start refactoring Windows allocator shim in prep for hooking in generic allocator shim on Windows. BUG=550886 Committed: https://crrev.com/25da2dbf685abbc6ee87f1bd527ddf93e298f3c4 Cr-Commit-Position: refs/heads/master@{#405984} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/25da2dbf685abbc6ee87f1bd527ddf93e298f3c4 Cr-Commit-Position: refs/heads/master@{#405984} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
