This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Plugins] Introduce Scripted Platform Plugin
Needs ReviewPublic

Authored by mib on Dec 3 2022, 12:00 AM.

Details

Summary

This patch introduces Scripted Platform, a new platform plugin that can
be customized with a python script.

For now this can list processes described in the python script file but
eventually, it will be used to spawn scripted processes and act as an
interface between them.

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

Diff Detail

Event Timeline

mib created this revision.Dec 3 2022, 12:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2022, 12:00 AM
mib requested review of this revision.Dec 3 2022, 12:00 AM
mib updated this revision to Diff 480164.Dec 5 2022, 10:59 AM

Update ScriptedMetadata header location

mib updated this revision to Diff 480249.Dec 5 2022, 2:36 PM

There's some overlap in implementation between ScriptedPlatform::GetProcessInfo and ScriptedPlatform::FindProcesses. If you don't anticipate these diverging dramatically, it might make sense to extract out common functionality into something like ScriptedPlatform::ExtractProcessInfoFromDict (or something to that effect).

lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp
1

This file needs a header.

66–67

This was already checked above on line 60/61.

143

This should be unconditionally true because you set host_os to llvm::Triple::MacOSX right above this.

lldb/source/Plugins/Platform/scripted/ScriptedPlatform.h
1

PlatformPOSIX.h -> ScriptedPlatform.h

mib updated this revision to Diff 482112.Dec 12 2022, 7:04 AM
mib marked 3 inline comments as done.

Address @bulbazord comments

mib updated this revision to Diff 482113.Dec 12 2022, 7:05 AM
JDevlieghere added inline comments.Dec 12 2022, 10:08 AM
lldb/source/Plugins/Platform/CMakeLists.txt
9
lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp
209

What's your intention by calling this function? In a release build this is still going to crash. Is there a way to initialize the platform with an invalid m_script_object_sp or m_interpreter? If so then we should have proper error handling. If not then these can be regular asserts.

lldb/source/Plugins/Platform/scripted/ScriptedPlatform.h
50–53

This should be a Doxygen comment.

71–75

This is hard to parse, do they really need to be grouped together? I'd add a newline between them like the public functions.

77

Useless comment

81

Missing group opener (@{), but you can just remove it if you do the same for line 76

mib updated this revision to Diff 482413.Dec 13 2022, 2:49 AM
mib marked 7 inline comments as done.

Address @JDevlieghere comments.

JDevlieghere added inline comments.Dec 13 2022, 1:41 PM
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
25–28 ↗(On Diff #482413)

I guess this can be inlined now?

mib marked an inline comment as done.Dec 15 2022, 4:21 AM
mib added inline comments.
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
25–28 ↗(On Diff #482413)

Right, may be I should even move this to the ScriptedInterface base, so every scripted class can benefit from it.

labath added a subscriber: labath.Dec 15 2022, 11:06 AM
labath added inline comments.
lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp
76

Use a shared_ptr directly here. This currently leaks platform object in the error.Fail() case.

262

When I saw the amount of effort put into the error messages in the ParseProcessFunction, I assumed you are going to give those messages to the user somehow, but now I see they are just thrown away. Do you plan to change that? Maybe you could at least log (LLDB_LOG_ERROR) them?

mib marked 3 inline comments as done.Dec 16 2022, 9:17 AM
mib added inline comments.
lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp
262

I wish there was a way to surface these error but the current implementation of the platform plugin doesn't allow it. I guess I'll just log them for now.

mib updated this revision to Diff 488014.Jan 10 2023, 3:25 PM
mib marked an inline comment as done.

Log the error message instead of consuming it

mib updated this revision to Diff 494107.EditedFeb 1 2023, 4:29 PM

Add support for process attach to ScriptedPlatform