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

[Teresa Johnson via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.cpp:561
+    }
+    if (using_primary_allocator) {
+      CHECK(size);
----------------
vitalybuka wrote:
> 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
> 
Ok thanks. My inclination then is to make the size a 40 bit field, so that I can make note of the extra bits available if needed in the future for something else, and then have a check somewhere to ensure that they stay in sync.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120