This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Prevent 'process connect' from using local-only plugins
ClosedPublic

Authored by mgorny on Nov 19 2020, 11:11 AM.

Details

Summary

Add a 'can_connect' parameter to Process plugin initialization, and use
it to filter plugins to these capable of remote connections. This is
used to prevent 'process connect' from picking up a plugin that can only
be used locally, e.g. the legacy FreeBSD plugin.

Diff Detail

Event Timeline

mgorny created this revision.Nov 19 2020, 11:11 AM
mgorny requested review of this revision.Nov 19 2020, 11:11 AM
emaste accepted this revision.Nov 19 2020, 11:20 AM

Looks ok to me

This revision is now accepted and ready to land.Nov 19 2020, 11:20 AM
krytarowski accepted this revision.Nov 19 2020, 3:29 PM

This is all weird (not in a "your fault" way). ProcessFreeBSD is decidedly a local-only plugin. It could never successfully complete the "process connect" command, regardless of whether we have some other remote plugin or not.

I think that the real fix here would be to change the plugin selection logic (Target::CreateProcess) to convey the fact that we're looking for a connectable plugin. The method already contains a FileSpec *core_file argument, which I guess is needed to select the proper core file subclass, so adding a can_connect argument would not be unreasonable (though not exactly pretty either).

Though, for the purposes of this test, just adding a "--plugin gdb-remote" argument to the "process connect" command should be sufficient as well.

I think that the real fix here would be to change the plugin selection logic (Target::CreateProcess) to convey the fact that we're looking for a connectable plugin. The method already contains a FileSpec *core_file argument, which I guess is needed to select the proper core file subclass, so adding a can_connect argument would not be unreasonable (though not exactly pretty either).

Would maybe a static class variable ('property'?) be cleaner?

I think that the real fix here would be to change the plugin selection logic (Target::CreateProcess) to convey the fact that we're looking for a connectable plugin. The method already contains a FileSpec *core_file argument, which I guess is needed to select the proper core file subclass, so adding a can_connect argument would not be unreasonable (though not exactly pretty either).

Would maybe a static class variable ('property'?) be cleaner?

It might, though I am not sure how would you make that interact with the current selection logic. And I think you'll still need to pass a flag all the way to Process::FindPlugin, as that's the thing which iterates over registered classes.

mgorny updated this revision to Diff 306699.Nov 20 2020, 8:15 AM
mgorny retitled this revision from [lldb] [Process/FreeBSD] Fix 'process connect' plugin choice to [lldb] Prevent 'process connect' from using local-only plugins.
mgorny edited the summary of this revision. (Show Details)

Updated to pass can_debug parameter, as suggested by @labath.

labath accepted this revision.Nov 23 2020, 12:46 AM

Seems ok. The code could really use a refactor though...

lldb/test/Shell/Commands/command-process-connect.test
1–2

I'm pretty sure windows suffered from the same problem.

mgorny marked an inline comment as done.Nov 23 2020, 12:47 AM
mgorny added inline comments.
lldb/test/Shell/Commands/command-process-connect.test
1–2

Ok, I will remove this and see what buildbots say.

This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 12:49 AM