[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);
+  }
----------------
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



================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.cpp:561
+    }
+    if (using_primary_allocator) {
+      CHECK(size);
----------------
vitalybuka wrote:
> tejohnson wrote:
> > vitalybuka wrote:
> > > tejohnson wrote:
> > > > vitalybuka wrote:
> > > > > there is also asan specific inconsistency
> > > > > primiary/secondary difference is because primiary is reuse redzones and secondary not
> > > > > unfortunately asan is the worst example. 
> > > > > msan, hwasan and lsan have less unneeded stuff
> > > > Can you clarify why this isn't needed here? I do see references to the primary/secondary allocator in both hwasan and lsan. In fact lsan has code that conditionally memsets to 0 if it is from primary (similar to what is further below here in asan).
> > > primary vs secondary for asan are result of:
> > > 1. right redzone is needed for the secondary - irrelevant here
> > > 2. meta[1] contains chunk_beg, but I already removing this part from asan D86932, and explained above how to avoid to use GetMetaData here as well
> > BTW I found that the  "meta[0] = size" was removed in D86922, you might want to mark that as a parent to D86932. Question on that though. In D86922 you are using a total of at most 45 bits for holding the size. What is in fact the max size we need to represent?
> I guess it's is not a problem for heapprov, it has plenty of bits for full uptr?
> 
> I am not sure what is required max size should be. I hope 45bit is enough.
> If not, asan has large red zones for large allocations. So I still can avoid using metadata.
> Those changes are not realy important for asan. I was fixing bug with lsan, found another one with allot_tid, and metadata patches are just a cleanup.
> 
I just noticed that Asan already has hardcoded check that size fits in 40bit. see kMaxAllowedMallocSize



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120