[lldb] Fix "code unreachable" warning in HostThreadPosix::Cancel

[aprantl]

source/Host/posix/HostThreadPosix.cpp

44

What about:

#ifdef __ANDROID__
    error.SetErrorString("HostThreadPosix::Cancel() not supported on Android");
#else
#ifdef __FreeBSD__
    int err = ::pthread_cancel(m_thread);
    error.SetError(err, eErrorTypePOSIX);
#else
    llvm_unreachable("someone is calling HostThread::Cancel()");
#endif
#endif
  }
  return error;
}

[xiaobai]

source/Host/posix/HostThreadPosix.cpp

44

I agree with Adrian's suggestion, but I would add that you can remove one of the #endif if you use #elif defined(__FreeBSD__) instead of an #else + #ifdef __FreeBSD__.

[labath]

source/Host/posix/HostThreadPosix.cpp

44

I think we can just unify the ANDROID and FreeBSD cases (turn both into unreachable). We only run lldb-server on android, and there we're mostly single-threaded, so there shouldn't be any thread cancelling going on anyway...

[krytarowski]

::pthread_cancel is available for NetBSD as well.

[xiaobai]

source/Host/posix/HostThreadPosix.cpp

44

I don't think we can make the FreeBSD case unreachable (if I understand the code correctly) since that's the one case when ::pthread_cancel is actually getting called.

If we can make the __ANDROID__ case unreachable, this would very easily turn into

#if defined(__FreeBSD__) || defined(__NetBSD__)
    int err = ::pthread_cancel(m_thread);
    error.SetError(err, eErrorTypePOSIX);
#else
    llvm_unreachable("Someone is calling HostThreadPosix::Cancel()");
#endif

I'm somewhat unfamiliar with how exactly this code is used on android. If I understand correctly, you're saying it's used in lldb-server and you meant that lldb-server is mostly single threaded so this code shouldn't get run there?

[labath]

source/Host/posix/HostThreadPosix.cpp

44

Right, sorry, I misinterpreted the ifdefs. We should then merge the android case into the !FreeBsd case like you suggest (although I'm not sure why NetBSD has appeared there now).

[xiaobai]

source/Host/posix/HostThreadPosix.cpp

44

Ah, I added NetBSD since @krytarowski pointed out NetBSD has pthread_cancel available as well. However that probably prevents this patch from being an NFC, so that could probably be added separately.

[labath]

source/Host/posix/HostThreadPosix.cpp

44

I see. Others systems have pthread_cancel as well (android is the main exception here), but that function is very c++-unfriendly, so I think we should stop using it (and that's probably the reason why we have the llvm_unreachable there in the first place).

[clayborg]

Yes we want to avoid ever using this function. It only leads to bad things happening.

[labath]

Since the android part wasn't obviously NFC, I figured I should be the one to do it. And since the pthread_cancel is not available on android, I've had to add the #else clause as well, which should fix the unreachable warning as well. So I guess you could say this got committed as r326777.

source/Host/posix/HostThreadPosix.cpp

Status HostThreadPosix::Join(lldb::thread_result_t *result) {
 
   Reset();
   return error;
 }
 
 Status HostThreadPosix::Cancel() {
   Status error;
   if (IsJoinable()) {
 #ifndef __ANDROID__
 #ifndef __FreeBSD__
     llvm_unreachable("someone is calling HostThread::Cancel()");
-#endif
+#else
     int err = ::pthread_cancel(m_thread);
     error.SetError(err, eErrorTypePOSIX);
+#endif
 #else
     error.SetErrorString("HostThreadPosix::Cancel() not supported on Android");
 #endif
   }
   return error;
 }
 
 Status HostThreadPosix::Detach() {
   Status error;
   if (IsJoinable()) {
     int err = ::pthread_detach(m_thread);
     error.SetError(err, eErrorTypePOSIX);
   }
   Reset();
   return error;
 }