Page MenuHomePhabricator

[lldb] [llgs] Report QPassSignals and qXfer via extensions API
ClosedPublic

Authored by mgorny on Apr 24 2021, 2:20 PM.

Details

Summary

Remove hardcoded platform list for QPassSignals, qXfer:auxv:read
and qXfer:libraries-svr4:read and instead query the process plugin
via the GetSupportedExtensions() API.

Diff Detail

Event Timeline

mgorny requested review of this revision.Apr 24 2021, 2:20 PM
mgorny created this revision.
labath accepted this revision.Apr 27 2021, 5:17 AM

I like this, though I am still unsure about the reason for existence of the "multiprocess" extension.

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
287

Technically, libraries_svr4 functionality is implemented in the base class, so it would be more correct to implement this as | NativeProcessELF::Factory::GetSupportedExtensions() (this would be a new class, which exists only to override GetSupportedExtensions).

lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
977

I guess it would be even better to run this everywhere, but assert that the server returns the right value for the particular platform.

This revision is now accepted and ready to land.Apr 27 2021, 5:17 AM
mgorny added inline comments.Apr 27 2021, 5:32 AM
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
287

Doesn't it require some extra logic in the NativeProcess* class though, like memory maps or something like that? If not, I sure can move it, I guess.

labath added inline comments.Apr 27 2021, 6:33 AM
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
287

Hmm.. that's a good question. Fortunately, we have a unit test for this class, so it should be easy to answer it. It seems it does require reading an auxiliary vector (and memory). However, I don't think that disqualifies it from this (svr4 support is the only reason for this class existence -- if a plugin did not support auxv reading, it would not inherit from it). OTOH, I can sort of see how it makes sense to implement this here, so I'm not going to push for that..

Thanks for the review. I'm going to update the tests as you suggested before pushing.

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
287

Now that I think of it, I'm pretty sure Kamil told me once that NetBSD used not to support SVR4 libs properly (dyld stuff, I guess). I suppose it makes more sense to enable it per platform.

mgorny marked 3 inline comments as done.Apr 27 2021, 10:33 AM
This revision was landed with ongoing or failed builds.Apr 27 2021, 10:34 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 10:34 AM