[memprof] Simplify initialized flags.

[snehasish]

Summary

As discussed in D145428, the memprof_init_is_running check can be moved
to the end of the initialization routine to avoid intercepting
allocations during initialization. Also, the memprof_init_done flag can
be removed and replaced with memprof_inited. Finally, memprof_inited can
also be moved to the end of the method.

Tested on the existing check-memprof tests; memprof profile collection
succeeded on a large internal workload.

[snehasish]

compiler-rt/lib/memprof/memprof_rtl.cpp

198

@tejohnson

I stepped through the code for ThreadStart and identified potential allocation calls. I found that:

• The thread itself uses MmapOrDie [1]
• The thread registry uses placement new [2]
• The allocator context uses placement new [3]
• The sanitizer thread registry push_back calls sanitizer internal Realloc [4]

So perhaps this comment is stale? Either way, I think even if there is an allocation call, intercepting it isn't worthwhile. Do you have any concerns based on this comment?

[1] https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/memprof/memprof_thread.cpp#L79
[2] https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/memprof/memprof_thread.cpp#L60
[3] https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/memprof/memprof_thread.cpp#L48
[4] https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp#L147

[davidxl]

looks good to me (and less confusing ;) ).

[snehasish]

clang-format, rebase.

[tejohnson]

lgtm, although you may want to try to test I suggest below if we want to be more sure that my comment is stale.

compiler-rt/lib/memprof/memprof_rtl.cpp

198

I can't recall exactly what motivated this, but I don't think it was wanting to intercept those allocation calls. I'm guessing that at one point the code was structured so that making an allocation without things being inited properly caused a crash or other issue (I don't think there should be anything special about malloc vs new in that regard - we try to intercept both). If you want to be extra sure, you could just add a temporary call to malloc in the code below and run a simple instrumented test to make sure nothing breaks.

[snehasish]

compiler-rt/lib/memprof/memprof_rtl.cpp

198

Done, added a call to malloc in the MemprofInitInternal and stepped through it and it looks good. Also Symbolizer::LateInitialize() allocates memory (intercepted via DlsymAlloc) so there was already an allocation after moving the inited flag.

compiler-rt/lib/memprof/memprof_allocator.cpp

void Print(const MemInfoBlock &M, const u64 id, bool print_terse) {
   }
 }
 } // namespace
 
 static int GetCpuId(void) {
   // _memprof_preinit is called via the preinit_array, which subsequently calls
   // malloc. Since this is before _dl_init calls VDSO_SETUP, sched_getcpu
   // will seg fault as the address of __vdso_getcpu will be null.
-  if (!memprof_init_done)
+  if (!memprof_inited)
     return -1;
   return sched_getcpu();
 }
 
 // Compute the timestamp in ms.
 static int GetTimestamp(void) {
   // timespec_get will segfault if called from dl_init
   if (!memprof_timestamp_inited) {
void Deallocate(void *ptr, uptr delete_size, uptr delete_alignment,
 
     RunFreeHooks(ptr);
 
     uptr chunk_beg = p - kChunkHeaderSize;
     MemprofChunk *m = reinterpret_cast<MemprofChunk *>(chunk_beg);
 
     u64 user_requested_size =
         atomic_exchange(&m->user_requested_size, 0, memory_order_acquire);
-    if (memprof_inited && memprof_init_done &&
-        atomic_load_relaxed(&constructed) &&
+    if (memprof_inited && atomic_load_relaxed(&constructed) &&
         !atomic_load_relaxed(&destructing)) {
       u64 c = GetShadowCount(p, user_requested_size);
       long curtime = GetTimestamp();
 
       MemInfoBlock newMIB(user_requested_size, c, m->timestamp_ms, curtime,
                           m->cpu_id, GetCpuId());
       InsertOrMerge(m->alloc_context_id, newMIB, MIBMap);
     }

compiler-rt/lib/memprof/memprof_internal.h

 void *TSDGet();
 void TSDSet(void *tsd);
 void PlatformTSDDtor(void *tsd);
 
 void *MemprofDlSymNext(const char *sym);
 
 extern int memprof_inited;
 extern int memprof_timestamp_inited;
-extern int memprof_init_done;
 // Used to avoid infinite recursion in __memprof_init().
 extern bool memprof_init_is_running;
 extern void (*death_callback)(void);
 extern long memprof_init_timestamp_s;
 
 } // namespace __memprof
 
 #endif // MEMPROF_INTERNAL_H

compiler-rt/lib/memprof/memprof_rtl.cpp

 
 static void CheckUnwind() {
   GET_STACK_TRACE(kStackTraceMax, common_flags()->fast_unwind_on_check);
   stack.Print();
 }
 
 // -------------------------- Globals --------------------- {{{1
 int memprof_inited;
-int memprof_init_done;
 bool memprof_init_is_running;
 int memprof_timestamp_inited;
 long memprof_init_timestamp_s;
 
 uptr kHighMemEnd;
 
 // -------------------------- Run-time entry ------------------- {{{1
 // exported functions
static void MemprofInitInternal() {
 
   InitializeShadowMemory();
 
   TSDInit(PlatformTSDDtor);
   InstallDeadlySignalHandlers(MemprofOnDeadlySignal);
 
   InitializeAllocator();
 
-  // On Linux MemprofThread::ThreadStart() calls malloc() that's why
-  // memprof_inited should be set to 1 prior to initializing the threads.
-  memprof_inited = 1;
-  memprof_init_is_running = false;
-
   if (flags()->atexit)
     Atexit(memprof_atexit);
 
   InitializeCoverage(common_flags()->coverage, common_flags()->coverage_dir);
 
   // interceptors
   InitTlsSize();
 
   // Create main thread.
   MemprofThread *main_thread = CreateMainThread();
   CHECK_EQ(0, main_thread->tid());
   force_interface_symbols(); // no-op.
   SanitizerInitializeUnwinder();
 
   Symbolizer::LateInitialize();
 
   VReport(1, "MemProfiler Init done\n");
 
-  memprof_init_done = 1;
+  memprof_init_is_running = false;
+  memprof_inited = 1;
 }
 
 void MemprofInitTime() {
   if (LIKELY(memprof_timestamp_inited))
     return;
   timespec ts;
   clock_gettime(CLOCK_REALTIME, &ts);
   memprof_init_timestamp_s = ts.tv_sec;