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

Sat Oct 3 00:27:05 PDT 2020

vitalybuka added inline comments.

================
Comment at: compiler-rt/lib/memprof/CMakeLists.txt:86-114
+  add_compiler_rt_object_libraries(RTMemprof
+    ARCHS ${MEMPROF_SUPPORTED_ARCH}
+    SOURCES ${MEMPROF_SOURCES}
+    ADDITIONAL_HEADERS ${MEMPROF_HEADERS}
+    CFLAGS ${MEMPROF_CFLAGS}
+    DEFS ${MEMPROF_COMMON_DEFINITIONS}
+    DEPS ${MEMPROF_DEPS})
----------------
text is misaligned 

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:16-34
+#include "memprof_allocator.h"
+#include "memprof_mapping.h"
+#include "memprof_report.h"
+#include "memprof_stack.h"
+#include "memprof_thread.h"
+#include "sanitizer_common/sanitizer_allocator_checks.h"
+#include "sanitizer_common/sanitizer_allocator_interface.h"
----------------
clang-formated version works for me

================
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;
----------------
we don't have to use bit fields

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:320-324
+    char buffer[100];
+    sprintf(buffer, "%5.2f%%",
+            SetAccessCount ? SetMissCount * 100.0 / SetAccessCount : 0.0);
+    Printf("Set %d miss rate: %d / %d = %s\n", i, SetMissCount, SetAccessCount,
+           buffer);
----------------
sprintf can be avoided

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:363
+      return;
+    char buffer[100];
+    sprintf(buffer, "%5.2f%%",
----------------
same here

================
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;
+    }
----------------
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?

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:619
+
+    MEMPROF_FREE_HOOK(ptr);
+  }
----------------
Better to call MEMPROF_FREE_HOOK before ptr is released to allocator

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:679
+
+  uptr AllocationSize(uptr p) {
+    MemprofChunk *m = GetMemprofChunkByAddr(p);
----------------
can you use "uptr chunk_beg = p - kChunkHeaderSize;" as in deallocate ?
to remove one kAllocBegMagic user

================
Comment at: compiler-rt/lib/memprof/memprof_allocator.cpp:822-829
+
+uptr memprof_mz_size(const void *ptr) {
+  return instance.AllocationSize(reinterpret_cast<uptr>(ptr));
+}
+
+void memprof_mz_force_lock() { instance.ForceLock(); }
+
----------------
unused?

================
Comment at: compiler-rt/lib/memprof/memprof_flags.inc:19-25
+MEMPROF_FLAG(
+    bool, replace_str, true,
+    "If set, uses custom wrappers and replacements for libc string functions "
+    "to find more errors.")
+MEMPROF_FLAG(
+    bool, replace_intrin, true,
+    "If set, uses custom wrappers for memset/memcpy/memmove intrinsics.")
----------------
I think both these other flags copied from asan were added after asan release. So asan needed to them to avoid breaking existing users.
Existing users is not a problem for memprof so I recommend to remove them.

Also in future it's going to be easier to add them than remove without breaking users. So I advice to review the list and keep as few as possible.

================
Comment at: compiler-rt/lib/memprof/memprof_flags.inc:27
+MEMPROF_FLAG(
+    int, sleep_before_dying, 0,
+    "Number of seconds to sleep between printing an error report and "
----------------
same for sleep_* 

================
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
+}
----------------
I think it's not needed.
Only asan has this and it was not updated as expected.

================
Comment at: compiler-rt/lib/memprof/memprof_interceptors.cpp:130
+#include "sanitizer_common/sanitizer_common_interceptors.inc"
+#include "sanitizer_common/sanitizer_signal_interceptors.inc"
+
----------------
memprof does not need intercept signal related calls?

================
Comment at: compiler-rt/lib/memprof/memprof_mapping.h:18-20
+#if defined(MEMPROF_SHADOW_SCALE)
+static const u64 kDefaultShadowScale = MEMPROF_SHADOW_SCALE;
+#else
----------------
not needed

================
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
----------------
this looks like some pieces of debugging code

================
Comment at: compiler-rt/lib/memprof/memprof_posix.cpp:19-33
+#include "memprof_interceptors.h"
+#include "memprof_internal.h"
+#include "memprof_mapping.h"
+#include "memprof_report.h"
+#include "memprof_stack.h"
+#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_posix.h"
----------------
number of includes in many files is suspiciously large.

================
Comment at: compiler-rt/lib/memprof/memprof_posix.cpp:41
+static bool tsd_key_inited = false;
+void MemprofTSDInit(void (*destructor)(void *tsd)) {
+  CHECK(!tsd_key_inited);
----------------
it's already in __memprof namespace
maybe prefix can be avoided.

================
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,
----------------
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

================
Comment at: compiler-rt/lib/memprof/memprof_rtl.cpp:177
+  // dlopen() and the platform supports it.
+  if (SANITIZER_SUPPORTS_INIT_FOR_DLOPEN && UNLIKELY(HandleDlopenInit())) {
+    memprof_init_is_running = false;
----------------
this is Darwin specific stuff SANITIZER_SUPPORTS_INIT_FOR_DLOPEN, should not be needed.

================
Comment at: compiler-rt/lib/memprof/memprof_rtl.cpp:183-184
+
+  MemprofCheckIncompatibleRT();
+  MemprofCheckDynamicRTPrereqs();
+  AvoidCVE_2016_2143();
----------------
Probably not needed, I don't see similar code in other sanitizers

================
Comment at: compiler-rt/lib/memprof/memprof_rtl.cpp:185
+  MemprofCheckDynamicRTPrereqs();
+  AvoidCVE_2016_2143();
+
----------------
maybe not needed. it's something s390 specific and old

================
Comment at: compiler-rt/lib/memprof/memprof_stack.cpp:32-50
+class ScopedUnwinding {
+public:
+  explicit ScopedUnwinding(MemprofThread *t) : thread(t) {
+    if (thread) {
+      can_unwind = !thread->isUnwinding();
+      thread->setUnwinding(true);
+    }
----------------
most sanitizers does not need this
I suspect that it's not needed here.

================
Comment at: compiler-rt/lib/memprof/memprof_stack.h:54
+
+#define GET_STACK_TRACE_SIGNAL(sig)                                            \
+  BufferedStackTrace stack;                                                    \
----------------
unused here and in asan

================
Comment at: compiler-rt/lib/memprof/memprof_stats.cpp:25
+void MemprofStats::Clear() {
+  CHECK(REAL(memset));
+  REAL(memset)(this, 0, sizeof(MemprofStats));
----------------
I assuming that this call only one per thread and can happen early it can be replaced with internal_memset
and CHECK removed

internal_memset always works
REAL(memset) can be better optimized.

================
Comment at: compiler-rt/lib/memprof/memprof_thread.cpp:100
+
+  malloc_storage().CommitBack();
+  if (common_flags()->use_sigaltstack)
----------------
I suspect no signal related code is needed for memprof.
Asan handles some signals which should not be needed there.

================
Comment at: compiler-rt/lib/memprof/memprof_thread.cpp:110
+
+void MemprofThread::StartSwitchFiber(uptr bottom, uptr size) {
+  if (atomic_load(&stack_switching_, memory_order_relaxed)) {
----------------
Asan version is over complicated as it expects call for stack bounds from other threads (LSAN).
I suspect it's not true for memprof.
You can take a look at corresponding code in Msan.

However all these stuff is needed for __sanitizer_start_switch_fiber implementation which is called by some libs only with asan. I see one boost.
Msan didn't need it until recent GO integration.
Unless you are going to update that software it's just a dead code. I advise to remove it and add only when needed.

================
Comment at: compiler-rt/lib/memprof/memprof_thread.h:37
+// so we can find them by tid even if the thread is long dead.
+class MemprofThreadContext : public ThreadContextBase {
+public:
----------------
struct?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120