Avoid getpid call after kill

[captain5050]

Summary

kill maybe used to synchronize with ptrace where PTRACE_SYSCALL will run to the next syscall. By having a getpid call after the kill this may cause certain issues with unit tests running with ptrace and tsan. Record the result of the getpid comparison so that a second getpid call isn't necessary after the real kill call.
Do similar for pthread_kill.

[dvyukov]

We are querying own PID, I don't understand how it can be wrong. We definitely did not kill itself if we are still running :)
Please add a test that exposes the problem. Will also help to understand what scenario we are fixing.

[captain5050]

Thanks Dmitry, the problem isn't that the getpid is wrong per se, the scenario is:

• ptraced program self stops with a SIGSTOP either by raise, kill, ..
• ptracing program attaches... and issues PTRACE_SYSCALL to resume the ptraced program until the next syscall,
• normally this would be some expected syscall, with tsan it will always be the getpid here.

As we'd like tests to pass with tsan, then we either teach the code to ignore getpid when running with tsan or avoid the interception using assembly - both of which are somewhat ugly workarounds.
While adding a test is attractive the harnessing of a parent/child process plus variants for OS and architecture, mean test code would dwarf what is a somewhat straightforward change.

[dvyukov]

In D88616#2306269, @captain5050 wrote:

Thanks Dmitry, the problem isn't that the getpid is wrong per se, the scenario is:

• ptraced program self stops with a SIGSTOP either by raise, kill, ..
• ptracing program attaches... and issues PTRACE_SYSCALL to resume the ptraced program until the next syscall,
• normally this would be some expected syscall, with tsan it will always be the getpid here.

As we'd like tests to pass with tsan, then we either teach the code to ignore getpid when running with tsan or avoid the interception using assembly - both of which are somewhat ugly workarounds.
While adding a test is attractive the harnessing of a parent/child process plus variants for OS and architecture, mean test code would dwarf what is a somewhat straightforward change.

I see.
A test for this would help to keep this property in future.

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

TSAN_INTERCEPTOR(int, raise, int sig) {
   return res;
 }
 
 TSAN_INTERCEPTOR(int, kill, int pid, int sig) {
   SCOPED_TSAN_INTERCEPTOR(kill, pid, sig);
   ThreadSignalContext *sctx = SigCtx(thr);
   CHECK_NE(sctx, 0);
   int prev = sctx->int_signal_send;
-  if (pid == (int)internal_getpid()) {
+  bool same_pid = (pid == (int)internal_getpid());
+  if (same_pid) {
     sctx->int_signal_send = sig;
   }
   int res = REAL(kill)(pid, sig);
-  if (pid == (int)internal_getpid()) {
+  if (same_pid) {
     CHECK_EQ(sctx->int_signal_send, sig);
     sctx->int_signal_send = prev;
   }
   return res;
 }
 
 TSAN_INTERCEPTOR(int, pthread_kill, void *tid, int sig) {
   SCOPED_TSAN_INTERCEPTOR(pthread_kill, tid, sig);
   ThreadSignalContext *sctx = SigCtx(thr);
   CHECK_NE(sctx, 0);
   int prev = sctx->int_signal_send;
-  if (tid == pthread_self()) {
+  bool same_thread = (tid == pthread_self());
+  if (same_thread) {
     sctx->int_signal_send = sig;
   }
   int res = REAL(pthread_kill)(tid, sig);
-  if (tid == pthread_self()) {
+  if (same_thread) {
     CHECK_EQ(sctx->int_signal_send, sig);
     sctx->int_signal_send = prev;
   }
   return res;
 }
 
 TSAN_INTERCEPTOR(int, gettimeofday, void *tv, void *tz) {
   SCOPED_TSAN_INTERCEPTOR(gettimeofday, tv, tz);