This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Add scripted process launch/attach option to {,platform }process commands
ClosedPublic

Authored by mib on Dec 13 2022, 8:44 AM.

Details

Summary

This patch does several things:

First, it refactors the CommandObject{,Platform}ProcessObject command
option class into a separate CommandOptionsProcessAttach option group.

This will make sure both the platform process attach and process attach
command options will always stay in sync without having with duplicate
them each time. But more importantly, making this class an OptionGroup
allows us to combine with a OptionGroupPythonClassWithDict to add
support for the scripted process managing class name and user-provided
dictionary options.

This patch also improves feature parity between ProcessLaunchInfo and
ProcessAttachInfo with regard to ScriptedProcesses, by exposing the
various getters and setters necessary to use them through the SBAPI.

This is foundation work for adding support to "attach" to a process from
the scripted platform.

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Dec 13 2022, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2022, 8:44 AM
mib requested review of this revision.Dec 13 2022, 8:44 AM

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

(Apart from that, this patch seems fine.)

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

(Apart from that, this patch seems fine.)

Maybe one way around this is to have some generic metadata that gets passed to the plugin, that can be different per plugin?

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

(Apart from that, this patch seems fine.)

Maybe one way around this is to have some generic metadata that gets passed to the plugin, that can be different per plugin?

That might work, but I don't think it's really necessary. My only issue is one of nomenclature. I don't see a problem with having a concrete Process subclass in the lldb core. My problem is with calling that class a "plugin", and pretending it's pluggable, but then introducing backdoor dependencies to it (usually by referring to with via its (string) plugin name). You could try to solve that by making it genuinely pluggable, but that could be overkill, given that it already contains *is* pluggable, only in a different way -- one can plugin a different scripting language to implement the bindings (and then obviously the user can plug in to implement those bindings).

mib added a comment.Jan 11 2023, 11:41 PM

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

(Apart from that, this patch seems fine.)

Maybe one way around this is to have some generic metadata that gets passed to the plugin, that can be different per plugin?

That might work, but I don't think it's really necessary. My only issue is one of nomenclature. I don't see a problem with having a concrete Process subclass in the lldb core. My problem is with calling that class a "plugin", and pretending it's pluggable, but then introducing backdoor dependencies to it (usually by referring to with via its (string) plugin name). You could try to solve that by making it genuinely pluggable, but that could be overkill, given that it already contains *is* pluggable, only in a different way -- one can plugin a different scripting language to implement the bindings (and then obviously the user can plug in to implement those bindings).

@labath I'm taking note of your comment but as you said it's the pluggable design of scripted processes is not introduced in this patch. I also have some plans for Scripted Processes that may involve rethinking its pluggable design, so I'll keep your feedback in mind for when I implement those changes :)

mib updated this revision to Diff 494110.Feb 1 2023, 4:31 PM

Rebase

mib updated this revision to Diff 502259.Mar 3 2023, 2:15 PM

Fix build failures after rebase

mib retitled this revision from [lldb] Add scripted process launch/attach option to platform <process> commands to [lldb] Add scripted process launch/attach option to {,platform }process commands.Mar 3 2023, 2:21 PM
JDevlieghere accepted this revision.Mar 3 2023, 5:09 PM

LGTM modulo inline comments.

lldb/include/lldb/Target/Process.h
234–238

I know you're just doing this for consistency but please use Doxygen style comments /// on the line before. Bonus points if you fix the ones above as well.

lldb/source/Commands/CommandOptionsProcessAttach.h
18

Let's stop cargo culting these frankly useless comments.

41

Remove

44
This revision is now accepted and ready to land.Mar 3 2023, 5:09 PM
This revision was landed with ongoing or failed builds.Mar 3 2023, 7:33 PM
This revision was automatically updated to reflect the committed changes.