Heap checking currently does not support FreeBSD6 (requesting feedback on porting)

[GoogleCodeExporter]

What steps will reproduce the problem?

1. build the tools with --enable-heap-check

2. write a small program. Compile and link it with libtcmalloc.

#include <stdio.h>
#include <stdlib.h>

int main(int argc, char** argv)
{
    printf("********** start **********\n");
    int* i1 = new int(1000);
    int* i2 = new int(1000);
    int* i3 = new int(1000);
    int* i4 = new int(1000);
    int* i5 = new int(1000);
    int* i6 = new int(1000);
    printf("*********** end ***********\n");

    return 0;
}

$ g++ -L/usr/local/lib main.cpp -o main -ltcmalloc

3. Run the heap checker as follows: 

$ export PERFTOOLS_VERBOSE=10
$ HEAPCHECK=normal ./main
Unable to open /proc/self/environ, falling back on getenv("HEAPCHECK"), which 
may not work
MemoryRegionMap Init
MemoryRegionMap Init done
Starting tracking the heap
Found hooked allocator at 3: 0x680a2a69 <- 0x682a5b8f
Found hooked allocator at 2: 0x680a2a69 <- 0x682a3114
Found hooked allocator at 2: 0x680a2a69 <- 0x6808713a
Found hooked allocator at 2: 0x680a2a69 <- 0x682a1b51
Found hooked allocator at 2: 0x680a2a69 <- 0x68087154
Found hooked allocator at 2: 0x680a0e03 <- 0x68087172
Found hooked allocator at 2: 0x680a2a69 <- 0x682a3114
Found hooked allocator at 2: 0x680a2a69 <- 0x682a7a04
Found hooked allocator at 2: 0x680a0e03 <- 0x68097b4a
Going to ignore live object at 0x80cd000 of 4 bytes
Found hooked allocator at 2: 0x680a2a69 <- 0x682a698a
Found hooked allocator at 2: 0x680a0e03 <- 0x681330ff
Found hooked allocator at 2: 0x680a0e03 <- 0x68095fae
Found hooked allocator at 2: 0x680a0e03 <- 0x681330ff
Found hooked allocator at 2: 0x680a0e03 <- 0x681330ff
Found hooked allocator at 2: 0x680a0e03 <- 0x681330ff
No shared libs detected. Will likely report false leak positives for statically 
linked executables.
Turning perftools heap leak checking off
MemoryRegionMap Shutdown
MemoryRegionMap Shutdown done
********** start **********
*********** end ***********
$ 

What is the expected output? What do you see instead?

As you can see, we hit a condition in the heap check startup/init that causes 
heap checking to be disabled as indicated by the message:

No shared libs detected. Will likely report false leak positives for statically 
linked executables.
Turning perftools heap leak checking off

What version of the product are you using? On what operating system?

google-perftools-1.7/FreeBSD6

Please provide any additional information below.

Here is the code snippet where the heap check is bailing out:

src/heap-checker.cc

HeapLeakChecker::ProcMapsResult HeapLeakChecker::UseProcMapsLocked(
                                  ProcMapsTask proc_maps_task) {
  RAW_DCHECK(heap_checker_lock.IsHeld(), "");
  // Need to provide own scratch memory to ProcMapsIterator:
  ProcMapsIterator::Buffer buffer; 
  ProcMapsIterator it(0, &buffer);
  if (!it.Valid()) {
    int errsv = errno;
    RAW_LOG(ERROR, "Could not open /proc/self/maps: errno=%d. "
                   "Libraries will not be handled correctly.", errsv); 
    return CANT_OPEN_PROC_MAPS;
  }
  uint64 start_address, end_address, file_offset;
  int64 inode;
  char *permissions, *filename;
  bool saw_shared_lib = false;
  while (it.Next(&start_address, &end_address, &permissions,
                 &file_offset, &inode, &filename)) {
    if (start_address >= end_address) {
      // Warn if a line we can be interested in is ill-formed:
      if (inode != 0) { 
        RAW_LOG(ERROR, "Errors reading /proc/self/maps. "
                       "Some global memory regions will not "
                       "be handled correctly.");
      }
      // Silently skip other ill-formed lines: some are possible
      // probably due to the interplay of how /proc/self/maps is updated
      // while we read it in chunks in ProcMapsIterator and
      // do things in this loop.
      continue;
    }
    // Determine if any shared libraries are present.
    if (inode != 0 && strstr(filename, "lib") && strstr(filename, ".so")) {
      saw_shared_lib = true; 
    }
    switch (proc_maps_task) {
      case DISABLE_LIBRARY_ALLOCS:
        // All lines starting like
        // "401dc000-4030f000 r??p 00132000 03:01 13991972  lib/bin"
        // identify a data and code sections of a shared library or our binary
        if (inode != 0 && strncmp(permissions, "r-xp", 4) == 0) { 
          DisableLibraryAllocsLocked(filename, start_address, end_address);
        }       
        break;  
      case RECORD_GLOBAL_DATA:
        RecordGlobalDataLocked(start_address, end_address,
                               permissions, filename);
        break;  
      default:
        RAW_CHECK(0, "");
    }
  }
******************************* HERE *********************************  
  if (!saw_shared_lib) {
    RAW_LOG(ERROR, "No shared libs detected. Will likely report false leak "
                   "positives for statically linked executables.");
    return NO_SHARED_LIBS_IN_PROC_MAPS;
  }
  return PROC_MAPS_USED;
}

The documentation seems to suggest that the heap check is only currently 
supported on Linux. I suspect that whatever linux specific code could be made 
platform independent or easily ported to work with FreeBSD. I would like to add 
support for FreeBSD but need a bit of a background overview on the Linux 
specific bits that would need to change. Is someone willing to get me up to 
speed here to get this work underway? 

Regards,

Dave

Original issue reported on code.google.com by chapp...@gmail.com on 11 Feb 2011 at 10:01

[GoogleCodeExporter]

The biggest linux-specific part is the code that freezes all threads while the 
heap-checker does its data gathering.  It also has to be careful to only call 
library routines that never allocate memory; in practice, this means making 
mostly syscalls, which it does via a syscall-wrapper file (in src/base) which 
is linux-specific.  There may also be other linux-isms, like the code-block 
that you identified.  I believe FreeBSD has support for /proc but it's not on 
by default; you may need to run with /proc to get any useful data out of the 
heap-profiler in freebsd.

These are the linux-isms I can think of offhand.  There may be others I don't 
know about.

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

• Added labels: Type-Enhancement, Priority-Medium

[GoogleCodeExporter]

FreeBSD-6 is long obsolete. Current version is 8.1. You should upgrade and use 
the current port devel/google-perftools there.

I am getting the output on 8.1:
********** start **********
*********** end ***********

If you don't have /proc mounted -- add this line to /etc/fstab:
proc /proc procfs rw 0 0

Original comment by y...@tsoft.com on 20 Feb 2011 at 12:25

[GoogleCodeExporter]

Yes. We are moving to freebsd8 in the fall. I work in telecom/cable/wireless 
networking equipment industry so platform stability is crucial. In other words, 
once things are stable keep it that way. Unfortunately that means falling 
behind the times as well.

I have /proc mounted but still no berries. Its not a huge deal since the heap 
profiler works. The others were just nice to haves.

This can now be closed off as it doesn't seem like its worth the effort to 
address.

Original comment by chapp...@gmail.com on 3 Mar 2011 at 4:36

[GoogleCodeExporter]

On a side note it looks like it could just be that the layout of my /procs is 
just different than expected. I have /proc/curproc/map instead of 
/proc/self/maps. Easy enough to patch locally. I'll give it a shot.

$ cat /proc/curproc/map 
0x8048000 0x804a000 2 0 0xa90e9210 r-x 1 0 0x0 COW NC vnode /bin/cat
0x804a000 0x804b000 1 0 0xa8ec36b4 rw- 2 0 0x2180 NCOW NNC default -
0x804b000 0x804d000 2 0 0xa8ec36b4 rwx 2 0 0x2180 NCOW NNC default -
0x6804a000 0x68069000 24 0 0xa10649cc r-x 187 64 0x4 COW NC vnode 
/libexec/ld-elf.so.1
0x68069000 0x6806a000 1 0 0xa9b1a294 rw- 1 0 0x2180 COW NNC vnode 
/libexec/ld-elf.so.1
0x6806a000 0x6806f000 5 0 0xa9ae7000 rw- 2 0 0x2180 NCOW NNC default -
0x6806f000 0x68080000 11 0 0xa9ae7000 rwx 2 0 0x2180 NCOW NNC default -
0x68080000 0x6814c000 79 0 0xa9afe840 r-x 1 0 0x2180 COW NNC vnode 
/lib/libc.so.6
0x6814c000 0x6814d000 1 0 0xa9b5f8c4 r-x 1 0 0x2180 COW NNC vnode /lib/libc.so.6
0x6814d000 0x68152000 5 0 0xa7607000 rwx 1 0 0x2180 COW NNC vnode /lib/libc.so.6
0x68152000 0x68167000 4 0 0xa9b28948 rwx 1 0 0x2180 NCOW NNC default -
0x9fbe0000 0x9fc00000 3 0 0xa9b28e70 rwx 1 0 0x2180 NCOW NNC default -

Original comment by chapp...@gmail.com on 3 Mar 2011 at 4:56

[GoogleCodeExporter]

Right -- this may just require some tweaks to src/base/sysinfo.cc (it looks 
like the format of the procs file is different too, which may require a few 
more changes, though we do try to have freebsd support in sysinfo.cc already).  
Let me know how it turns out!

Original comment by csilv...@gmail.com on 3 Mar 2011 at 5:46

[GoogleCodeExporter]

Ok. So I did some late night hackery and found some interesting tidbits.

1. /proc/curproc/map does not list any inode information. In the freebsd 
specific bits of src/base/sysinfo.cc we explicitly set inode = 0 because it 
does not exist in the proc map.

    693 #elif defined(__FreeBSD__)
    694     // For the format, see http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/fs/procfs/procfs_map.c?rev=1.31&content-type=text/        x-cvsweb-markup
    695     tmpstart = tmpend = tmpoffset = 0;
    696     tmpinode = 0;
    697     major = minor = 0;   // can't get this info in freebsd
    698     if (inode)
    699       *inode = 0;        // nor this
    700     if (offset)
    701       *offset = 0;       // seems like this should be in there, but maybe not

2. src/heap-checker.cc will always fail if it doesn't find at least one proc 
map entry with a non-zero inode number.

    882     if (inode != 0 && strstr(filename, "lib") && strstr(filename, ".so")) {
    883       saw_shared_lib = true;
    884     }

So I think that it is a valid observation that this currently does not work for 
freebsd6 or freebsd8 for that matter :(. I will see if I can test this and 
confirm under freebsd8 over the next few days.

Further to this, I decided to see what happens if I force it to ignore the maps 
checking. It seems that there are other issues unrelated to the proc maps 
stuff. The lines begining with DC: are printfs that I added:

TPC-G6-30$ export HEAPCHECK=normal 
TPC-G6-30$ ./main
Unable to open /proc/self/environ, falling back on getenv("HEAPCHECK"), which 
may not work
MemoryRegionMap Init
MemoryRegionMap Init done
Starting tracking the heap
Found hooked allocator at 3: 0x680ad939 <- 0x682c6b8f
Found hooked allocator at 2: 0x680ad939 <- 0x682c4114
Found hooked allocator at 2: 0x680ad939 <- 0x6808fd9e
Found hooked allocator at 2: 0x680ad939 <- 0x682c2b51
Found hooked allocator at 2: 0x680ad939 <- 0x6808fdb8
Found hooked allocator at 2: 0x680abceb <- 0x6808fdd6
Found hooked allocator at 2: 0x680ad939 <- 0x682c4114
Found hooked allocator at 2: 0x680ad939 <- 0x682c8a04
Found hooked allocator at 2: 0x680abceb <- 0x6809f97a
Found hooked allocator at 2: 0x680ad939 <- 0x682c798a
Found hooked allocator at 2: 0x680abceb <- 0x68169184
Found hooked allocator at 2: 0x680abceb <- 0x6809ddf2
Found hooked allocator at 2: 0x680abceb <- 0x68169184
Found hooked allocator at 2: 0x680abceb <- 0x68169184
Found hooked allocator at 2: 0x680abceb <- 0x68169184
Found hooked allocator at 2: 0x680ad939 <- 0x682936a2
DC: Heap checker init
DC: Constructing file with name '/proc/curproc/map'
inode=0 
filename=/d2/ports/devel/google-perftools/work/google-perftools-1.6/debug/main
inode=0 filename=-
inode=0 filename=-
inode=0 filename=/libexec/ld-elf.so.1
inode=0 filename=/libexec/ld-elf.so.1
inode=0 filename=-
inode=0 filename=-
inode=0 filename=/d2/local/lib/libtcmalloc.so.0
inode=0 filename=/d2/local/lib/libtcmalloc.so.0
inode=0 filename=/d2/local/lib/libtcmalloc.so.0
inode=0 filename=-
inode=0 filename=/usr/lib/libstdc++.so.6
inode=0 filename=/usr/lib/libstdc++.so.6
inode=0 filename=/usr/lib/libstdc++.so.6
inode=0 filename=-
inode=0 filename=/lib/libm.so.4
inode=0 filename=/lib/libm.so.4
inode=0 filename=/lib/libm.so.4
inode=0 filename=/lib/libc.so.6
inode=0 filename=/lib/libc.so.6
inode=0 filename=/lib/libc.so.6
inode=0 filename=-
inode=0 filename=/usr/lib/libthr.so.2
inode=0 filename=/usr/lib/libthr.so.2
inode=0 filename=/usr/lib/libthr.so.2
inode=0 filename=-
inode=0 filename=/lib/libgcc_s.so.1
inode=0 filename=/lib/libgcc_s.so.1
inode=0 filename=/lib/libgcc_s.so.1
inode=0 filename=-
inode=0 filename=-
inode=0 filename=-
Found hooked allocator at 2: 0x680abceb <- 0x68169184
Found hooked allocator at 2: 0x680abceb <- 0x68095e3e
Found hooked allocator at 2: 0x680abceb <- 0x68169184
Found hooked allocator at 2: 0x680abc4b <- 0x68095f9d
WARNING: Perftools heap leak checker is active -- Performance may suffer
DC: FLAGS_heap_check=normal
Found hooked allocator at 2: 0x680abceb <- 0x680966dd
Found hooked allocator at 2: 0x680abceb <- 0x68093b16
Found hooked allocator at 2: 0x680abc4b <- 0x68093849
Going to ignore live object at 0x80cd040 of 7 bytes
Start check "_main_" profile: 6683 bytes in 18 objects
DC: setting do_main_heap_check=true
********** start **********
Found hooked allocator at 2: 0x680abceb <- 0x8048626
Found hooked allocator at 2: 0x680abceb <- 0x804863c
Found hooked allocator at 2: 0x680abceb <- 0x8048652
Found hooked allocator at 2: 0x680abceb <- 0x8048668
Found hooked allocator at 2: 0x680abceb <- 0x804867e
Found hooked allocator at 2: 0x680abceb <- 0x8048694
*********** end ***********
DC: +HeapLeakChecker_AfterDestructors
DC: FLAGS_heap_check_after_destructors=0
Check failed: !do_main_heap_check: should have done it
DC: +RunHeapCleanups

I didn't get a chance to look too much into this failure but it seems to happen 
in all heap check modes. I suspect that it has something to do with the global 
initialization/destruction order of objects. Anyone else want to venture a 
hypothesis?

Original comment by chapp...@gmail.com on 3 Mar 2011 at 3:41

[GoogleCodeExporter]

Looks like the heap checker doesn't work on freebsd8 either:

(wheel)# export HEAP_CHECK_DUMP_DIRECTORY=/d2/tmp
(wheel)# export HEAPCHECK=normal
(wheel)# LD_PRELOAD=/usr/local/lib/libtcmalloc.so ./main
No shared libs detected. Will likely report false leak positives for statically 
linked executables.
Turning perftools heap leak checking off
********** start **********
*********** end ***********
(wheel)#

So this is an issue that will need to be fixed. I am up for the task and will 
keep the updates rolling as I make progress.

Original comment by chapp...@gmail.com on 3 Mar 2011 at 5:33

[GoogleCodeExporter]

Alright, so I have made some progress on this. I have been running against 
linux and freebsd6/8 to compare and contrast. These are my findings:

- linux is rock solid, everything works as expected (wish I worked with linux 
every day)

- freebsd6/8 both have the same fundamental issues
    - they will only run in draconian mode and the resulting output is a bit strange
    - they both crash in <= strict mode which I have a fix for however when they run to completion they claim that there is no leak detected which is incorrect
    -mmap, munmap, and sbrk hooks do not get honored and therefore we do not record any regions
    - mremap does not exist in freebsd which isn't a big deal since it can be and is better off implemented using mmap/munmap/etc which we will have wrappers for anyways.

Here is the output of one of the unit tests showing the mmap hook failure. I 
just tweaked it slightly to also run the mmap portion for freebsd.

TPC-G6-30$ ./tcmalloc_unittest 
Testing empty allocation
Testing STL use
Sanity-testing all the memory allocation functions
Check failed: g_MmapHook_calls > 0

So the hook isn't getting called at all. After a bit more digging in 
src/malloc_hook.cc I discovered that there is no support for wrapping mmap, 
munmap, sbrk, etc for freebsd. It doesn't look too challenging to add it in.

So to summarize I have the following items to address:

1. Fix the /proc parsing for freebsd
2. Fix the incorrect leak reporting when in any mode other than draconian
3. Fix the crash that occurs when no regions are created as a result of mmap, 
munmap, and sbrk not being correctly replaced.
4. Add support for mmap, munmap, sbrk to src/malloc_hook.cc
5. Fix up unit tests to also start verifying freebsd centric components
6. Test against linux and freebsd 6/8 (I don't easily have access to 7)
7. Submit patches for all of the above work.

Let me know if there is anything I missed or perhaps other freebsd-ish type 
things that need to be fixed :)

-Dave

Original comment by chapp...@gmail.com on 4 Mar 2011 at 7:17

[GoogleCodeExporter]

Wow, I had forgotten that we only override mmap for linux systems.  Fixing that 
would be great.  In fact, your whole agenda looks great.  I wholeheartedly 
approve!

I've taken a stab at (1), at least in the context of the heap-checker (making 
it so it deals ok with inode being 0).  I'm currently having the heap-checker 
author look it over to make sure it's ok, but assuming he gives, the ok, I'll 
submit it to svn maybe tomorrow.

Also, feel free to intersperse (7) with the other steps. :-)  Also, if you 
haven't already, can you please sign the CLA at 
http://code.google.com/legal/individual-cla-v1.0.html?

Original comment by csilv...@gmail.com on 4 Mar 2011 at 8:46

[GoogleCodeExporter]

Ok, I just checked in code to the svn that should help with (1).

Original comment by csilv...@gmail.com on 4 Mar 2011 at 11:53

[GoogleCodeExporter]

Great! I have a fix ready for (3). I am hoping to find some cycles this weekend 
to tackle (4).

Original comment by chapp...@gmail.com on 5 Mar 2011 at 2:41

[GoogleCodeExporter]

Quick patch for (3). It was either this way or add a new method for checking if 
the regions_ were empty/null and protect attempted iterations of regions_. Your 
call :) It seems that we can really only end up here if mmap/mremap/sbrk hooks 
are not correctly overidden for the taget platform. Maybe a log message would 
also be useful to warn the user that their platform is not fully supported.

~/development/google-perftools-read-only/src$ diff 
./.svn/text-base/memory_region_map.h.svn-base memory_region_map.h
282a283,291
>   // A special iterator returned when attempting to retrieve
>   // begin and end iterators for iterating over the memory
>   // regions. Under normal operating conditions there should
>   // always be at least one region. If there isn't, then the
>   // regions_ can be NULL and therefore we need a meaningful
>   // sentinel iterator to return. This can occur on platforms
>   // that do not have mmap/sbrk/mremap hooks correctly overridden.
>   static RegionSet::iterator empty_regions_itr_;
> 

~/development/google-perftools-read-only/src$ diff 
./.svn/text-base/memory_region_map.cc.svn-base memory_region_map.cc
140a141
> MemoryRegionMap::RegionSet::iterator MemoryRegionMap::empty_regions_itr_ = 
MemoryRegionMap::RegionSet::iterator();
352,353c353
<   RAW_CHECK(regions_ != NULL, "");
<   return regions_->begin();
---
>   return regions_ ? regions_->begin() : empty_regions_itr_;
358,359c358
<   RAW_CHECK(regions_ != NULL, "");
<   return regions_->end();
---
>   return regions_ ? regions_->end() : empty_regions_itr_;

Original comment by chapp...@gmail.com on 9 Mar 2011 at 3:58

[GoogleCodeExporter]

Hmm, looking at this, it doesn't seem actually all that helpful.  What happens 
if you skipped (3) and went straight to (4)?  Then (3) would be unnecessary, 
right?

Looking at the heap-checker, it looks like fixing (3) won't actually do much to 
make the heap-checker actually work better.  So maybe it's better to focus on 
(4).

Original comment by csilv...@gmail.com on 10 Mar 2011 at 7:47

[GoogleCodeExporter]

Fair enough :) As far as (4) goes I have things working under freebsd now. I 
just need to do some cleanup and additional testing.

Original comment by chapp...@gmail.com on 11 Mar 2011 at 8:37

[GoogleCodeExporter]

I don't understand much of what is going on above, but just for the record: I 
have the same problem ("No shared libraries detected") in linux.

My OS: Debian squeeze/sid amd64 in VirtualBox on Windows 7

I have both google-perftools7 and libgoogle-perftools-7 installed.

Original comment by hanul...@gmail.com on 15 Mar 2011 at 2:54

[GoogleCodeExporter]

This sounds like a new bug, since it's not related to freebsd.  It could have 
something to do with VirtualBox; I'm not clear on what that is or how well it 
emulates linux.  What does 'cat /proc/self/maps' say?

Original comment by csilv...@gmail.com on 15 Mar 2011 at 7:32

[GoogleCodeExporter]

@Dave

Can you share the patches for (2) and (4) - I'm stuck with the same problem too 
and I feel your fix might help.

Original comment by rajterm...@gmail.com on 30 Mar 2011 at 11:57

[GoogleCodeExporter]

Patch is almost ready. This coming Monday at the latest.

Original comment by chapp...@gmail.com on 30 Mar 2011 at 12:54

[GoogleCodeExporter]

@Dave

Is the patch ready yet?

Original comment by rajterm...@gmail.com on 7 Apr 2011 at 6:01

[GoogleCodeExporter]

I have attached the patch. Everything appears to be running fine except for 4 
failing unit tests in the heap-checker_unittest suite. As far as I can tell, 
the failures are related to the background busy thread in these tests as well 
as the use of thread specific storage and some underlying pthread api calls. 
There are possibly some freebsd-isms that remain to be worked out here. I 
attached a copy of the output from 'make check'. Unfortuantely, I am out of 
cycles to investigate further right now. As it is, I have written a few sample 
processes to test functionality of the heap checker and it is operating 
correctly running those isolated tests.

Regards,

Dave

Original comment by chapp...@gmail.com on 20 Apr 2011 at 4:40

Attachments:

• gpt-freebsd-patch.tgz

[GoogleCodeExporter]

Thank you for the patch!  I'm currently swamped with work, but will definitely 
take a look at this before the next perftools release (which may not be for a 
month or two :-( ).

I briefly looked this over now and was surprised by the destructor ordering 
issue you saw.  The language guarantees that destructors are called in reverse 
order of construction.  This suggests that these things were constructed in a 
different order on linux and freebsd.  Can you confirm that that's what you are 
seeing, in gdb or the like?

If so, it has to do with what the linker does -- it's implementation-defined 
what order constructors are run in, but I think the linker typically decides 
this.  Maybe there's a way of reordering the files in libtcmalloc.la, in 
Makefile.am, to get things to work properly under freebsd 6.

Original comment by csilv...@gmail.com on 21 Apr 2011 at 2:26

[GoogleCodeExporter]

Just curious if this is available yet in the latest development snapshot.

Regards,

Dave

Original comment by chapp...@gmail.com on 8 Jun 2011 at 2:36

[GoogleCodeExporter]

I've not had a chance to try to apply this yet.  Did you ever have a chance to 
look at the cause of the destructor ordering issues, as I mentioned in my 
previous comment?  I'd like to make sure I understand what the problems are 
that this patch is fixing, before applying it.

(As I mentioned before, (1) should already be in the svn-root.  But (4) 
definitely isn't yet.  I apologize for it taking so long; I've got a big 
backlog of perftools changes I'm slowly making my way through.)

Original comment by csilv...@gmail.com on 8 Jun 2011 at 9:34

[GoogleCodeExporter]

OK, I had a few free moments to look at this today.  I've made the changes to 
malloc_hook.cc to add mmap/etc overriding for freebsd, and currently have that 
out for review.  I still don't understand the issue with destructor order, so I 
haven't tried to apply the patch to heap-checker.cc yet.  I am also waiting to 
patch the tests (tcmalloc_unittest.cc, and malloc_hook_test.cc) to enable 
freebsd until the other changes are in.  Likewise with the change to 
configure.ac to enable heap-checking on freebsd by default.  I guess I'll also 
need to update the INSTALL file.

I think once this is in then you've done all of (1)-(7)? Except perhaps (2).  
I'm hoping to do a new release shortly, and it would be great to have freebsd 
fixes be in it.

Original comment by csilv...@gmail.com on 8 Jul 2011 at 1:50

• Changed state: Started

[GoogleCodeExporter]

Here is the feedback I got on the review of the malloc_hook changes:

------------------------------------
>> MallocHook::InvokeMunmapHook(start, length);
Did you intentionally leave out the MMap and Munmap replacements here?
------------------------------------
>> return mmap(start, length, prot, flags, fd, offset);
Shouldn't this be do_mmap?
------------------------------------
>> return munmap(start, length);
Won't this be hooked?
------------------------------------

These last two comments refer to the UnhookedMmap/unmmap calls, and seem right 
to me -- these are actually calling the hooked versions.  I'm not sure what the 
first comment is referring to; perhaps it will be clearer to you.

Original comment by csilv...@gmail.com on 8 Jul 2011 at 10:16

[GoogleCodeExporter]

I tried compiling this on our own freebsd 8.1 machine, and it said:
---
/var/tmp//ccT3ufoe.s: Assembler messages:
/var/tmp//ccT3ufoe.s:3891: Error: suffix or operands invalid for `mov'
---
I have a 64-bit machine; would that explain it?

I'd like to get this patch into the next release, but I feel it's not ready yet.

Original comment by csilv...@gmail.com on 12 Jul 2011 at 1:00

[GoogleCodeExporter]

OK, I added the relevant code to perftools 1.8, just released, but commented it 
out because of the compile errors I was seeing.  Feel free to hack on the new 
version! -- hopefully it's pretty easy to work with.

Original comment by csilv...@gmail.com on 16 Jul 2011 at 1:22

• Changed state: Accepted

[GoogleCodeExporter]

Any more word on this?  It would be great to get it working, but it's not there 
yet (at least in my tests).

Original comment by csilv...@gmail.com on 2 Sep 2011 at 1:15

[GoogleCodeExporter]

    Apologies for letting this go so long. Unfortunately I have left the company I was working with and no longer have access to a machine running freebsd. However, I see the fundamental issue here in malloc_hook_mmap_freebsd.h.

1. movl .curbrk, %%eax

    movl is a 32-bit move instruction. So effectively here we are saying, take the address of the symbol .curbrk and place it in register eax. However, since addresses on a 64-bit machine will be 64-bit, the operands do not agree with the move instruction.

2. "movl %%eax, %0

    Again we have an issue here because the output operand curbrk is a void*. So we are trying to take a 32-bit value from eax and place that in a 64-bit memory location. 

3. The register eax remains a 32 bit register in the amd64 register set. So the 
register used needs to change between 32-bit and 64-bit modes.

The fix here for (1) and (2) is to just use the 'mov' instruction instead of 
the 'movl' instruction. The 'mov' instruction will select the correct move 
instruction based on the operands. To fix (3) we need to use a preprocessor 
macro to determine whether we are compiling for a 32-bit or 64-bit 
architecture. So the correct replacement for this inline assembly is:

 #if defined(__x86_64__) || defined(__amd64__)
    __asm__ __volatile__(
        "mov .curbrk, %%rax;"
        "mov %%rax, %0"
        : "=r" (curbrk)
        :: "%rax");
#else
    __asm__ __volatile__(
        "mov .curbrk, %%eax;"
        "mov %%eax, %0"
        : "=r" (curbrk)
        :: "%eax");
#endif

Regards,

Dave

Original comment by chapp...@gmail.com on 11 Oct 2011 at 9:18

[GoogleCodeExporter]

Thanks for the suggested fix! -- I appreciate your looking after this, even 
though your life must be in flux these days.  I'll try it out as I prepare the 
next release, and let you know how it works.

Original comment by csilv...@gmail.com on 11 Oct 2011 at 9:43

[GoogleCodeExporter]

A colleague just pointed out that AT&T assembly format requires the suffix so 
in the patch above:

64-bit
    change the mov to movq
32-bit
    change the mov to movl

Original comment by chapp...@gmail.com on 11 Oct 2011 at 10:09

[GoogleCodeExporter]

OK, thanks.  I changed all the 'mov's like you suggested.

The only freebsd expert I know here is currently on vacation, so I won't be 
able to get this in for a few weeks, until he gets back and can review.  But 
I'll try testing it out in the meantime.

Original comment by csilv...@gmail.com on 11 Oct 2011 at 10:18

[GoogleCodeExporter]

So curiosity got the better of me and I setup a vmware player with a Freebsd 
8.1 amd-64 image. There is an issue here with PIC that I am fighting with :) 
Seems that amd-64 doesn't like the use of .curbrk because of some PIC issues. I 
am looking back over the sbrk.S freebsd source to try and figure out how to 
deal with this. I am close to a solution but my assembly skills are weak sauce. 
I'll keep you posted. 

Original comment by chapp...@gmail.com on 13 Oct 2011 at 5:12

[GoogleCodeExporter]

Sounds great!

Original comment by csilv...@gmail.com on 18 Oct 2011 at 4:55

[GoogleCodeExporter]

I expect to have a patch ready by the end of next week. May need to lean on you 
a bit early next week to help me through some background and tricky bits :) 

Original comment by chapp...@gmail.com on 28 Oct 2011 at 8:20

[GoogleCodeExporter]

Sounds good.  Will do the best I can!

Original comment by csilv...@gmail.com on 28 Oct 2011 at 10:00

[GoogleCodeExporter]

Any more news on this?  I'm planning on making a new release in the next few 
days, and it would be great to have this patch in if possible.

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

[GoogleCodeExporter]

This went in with all of the other freebsd porting work.

Original comment by chapp...@gmail.com on 26 Jan 2012 at 1:36

[GoogleCodeExporter]

Closing this off as all of the core patches have been tested and applied. First 
supported release for the FreeBSD porting work above is google-perftools-1.9.1 
and gperftools-2.0.

Original comment by chapp...@gmail.com on 7 Feb 2012 at 2:56

[GoogleCodeExporter]

Original comment by chapp...@gmail.com on 7 Feb 2012 at 2:59

• Changed state: Fixed