This is an archive of the discontinued LLVM Phabricator instance.

[memprof] Simplify initialized flags.

Authored by snehasish on Mar 7 2023, 3:44 PM.



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.

Diff Detail

Event Timeline

snehasish created this revision.Mar 7 2023, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 3:44 PM
Herald added a subscriber: Enna1. · View Herald Transcript
snehasish requested review of this revision.Mar 7 2023, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 3:44 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
snehasish added a subscriber: davidxl.
snehasish added inline comments.Mar 7 2023, 4:02 PM


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?


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

snehasish updated this revision to Diff 503192.Mar 7 2023, 5:09 PM

clang-format, rebase.

tejohnson accepted this revision.Mar 7 2023, 5:14 PM

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


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.

This revision is now accepted and ready to land.Mar 7 2023, 5:14 PM
snehasish added inline comments.Mar 8 2023, 10:01 AM

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.

This revision was automatically updated to reflect the committed changes.