Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 26 additions & 13 deletions lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ Status ScriptedProcess::DoLoadCore() {
Status ScriptedProcess::DoLaunch(Module *exe_module,
ProcessLaunchInfo &launch_info) {
LLDB_LOGF(GetLog(LLDBLog::Process), "ScriptedProcess::%s launching process", __FUNCTION__);

/* MARK: This doesn't reflect how lldb actually launches a process.
In reality, it attaches to debugserver, then resume the process.
That's not true in all cases. If debugserver is remote, lldb
Expand Down Expand Up @@ -422,9 +422,11 @@ bool ScriptedProcess::GetProcessInfo(ProcessInstanceInfo &info) {
lldb_private::StructuredData::ObjectSP
ScriptedProcess::GetLoadedDynamicLibrariesInfos() {
Status error;
auto error_with_message = [&error](llvm::StringRef message) {
return ScriptedInterface::ErrorWithMessage<bool>(LLVM_PRETTY_FUNCTION,
message.data(), error);
auto handle_error_with_message = [&error](llvm::StringRef message,
bool ignore_error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Based on the recent discussion to make it the default, I can remove this

ScriptedInterface::ErrorWithMessage<bool>(LLVM_PRETTY_FUNCTION,
message.data(), error);
return ignore_error;
};

StructuredData::ArraySP loaded_images_sp = GetInterface().GetLoadedImages();
Expand All @@ -436,22 +438,25 @@ ScriptedProcess::GetLoadedDynamicLibrariesInfos() {
ModuleList module_list;
Target &target = GetTarget();

auto reload_image = [&target, &module_list, &error_with_message](
auto reload_image = [&target, &module_list, &handle_error_with_message](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just revert the error_with_message & reload_image lambda exactly how it was before this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

StructuredData::Object *obj) -> bool {
StructuredData::Dictionary *dict = obj->GetAsDictionary();

if (!dict)
return error_with_message("Couldn't cast image object into dictionary.");
return handle_error_with_message(
"Couldn't cast image object into dictionary.", false);

ModuleSpec module_spec;
llvm::StringRef value;

bool has_path = dict->HasKey("path");
bool has_uuid = dict->HasKey("uuid");
if (!has_path && !has_uuid)
return error_with_message("Dictionary should have key 'path' or 'uuid'");
return handle_error_with_message(
"Dictionary should have key 'path' or 'uuid'", false);
if (!dict->HasKey("load_addr"))
return error_with_message("Dictionary is missing key 'load_addr'");
return handle_error_with_message("Dictionary is missing key 'load_addr'",
false);

if (has_path) {
dict->GetValueForKeyAsString("path", value);
Expand All @@ -467,16 +472,21 @@ ScriptedProcess::GetLoadedDynamicLibrariesInfos() {
ModuleSP module_sp =
target.GetOrCreateModule(module_spec, true /* notify */);

bool ignore_module_load_error = false;
dict->GetValueForKeyAsBoolean("ignore_module_load_error",
ignore_module_load_error);
if (!module_sp)
return error_with_message("Couldn't create or get module.");
return handle_error_with_message("Couldn't create or get module.",
ignore_module_load_error);

lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
lldb::offset_t slide = LLDB_INVALID_OFFSET;
dict->GetValueForKeyAsInteger("load_addr", load_addr);
dict->GetValueForKeyAsInteger("slide", slide);
if (load_addr == LLDB_INVALID_ADDRESS)
return error_with_message(
"Couldn't get valid load address or slide offset.");
return handle_error_with_message(
"Couldn't get valid load address or slide offset.",
ignore_module_load_error);

if (slide != LLDB_INVALID_OFFSET)
load_addr += slide;
Expand All @@ -486,13 +496,16 @@ ScriptedProcess::GetLoadedDynamicLibrariesInfos() {
changed);

if (!changed && !module_sp->GetObjectFile())
return error_with_message("Couldn't set the load address for module.");
return handle_error_with_message(
"Couldn't set the load address for module.",
ignore_module_load_error);

dict->GetValueForKeyAsString("path", value);
FileSpec objfile(value);
module_sp->SetFileSpecAndObjectName(objfile, objfile.GetFilename());

return module_list.AppendIfNeeded(module_sp);
return ignore_module_load_error ? true
: module_list.AppendIfNeeded(module_sp);
};

if (!loaded_images_sp->ForEach(reload_image))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't make a comment on the next line, but all you need to do here is to remove the return keyword on the next line. This way, we will still log Couldn't reload all images in case some modules failed to load but we wouldn't return false.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,29 @@ def cleanup():
)
self.assertTrue(corefile_process, PROCESS_IS_VALID)

# Create a random lib which does not exist in the corefile.
random_dylib = self.get_module_with_name(corefile_target, "random.dylib")
self.assertFalse(
random_dylib, "Dynamic library random.dylib should not be found."
)

structured_data = lldb.SBStructuredData()
structured_data.SetFromJSON(
json.dumps(
{
"backing_target_idx": self.dbg.GetIndexOfTarget(
corefile_process.GetTarget()
),
"libbaz_path": self.getBuildArtifact("libbaz.dylib"),
"custom_modules": [
{
"path": self.getBuildArtifact("libbaz.dylib"),
},
{
"path": "/random/path/random.dylib",
"load_addr": 12345678,
"ignore_module_load_error": True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this anymore

Suggested change
"ignore_module_load_error": True,

},
],
}
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,82 @@ def __init__(self, exe_ctx: lldb.SBExecutionContext, args: lldb.SBStructuredData
if len(self.threads) == 2:
self.threads[len(self.threads) - 1].is_stopped = True

corefile_module = self.get_module_with_name(
self.corefile_target, "libbaz.dylib"
)
if not corefile_module or not corefile_module.IsValid():
return
module_path = os.path.join(
corefile_module.GetFileSpec().GetDirectory(),
corefile_module.GetFileSpec().GetFilename(),
)
if not os.path.exists(module_path):
return
module_load_addr = corefile_module.GetObjectFileHeaderAddress().GetLoadAddress(
self.corefile_target
)
custom_modules = args.GetValueForKey("custom_modules")
if custom_modules.GetType() == lldb.eStructuredDataTypeArray:
for id in range(custom_modules.GetSize()):

custom_module = custom_modules.GetItemAtIndex(id)
if (
not custom_module
or not custom_module.IsValid()
or not custom_module.GetType() == lldb.eStructuredDataTypeDictionary
):
continue

# Get custom module path from args
module_path_arg = custom_module.GetValueForKey("path")
module_path = None
if (
not module_path_arg
or not module_path_arg.IsValid()
or not module_path_arg.GetType() == lldb.eStructuredDataTypeString
):
return

module_path = module_path_arg.GetStringValue(100)
module_name = os.path.basename(module_path)

# Get ignore_module_load_error boolean from args
ignore_module_load_error = False
ignore_module_load_error_arg = custom_module.GetValueForKey(
"ignore_module_load_error"
)
if (
ignore_module_load_error_arg
and ignore_module_load_error_arg.IsValid()
and ignore_module_load_error_arg.GetType()
== lldb.eStructuredDataTypeBoolean
):
ignore_module_load_error = (
ignore_module_load_error_arg.GetBooleanValue()
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this anymore

Suggested change
# Get ignore_module_load_error boolean from args
ignore_module_load_error = False
ignore_module_load_error_arg = custom_module.GetValueForKey(
"ignore_module_load_error"
)
if (
ignore_module_load_error_arg
and ignore_module_load_error_arg.IsValid()
and ignore_module_load_error_arg.GetType()
== lldb.eStructuredDataTypeBoolean
):
ignore_module_load_error = (
ignore_module_load_error_arg.GetBooleanValue()
)

Copy link
Contributor Author

@rchamala rchamala Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will remove it based on our recent discussion to enable it by default


self.loaded_images.append({"path": module_path, "load_addr": module_load_addr})
if not os.path.exists(module_path) and not ignore_module_load_error:
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Suggested change
if not os.path.exists(module_path) and not ignore_module_load_error:
return


# Get custom module load address from args
module_load_addr = None
module_load_addr_arg = custom_module.GetValueForKey("load_addr")
if (
module_load_addr_arg
and module_load_addr_arg.IsValid()
and module_load_addr_arg.GetType()
== lldb.eStructuredDataTypeInteger
):
module_load_addr = module_load_addr_arg.GetIntegerValue()

# If module load address is not specified/valid, try to find it from corefile module
if module_load_addr is None:
corefile_module = self.get_module_with_name(
self.corefile_target, module_name
)

if not corefile_module or not corefile_module.IsValid():
return

module_load_addr = (
corefile_module.GetObjectFileHeaderAddress().GetLoadAddress(
self.corefile_target
)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Seems like you're re-implementing the C++ module loading logic in python here which doesn't seems necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current test logic is hardcoded to find a single module named "libbaz.dylib" and get load address from corefile_module. I was hoping to have a test that accepts different modules given module path and module load address from the caller. If it is not given, I was planning to get the load address from corefile_module similar to how the current test is doing. Lmk if that sounds fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, the reason I mentioned this is because I wanted to exercise the error handling code path in C++ rather than do in python


self.loaded_images.append(
{
"path": module_path,
"load_addr": module_load_addr,
"ignore_module_load_error": ignore_module_load_error,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need that anymore

Suggested change
"ignore_module_load_error": ignore_module_load_error,

}
)

def get_memory_region_containing_address(
self, addr: int
Expand Down