[PATCH] D64057: Add NetBSD/amd64 LSan support

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc:423
   StopTheWorldScope() {
+#ifdef PR_GET_DUMPABLE
     // Make this process dumpable. Processes that are not dumpable cannot be
----------------
now it will silently compile if we remove include <sys/prctl.h> 

it should be internal_set_dumpable with noop on NETBSD
or better two StopTheWorldScope for Linux and NETBSD





================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc:512
     // by the tracer thread.
     internal_prctl(PR_SET_PTRACER, tracer_pid, 0, 0, 0);
+#endif
----------------
internal_set_ptracer


================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc:585
 
 tid_t SuspendedThreadsListLinux::GetThreadID(uptr index) const {
   CHECK_LT(index, thread_ids_.size());
----------------
SuspendedThreadsListLinux is also can be forked


================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc:614
+      internal_ptrace(PT_GETREGS, ppid, &regs, (void *)(uptr)tid), &pterrno);
+  if (isErr) {
+    VReport(1,
----------------
krytarowski wrote:
> vitalybuka wrote:
> > can you put this and other forked function into sanitizer_linux.cc/sanitizer_netbst.cc/sanitizer_linux_libcdep.cc ?
> > 
> Do you mean to split `sanitizer_stoptheworld_linux_libcdep.cc` into ptrace-generic and linux and NetBSD?
> 
> I think it is not a good idea:
> 
>  - this cannot work on other ptrace(2) fueled systems (at least NetBSD is the only one having enough batteries to do it in a similar way to Linux)
>  - duplicating or splitting stoptheworld implementation that is a hack is not a good idea, better to keep it in a single file - I don't want to see it spread to multiple files
>  - the amount of ifdefs is not that large and they are only for essential operation
>  - I plan to write an alternative implementation with a dedicated syscall for StopTheWorld(), but that would happen not sooner than in NetBSD 10.0 and we want to keep this ptrace(2) version here for the time being
Sanitizers are full of hack.
Also it looks like you just need to fork completely entire classes like: SuspendedThreadsListLinux, StopTheWorldScope, ThreadSuspender...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64057