[PATCH] D87120: [HeapProf] Heap profiling runtime support

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

The next patch update should address all of the comments except:

1. @vitalybuka's last response, I have a follow up question to help me decide what to do there (thread safety related to user_requested_size)
2. Renaming heapprof to memprof - will do in a follow up update
3. Renaming the *_linux files to *_posix. It's not clear to me why they are named as such on asan and whether everything in them in fact is posix compliant.



================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.cpp:689
+      return reinterpret_cast<HeapprofChunk *>(alloc_magic[1]);
+    return reinterpret_cast<HeapprofChunk *>(alloc_beg);
+  }
----------------
vitalybuka wrote:
> tejohnson wrote:
> > vitalybuka wrote:
> > > AsanChunk is used in such way that any garbage works there.
> > > Here we return alloc_beg but we have no prove that it's initialized. 
> > > There is a data race by design, I guess, when lsan can start using that data when it's not yet fully initialized.
> > > lsan has sanity checks for all fields so it should not crash/hang and in the worst case it just misses memory leaks. 
> > > However If block is alive for user or it's in quarantine then chunk is valid and lsan will do correct leak checking.
> > > I can imagine that author tried to avoid locking between lsan and fast thread-local allocator cache.
> > > 
> > > Is it true for heapprof?
> > > Looks like heapprof_mz_size can get here. Can valid, data race free, program get into GetHeapprofChunk in one thread for a memory block which is being allocated/deallocated by another thread? 
> > > 
> > > BTW. Here is my another Asan patch to avoid metadata and more importantly make sure header is in consistent state D87359
> > > 
> > The user_requested_size is set to 0 on deallocation (min size is 1 per Allocate). All of the callers of this function do specifically check for a 0 user_requested_size and handle it specially.
> If block is allocated from primary allocator, there is not guaranty that it's zeroed. E.g. allocator may uses that mem to maintain free list.
> So user_requested_size may not be 0 just after return from allocator.Allocate
> If HeapprofChunk is alloc_beg then we have no kAllocBegMagic and GetHeapprofChunk will return this not yet initialized HeapprofChunk.
> 
> For the secondary it's not a problem as it's from mmap so it's zero-ed.
> For the primary with Magic also should not be a problem (if we fix Allocate to set Magic after initialization)
How does asan ensure that chunk_state is CHUNK_INVALID (0) on allocation from the primary allocator?


================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.h:44-95
+#if SANITIZER_CAN_USE_ALLOCATOR64
+const uptr kAllocatorSpace = 0x600000000000ULL;
+const uptr kAllocatorSize = 0x40000000000ULL; // 4T.
+typedef DefaultSizeClassMap SizeClassMap;
+template <typename AddressSpaceViewTy>
+struct AP64 { // Allocator64 parameters. Deliberately using a short name.
+  static const uptr kSpaceBeg = kAllocatorSpace;
----------------
vitalybuka wrote:
> this probably can be moved into cc file
I will do this as a follow on patch update, so it is easier to see the diffs.


================
Comment at: compiler-rt/lib/heapprof/heapprof_report.cpp:83
+    // Print memory stats.
+    if (flags()->print_stats)
+      __heapprof_print_accumulated_stats();
----------------
tejohnson wrote:
> MaskRay wrote:
> > Does print_stats=1 have a test demonstrating how it works?
> > 
> > I picked one test and added `HEAPPROF_OPTIONS=print_stats=1` but did not observe any difference.
> I thought so but I guess not, will add.
This stats printing option only affects error conditions, so I have added a test of it to malloc-size-too-big.cpp. There was already some testing of the stats printing infrastructure as noted on your other comment about that.


================
Comment at: compiler-rt/lib/heapprof/heapprof_stats.cpp:1
+//===-- heapprof_stats.cpp ------------------------------------------------===//
+//
----------------
MaskRay wrote:
> It seems that this important file is untested.
Note that atexit=1 also causes the stats printing to occur (on clean exit). I was testing that in atexit_stats.cpp, but I have beefed up the matching a bit. I don't want to match exact numbers though, since a number of allocations occur from places like libc.so and the dynamic loader, which might vary in behavior across systems/versions.

I've also added another test that exercises it via print_stats.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87120/new/

https://reviews.llvm.org/D87120