Page MenuHomePhabricator

[lldb] [MainLoop] Initialize empty sigset_t correctly
ClosedPublic

Authored by mgorny on Feb 8 2019, 9:27 AM.

Details

Summary

Fix MainLoop::RunImpl::get_sigmask() to correctly return empty sigset_t
when SIGNAL_POLLING_UNSUPPORTED is true. On NetBSD (and probably
on some other platforms), integers are not implicitly convertible to
sigset_t, so 'return 0' is erraneous. Instead, sigset_t should be reset
through sigemptyset().

While at it, move common parts out of the #ifdef.

// FTR: I don't know if this wouldn't break Windows. I'd appreciate if somebody could test it. Alternatively, I can watch buildbots after merging.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

mgorny created this revision.Feb 8 2019, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 9:27 AM

I think that sigemptyset(2) is unsupported on Windows.

lldb/source/Host/common/MainLoop.cpp
149 ↗(On Diff #185992)

Shouldn't we initialize sigmask always before adding/deleting particular signals?

149 ↗(On Diff #185992)

Ah, I can see it now. It's passed to pthread_sigmask(3) so it's fine.

mgorny updated this revision to Diff 186011.Feb 8 2019, 11:38 AM

Ok, I see that _WIN32 actually redefines sigset_t, so I've added a separate branch for it.

Ok, I see that _WIN32 actually redefines sigset_t, so I've added a separate branch for it.

Maybe #ifdef LLVM_ON_UNIX?

But to what purpose? I think it's better to use consistent macros to refer to the same scenario.

But to what purpose? I think it's better to use consistent macros to refer to the same scenario.

I had an impression that LLVM_ON_UNIX is LLVM homegrown symbol for this exact purpose to differentiate Windows (out of UNIX [and probably next to Fuchsia today, but it's not supported in LLDB].

labath accepted this revision.Feb 11 2019, 1:12 AM

Lgtm, though I strongly suspect you will need to make sure NetBSD falls into the ! SIGNAL_POLLING_UNSUPPORTED category to have even basic lldb-server functionality working. Have you looked at why that isn't happening?

This revision is now accepted and ready to land.Feb 11 2019, 1:12 AM

I was experimenting with disabling kevent, and apparently no other NetBSD mechanism is supported by LLDB. Noticed this issue, so figured I'll fix it while at it.

This revision was automatically updated to reflect the committed changes.

I was experimenting with disabling kevent, and apparently no other NetBSD mechanism is supported by LLDB. Noticed this issue, so figured I'll fix it while at it.

Ok, thanks. Just FYI: I think you should at least have pselect, but that one is currently not selected by cmake, but it's hardcoded for android. Fixing that should be easy, if you ever need it.