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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.cpp:689
+      return reinterpret_cast<HeapprofChunk *>(alloc_magic[1]);
+    return reinterpret_cast<HeapprofChunk *>(alloc_beg);
+  }
----------------
tejohnson wrote:
> 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?
Unfortunately It does not.
If we see kAllocBegMagic than we quite sure that it's already updated AsanChunk.
As I wrote, without kAllocBegMagic (for smallest allocations) we have no idea if this real chunk state which is in the beginning of allocated block, or it's random garbage from primary allocator. D87359 was attempt to always have kAllocBegMagic to avoid that.
But it's not important for Asan. Asan theoretically should work with any random bytes in AsanChunk.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120