Some issues of HeapProfile in windows

[GoogleCodeExporter]

What steps will reproduce the problem?
1.download the latest release version.
2.open google-perftools.sln in VStudio9 
3.Add heap-profiler.cc into libtcmalloc_minimal.vcproj and build
4.Compile will report link error: HeapProfileTable::kMaxStackDepth is defined 
twice.
5. fixing the build and run one of the unit_test, the program will terminate 
due to WindowsInfo:Patch() occasionally fails.
6. fixing the issue in WindowsInfo:Patch(), and define HEAPPROFILE environment 
variable, the unit test will deadlock. see 
http://groups.google.com/group/google-perftools/browse_frm/thread/6d38023bee9733
35

What is the expected output? What do you see instead?
1 moving initialization of kMaxStackDepth to heap-profile-table.cc will fix 
build in VStudio 9.

2 When WindowsInfo:Patch() is called, function_info_ hasn't been initialized, 
this is a very strange issue. I fixd it by manually initialize it in 
WindowsInfo:Patch(). 

Please use labels and text to provide additional information.

Original issue reported on code.google.com by baibaic...@gmail.com on 2 Sep 2010 at 1:32

[GoogleCodeExporter]

sorry missing summary for deadlock in windows

I test it in Win7, in DumpProfileLocked, it will call RawOpenForWriting. 

Write_to_stderr is the first deadlock that i found,  before 
calling RawOpenForWriting, we will call RAW_VLOG(which eventually 
calls write to stderr) for the reason why we dump the profile. 

CreateFileA is the second deadlock. 

Not sure WriteFile, it can only be verfiied after CreateFile is 
avoided to allocate memory. 

Solution
1. write_to_ stderror may cause windows allocating memory if the output string 
size is greater than 80. I fixed by 
#define WRITE_TO_STDERR(buf, len) write(STDERR_FILENO, buf, len<=80? len:80); 
the better solution would be using loop instead of truncating.

2. we need calling NtCreateFile directly to avoiding allocating memmory!

Original comment by baibaic...@gmail.com on 2 Sep 2010 at 1:41

[GoogleCodeExporter]

Thanks for writing this up.  Feel free to add more issues as you see them.  
Best is to have a patch, eventually, that fixes up all the problems.

Original comment by csilv...@gmail.com on 3 Sep 2010 at 10:25

• Added labels: Priority-Medium, Type-Defect

[GoogleCodeExporter]

I can verify the CreateFile deadlock issue (VC9, WinXP).

Attached two patches - the first fixes the HeapProfileTable::kMaxStackDepth 
issue, and the second fixed the CreateFile deadlock issue by using NtCreateFile 
and friends.

Original comment by 5u1135t...@gmail.com on 24 Feb 2011 at 9:36

Attachments:

• heap-profile-table.cc-patch
• logging.cc-patch

[GoogleCodeExporter]

Another issue I discovered:

heap-profiler.cc declares

  static const TCMallocGuard tcmalloc_initializer;

which relies upon WindowsInfo::function_info_ (in windows/patch_functions.cc) 
being initialized first; however, this is not necessarily the case.

This patch uses the MSVC-specific #pragma init_seg directive to fix that.

Original comment by 5u1135t...@gmail.com on 24 Feb 2011 at 11:23

Attachments:

• init-order-patch

[GoogleCodeExporter]

This patch fixes a deadlock in abort().

Original comment by 5u1135t...@gmail.com on 25 Feb 2011 at 10:10

Attachments:

• abort-patch

[GoogleCodeExporter]

A new version of the NtCreateFile() patch. Turns out that it sometimes returns 
STATUS_PENDING.

This patch also makes RawOpenForWriting() work for absolute paths.

Original comment by 5u1135t...@gmail.com on 25 Feb 2011 at 1:33

Attachments:

• logging.cc-patch-2

[GoogleCodeExporter]

Thanks for the patches!  Before I can apply them, can you sign the CLA at
  http://code.google.com/legal/individual-cla-v1.0.html
?

As for the specifics of the patches:

-- heap-profile-table.cc patch:

I don't understand the reported error.  The language requires that we provide 
storage for this variable.  I don't see any other place we define the variable. 
 Where is it claiming the two definition are?

Oh, I bet it's complaining that we're initializing the var in the .h file 
rather than the .cc file.  As far as I understand it, this is legal c++, though 
it looks like the msvc compiler doesn't accept it anyway.  I'm not sure what 
the best fix is.  Other compilers definitely want the storage, which is within 
their rights.  I'd hate to make an msvc-specific hack for this, but may have to.

-- logging.cc patch:

Almost all this functionality should be moved to src/windows/port.cc, with a 
small interface in port.h.  Can you rewrite it in that way?

-- init-order patch:

I don't see why there's a problem here.  function_info_ is a POD, so it should 
be initialized at link time, not dynamically initialized at runtime.  Are you 
sure the problem was with function_info_?  I could see there being a problem 
with libc1 et al., which are not PODs.  We can change them to be pointers and 
initialize them before first use.

Maybe this is another msvc weirdness.  Does changing function_info_ so it's a 
global variable rather than a static method variable, make it work for you?

In any case, I'd rather not use these pragmas, which it looks like is 
MSVC-specific and therefore doesn't solve the problem for mingw (probably), and 
isn't very flexible.  (The pragma supports 2, maybe 3, layers, but what happens 
when we need 4?)  I'd rather fix this properly.

-- abort patch

This makes me uncomfortable: you're releasing a lock that you may not be 
holding.  Sure, we're about to crash anyway, but that doesn't mean it's a good 
idea to write such unsafe code.

A better solution would be to replace the call to abort() with a call to a 
windows-specific aborting function that behaves the same way (doesn't call 
global destructors, etc).  Is there such a thing?

Original comment by csilv...@gmail.com on 25 Feb 2011 at 10:50

[GoogleCodeExporter]

re init-order: Just discard this - initially I linked statically, but with 
dynamic linking it seems to work as-is.

re logging: Sure, I can move stuff around. In the meantime, I have discovered 
that the BINDNTDLL macro can also allocate memory, so it cannot be called in 
RawOpenForWriting(). Currently, I call it from HeapProfilerInit(), but perhaps 
there is a better approach.

re abort: I can give TerminateProcess() a try.

Original comment by 5u1135t...@gmail.com on 2 Mar 2011 at 3:16

[GoogleCodeExporter]

Just checking in -- based on your last comment, I'm waiting for a new version 
of this patch from you.  No rush, just wanted to make sure you weren't waiting 
on something from me.

Original comment by csilv...@gmail.com on 7 Apr 2011 at 3:40

[GoogleCodeExporter]

Yeah, there are a few things that need fixing - I also found out that in a 
release build, tcmalloc fails to patch _expand() (probably since it is only a 
'return 0'), so patching that should probably just be skipped.

But I have no idea when I will get around to looking at this again.

Original comment by 5u1135t...@gmail.com on 8 Apr 2011 at 5:30

[GoogleCodeExporter]

Pinging (as I'll do every few months) just to make sure this doesn't fall off 
the radar.  No rush!

Original comment by csilv...@gmail.com on 8 Jul 2011 at 12:37

[GoogleCodeExporter]

Pinging again (as I'll do every few months) just to make sure this doesn't fall 
off the radar.

Original comment by csilv...@gmail.com on 14 Oct 2011 at 1:24

[GoogleCodeExporter]

Pinging again (as I'll do every few months) just to make sure this doesn't fall 
off the radar.

I'm turning over maintainence of google-perftools, so someone else will have to 
take over the pinging in the future!

Original comment by csilv...@gmail.com on 25 Jan 2012 at 11:19

[GoogleCodeExporter]

Ping?

Original comment by chapp...@gmail.com on 11 Mar 2013 at 2:29