This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Plugins] Add Attach capabilities to ScriptedProcess
ClosedPublic

Authored by mib on Feb 1 2023, 12:52 PM.

Details

Summary

This patch adds process attach capabilities to the ScriptedProcess
plugin. This doesn't really expects a PID or process name, since the
process state is already script, however, this allows to create a
scripted process without requiring to have an executuble in the target.

In order to do so, this patch also turns the scripted process related
getters and setters from the ProcessLaunchInfo and
ProcessAttachInfo classes to a ScriptedMetadata instance and moves
it in the ProcessInfo class, so it can be accessed interchangeably.

This also adds the necessary SWIG wrappers to convert the internal
Process{Attach,Launch}InfoSP into a SB{Attach,Launch}Info to pass it
as argument the scripted process python implementation and convert it
back to the internal representation.

rdar://104577406

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

Diff Detail

Event Timeline

mib created this revision.Feb 1 2023, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 12:52 PM
mib requested review of this revision.Feb 1 2023, 12:52 PM
bulbazord accepted this revision.Feb 1 2023, 3:16 PM

Makes sense to me. It does seem a little weird that you can specify a name or a pid for attaching to a scripted process but I suppose that's just a part of the interface.

This revision is now accepted and ready to land.Feb 1 2023, 3:16 PM
JDevlieghere requested changes to this revision.Feb 2 2023, 1:04 PM
JDevlieghere added inline comments.
lldb/include/lldb/Utility/ProcessInfo.h
90–110

What's the relationship between this an the ScriptedMetadata? It seems like this is storing the same stuff. Also IsScriptedProcess is a pretty blatant violation of the scripted process being a plugin...

This revision now requires changes to proceed.Feb 2 2023, 1:04 PM
mib requested review of this revision.Feb 2 2023, 1:24 PM
mib added inline comments.
lldb/include/lldb/Utility/ProcessInfo.h
90–110

I think you have a misunderstanding here: This is addition is not made in a process plugin, but rather in the ProcessInfo class, the base class for ProcessLaunchInfo and ProcessAttachInfo, so I don't think this violates in any way the plugin architecture.

Regarding the relationship with ScriptedMetadata: I guess we could replace m_scripted_process_class_name & m_scripted_process_dictionary_sp by a m_scripted_metadata object, but I didn't want to process info hold scripted process specific stuff. I don't have a strong opinion against it, either.

JDevlieghere added inline comments.Feb 2 2023, 1:36 PM
lldb/include/lldb/Utility/ProcessInfo.h
90–110

I think you have a misunderstanding here: This is addition is not made in a process plugin, but rather in the ProcessInfo class, the base class for ProcessLaunchInfo and ProcessAttachInfo, so I don't think this violates in any way the plugin architecture.

I know, but I would argue that the distinction between those two is pretty much irrelevant. That said, I do understand the need to pass this information, especially as more things become more scriptable (process, platform, etc). That's why I suggested using the ScriptedMetadata here because it's more generic.

Concretely my suggestion would be to have {Set,Get}ScriptedMetadata and if you think you really need it, maybe an IsScripted method that checks if the metadata is set (either by storing an optional ScriptedMetadata in the ProcessInfo or maybe through calling an operator bool on the ScriptedMetadata (which would check the name being empty like you do here).

but I didn't want to process info hold scripted process specific stuff.

I totally agree, as of now ScriptedMetadata contains the exact two fields that you have here. If other patches are extending that for the scripted processes or platforms, we could keep the common stuff in a base class and have a ScriptedProcessMetadata and ScriptedPlatformMetadata respectively. WDYT?

mib added inline comments.Feb 2 2023, 1:39 PM
lldb/include/lldb/Utility/ProcessInfo.h
90–110

That sounds fair to me. Let's go with your suggestion.

mib updated this revision to Diff 495971.Feb 8 2023, 3:15 PM
mib marked an inline comment as done.
mib edited the summary of this revision. (Show Details)

Addressed @JDevlieghere comments.

JDevlieghere added inline comments.Feb 9 2023, 2:44 PM
lldb/include/lldb/Utility/ProcessInfo.h
90

Can we not ask this from the ScriptedMetadata?

lldb/source/API/SBAttachInfo.cpp
272 ↗(On Diff #495971)

Newline after LLDB_INSTRUMENT_VA

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
213–222

Wouldn't you want to at least pass this information down to the scripted process? I could imagine that the PID and the process name, if known, could be pretty interesting to store in the scripted process, even if it always succeeds regardless.

mib marked 4 inline comments as done.Feb 16 2023, 6:22 PM
mib added inline comments.
lldb/include/lldb/Utility/ProcessInfo.h
90

I think this is fine to ask the ProcessInfo for this.

If we got rid of this, instead of calling IsScriptedProcess, we would need first to get ScriptedMetadata shared pointer and make sure it's not null, then deference it to call the operator bool(). I don't think it's worth it.

mib updated this revision to Diff 498229.Feb 16 2023, 6:29 PM
mib marked an inline comment as done.

Address @JDevlieghere feedback.

I'm not sure how much this matters but the fact that ScriptedMetadata::IsValid (and the bool operator overload) relies on the class name being empty is a little strange to me. I feel like you could get yourself into a strange situation where you set the ScriptedMetadata class name to "" and then ask for it right back and instead of giving you "" it'll give you nullptr (or None in python). This also "invalidates" the ScriptedMetadata. Not a hard blocker, just a little strange to me.

lldb/source/API/SBAttachInfo.cpp
273–278 ↗(On Diff #498229)

Maybe I'm misunderstanding but it seems like this implementation doesn't allow you to actually change the class name more than once? If that's not intentional, then we should be creating a new ScriptedMetadata and calling SetScriptedMetadata every time. If it is intentional, why?

312–317 ↗(On Diff #498229)

Same as above: This doesn't actually let you change the arguments if we already have an existing ScriptedMetadata. If its already set, we're just calling SetScriptedMetadata with what already exists and discarding the argument.

272 ↗(On Diff #495971)

There needs to be a newline after LLDB_INSTRUMENT_VA here I believe.

lldb/source/API/SBLaunchInfo.cpp
348 ↗(On Diff #498229)

Needs to be a newline after LLDB_INSTRUMENT_VA

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
200–205

I'm not sure I understand what the point of setting the state to Running is and only setting it to Stopped if the attach failed? Should we be mucking with state at all if the attach failed?

mib added inline comments.Feb 17 2023, 5:28 PM
lldb/source/API/SBAttachInfo.cpp
273–278 ↗(On Diff #498229)

Good catch! I forgot to check-in some code --'

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
200–205

I guess we can do it subsequently

mib updated this revision to Diff 502265.Mar 3 2023, 2:44 PM
mib edited the summary of this revision. (Show Details)
  • Address @bulbazord comment about only being able to assign the Scripted Metadata attributes once.
  • Add necessary SWIG wrappers to convert the internal type into SB counterpart and back.
mib edited the summary of this revision. (Show Details)Mar 3 2023, 2:45 PM

I still think it's a little weird that you can create LaunchInfo or AttachInfo, call SetScriptedProcessDictionary, and still have the ScriptedMetadata be "invalid", but I suppose it makes no sense if there is no class name anyway.

Just a few small things. Everything else looks fine to me.

lldb/source/API/SBAttachInfo.cpp
272 ↗(On Diff #502265)

Add a new line after the instrument macro

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
200–205

I'm not sure this was addressed and maybe I don't understand something but why do we check to see if the error failed only after setting the state to running? If we actually did fail to attach, does it make sense to think of the ScriptedProcess as "running"?

mib marked 5 inline comments as done.Mar 3 2023, 3:17 PM
mib updated this revision to Diff 502270.Mar 3 2023, 3:19 PM
  • Address last feedbacks
  • Fix unittest
bulbazord accepted this revision.Mar 3 2023, 3:20 PM
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.
labath added a subscriber: labath.Mar 6 2023, 5:43 AM
labath added inline comments.
lldb/source/Utility/ProcessInfo.cpp
11

This is a layering violation. Can we restructure this so that it this does not depend on code outside the Utility library? Maybe the ScriptedMetadata thingy could be moved into the Utility library?

mib marked an inline comment as done.Mar 6 2023, 12:00 PM
mib added inline comments.
lldb/source/Utility/ProcessInfo.cpp
11

@labath Thanks for pointing this out! I fixed it in D145414

mib marked an inline comment as done.Mar 6 2023, 4:58 PM
mib added a subscriber: omjavaid.