This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Support gdbserver signals
ClosedPublic

Authored by mgorny on Aug 14 2021, 2:56 PM.

Details

Summary

GDB and LLDB use different signal models. GDB uses a predefined set
of signal codes, and maps platform's signos to them. On the other hand,
LLDB has historically simply passed native signos.

In order to improve compatibility between LLDB and gdbserver, the GDB
signal model should be used. However, GDB does not provide a mapping
for all existing signals on Linux and unsupported signals are passed
as 'unknown'. Limiting LLDB to this behavior could be considered
a regression.

To get the best of both worlds, use the LLDB signal model when talking
to lldb-server, and the GDB signal model otherwise. For this purpose,
new versions of lldb-server indicate "native-signals+" via qSupported.
At the same time, we also detect older versions of lldb-server
via QThreadSuffixSupported for backwards compatibility. If neither test
succeeds, we assume gdbserver or another implementation using GDB model.

Diff Detail

Event Timeline

mgorny requested review of this revision.Aug 14 2021, 2:56 PM
mgorny created this revision.
mgorny updated this revision to Diff 366487.Aug 15 2021, 3:51 AM
mgorny edited the summary of this revision. (Show Details)

Added client tests for GDB and LLDB signal variants. Moved signal init into DidLaunchOrAttach, in order to have normalized target triple already (needed for correct os-name in test).

mgorny updated this revision to Diff 366621.Aug 16 2021, 7:15 AM
mgorny retitled this revision from [lldb] Support gdbserver signals [WIP] to [lldb] Support gdbserver signals.
mgorny edited the summary of this revision. (Show Details)

Included all signals from include/gdb/signals.def.

JDevlieghere accepted this revision.Aug 18 2021, 5:03 PM

LGTM

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
1349

What about debugserver, I assume that would need to be updated as well?

This revision is now accepted and ready to land.Aug 18 2021, 5:03 PM
JDevlieghere requested changes to this revision.Aug 18 2021, 5:05 PM

Didn't mean to accept yet (originally I thought the native-signals check was inverted)

This revision now requires changes to proceed.Aug 18 2021, 5:05 PM
mgorny updated this revision to Diff 367414.Aug 19 2021, 12:40 AM

Advertise native-signals+ in debugserver too.

mgorny marked an inline comment as done.Aug 19 2021, 12:40 AM
mgorny added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
1349

I've attempted doing that. Please lemme know if that's ok.

mgorny updated this revision to Diff 371011.Sep 7 2021, 2:10 AM
mgorny marked an inline comment as done.

Rebased.

labath added a comment.Nov 9 2021, 6:09 AM

It would be nice if the commit message/summary included more details about the differences in signal handling (the things we talked about on irc), but generally, I think this patch looks good.

The only thing that's missing is interoperability with older lldb-compatible stubs (debugservers mainly). I believe we already have some "standard" solution for that, like checking for QThreadSuffixSupported or some similar lldb-only packet (?)

mgorny updated this revision to Diff 385917.Nov 9 2021, 12:05 PM
mgorny edited the summary of this revision. (Show Details)

Add fallback to QThreadSuffixSupported and a respective test. Improve the summary.

labath accepted this revision.Nov 9 2021, 11:37 PM
labath added inline comments.
lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
486

I believe this will always use remote-linux (or something), as the platform is determined by the binary. And even in case it isn't , it would probably be better to hardcode the platform by using the appropriate CreateTarget overload

mgorny marked an inline comment as done.Nov 9 2021, 11:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 10 2021, 12:39 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 12:39 AM