[PATCH] D40349: [LSan] New experimental flag for background leak checking before exit.

[Michail Kashkarov via Phabricator via llvm-commits]

m.kashkarov marked 4 inline comments as done.
m.kashkarov added inline comments.


================
Comment at: lib/lsan/lsan_common.cc:767
+
+void MaybeAppendToLogPath(const char *suffix) {
+  if (common_flags()->leak_check_interval_s == kLeaksIntervalNotSet) return;
----------------
m.ostapenko wrote:
> Why do you need all this stuff? Can't you use separate file descriptor from main report_fd? Because changing report_fd on the fly looks like a bad idea: what will happen if some another thread is reporting concurrently (corrupted reports, I guess)?
If it's ok to create another file only for leak detected in the background thread - i can change the current logging behaviour. 


================
Comment at: lib/lsan/lsan_common.h:155
+
+static const uptr kLeaksIntervalNotSet = 0;
+void MaybeStartBackgroudLeakCheckingThread();
----------------
m.ostapenko wrote:
> You don't need to explicitly initialize global variables with 0.
There is build error `error: uninitialized const ‘__lsan::kLeaksIntervalNotSet’ [-fpermissive]` when not set to 0


================
Comment at: lib/lsan/lsan_interceptors.cc:34
 
+#if SANITIZER_POSIX
+#include "sanitizer_common/sanitizer_posix.h"
----------------
m.ostapenko wrote:
> Please don't. Why do you need this at all?
Oh, didn't notice that it was already included here, tested on gcc without `#include "sanitizer_common/sanitizer_posix.h"`.


================
Comment at: lib/lsan/lsan_interceptors.cc:418
 
+DEFINE_REAL_PTHREAD_FUNCTIONS
+
----------------
m.ostapenko wrote:
> Likewise, why do you need this?
This is for the `real_pthread_create` used in `internal_start_thread`.


Repository:
  rL LLVM

https://reviews.llvm.org/D40349