This is an archive of the discontinued LLVM Phabricator instance.

Add NetBSD improvements in sanitizers
ClosedPublic

Authored by krytarowski on Oct 20 2017, 8:28 AM.

Details

Summary

Changes:

  • Add initial msan stub support.
  • Handle NetBSD specific pthread_setname_np(3).
  • NetBSD supports attribute((tls_model("initial-exec"))), define it in SANITIZER_TLS_INITIAL_EXEC_ATTRIBUTE.
  • Add ReExec() specific bits for NetBSD.
  • Simplify code and add syscall64 and syscall_ptr for !NetBSD.
  • Correct bunch of syscall wrappers for NetBSD.
  • Disable test/tsan/map32bit on NetBSD as not applicable.
  • Port test/tsan/strerror_r to a POSIX-compliant OSes.
  • Disable __libc_stack_end on NetBSD.
  • Disable ReadNullSepFileToArray() on NetBSD.
  • Define struct_ElfW_Phdr_sz, detected missing symbol by msan.
  • Change type of __sanitizer_FILE from void to char. This helps to reuse this type as an array. Long term it will be properly implemented along with SANITIZER_HAS_STRUCT_FILE setting to 1.
  • Add initial NetBSD support in lib/tsan/go/buildgo.sh.
  • Correct referencing stdout and stderr in tsan_interceptors.cc on NetBSD.
  • Document NetBSD x86_64 specific virtual memory layout in tsan_platform.h.
  • Port tests/rtl/tsan_test_util_posix.cc to NetBSD.
  • Enable NetBSD tests in test/msan/lit.cfg.
  • Enable NetBSD tests in test/tsan/lit.cfg.

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Oct 20 2017, 8:28 AM
vitalybuka edited edge metadata.Oct 20 2017, 11:10 AM

Why does this need to be in a single patch?

Why does this need to be in a single patch?

Changes are relatively small and can be related to one topic 'sanitizers/NetBSD'. Keeping them in small patches does not speed up the review (D37319).

I'm still working on the remaining tsan and msan bits and I want to unload my local sources from patches that could be just upstreamed.

krytarowski added a comment.EditedOct 20 2017, 1:10 PM

With local kludges I can get:

check-tsan:

  Expected Passes    : 248
  Expected Failures  : 1
  Unsupported Tests  : 83
  Unexpected Failures: 44

I'm struggling with the remaining bugs.

Upstreaming this delta will shorten my list of local changes and ease the process of rebasing to newer snapshots.

dvyukov added inline comments.Oct 21 2017, 12:03 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
4420

Please move everything except "void *ctx" into COMMON_INTERCEPTOR_ENTER scope as all other interceptors do. This reduces chances of actual code appear outside of interceptor scope.

4424

This should be:

COMMON_INTERCEPTOR_READ_STRING(ctx, name, 0);

reading our local stack variable does not have any effect.

lib/sanitizer_common/sanitizer_linux.cc
191

Uh, this doubles number of ifdefs because of netbsd. The code will be much cleaner if we introduce internal_syscall/syscall64/syscall_ptr for all OSes. Just on all other OSes they all will be defined as syscall. Are there any major obstacles for doing it this way?

429

nanosleep declared as returning int, why is this syscall_ptr?

lib/tsan/rtl/tsan_platform.h
47

s/startx/starts/

krytarowski added inline comments.Oct 23 2017, 7:14 AM
lib/sanitizer_common/sanitizer_linux.cc
429

_ptr here means that input arguments are pointers.

dvyukov added inline comments.Oct 23 2017, 7:36 AM
lib/sanitizer_common/sanitizer_linux.cc
429

I see.
Is it possible to use syscall64 (__syscall) for all syscalls?

krytarowski added inline comments.Oct 23 2017, 7:39 AM
lib/sanitizer_common/sanitizer_linux.cc
429

Not in a portable way across architectures.

dvyukov added inline comments.Oct 23 2017, 7:45 AM
lib/sanitizer_common/sanitizer_linux.cc
429

This just looks messy code-wise and additional maintenance burden (e.g. we change the !NETBSD case, but forgot to update the NETBSD case):

#if SANITIZER_NETBSD
  return internal_syscall_ptr(SYSCALL(execve), (uptr)filename, (uptr)argv,
                          (uptr)envp);
#else
  return internal_syscall(SYSCALL(execve), (uptr)filename, (uptr)argv,
                          (uptr)envp);
#endif

What do you think about introducing internal_syscall/syscall64/syscall_ptr for all OSes?

krytarowski added inline comments.Oct 23 2017, 7:53 AM
lib/sanitizer_common/sanitizer_linux.cc
429

Agreed. I'm still testing local patches. Once verified I will upload them!

krytarowski edited the summary of this revision. (Show Details)
krytarowski marked 9 inline comments as done.
dvyukov edited edge metadata.Oct 24 2017, 12:40 AM

Besides the two details this generally looks good to me.

lib/sanitizer_common/sanitizer_common_interceptors.inc
4422

COMMON_INTERCEPTOR_* is interface with the rest of the system (asan, tsan, msan, lsan, ...). It _must_ be platform-independent. We don't want gazillion of OS/arch-specific details plague rest of the codebase. See how you now need to update all of asan/tsan/msan (and forgot esan/esan_interceptors.cpp).
Keep the interface as it is now and handle NetBSDs-specific difference in NetBSDs-specific interceptor. Namely, do the printf here and pass formatted name to COMMON_INTERCEPTOR_SET_PTHREAD_NAME.

lib/tsan/rtl/tsan_platform_posix.cc
127 ↗(On Diff #119894)

Why is this necessary?
VdsoBeg is 0xf000000000000000ull, why do we need to continue after this address on NetBSD?

Do you have commit access?

Do you have commit access?

Yes.

krytarowski added inline comments.Oct 24 2017, 5:29 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
4422

Unless esan will be simple to be ported, it will wait for finishing LLDB.

Right now lsan isn't possible due to StopTheWorld() OS capability requirements. I plan to introduce a new ptrace(2) API to self-introspect in the future. But this must wait for finishing ptrace(2)/signals/etc correctness for debuggers (there are few bugs out there).

I'm going to research this NetBSD-specific interceptor.

lib/tsan/rtl/tsan_platform_posix.cc
127 ↗(On Diff #119894)

This can be considered as a leftover from porting efforts. I will drop it.

krytarowski added inline comments.Oct 24 2017, 9:13 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
4422

I'm not sure I understand correctly the suggestion, but I think I need to partially revert to the previous state and reintroduce internal_snprintf() into SANITIZER_INTERCEPT_PTHREAD_SETNAME_NP again.

dvyukov added inline comments.Oct 24 2017, 9:26 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
4422

I mean that even if we split the interceptor itself into netbsd/!netbsd versions, they both should call the SANITIZER_INTERCEPT_PTHREAD_SETNAME_NP with the same arguments, so that the rest of the system is not affected by the difference. Then you don't need to touch this in asan/tsan/msan (and esan).

krytarowski added inline comments.Oct 24 2017, 9:34 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
4422

I think you mean to preserve the same COMMON_INTERCEPTOR_SET_PTHREAD_NAME macro. I'm going to upload a new patch.

Apply changes from review.

krytarowski edited the summary of this revision. (Show Details)Oct 24 2017, 10:35 AM
krytarowski marked 5 inline comments as done.Oct 24 2017, 10:49 AM

Is it ready now?

dvyukov accepted this revision.Oct 25 2017, 9:48 AM

Looks good to me. Thanks for bearing with me.

I like how we get rid of a bunch of #if SANITIZER_NETBSD in internal_* functions.

This revision is now accepted and ready to land.Oct 25 2017, 9:48 AM
krytarowski closed this revision.Oct 25 2017, 10:09 AM

It apparently broke Linux..

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fuzzer/builds/9764/steps/annotate/logs/stdio

Investigating why there is unknown internal_syscall_ptr.

eugenis edited edge metadata.Oct 25 2017, 11:21 AM

Please revert unless you can fix quickly.