[PATCH] D25973: [tsan] Implement WriteMemoryProfile for Darwin

[Kuba Brecka via llvm-commits]

kubabrecka added inline comments.


================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:108
 
-void FlushShadowMemory() {
+static void RegionMemUsage(uptr start, uptr end, uptr *res, uptr *dirty) {
+  vm_address_t address = start;
----------------
dvyukov wrote:
> zaks.anna wrote:
> > This is not TSan-specific. Should it go into sanitizer-common?
> I would leave it here, unless you have a plan to implement memory profiler for asan on mac.
> sanitizer_common contains too much stuff already.
> E.g. linux GetMemoryProfile was moved from tsan to sanitizer_common, but it is still used only in tsan. While moving empty stubs were added for other OSes, but the interface is actually such that it cannot be possibly ported to other OSes.
> Generalizing only one use case does not usually lead to good results.
It definitely makes sense to implement a memory profiler for ASan as well.  The main issue is that some common memory profiling tools on macOS don't work against ASanified/TSanified processes because of the huge reserved regions, so this code is the only way to get the amount of dirty/resident memory (for a specific region).

I'll leave the code here (in `tsan/`) for now, but when I get to implement this for ASan, I'll probably move this to `sanitizer_common/sanitizer_mac.cc`.  Sounds good?


================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:143
+  RegionMemUsage(LoAppMemBeg(), LoAppMemEnd(), &low_res, &low_dirty);
+  RegionMemUsage(HeapMemBeg(), HeapMemEnd(), &heap_res, &heap_dirty);
+  RegionMemUsage(HiAppMemBeg(), HiAppMemEnd(), &high_res, &high_dirty);
----------------
dvyukov wrote:
> I suspect this won't compile when SANITIZER_GO is defined. Go does not have HeapMemBeg.
> You can surround it all with #if !SANITIZER_GO as WriteMemoryProfile is not called in Go.
> You can test it by running lib/tsan/go/buildgo.sh
Sure, I'll fix that.  The Go test is run in "check-all" and "check-tsan" on Darwin, so we'll notice if we break it.  (I run those before committing.)


================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:158
+    ShadowBeg(), ShadowEnd(), shadow_res / 1024, shadow_dirty / 1024,
+    MetaShadowBeg(), MetaShadowEnd(), shadow_res / 1024, shadow_dirty / 1024,
+    TraceMemBeg(), TraceMemEnd(), trace_res / 1024, trace_dirty / 1024,
----------------
dvyukov wrote:
> s/shadow_res/meta_res/
> s/shadow_dirty/meta_dirty/
Right, thanks for noticing!


Repository:
  rL LLVM

https://reviews.llvm.org/D25973