Remove tcmalloc when not being used. Restore shim on Windows.

Restore the allocator shim on Windows.  Add 2Gb limit on allocations via the shim.  Remove tcmalloc from Windows.

Restore working tcmalloc_unittests on Linux and allocator_unittests on other platforms.  Add Death tests to base_unittests SecurityTests.  Fix prep_libc.py to actually strip the objects correctly.

BUG=169327, 434397
TEST=base_unittests --gtest_filter=SecurityTest.*
TEST=allocator_unittests, tcmalloc_unittest

Committed: https://crrev.com/74a449cac1340e85b3ef06be59a91fdd29110454
Cr-Commit-Position: refs/heads/master@{#311194}

[Will Harris, 2014-12-10 16:58:42]

wfh@chromium.org changed reviewers:
+ cpu@chromium.org, jschuh@chromium.org

[Will Harris, 2014-12-10 16:59:27]

We shouldn't drop the 2Gb memory limits, so adding the shim back.  Need to have
a think about which allocator to use on which platforms.

WDYT?

[cpu_(ooo_6.6-7.5), 2014-12-11 20:20:34]

The issue with tcmalloc is that it eat the address space which affects XP as
well.

While there were some regressions when I switched to winheap on XP, there are
not all over the place, just a couple of places and the improvements seem more
global.

I also landed a LFH change for XP later (see bug) but my spider sense is that it
is not working.

One thing we could do is enforce the memory limits via the job object, assuming
that we are only afraid of the internet exposed processes, which I assume you
are.

[cpu_(ooo_6.6-7.5), 2014-12-11 20:21:50]

ah never mind, it is not a global limit, its a 2GB per allocation (!).

[Will Harris, 2014-12-11 20:24:32]

On 2014/12/11 20:21:50, cpu wrote:
> ah never mind, it is not a global limit, its a 2GB per allocation (!).

to be honest I'm happy to make the default WINLFH on all platforms, but I don't
think we want to lose the 2Gb per-allocation limit on 64-bit platforms, and the
shim seems like the best way of doing that.  we didn't get any performance
regressions when moving from tcmalloc to winlfh?

[cpu_(ooo_6.6-7.5), 2014-12-11 20:27:34]

and there is one more thing this changes.

As is, the heap is the process heap, the MS CRT for 2012 and onwards does not
create its own heap, this opens the door to splitting more things into dlls
since all the crts regardless of module use the same heap.

[Will Harris, 2014-12-13 06:50:23]

On 2014/12/11 20:27:34, cpu wrote:
> and there is one more thing this changes.
> 
> As is, the heap is the process heap, the MS CRT for 2012 and onwards does not
> create its own heap, this opens the door to splitting more things into dlls
> since all the crts regardless of module use the same heap.

So shall I remove the OS version check and just leave the shim and the 2Gb
limit?  Behavior then should be identical to the shim not being there except we
keep the current security benefits.

[jschuh, 2014-12-15 14:36:06]

Just to clarify, to me it looks like the change here is using WINHEAP but just
keeping our 2GB limit on the max allocation. So, as long as you change it to use
WINHEAP everywhere cpu@ should be happy?

[cpu_(ooo_6.6-7.5), 2014-12-15 23:17:03]

we talked about f2f. wfh will investigate if we can have our cake and eat it
too.

[Will Harris, 2014-12-16 22:22:59]

On 2014/12/15 23:17:03, cpu wrote:
> we talked about f2f. wfh will investigate if we can have our cake and eat it
> too.

I spoke to others about this.  I think it would be a security regression to lose
the 2Gb limit. We have a lot of 3rd party code that uses signed ints for lengths
and allowing allocations > 2Gb might open us to security vulnerabilities.  I
think we have to keep the shim.

I've changed the default heap to WINLFH which tries to enable the LFH (only
really needed on XP).  Also, added HEAP_GENERATE_EXCEPTIONS so that OOM causes
an exception similar to the current behavior of tcmalloc and partitionalloc.

PTAL

[cpu_(ooo_6.6-7.5), 2014-12-17 22:32:21]

alright, I won't be that guy who increases the risk of pwnage, although I think
that is very small.

I could not make the 'load options' PE trick work and at the same time keep the
LFH. One or the other work well, but both, no dice. I'll raise the issue with
Microsoft. This should be fixable at the CRT level.

So fine you can have the shim back. However, I want a very simple shim, not at
all the circus that the shim is today.

<puts on skimask>
<activates voice scrambler>

Here is my list of demands
1- off with all the spurious helpers, env vars, 'todos' and BS
2- don't create a new heap, use the crt heap as seen in the sample code. Set the
LFH there.
3- the shim does one thing and one thing only, the 2GB limit.


Hopefully what we need fits in one 50 line file.

[jschuh, 2014-12-18 17:30:03]

I am comfortable with your demands.

[Will Harris, 2014-12-18 17:41:48]

On 2014/12/18 17:30:03, jschuh wrote:
> I am comfortable with your demands.

Indeed, after a tough round of negotiation, I folded to all of cpu's demands. 
I'll update the CL.  Completely ripping tcmalloc out of the Windows build
actually sounds like fun.

[Will Harris, 2015-01-07 07:34:28]

Patchset #3 (id:40001) has been deleted

[Will Harris, 2015-01-07 07:37:16]

WIP CL.

Tcmalloc code should be now gone on Windows.  Added some new death tests.
This doesn't yet work on Windows debug.  Also, I very much doubt this will
compile on Linux yet.

scottmg@ can you check my gyp?

This needs cpu's CL - https://codereview.chromium.org/836733006/ to crash on
malloc in Chrome code.

[cpu_(ooo_6.6-7.5), 2015-01-07 17:25:26]

https://codereview.chromium.org/774683003/diff/60001/base/allocator/win_alloc...
File base/allocator/win_allocator.cc (right):

https://codereview.chromium.org/774683003/diff/60001/base/allocator/win_alloc...
base/allocator/win_allocator.cc:15: win_heap =
static_cast<HANDLE>(GetProcessHeap());
use http://msdn.microsoft.com/en-us/library/csd157zx.aspx ?

so you don't need that global.

also if we keep the LFH setting here we need to remove the one I added.

https://codereview.chromium.org/774683003/diff/60001/base/allocator/win_alloc...
base/allocator/win_allocator.cc:46: if (size < kMaxWindowsAllocation)
if (size > kMaxWindowsAllocation)
  return NULL;

return HeapRealloc(..);

[cpu_(ooo_6.6-7.5), 2015-01-07 17:28:42]

https://codereview.chromium.org/774683003/diff/60001/base/allocator/allocator...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/774683003/diff/60001/base/allocator/allocator...
base/allocator/allocator_shim.cc:19: static int new_mode = 0;
I don't see anybody setting new_mode

[Will Harris, 2015-01-07 17:29:37]

working on fixing up Linux now.  Finding a lot of issues so far :(

https://codereview.chromium.org/774683003/diff/60001/base/allocator/win_alloc...
File base/allocator/win_allocator.cc (right):

https://codereview.chromium.org/774683003/diff/60001/base/allocator/win_alloc...
base/allocator/win_allocator.cc:15: win_heap =
static_cast<HANDLE>(GetProcessHeap());
On 2015/01/07 17:25:26, cpu wrote:
> use http://msdn.microsoft.com/en-us/library/csd157zx.aspx ?
> 
> so you don't need that global.
> 
> also if we keep the LFH setting here we need to remove the one I added.

yes I was going to use _get_heap_handle, but for some reason this is
re-implemented in the shim

https://code.google.com/p/chromium/codesearch#chromium/src/base/allocator/all...

I checked the CRT source and _get_heap_handle just calls down into
GetProcessHeap().

[Will Harris, 2015-01-07 17:32:47]

https://codereview.chromium.org/774683003/diff/60001/base/allocator/allocator...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/774683003/diff/60001/base/allocator/allocator...
base/allocator/allocator_shim.cc:19: static int new_mode = 0;
On 2015/01/07 17:28:42, cpu wrote:
> I don't see anybody setting new_mode

new_mode global is removed from libcmt in prep_libc.py

https://code.google.com/p/chromium/codesearch#chromium/src/base/allocator/pre...

and this new_mode static replaces it by link-fu, so it's actually accessed from
the CRT _set_new_mode function, called from your CL.

[cpu_(ooo_6.6-7.5), 2015-01-08 18:05:43]

https://codereview.chromium.org/774683003/diff/140001/base/allocator/allocato...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/774683003/diff/140001/base/allocator/allocato...
base/allocator/allocator_shim.cc:24: #include "win_allocator.cc"
shouldn't we simply copy that code over here?

https://codereview.chromium.org/774683003/diff/140001/base/allocator/allocato...
base/allocator/allocator_shim.cc:142: DCHECK_EQ(alignment & (alignment - 1),
0U);
I don't think we can use logging at all here because it most certainly will try
to allocate memory from us.

We are logically below logging.

https://codereview.chromium.org/774683003/diff/140001/base/allocator/win_allo...
File base/allocator/win_allocator.cc (right):

https://codereview.chromium.org/774683003/diff/140001/base/allocator/win_allo...
base/allocator/win_allocator.cc:65: DCHECK_LT(alignment, allocation_size);
same deal with all these logging stuff.

[Will Harris, 2015-01-08 22:10:57]

now just need to fix up the GN parts.

https://codereview.chromium.org/774683003/diff/140001/base/allocator/allocato...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/774683003/diff/140001/base/allocator/allocato...
base/allocator/allocator_shim.cc:24: #include "win_allocator.cc"
On 2015/01/08 18:05:43, cpu wrote:
> shouldn't we simply copy that code over here?

Done.

https://codereview.chromium.org/774683003/diff/140001/base/allocator/allocato...
base/allocator/allocator_shim.cc:142: DCHECK_EQ(alignment & (alignment - 1),
0U);
On 2015/01/08 18:05:43, cpu wrote:
> I don't think we can use logging at all here because it most certainly will
try
> to allocate memory from us.
> 
> We are logically below logging.

Done.  In fact, allocator can't depend on base anyway because of circular gyp
dependencies, so this dropped out when I removed the base dependency.

https://codereview.chromium.org/774683003/diff/140001/base/allocator/win_allo...
File base/allocator/win_allocator.cc (right):

https://codereview.chromium.org/774683003/diff/140001/base/allocator/win_allo...
base/allocator/win_allocator.cc:65: DCHECK_LT(alignment, allocation_size);
On 2015/01/08 18:05:43, cpu wrote:
> same deal with all these logging stuff.

Done.

[cpu_(ooo_6.6-7.5), 2015-01-08 22:38:45]

now what is the disadvantage of using _get_heap_handle() ?

the way I see it, if the crt changes its heap in 2017 we don't have to screw
with this again.

[cpu_(ooo_6.6-7.5), 2015-01-08 22:40:28]

also note that there is no "D" on the left of /win_allocator.cc like there is
one for allocator_shim.h

[Will Harris, 2015-01-09 01:16:35]

PTAL (Chrome ate my comment)

As discussed we need to implement _get_heap_handle() ourselves since we strip
heapinit.obj from the C runtime.

It also turns out we were only stripping C++ symbols, and only on x86 (!!) so
I'm not entirely sure how the shim was working before, I think it was luck of
the link ordering, and the fact that we always completely implemented the
exports for each of the obj files so they were overridden by the linker.  I've
fixed prep_libc.py and now it runs faster (no more brute force) and also will
error if it can't strip any symbols (e.g. the CRT changes).

[Will Harris, 2015-01-09 01:25:05]

Patchset #11 (id:220001) has been deleted

[scottmg, 2015-01-09 23:32:47]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-01-09 23:32:48]

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
File base/allocator/allocator_shim_win.cc (right):

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:11: //#include
"base/allocator/allocator_extension_thunks.h"
remove these?

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:30: std::numeric_limits<int>::max() -
kWindowsPageSize;
less confusing to delete this and leave the initialization only in win_heap_init

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:188: #ifdef WIN32
remove this #if since we're in a _win.cc.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
File base/allocator/prep_libc.py (right):

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
base/allocator/prep_libc.py:7: # This script takes libcmt.lib for
VS2005/08/10/12/13 and removes the allocation
Update comment, this only supports VS2013 now, right?

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
base/allocator/prep_libc.py:60: (cvspath, obj, output_lib))
nit; same indent as 'lib

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
base/allocator/prep_libc.py:64: (cppvspath, obj, output_lib))
ditto

https://codereview.chromium.org/774683003/diff/200001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/774683003/diff/200001/base/security_unittest....
base/security_unittest.cc:61: #if (!defined(NO_TCMALLOC) ||
defined(ALLOCATOR_SHIM)) &&                     \
Do we have to have another define? i.e. is there still a reason to keep
win_use_allocator_shim% and ALLOCATOR_SHIM, or could this just be OS_WIN? It
seems like these variables are a bit confusing as is.

[cpu_(ooo_6.6-7.5), 2015-01-09 23:46:56]

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
File base/allocator/allocator_shim_win.cc (right):

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:11: //#include
"base/allocator/allocator_extension_thunks.h"
remove commented includes.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:13: //#include
"base/profiler/alternate_timer.h"
ditto

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:26: extern "C" {
I don't think we need extern here, since these guys are not exposed.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:33: bool win_heap_init() {
add comment, along the lines
// VS2013 crt uses the process heap as its heap, so we do the same here.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:47: kMaxWindowsAllocation =
std::numeric_limits<int>::max() - kWindowsPageSize;
I think if define kMaxWindowAllocation without ...max() and instead as just
kMaxWindowAllocation = number

Then it can be const and we don't need line 47

of course we need ifdef() for different numbers for 32 and 64.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:113: #if (defined(__GNUC__) &&
!defined(__EXCEPTIONS)) || \
kill _GNUC_

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:137: #endif  // (defined(__GNUC__) &&
!defined(__EXCEPTIONS)) || (defined(_HAS_EXCEPTIONS) && !_HAS_EXCEPTIONS)
instead of this monkeing, we should just #error if __EXCEPTIONS or the other one
are defined

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:182: // TODO(mbelshe): Implement this for
other allocators.
I think this can be deleted.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:246: #endif  // WIN32
kill this

[Will Harris, 2015-01-11 06:30:40]

PTAL.

Also, added a new test to exhaust memory and check for death.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
File base/allocator/allocator_shim_win.cc (right):

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:11: //#include
"base/allocator/allocator_extension_thunks.h"
On 2015/01/09 23:32:48, scottmg wrote:
> remove these?

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:11: //#include
"base/allocator/allocator_extension_thunks.h"
On 2015/01/09 23:46:56, cpu wrote:
> remove commented includes.

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:13: //#include
"base/profiler/alternate_timer.h"
On 2015/01/09 23:46:56, cpu wrote:
> ditto

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:26: extern "C" {
On 2015/01/09 23:46:56, cpu wrote:
> I don't think we need extern here, since these guys are not exposed.

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:30: std::numeric_limits<int>::max() -
kWindowsPageSize;
On 2015/01/09 23:32:48, scottmg wrote:
> less confusing to delete this and leave the initialization only in
win_heap_init

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:33: bool win_heap_init() {
On 2015/01/09 23:46:56, cpu wrote:
> add comment, along the lines
> // VS2013 crt uses the process heap as its heap, so we do the same here.

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:47: kMaxWindowsAllocation =
std::numeric_limits<int>::max() - kWindowsPageSize;
On 2015/01/09 23:46:56, cpu wrote:
> I think if define kMaxWindowAllocation without ...max() and instead as just
> kMaxWindowAllocation = number
> 
> Then it can be const and we don't need line 47
> 
> of course we need ifdef() for different numbers for 32 and 64.

Done.  The allocation limit is the same on both 32 and 64.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:113: #if (defined(__GNUC__) &&
!defined(__EXCEPTIONS)) || \
On 2015/01/09 23:46:56, cpu wrote:
> kill _GNUC_

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:182: // TODO(mbelshe): Implement this for
other allocators.
On 2015/01/09 23:46:56, cpu wrote:
> I think this can be deleted.

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:188: #ifdef WIN32
On 2015/01/09 23:32:48, scottmg wrote:
> remove this #if since we're in a _win.cc.

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:246: #endif  // WIN32
On 2015/01/09 23:46:56, cpu wrote:
> kill this

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
File base/allocator/prep_libc.py (right):

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
base/allocator/prep_libc.py:7: # This script takes libcmt.lib for
VS2005/08/10/12/13 and removes the allocation
On 2015/01/09 23:32:48, scottmg wrote:
> Update comment, this only supports VS2013 now, right?

yes, only VS2013 now.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
base/allocator/prep_libc.py:60: (cvspath, obj, output_lib))
On 2015/01/09 23:32:48, scottmg wrote:
> nit; same indent as 'lib

Done.

https://codereview.chromium.org/774683003/diff/200001/base/allocator/prep_lib...
base/allocator/prep_libc.py:64: (cppvspath, obj, output_lib))
On 2015/01/09 23:32:48, scottmg wrote:
> ditto

Done.

https://codereview.chromium.org/774683003/diff/200001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/774683003/diff/200001/base/security_unittest....
base/security_unittest.cc:61: #if (!defined(NO_TCMALLOC) ||
defined(ALLOCATOR_SHIM)) &&                     \
On 2015/01/09 23:32:48, scottmg wrote:
> Do we have to have another define? i.e. is there still a reason to keep
> win_use_allocator_shim% and ALLOCATOR_SHIM, or could this just be OS_WIN? It
> seems like these variables are a bit confusing as is.

Allocator shim isn't enabled for shared_library builds so the logic written in
gyp variables needs to be something like "if (use_allocator=='tcmalloc' or
(os=='win' and component=='shared_library'))" - if there are other #defines I
can use I'll use them but I couldn't see a way to achieve this without adding a
new one.

[scottmg, 2015-01-12 16:51:02]

lgtm

https://codereview.chromium.org/774683003/diff/240001/base/allocator/allocato...
File base/allocator/allocator_shim_win.cc (right):

https://codereview.chromium.org/774683003/diff/240001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:117: #else
Could probably just remove the _HAS_EXCEPTIONS arm, since I don't think any of
chrome is exception-safe anyway, so _HAS_EXCEPTIONS=1 is not imminent.

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest....
base/security_unittest.cc:40: void exhaustMemory() {
nit; exhaustMemory -> ExhaustMemory

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest....
base/security_unittest.cc:128: if (!IsTcMallocBypassed()) {
Remove the IsTcMallocBypassed's for OS_WIN? (and below)

(It seems odd that that returns false on Windows too -- wouldn't it always be
bypassed?)

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest....
base/security_unittest.cc:134:
HideValueFromCompiler(malloc(kTooBigAllocSize)))),
Maybe a similar test that uses realloc instead of malloc since that code path is
a bit strange.

[Will Harris, 2015-01-12 19:03:44]

scottmg@ comments addressed

PTAL

https://codereview.chromium.org/774683003/diff/240001/base/allocator/allocato...
File base/allocator/allocator_shim_win.cc (right):

https://codereview.chromium.org/774683003/diff/240001/base/allocator/allocato...
base/allocator/allocator_shim_win.cc:117: #else
On 2015/01/12 16:51:01, scottmg wrote:
> Could probably just remove the _HAS_EXCEPTIONS arm, since I don't think any of
> chrome is exception-safe anyway, so _HAS_EXCEPTIONS=1 is not imminent.

Done.  Added an #error like cpu suggested.

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest....
base/security_unittest.cc:40: void exhaustMemory() {
On 2015/01/12 16:51:01, scottmg wrote:
> nit; exhaustMemory -> ExhaustMemory

Done.

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest....
base/security_unittest.cc:128: if (!IsTcMallocBypassed()) {
On 2015/01/12 16:51:01, scottmg wrote:
> Remove the IsTcMallocBypassed's for OS_WIN? (and below)
> 
> (It seems odd that that returns false on Windows too -- wouldn't it always be
> bypassed?)

Done.

https://codereview.chromium.org/774683003/diff/240001/base/security_unittest....
base/security_unittest.cc:134:
HideValueFromCompiler(malloc(kTooBigAllocSize)))),
On 2015/01/12 16:51:01, scottmg wrote:
> Maybe a similar test that uses realloc instead of malloc since that code path
is
> a bit strange.

Done.

[cpu_(ooo_6.6-7.5), 2015-01-12 22:37:03]

lgtm

[Will Harris, 2015-01-13 01:05:22]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-01-13 01:06:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/774683003/260001

[commit-bot: I haz the power, 2015-01-13 03:12:25]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2015-01-13 03:13:06]

Patchset 12 (id:??) landed as
https://crrev.com/74a449cac1340e85b3ef06be59a91fdd29110454
Cr-Commit-Position: refs/heads/master@{#311194}

[Nico, 2015-01-14 03:36:40]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-01-14 03:36:40]

Where was this restored from? clang warns:

In file included from ..\..\base\allocator\allocator_shim_win.cc:219:
..\..\base\allocator/generic_allocators.cc(31,6) :  warning(clang): function
previously declared with an explicit exception specification redeclared with an
implicit exception specification [-Wimplicit-exception-spec-mismatch]
void operator delete(void* p) {
     ^
..\..\base\allocator/generic_allocators.cc(39,6) :  warning(clang): function
previously declared with an explicit exception specification redeclared with an
implicit exception specification [-Wimplicit-exception-spec-mismatch]
void operator delete[](void* p) {
     ^
..\..\base\allocator/generic_allocators.cc(47,6) :  warning(clang): function
previously declared with an explicit exception specification redeclared with an
implicit exception specification [-Wimplicit-exception-spec-mismatch]
void operator delete(void* p, const std::nothrow_t& nt) {
     ^
C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk\bin\..\..\VC\include\new.h(96,16)
:  note(clang): previous declaration is here
void __CRTDECL operator delete(void *, const std::nothrow_t&) throw();
               ^
In file included from ..\..\base\allocator\allocator_shim_win.cc:219:
..\..\base\allocator/generic_allocators.cc(55,6) :  warning(clang): function
previously declared with an explicit exception specification redeclared with an
implicit exception specification [-Wimplicit-exception-spec-mismatch]
void operator delete[](void* p, const std::nothrow_t& nt) {
     ^
C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk\bin\..\..\VC\include\new.h(97,16)
:  note(clang): previous declaration is here
void __CRTDECL operator delete[](void *, const std::nothrow_t&) throw();
               ^


I have vivid memories of fixing this exact thing before in Aug 2014, here:
https://codereview.chromium.org/419323002/
https://codereview.chromium.org/447513002/

[Will Harris, 2015-01-14 17:14:09]

On 2015/01/14 03:36:40, Nico wrote:
> Where was this restored from? clang warns:
> 
> In file included from ..\..\base\allocator\allocator_shim_win.cc:219:
> ..\..\base\allocator/generic_allocators.cc(31,6) :  warning(clang): function
> previously declared with an explicit exception specification redeclared with
an
> implicit exception specification [-Wimplicit-exception-spec-mismatch]
> void operator delete(void* p) {
>      ^
> ..\..\base\allocator/generic_allocators.cc(39,6) :  warning(clang): function
> previously declared with an explicit exception specification redeclared with
an
> implicit exception specification [-Wimplicit-exception-spec-mismatch]
> void operator delete[](void* p) {
>      ^
> ..\..\base\allocator/generic_allocators.cc(47,6) :  warning(clang): function
> previously declared with an explicit exception specification redeclared with
an
> implicit exception specification [-Wimplicit-exception-spec-mismatch]
> void operator delete(void* p, const std::nothrow_t& nt) {
>      ^
>
C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk\bin\..\..\VC\include\new.h(96,16)
> :  note(clang): previous declaration is here
> void __CRTDECL operator delete(void *, const std::nothrow_t&) throw();
>                ^
> In file included from ..\..\base\allocator\allocator_shim_win.cc:219:
> ..\..\base\allocator/generic_allocators.cc(55,6) :  warning(clang): function
> previously declared with an explicit exception specification redeclared with
an
> implicit exception specification [-Wimplicit-exception-spec-mismatch]
> void operator delete[](void* p, const std::nothrow_t& nt) {
>      ^
>
C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk\bin\..\..\VC\include\new.h(97,16)
> :  note(clang): previous declaration is here
> void __CRTDECL operator delete[](void *, const std::nothrow_t&) throw();
>                ^
> 
> 
> I have vivid memories of fixing this exact thing before in Aug 2014, here:
> https://codereview.chromium.org/419323002/
> https://codereview.chromium.org/447513002/

generic_allocators.cc was already there, but it's included into
allocator_shim_win.cc which now includes new.h, so it's possible we're now
re-declaring these functions without the throw().

This code wasn't being compiled until this CL, because cpu@ removed the shim
back in r305567.  I imagine re-adding the throw() will fix this.  I can take a
look when I'm back in the office, unless it's urgent then you're welcome to add
the throw().

[Nico, 2015-01-16 22:03:11]

On Wed, Jan 14, 2015 at 9:14 AM, <wfh@chromium.org> wrote:

> On 2015/01/14 03:36:40, Nico wrote:
>
>> Where was this restored from? clang warns:
>>
>
>  In file included from ..\..\base\allocator\allocator_shim_win.cc:219:
>> ..\..\base\allocator/generic_allocators.cc(31,6) :  warning(clang):
>> function
>> previously declared with an explicit exception specification redeclared
>> with
>>
> an
>
>> implicit exception specification [-Wimplicit-exception-spec-mismatch]
>> void operator delete(void* p) {
>>       ^
>> ..\..\base\allocator/generic_allocators.cc(39,6) :  warning(clang):
>> function
>> previously declared with an explicit exception specification redeclared
>> with
>>
> an
>
>> implicit exception specification [-Wimplicit-exception-spec-mismatch]
>> void operator delete[](void* p) {
>>       ^
>> ..\..\base\allocator/generic_allocators.cc(47,6) :  warning(clang):
>> function
>> previously declared with an explicit exception specification redeclared
>> with
>>
> an
>
>> implicit exception specification [-Wimplicit-exception-spec-mismatch]
>> void operator delete(void* p, const std::nothrow_t& nt) {
>>       ^
>>
>
> C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk\bin\..\..\VC\include\
> new.h(96,16)
>
>> :  note(clang): previous declaration is here
>> void __CRTDECL operator delete(void *, const std::nothrow_t&) throw();
>>                 ^
>> In file included from ..\..\base\allocator\allocator_shim_win.cc:219:
>> ..\..\base\allocator/generic_allocators.cc(55,6) :  warning(clang):
>> function
>> previously declared with an explicit exception specification redeclared
>> with
>>
> an
>
>> implicit exception specification [-Wimplicit-exception-spec-mismatch]
>> void operator delete[](void* p, const std::nothrow_t& nt) {
>>       ^
>>
>
> C:\b\depot_tools\win_toolchain\vs2013_files\win8sdk\bin\..\..\VC\include\
> new.h(97,16)
>
>> :  note(clang): previous declaration is here
>> void __CRTDECL operator delete[](void *, const std::nothrow_t&) throw();
>>                 ^
>>
>
>
>  I have vivid memories of fixing this exact thing before in Aug 2014, here:
>> https://codereview.chromium.org/419323002/
>> https://codereview.chromium.org/447513002/
>>
>
> generic_allocators.cc was already there, but it's included into
> allocator_shim_win.cc which now includes new.h, so it's possible we're now
> re-declaring these functions without the throw().
>
> This code wasn't being compiled until this CL, because cpu@ removed the
> shim
> back in r305567.  I imagine re-adding the throw() will fix this.  I can
> take a
> look when I'm back in the office,


Thanks, looking forward to it :-)


> unless it's urgent then you're welcome to add
> the throw().
>
> https://codereview.chromium.org/774683003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.