Page MenuHomePhabricator

[lldb] [gdb-remote] Support getting siginfo via API
ClosedPublic

Authored by mgorny on Jan 24 2022, 9:55 AM.

Details

Summary

Add Thread::GetSiginfo() and SBThread::GetSiginfo() methods to retrieve
the siginfo value from server.

Diff Detail

Event Timeline

mgorny requested review of this revision.Jan 24 2022, 9:55 AM
mgorny created this revision.
mgorny updated this revision to Diff 402622.Jan 24 2022, 12:09 PM

Add another test using FreeBSD siginfo_t, to make sure we select the platform correctly.

Seems reasonable to me. Jim, do you have any thoughts on this?

lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
494

I'd probably factor the out the common parts into a helper fn, as these two tests only differ in constants.

mgorny updated this revision to Diff 402927.Jan 25 2022, 8:28 AM
mgorny marked an inline comment as done.

Add a common test function. Document the raw data. Include trapno check on FreeBSD.

That said, it just occurred to me that I need to add some packet indicating the architecture.

mgorny updated this revision to Diff 402933.Jan 25 2022, 8:39 AM

I've forgotten that the architecture is inferred from target here. Added i386 test for completeness.

mgorny updated this revision to Diff 402992.Jan 25 2022, 12:05 PM

Update following changes in GetSiginfoType() — notably stop relying on the SBPlatform API and update for new prototype.

labath accepted this revision.Jan 27 2022, 1:43 AM
labath added inline comments.
lldb/source/API/SBThread.cpp
1326–1334

It should be fine to chain these, relying on the fact that the SB methods on an empty object will return another empty object.

This revision is now accepted and ready to land.Jan 27 2022, 1:43 AM
mgorny added inline comments.Jan 27 2022, 3:45 AM
lldb/source/API/SBThread.cpp
1326–1334

I suppose I could avoid process and switch to getting byte order from target but tbh I think the explicit error message for lack of process is worth keeping the split.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 4:34 AM
jingham reopened this revision.Jan 27 2022, 9:02 AM

We don't return errors separately for any of the API's that return an SBValue. SBValue's carry their error with them (SBValue.GetError()) so it would be confusing to have two errors.

This revision is now accepted and ready to land.Jan 27 2022, 9:02 AM

We don't return errors separately for any of the API's that return an SBValue. SBValue's carry their error with them (SBValue.GetError()) so it would be confusing to have two errors.

I'm sorry but I can't figure out how to set the error inside SBValue. The only user-facing thing that seems to allow passing Status seems to be ValueObjectConstResult::Create() but it feels like it's not supposed to be used like this (and it doesn't seem to preserve my Status either).

Yes, you probably need to make another change, which is actually probably a good idea as well.

The immediate problem is that the default constructed SBValue is empty, it has a null ValueObjectSP, so until you somehow make it valid you can't get a valid error from it, in which to set your error string. By the time you've made a valid SBValue it's too late.

But I think actually you should fix this by having a

ValueObjectSP lldb_private::Thread::GetSiginfoValue()

then the SB API will just wrap that call. That way you can make a real ValueObject, with a real error, and return that at any stage.

That will fix your problem, but the real reason for doing this is that lldb_private code can't call back into the SB API's. So if anybody in lldb_private land needed the ValueObject that wrapped the Siginfo information, which is after all the handiest form in which to have it, they would be out of luck.

I didn't get a chance to review this, sorry about that. But in general, SB API's should not do any more work than is necessary to marshal the incoming arguments & figure out which lldb_private API to dispatch it to. Otherwise you've written code is going to end up getting duplicated somewhere (in a command or somewhere else it's needed...)

jingham requested changes to this revision.Jan 27 2022, 2:57 PM
This revision now requires changes to proceed.Jan 27 2022, 2:57 PM
mgorny updated this revision to Diff 403948.Jan 28 2022, 3:18 AM

Add Thread::GetSiginfoValue() to perform baseline value construction, and limit SBThread::GetSiginfo() to wrapping that. Return errors via ValueObjectConstResult.

jingham accepted this revision.Jan 28 2022, 8:41 AM

Thanks for making that change! LGTM.

This revision is now accepted and ready to land.Jan 28 2022, 8:41 AM
This revision was automatically updated to reflect the committed changes.