labath added inline comments.
================
Comment at: source/Host/posix/HostThreadPosix.cpp:44
if (IsJoinable()) {
#ifndef __ANDROID__
#ifndef __FreeBSD__
----------------
xiaobai wrote:
> 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.
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).
https://reviews.llvm.org/D44056
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
