Page MenuHomePhabricator

Add NetBSD support in sanitizer common
AbandonedPublic

Authored by krytarowski on Jul 9 2017, 9:45 AM.

Details

Summary

This includes common_sanitizer and interception pieces.

Part of the code inspired by the original work on libsanitizer in GCC 5.4 by Christos Zoluas.

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Jul 9 2017, 9:45 AM

Almost all tests pass with check-sanitizer.

Failing Tests (6):
    SanitizerCommon-Unit :: ./Sanitizer-i386-Test/SanitizerCommon.IsAccessibleMemoryRange
    SanitizerCommon-Unit :: ./Sanitizer-i386-Test/SanitizerCommon.PthreadDestructorIterations
    SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.IsAccessibleMemoryRange
    SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.PthreadDestructorIterations
    SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.SizeClassAllocator64MapUnmapCallback
    SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.SizeClassAllocator64Overflow

  Expected Passes    : 255
  Unexpected Failures: 6

These failures are caused by Operating System bugs, they have been investigated and reported upstream in NetBSD.

kern/52384 write(2) from a nonreadable memory region returns EACCES
lib/52386 pthread(3) doesn't respect PTHREAD_DESTRUCTOR_ITERATIONS

These tests stopped reporting failures, temporarily I moved on.

SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.SizeClassAllocator64MapUnmapCallback
SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.SizeClassAllocator64Overflow

Joerg asked me to prepare the list of syscalls used by sanitizers, I'm going to do it and move this discussion here.

With recent changes to NetBSD-current, all tests pass.

Testing Time: 5.35s
  Expected Passes    : 261

On request by @joerg, I'm pasting the list of syscalls that are wrapped with internal_syscall().

$ git grep internal_syscall|grep SYSCALL|sed 's!^.*SYSCALL(!!'|sed 's!).*$!!'|grep -v define|sort -u|nl
     1  clone
     2  close
     3  dup2
     4  dup3
     5  execve
     6  exit
     7  exit_group
     8  fork
     9  fstat
    10  fstat64
    11  fstatat
    12  ftruncate
    13  futex
    14  getdents
    15  getdents64
    16  getdirentries
    17  getpid
    18  getppid
    19  getrandom
    20  gettid
    21  gettimeofday
    22  lseek
    23  lstat
    24  lstat64
    25  mmap
    26  mmap2
    27  mprotect
    28  munmap
    29  nanosleep
    30  newfstatat
    31  open
    32  openat
    33  prctl
    34  ptrace
    35  read
    36  readlink
    37  readlinkat
    38  rename
    39  renameat
    40  rt_sigaction
    41  rt_sigprocmask
    42  sched_yield
    43  sigaltstack
    44  sigprocmask
    45  stat
    46  stat64
    47  unlink
    48  unlinkat
    49  wait4
    50  write
``
filcab edited edge metadata.Jul 10 2017, 9:40 AM

With recent changes to NetBSD-current, all tests pass.

Testing Time: 5.35s
  Expected Passes    : 261

Is that just check-sanitizer, or check-sanitizer check-interception?

lib/sanitizer_common/sanitizer_platform_limits_posix.h
620

Maybe just have a comment like the one for Android? I don't like having the #if 0.

lib/sanitizer_common/sanitizer_syscall_generic.inc
34

"to address differences in calling conventions (e.g. registers to place the return value in)"
or something like that.

With recent changes to NetBSD-current, all tests pass.

Testing Time: 5.35s
  Expected Passes    : 261

Is that just check-sanitizer, or check-sanitizer check-interception?

This is check-sanitizer, it required code for interception. I will give a try to check-interception.

Hmm, the result of check-interception:

lit.py: /public/pkgsrc-tmp/wip/llvm-all-in-one/work/llvm/utils/lit/lit/discovery.py:224: warning: input '/public/pkgsrc-tmp/wip/llvm-all-in-one/work/build/projects/compiler-rt/test/interception/Unit' contained no tests
-- Testing: 0 tests, 0 threads --
Testing Time: 0.00s

1 warning(s) in tests.
[100%] Built target check-interception

Checking.

  • remove 'if 0' code
  • rephrase description about syscall(2)/__syscall(2)

Ping? Can we get it done before branching llvm 5.0.0?

kcc edited edge metadata.Jul 13 2017, 1:49 PM

Kamil, I'm afraid this patch is too big to be reviewed in one go.
Please split it.
Also, make sure that you reduce the amount of #ifdefs to the absolute minimum and instead move netbsd-specific code to separate file(s).

E.g. adding !defined(NetBSD) to the line containing !defined(APPLE) && !defined(FreeBSD)
is perfectly fine, but adding
uptr internal_waitpid(int pid, int *status, int options) {
#if SANITIZER_NETBSD

into a file called *linux.cc is absolutely not fine -- such code needs to go to a separate file

In D35183#808727, @kcc wrote:

Kamil, I'm afraid this patch is too big to be reviewed in one go.
Please split it.
Also, make sure that you reduce the amount of #ifdefs to the absolute minimum and instead move netbsd-specific code to separate file(s).

E.g. adding !defined(NetBSD) to the line containing !defined(APPLE) && !defined(FreeBSD)
is perfectly fine, but adding
uptr internal_waitpid(int pid, int *status, int options) {
#if SANITIZER_NETBSD

into a file called *linux.cc is absolutely not fine -- such code needs to go to a separate file

I can split it up, but all the *linux.cc files already support at least FreeBSD. Some of them support Android and MacOSX.

What I am asked is to leave alone files used by Linux, Android, FreeBSD in some cases MacOSX and create separate ones just for NetBSD.

There is coming Solaris next. Solaris is pending on bugzilla and they followed the same approach. In future there will certainly join OpenBSD and face the same issue.

Can we just rename linux to posix?

I will try to split the diff into smaller chunks and we will see how to progress.

krytarowski abandoned this revision.Jul 16 2017, 4:53 PM