<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 15, 2013 at 4:21 PM, Alexey Samsonov <span dir="ltr"><<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><div class="im">On Tue, Oct 15, 2013 at 4:16 PM, Dmitry Vyukov <span dir="ltr"><<a href="mailto:dvyukov@google.com" target="_blank">dvyukov@google.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On Tue, Oct 15, 2013 at 4:13 PM, Alexey Samsonov <<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>> wrote:<br>


><br>
> On Tue, Oct 15, 2013 at 4:09 PM, Dmitry Vyukov <<a href="mailto:dvyukov@google.com" target="_blank">dvyukov@google.com</a>> wrote:<br>
>><br>
>> the following lives in sanitizer_linux.h:<br>
>><br>
>> uptr internal_getdents(fd_t fd, struct linux_dirent *dirp, unsigned int<br>
>> count);<br>
>> uptr internal_prctl(int option, uptr arg2, uptr arg3, uptr arg4, uptr<br>
>> arg5);<br>
>> uptr internal_sigaltstack(const struct sigaltstack* ss,<br>
>>                           struct sigaltstack* oss);<br>
>><br>
>> sigaction naturally fits here<br>
><br>
><br>
> I disagree. I find it more convenient to have all re-definitions of POSIX<br>
> structures in one place. Also,<br>
> sanitizer_platform_limits_posix.cc has some useful compiler checks verifying<br>
> that our re-definitions of<br>
> POSIX structs are actually correct.<br>
<br>
<br>
</div>Do you mean only struct definititions?<br></blockquote></div><div>Yeah, I've meant kernel_sigset_t and kernel_sigaction_t struct definitions.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">


What should I move to sanitizer_platform_limits_posix.cc?<br></blockquote></div><div>See CHECK_SIZE_AND_OFFSET macro in that file - can we apply them to our re-definitions of</div><div>kernel_sigset_t and kernel_sigaction_t?<br>
</div></div></div></div></blockquote><div><br></div><div>I'm not sure system definitions for those can be accessed in a portable way.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
</div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
<br>
>> On Tue, Oct 15, 2013 at 4:03 PM, Alexey Samsonov <<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>><br>
>> wrote:<br>
>> ><br>
>> > On Tue, Oct 15, 2013 at 3:31 PM, Dmitry Vyukov <<a href="mailto:dvyukov@google.com" target="_blank">dvyukov@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Author: dvyukov<br>
>> >> Date: Tue Oct 15 06:31:51 2013<br>
>> >> New Revision: 192686<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=192686&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=192686&view=rev</a><br>
>> >> Log:<br>
>> >> tsan: implement internal syscall-based versions of<br>
>> >> sigaction/sigprocmask<br>
>> >> use them in stoptheworld<br>
>> >> fixes applications that intercept sigaction/sigprocmask<br>
>> >><br>
>> >><br>
>> >><br>
>> >> Modified:<br>
>> >><br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_syscalls.inc<br>
>> >>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc<br>
>> >>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.h<br>
>> >><br>
>> >><br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc<br>
>> >><br>
>> >> Modified:<br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_syscalls.inc<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_syscalls.inc?rev=192686&r1=192685&r2=192686&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_syscalls.inc?rev=192686&r1=192685&r2=192686&view=diff</a><br>


>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> ---<br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_syscalls.inc<br>
>> >> (original)<br>
>> >> +++<br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_syscalls.inc<br>
>> >> Tue Oct 15 06:31:51 2013<br>
>> >> @@ -37,6 +37,7 @@<br>
>> >>  #if SANITIZER_LINUX<br>
>> >><br>
>> >>  #include "sanitizer_libc.h"<br>
>> >> +#include "sanitizer_linux.h"<br>
>> >><br>
>> >>  #define PRE_SYSCALL(name)<br>
>> >> \<br>
>> >>    SANITIZER_INTERFACE_ATTRIBUTE void<br>
>> >> __sanitizer_syscall_pre_impl_##name<br>
>> >> @@ -103,10 +104,6 @@ struct sanitizer_kernel_sockaddr {<br>
>> >>    char sa_data[14];<br>
>> >>  };<br>
>> >><br>
>> >> -// Real sigset size is always passed as a syscall argument.<br>
>> >> -// Declare it "void" to catch sizeof(kernel_sigset_t).<br>
>> >> -typedef void kernel_sigset_t;<br>
>> >> -<br>
>> >>  static void kernel_write_iovec(const __sanitizer_iovec *iovec,<br>
>> >>                          SIZE_T iovlen, SIZE_T maxlen) {<br>
>> >>    for (SIZE_T i = 0; i < iovlen && maxlen; ++i) {<br>
>> >><br>
>> >> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc?rev=192686&r1=192685&r2=192686&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc?rev=192686&r1=192685&r2=192686&view=diff</a><br>


>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc<br>
>> >> (original)<br>
>> >> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc Tue Oct<br>
>> >> 15<br>
>> >> 06:31:51 2013<br>
>> >> @@ -602,6 +602,31 @@ uptr internal_sigaltstack(const struct s<br>
>> >>    return internal_syscall(__NR_sigaltstack, ss, oss);<br>
>> >>  }<br>
>> >><br>
>> >> +uptr internal_sigaction(int signum, const kernel_sigaction_t *act,<br>
>> >> +    struct kernel_sigaction_t *oldact) {<br>
>> >> +  return internal_syscall(__NR_rt_sigaction, signum, act, oldact,<br>
>> >> +      sizeof(kernel_sigset_t));<br>
>> >> +}<br>
>> >> +<br>
>> >> +uptr internal_sigprocmask(int how, kernel_sigset_t *set,<br>
>> >> +    kernel_sigset_t *oldset) {<br>
>> >> +  return internal_syscall(__NR_rt_sigprocmask, (uptr)how,<br>
>> >> &set->sig[0],<br>
>> >> +      &oldset->sig[0], sizeof(kernel_sigset_t));<br>
>> >> +}<br>
>> >> +<br>
>> >> +void internal_sigfillset(kernel_sigset_t *set) {<br>
>> >> +  internal_memset(set, 0xff, sizeof(*set));<br>
>> >> +}<br>
>> >> +<br>
>> >> +void internal_sigdelset(kernel_sigset_t *set, int signum) {<br>
>> >> +  signum -= 1;<br>
>> >> +  CHECK_GE(signum, 0);<br>
>> >> +  CHECK_LT(signum, sizeof(*set) * 8);<br>
>> >> +  const uptr idx = signum / (sizeof(set->sig[0]) * 8);<br>
>> >> +  const uptr bit = signum % (sizeof(set->sig[0]) * 8);<br>
>> >> +  set->sig[idx] &= ~(1 << bit);<br>
>> >> +}<br>
>> >> +<br>
>> >>  // ThreadLister implementation.<br>
>> >>  ThreadLister::ThreadLister(int pid)<br>
>> >>    : pid_(pid),<br>
>> >><br>
>> >> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.h<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.h?rev=192686&r1=192685&r2=192686&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.h?rev=192686&r1=192685&r2=192686&view=diff</a><br>


>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.h (original)<br>
>> >> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.h Tue Oct 15<br>
>> >> 06:31:51 2013<br>
>> >> @@ -20,17 +20,39 @@<br>
>> >><br>
>> >>  struct link_map;  // Opaque type returned by dlopen().<br>
>> >>  struct sigaltstack;<br>
>> >> +typedef struct siginfo siginfo_t;<br>
>> >><br>
>> >>  namespace __sanitizer {<br>
>> >>  // Dirent structure for getdents(). Note that this structure is<br>
>> >> different<br>
>> >> from<br>
>> >>  // the one in <dirent.h>, which is used by readdir().<br>
>> >>  struct linux_dirent;<br>
>> >><br>
>> >> +struct kernel_sigset_t {<br>
>> >> +  u8 sig[FIRST_32_SECOND_64(16, 8)];<br>
>> >> +};<br>
>> >> +<br>
>> >> +struct kernel_sigaction_t {<br>
>> >> +  union {<br>
>> >> +    void (*sigaction)(int signo, siginfo_t *info, void *ctx);<br>
>> >> +    void (*handler)(int signo);<br>
>> >> +  };<br>
>> >> +  unsigned long sa_flags;<br>
>> >> +  void (*sa_restorer)(void);<br>
>> >> +  kernel_sigset_t sa_mask;<br>
>> >> +};<br>
>> ><br>
>> ><br>
>> > Hm... Consider moving this to sanitizer_platform_limits_posix.(h|cc).<br>
>> > I think that's where we collect definitions of OS-specific structures.<br>
>> ><br>
>> >><br>
>> >> +<br>
>> >>  // Syscall wrappers.<br>
>> >>  uptr internal_getdents(fd_t fd, struct linux_dirent *dirp, unsigned<br>
>> >> int<br>
>> >> count);<br>
>> >>  uptr internal_prctl(int option, uptr arg2, uptr arg3, uptr arg4, uptr<br>
>> >> arg5);<br>
>> >>  uptr internal_sigaltstack(const struct sigaltstack* ss,<br>
>> >>                            struct sigaltstack* oss);<br>
>> >> +uptr internal_sigaction(int signum, const kernel_sigaction_t *act,<br>
>> >> +    kernel_sigaction_t *oldact);<br>
>> >> +uptr internal_sigprocmask(int how, kernel_sigset_t *set,<br>
>> >> +    kernel_sigset_t *oldset);<br>
>> >> +void internal_sigfillset(kernel_sigset_t *set);<br>
>> >> +void internal_sigdelset(kernel_sigset_t *set, int signum);<br>
>> >> +<br>
>> >>  #ifdef __x86_64__<br>
>> >>  uptr internal_clone(int (*fn)(void *), void *child_stack, int flags,<br>
>> >> void<br>
>> >> *arg,<br>
>> >>                      int *parent_tidptr, void *newtls, int<br>
>> >> *child_tidptr);<br>
>> >><br>
>> >> Modified:<br>
>> >><br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc?rev=192686&r1=192685&r2=192686&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc?rev=192686&r1=192685&r2=192686&view=diff</a><br>


>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> ---<br>
>> >><br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc<br>
>> >> (original)<br>
>> >> +++<br>
>> >><br>
>> >> compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc<br>
>> >> Tue Oct 15 06:31:51 2013<br>
>> >> @@ -252,12 +252,12 @@ static int TracerThread(void* argument)<br>
>> >>    // the mask we inherited from the caller thread.<br>
>> >>    for (uptr signal_index = 0; signal_index <<br>
>> >> ARRAY_SIZE(kUnblockedSignals);<br>
>> >>         signal_index++) {<br>
>> >> -    struct sigaction new_sigaction;<br>
>> >> +    kernel_sigaction_t new_sigaction;<br>
>> >>      internal_memset(&new_sigaction, 0, sizeof(new_sigaction));<br>
>> >> -    new_sigaction.sa_sigaction = TracerThreadSignalHandler;<br>
>> >> +    new_sigaction.sigaction = TracerThreadSignalHandler;<br>
>> >>      new_sigaction.sa_flags = SA_ONSTACK | SA_SIGINFO;<br>
>> >> -    sigfillset(&new_sigaction.sa_mask);<br>
>> >> -    sigaction(kUnblockedSignals[signal_index], &new_sigaction, NULL);<br>
>> >> +    internal_sigfillset(&new_sigaction.sa_mask);<br>
>> >> +    internal_sigaction(kUnblockedSignals[signal_index],<br>
>> >> &new_sigaction,<br>
>> >> NULL);<br>
>> >>    }<br>
>> >><br>
>> >>    int exit_code = 0;<br>
>> >> @@ -307,9 +307,9 @@ NOINLINE static void WipeStack() {<br>
>> >><br>
>> >>  // We have a limitation on the stack frame size, so some stuff had to<br>
>> >> be<br>
>> >> moved<br>
>> >>  // into globals.<br>
>> >> -static sigset_t blocked_sigset;<br>
>> >> -static sigset_t old_sigset;<br>
>> >> -static struct sigaction old_sigactions[ARRAY_SIZE(kUnblockedSignals)];<br>
>> >> +static kernel_sigset_t blocked_sigset;<br>
>> >> +static kernel_sigset_t old_sigset;<br>
>> >> +static kernel_sigaction_t<br>
>> >> old_sigactions[ARRAY_SIZE(kUnblockedSignals)];<br>
>> >><br>
>> >>  class StopTheWorldScope {<br>
>> >>   public:<br>
>> >> @@ -323,21 +323,21 @@ class StopTheWorldScope {<br>
>> >>      // We cannot allow user-defined handlers to run while the<br>
>> >> ThreadSuspender<br>
>> >>      // thread is active, because they could conceivably call some libc<br>
>> >> functions<br>
>> >>      // which modify errno (which is shared between the two threads).<br>
>> >> -    sigfillset(&blocked_sigset);<br>
>> >> +    internal_sigfillset(&blocked_sigset);<br>
>> >>      for (uptr signal_index = 0; signal_index <<br>
>> >> ARRAY_SIZE(kUnblockedSignals);<br>
>> >>           signal_index++) {<br>
>> >>        // Remove the signal from the set of blocked signals.<br>
>> >> -      sigdelset(&blocked_sigset, kUnblockedSignals[signal_index]);<br>
>> >> +      internal_sigdelset(&blocked_sigset,<br>
>> >> kUnblockedSignals[signal_index]);<br>
>> >>        // Install the default handler.<br>
>> >> -      struct sigaction new_sigaction;<br>
>> >> +      kernel_sigaction_t new_sigaction;<br>
>> >>        internal_memset(&new_sigaction, 0, sizeof(new_sigaction));<br>
>> >> -      new_sigaction.sa_handler = SIG_DFL;<br>
>> >> -      sigfillset(&new_sigaction.sa_mask);<br>
>> >> -      sigaction(kUnblockedSignals[signal_index], &new_sigaction,<br>
>> >> +      new_sigaction.handler = SIG_DFL;<br>
>> >> +      internal_sigfillset(&new_sigaction.sa_mask);<br>
>> >> +      internal_sigaction(kUnblockedSignals[signal_index],<br>
>> >> &new_sigaction,<br>
>> >>                        &old_sigactions[signal_index]);<br>
>> >>      }<br>
>> >>      int sigprocmask_status =<br>
>> >> -        sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);<br>
>> >> +        internal_sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);<br>
>> >>      CHECK_EQ(sigprocmask_status, 0); // sigprocmask should never fail<br>
>> >>      // Make this process dumpable. Processes that are not dumpable<br>
>> >> cannot<br>
>> >> be<br>
>> >>      // attached to.<br>
>> >> @@ -355,10 +355,10 @@ class StopTheWorldScope {<br>
>> >>      // Restore the signal handlers.<br>
>> >>      for (uptr signal_index = 0; signal_index <<br>
>> >> ARRAY_SIZE(kUnblockedSignals);<br>
>> >>           signal_index++) {<br>
>> >> -      sigaction(kUnblockedSignals[signal_index],<br>
>> >> +      internal_sigaction(kUnblockedSignals[signal_index],<br>
>> >>                  &old_sigactions[signal_index], NULL);<br>
>> >>      }<br>
>> >> -    sigprocmask(SIG_SETMASK, &old_sigset, &old_sigset);<br>
>> >> +    internal_sigprocmask(SIG_SETMASK, &old_sigset, &old_sigset);<br>
>> >>    }<br>
>> >><br>
>> >>   private:<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> > --<br>
>> > Alexey Samsonov, MSK<br>
><br>
><br>
><br>
><br>
> --<br>
> Alexey Samsonov, MSK<br>
</div></div></blockquote></div></div></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div>
</font></span></div></div>
</blockquote></div><br></div></div>