[PATCH] Allows for overriding system allocator on Windows

[GoogleCodeExporter]

I have implemented the missing support for setting the system allocator on 
Windows. This works the same way as in case of Linux and OS X, i.e. either by 
overriding the weakly defined tc_get_sysalloc_override function symbol or 
through MallocExtension::SetSystemAllocator method. The implementation closely 
follows the existing implementation in system-alloc.cc.

One of the consequences is that it is now possible to run system-alloc_unittest 
on Windows as well and I have added a Visual Studio project for that purpose. 
However, to make the project compilable, I had to inline the SysAllocator 
interface destructor definition as the original version didn't work when 
tcmalloc_minimal was built as DLL. This change shouldn't affect the existing 
code though.

Original issue reported on code.google.com by pho...@chromium.org on 25 Aug 2013 at 12:29

Attachments:

• tcmalloc_override.patch
• system-alloc_unittest-vcproj.patch
• sysallocator_destructor_inline.patch

[GoogleCodeExporter]

I'm not slow with review. Just wondering if we can do better with 
dll_export-ing entire class. Which is afaik standard way to make sure 
constructors/destructors are available for inheritance.

Original comment by alkondratenko on 14 Sep 2013 at 8:36

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Do you mean the SysAllocator class?

Original comment by pho...@chromium.org on 14 Sep 2013 at 9:40

[GoogleCodeExporter]

Yes. I've yet to double check it. But my memory from past experience suggests 
that it might be a better way. It might also be needed for switch to 
-fvisibility=hidden for GNU/Linux as well (pending double checking too).

Original comment by alkondratenko on 14 Sep 2013 at 9:43

[GoogleCodeExporter]

Shall I prepare the patch or are you already working on this?

Original comment by pho...@chromium.org on 14 Sep 2013 at 11:40

[GoogleCodeExporter]

I'm not working on this yet. If you can update patch that would be awesome.

Original comment by alkondratenko on 14 Sep 2013 at 11:41

[GoogleCodeExporter]

Attached; the patch is a replacement for the 
sysallocator_destructor_inline.patch, is this what you had in mind?

Original comment by pho...@chromium.org on 15 Sep 2013 at 12:55

Attachments:

• sysallocator_export.patch

[GoogleCodeExporter]

Yes. I hope it actually works

Original comment by alkondratenko on 15 Sep 2013 at 1:17

[GoogleCodeExporter]

I have tested it on my machine and it seems to be working.

Original comment by pho...@chromium.org on 15 Sep 2013 at 1:23

[GoogleCodeExporter]

Looks good. Currently fails on mingw. Due to a) that MSVC specific aliasing 
thing. Which appears easy to fix. b) Makefile issue(s). port.cc apparently gets 
linked to low_level_alloc-unittest and therefore brings with it dependency on 
~SysAllocator.

I'll take a look at this later.

Thanks for contribution.

Original comment by alkondratenko on 15 Sep 2013 at 2:12

[GoogleCodeExporter]

Original comment by alkondratenko on 15 Sep 2013 at 2:13

Attachments:

• wip-mingw-fix.patch

[GoogleCodeExporter]

And indeed low_level_alloc unittest fails to build on MSVC as well.

Original comment by alkondratenko on 15 Sep 2013 at 2:16

[GoogleCodeExporter]

perhaps we should indeed make ~SysAllocator inline. Let me think a bit more 
about that

Original comment by alkondratenko on 15 Sep 2013 at 2:19

[GoogleCodeExporter]

Let me know if you need any help with additional changes.

Original comment by pho...@chromium.org on 15 Sep 2013 at 7:09

[GoogleCodeExporter]

Any news on this? We would love to get this pushed through as soon as possible.

Original comment by pho...@chromium.org on 16 Sep 2013 at 5:12

[GoogleCodeExporter]

I'm generally only spending time on gperftools on Saturdays. Other days are 
spent on work and family.

Right now I'm thinking between two options:

a) inline SysAllocator destructor and not dll-export entire class

b) move windows-specific sys-allocator support out of port.cc, so that tests 
that need only what's already in port.cc do not have to depend on SysAllocator

Option (a) seems easier, but I'm not sure I'm able to see all consequences of 
doing it this way. What would you recommend ?

Original comment by alkondratenko on 16 Sep 2013 at 6:04

[GoogleCodeExporter]

The option (a) is definitely easier, but option (b) might be somewhat cleaner. 
I could try to split the SysAllocator related functionality to 
windows/system-alloc.cc akin to Linux/BSD version if you prefer.

Original comment by pho...@chromium.org on 16 Sep 2013 at 10:28

[GoogleCodeExporter]

Yes, please

Original comment by alkondratenko on 17 Sep 2013 at 12:20

[GoogleCodeExporter]

Here is the patch, I have tested it both in Visual Studio and Cygwin, but could 
you please try whether it works on your configuration as well?

Original comment by pho...@chromium.org on 17 Sep 2013 at 10:23

Attachments:

• system-alloc.cc.patch

[GoogleCodeExporter]

Of course I will. Thanks a lot.

Original comment by alkondratenko on 17 Sep 2013 at 10:27

[GoogleCodeExporter]

Somehow with all this changes most tests fail when built with mingw. I'll take 
a look this Saturday.

Original comment by alkondratenko on 18 Sep 2013 at 3:09

[GoogleCodeExporter]

What is your build configuration you're using for testing this? I've been 
trying to compile this using MinGW under MSYS and the compilation fails in some 
of the core classes even on master branch (Windows 7 x64). I managed to compile 
the everything under Cygwin using MinGW compilers, but all the tests are 
failing. However, this is also the case for the master, so it doesn't seem like 
these patches have broken anything which haven't been already broken, but maybe 
it's just my configuration.

Original comment by pho...@chromium.org on 18 Sep 2013 at 9:34

[GoogleCodeExporter]

Here's what I just did (on Debian GNU/Linux unstable with mingw64 packages 
installed):

 6634  ./autogen.sh 
 6635  ./configure --prefix ~/tcm-mingw --host=i686-w64-mingw32
 6636  make -j8
 6637  cd .libs/
 6638  cp /usr/lib/gcc/i686-w64-mingw32/4.6/*.dll .
 6639  cp *.exe *.dll /home/incoming//tt

(this is excerpt from history command output)

Then on windows VM I copied all tt directory and run 
.\tcmalloc_minimal_unittest.exe

With patches from this issue I'm getting crash. Without, test passes.

And I'm seeing exactly same situation with x86_64 target (--host 
x86_64-w64-mingw32).

This is obviously very weird given quite straightforwardly small surface of 
your changes. It is entirely possible that it merely uncovered bug that was 
already there.

I'll take a close look on Saturday.

Original comment by alkondratenko on 19 Sep 2013 at 3:22

[GoogleCodeExporter]

It turned out mingw was getting crazy on ATTRIBUTE_WEAK. Instead of calling 
tc_sys_alloc_override it was calling previous function (::Alloc) and 
predictably failing.

So for now we will not have system allocator overriding on mingw. Until we find 
a reliable way to make this "weak symbol" trick work on windows via mingw.

Will merge in few minutes. Thanks again.

Original comment by alkondratenko on 21 Sep 2013 at 3:50

[GoogleCodeExporter]

Original comment by alkondratenko on 21 Sep 2013 at 4:34

• Changed state: Fixed