[Lldb-commits] [PATCH] D44056: [lldb] Fix "code unreachable" warning in HostThreadPosix::Cancel

[Alex Langford via Phabricator via lldb-commits]

xiaobai added inline comments.


================
Comment at: source/Host/posix/HostThreadPosix.cpp:44
   if (IsJoinable()) {
 #ifndef __ANDROID__
 #ifndef __FreeBSD__
----------------
labath wrote:
> xiaobai wrote:
> > labath wrote:
> > > xiaobai wrote:
> > > > aprantl wrote:
> > > > > 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;
> > > > > }
> > > > > ```
> > > > 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__`.
> > > 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...
> > 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? 
> 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).
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.


https://reviews.llvm.org/D44056