Page MenuHomePhabricator

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

Authored by kubamracek on Mar 2 2018, 4:41 PM.

Diff Detail

Event Timeline

kubamracek created this revision.Mar 2 2018, 4:41 PM
aprantl added inline comments.Mar 2 2018, 4:53 PM
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 added a subscriber: xiaobai.Mar 2 2018, 5:35 PM
xiaobai added inline comments.
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 added a subscriber: labath.Mar 2 2018, 6:57 PM
labath added inline comments.
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...

::pthread_cancel is available for NetBSD as well.

xiaobai added inline comments.Mar 2 2018, 11:02 PM
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 added inline comments.Mar 4 2018, 7:11 PM
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 added inline comments.Mar 4 2018, 7:33 PM
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 added inline comments.Mar 4 2018, 7:43 PM
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).

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

labath added a comment.Mar 6 2018, 4:51 AM

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.