[PATCH] D87120: [MemProf] Memory profiling runtime support

Thu Oct 8 14:51:43 PDT 2020

tejohnson marked 28 inline comments as done.
tejohnson added a comment.

Thanks for the detailed comments! I think I have addressed them all, except for a few as noted in the comments below.

================
Comment at: compiler-rt/CMakeLists.txt:77-88
+  if (NOT (COMPILER_RT_MEMPROF_SHADOW_SCALE GREATER -1 AND
+	  COMPILER_RT_MEMPROF_SHADOW_SCALE LESS 8))
+    message(FATAL_ERROR "
+    Invalid Memprof Shadow Scale '${COMPILER_RT_MEMPROF_SHADOW_SCALE}'.")
+  endif()
+
+  set(COMPILER_RT_MEMPROF_SHADOW_SCALE_LLVM_FLAG
----------------
vitalybuka wrote:
> these flags were added recently to support some platform with memory constraints do not allow to use default vales.
> I would recommend do copy them, as corresponding -mllvm flags unless memprof is actually needs them.
At some point we may want to play around with other shadow granularities, but that will require adjusting the mask as well. For now I have removed it, we can add something back if we want to adjust this in the future.

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:88-93
+  // 4-th 4 bytes available
+  u32 dummy;
+  // 5-th and 6-th 4 bytes
+  // The max size of an allocation is 2^40 (kMaxAllowedMallocSize).
+  u64 user_requested_size : kMaxAllowedMallocBits;
+  u64 from_memalign : 1;
----------------
vitalybuka wrote:
> we don't have to use bit fields
True. I was trying to keep it to the minimal number of bits, but I can leave a comment that notes these can be shrunk to provide space for additional fields if needed.

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:554-555
+      CHECK_LE(alloc_beg + 2 * sizeof(uptr), chunk_beg);
+      reinterpret_cast<uptr *>(alloc_beg)[0] = kAllocBegMagic;
+      reinterpret_cast<uptr *>(alloc_beg)[1] = chunk_beg;
+    }
----------------
vitalybuka wrote:
> magic should better be atomic, set when entire Chunk is initialized and after chunk_beg is set
> as FinishAndPrint may run in another thread.
> I updated asan code, see LargeChunkHeader.
> 
> Also ForEachChunk appyes to all, including non allocated and deallocated chunks.
> So Deallocate needs to reset magic as well as we don't have guaranty that FinishAndPrint can see chunk_beg for deallocated block.
> Probably user_requested_size needs to be atomic as well?
Great, I stole your LargeChunkHeader code from asan and used that approach here. =)
I added the appropriate check to FinishAndPrint that we're getting back a non-null MemprofChunk from getMemprofChunk. With that change I'm not sure that we need to make user_requested_size atomic?

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:679
+
+  uptr AllocationSize(uptr p) {
+    MemprofChunk *m = GetMemprofChunkByAddr(p);
----------------
vitalybuka wrote:
> can you use "uptr chunk_beg = p - kChunkHeaderSize;" as in deallocate ?
> to remove one kAllocBegMagic user
This is only used by memprof_malloc_usable_size. Looks like it handles being invoked on a pointer that may not be the start of the block. I couldn't find any documentation about how malloc_usable_size should behave if called on a pointer in the middle of an allocation. I tried using the system version and it doesn't error (like a deallocation does), but returns 0 in that case. My guess is behavior is undefined? In any case, not sure if it is worth optimizing for this case. WDYT?

================
Comment at: compiler-rt/lib/memprof/memprof_init_version.h:18-24
+
+extern "C" {
+// Every time the Memprof ABI changes we also change the version number in the
+// __memprof_init function name.  Objects built with incompatible Memprof ABI
+// versions will not link with run-time.
+#define __memprof_version_mismatch_check __memprof_version_mismatch_check_v1
+}
----------------
vitalybuka wrote:
> I think it's not needed.
> Only asan has this and it was not updated as expected.
> 
I actually prefer to keep this to have some ability to detect inconsistencies.

================
Comment at: compiler-rt/lib/memprof/memprof_mapping.h:30-40
+#define DO_MEMPROF_MAPPING_PROFILE 0 // Set to 1 to profile the functions below.
+
+#if DO_MEMPROF_MAPPING_PROFILE
+#define PROFILE_MEMPROF_MAPPING() MemprofMappingProfile[__LINE__]++;
+#else
+#define PROFILE_MEMPROF_MAPPING()
+#endif
----------------
vitalybuka wrote:
> this looks like some pieces of debugging code
Yeah, not sure what this is for, was cloned from asan. Removed it and the related code.

================
Comment at: compiler-rt/lib/memprof/memprof_report.cpp:142-203
+void ReportCallocOverflow(uptr count, uptr size, BufferedStackTrace *stack) {
+  ScopedInErrorReport in_report(/*fatal=*/true);
+  ErrorCallocOverflow error(GetCurrentTidOrInvalid(), stack, count, size);
+  in_report.ReportError(error);
+}
+
+void ReportReallocArrayOverflow(uptr count, uptr size,
----------------
vitalybuka wrote:
> Most of these errors are already in sanitizer_common. and all sanitizers call them from there.
> Asan probably needs to include some additional info.
> 
> Having that memprof has no own errors, why it does not use common report functions?
> 
> I guess entire file can be removed.
> ReportRssLimitExceeded and ReportOutOfMemory can be avoidede or moved into sanitizer_allocator_report
> 
> 
> 
Ah, I didn't realize that there were baseline versions of these functions in sanitizer_common. It had all of them except ReportRssLimitExceeded, which I have added there as well. This allowed me to remove memprof_report.* and memprof_errors.*.

================
Comment at: compiler-rt/lib/memprof/memprof_rtl.cpp:183-184
+
+  MemprofCheckIncompatibleRT();
+  MemprofCheckDynamicRTPrereqs();
+  AvoidCVE_2016_2143();
----------------
vitalybuka wrote:
> Probably not needed, I don't see similar code in other sanitizers
Since these seem to be checking for valid inconsistencies, I'd prefer to leave them, unless there's a reason they are not needed or problematic.

================
Comment at: compiler-rt/lib/memprof/memprof_rtl.cpp:185
+  MemprofCheckDynamicRTPrereqs();
+  AvoidCVE_2016_2143();
+
----------------
vitalybuka wrote:
> maybe not needed. it's something s390 specific and old
Ditto here. I see calls to this in the other *san as well.

================
Comment at: compiler-rt/lib/memprof/memprof_thread.cpp:100
+
+  malloc_storage().CommitBack();
+  if (common_flags()->use_sigaltstack)
----------------
vitalybuka wrote:
> I suspect no signal related code is needed for memprof.
> Asan handles some signals which should not be needed there.
Just to clarify, the comment appears on the line with the call to CommitBack(), but I assume you are talking about the alternate signal stack handling below. I see that the other sanitizers don't have this handling, and I've removed it from here and ThreadStart.

================
Comment at: compiler-rt/test/memprof/TestCases/default_options.cpp:1-2
+// RUN: %clangxx_memprof -O2 %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
----------------
vitalybuka wrote:
> for 1:1 compile:run statements this way more convenient to copy entire testcase command line when debugging
> same other tests
Fixed here and in other tests where applicable.

================
Comment at: compiler-rt/test/memprof/TestCases/dump_process_map.cpp:4
+// RUN: %clangxx_memprof -O0 %s -o %t
+// RUN: %env_memprof_opts=dump_process_map=1 %run %t 2>&1 | FileCheck %s
+// RUN: %env_memprof_opts=dump_process_map=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOMAP
----------------
vitalybuka wrote:
> maybe better to add print_module_map=3 and document something
> 3 - print module map for debugging
> and assume sanitizer may use it when ever it wants.
I'm a little unsure of what you are suggesting here. There is an existing DumpProcessMap(), which is only implemented by sanitizer_posix. This is different than the PrintModuleMap() controlled by print_module_map that only has a non-empty implementation for sanitizer_mac. Are you suggesting that instead of adding dump_process_map, I add print_module_map=3 and have it invoke DumpProcessMap? What is the intended distinction between PrintModuleMap and DumpProcessMap?

================
Comment at: compiler-rt/test/memprof/TestCases/free_hook_realloc.cpp:3
+// RUN: %clangxx_memprof -O2 %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
----------------
vitalybuka wrote:
> Looks like this test belong to sanitizer_common
I don't think so, since this is testing that memprof sets up the __sanitizer_free_hook and invokes it correctly. This is almost an exact copy of an asan Posix test case that does the same thing there. (I did try moving the asan/TestCases/Posix version to sanitizer_common/TestCases/Posix and it fails for other sanitizers that apparently don't set up the free hook, so I think it is better to have copies for the clients that do).

================
Comment at: compiler-rt/test/memprof/TestCases/function-sections-are-bad.cpp:2
+// Check that --gc-sections does not throw away (or localize) parts of sanitizer
+// interface.
+// RUN: %clang_memprof %s -Wl,--gc-sections -ldl -o %t
----------------
vitalybuka wrote:
> Why this test is needed?
This is a copy of the asan version. Did some more digging - there is a comment in compiler-rt/CMakeLists.txt that it is for an issue on powerpc64le. Since there isn't support in memprof currently for that architecture, I've deleted this test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120