[Lldb-commits] [PATCH] D156066: [lldb][LocateModuleCallback] Call locate module callback in Platform too
splhack created this revision.
splhack added reviewers: clayborg, jingham, bulbazord, jasonmolenda,
JDevlieghere, mib.
Herald added a project: All.
splhack requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
This is an enhancement for the locate module callback.
https://discourse.llvm.org/t/rfc-python-callback-for-target-get-module/71580/6
On Android remote platform, module UUID is resolved by
Platform::GetRemoteSharedModule. Which means the current
Target::CallLocateModuleCallbackIfSet() call undesirably is not able to pass the
module UUID to the locate module callback.
This diff moves the CallLocateModuleCallbackIfSet() implementation from Target
to Platform to allows both Target and Platform can call it. One is from the
current Target call site, and second is from Platform after resolving the module
UUID.
As the result of this change, the locate module callback may be called twice
for a same module on remote platforms. And it should be ok.
- First, without UUID.
- The locate module callback is allowed to return an error if the callback
requires UUID.
- Second, with UUID, if the first callback call did not return a module.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156066
Files:
lldb/include/lldb/Target/Platform.h
lldb/include/lldb/Target/Target.h
lldb/source/Target/Platform.cpp
lldb/source/Target/Target.cpp
lldb/unittests/Target/LocateModuleCallbackTest.cpp
Index: lldb/unittests/Target/LocateModuleCallbackTest.cpp
===
--- lldb/unittests/Target/LocateModuleCallbackTest.cpp
+++ lldb/unittests/Target/LocateModuleCallbackTest.cpp
@@ -231,6 +231,7 @@
EXPECT_TRUE(m_process_sp);
m_module_spec = GetTestModuleSpec();
+m_module_spec_without_uuid = ModuleSpec(GetRemotePath(), ArchSpec(k_arch));
}
void TearDown() override {
@@ -244,15 +245,33 @@
}
void CheckCallbackArgs(const ModuleSpec &module_spec,
- FileSpec &module_file_spec,
- FileSpec &symbol_file_spec) {
-EXPECT_TRUE(m_module_spec.Matches(module_spec,
- /*exact_arch_match=*/true));
+ FileSpec &module_file_spec, FileSpec &symbol_file_spec,
+ const ModuleSpec &expected_module_spec,
+ int expected_callback_call_count) {
+EXPECT_TRUE(expected_module_spec.Matches(module_spec,
+ /*exact_arch_match=*/true));
EXPECT_FALSE(module_file_spec);
EXPECT_FALSE(symbol_file_spec);
-EXPECT_EQ(m_callback_call_count, 0);
-m_callback_call_count++;
+EXPECT_EQ(++m_callback_call_count, expected_callback_call_count);
+ }
+
+ void CheckCallbackArgsWithUUID(const ModuleSpec &module_spec,
+ FileSpec &module_file_spec,
+ FileSpec &symbol_file_spec,
+ int expected_callback_call_count) {
+CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec,
+ m_module_spec, expected_callback_call_count);
+EXPECT_TRUE(module_spec.GetUUID().IsValid());
+ }
+
+ void CheckCallbackArgsWithoutUUID(const ModuleSpec &module_spec,
+FileSpec &module_file_spec,
+FileSpec &symbol_file_spec,
+int expected_callback_call_count) {
+CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec,
+ m_module_spec_without_uuid, expected_callback_call_count);
+EXPECT_FALSE(module_spec.GetUUID().IsValid());
}
protected:
@@ -262,6 +281,7 @@
TargetSP m_target_sp;
ProcessSP m_process_sp;
ModuleSpec m_module_spec;
+ ModuleSpec m_module_spec_without_uuid;
ModuleSP m_module_sp;
int m_callback_call_count = 0;
};
@@ -331,14 +351,18 @@
// download the module and fails.
BuildEmptyCacheDir(m_test_dir);
- m_platform_sp->SetLocateModuleCallback([this](const ModuleSpec &module_spec,
-FileSpec &module_file_spec,
-FileSpec &symbol_file_spec) {
-CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec);
-return Status("The locate module callback failed");
- });
+ int callback_call_count = 0;
+ m_platform_sp->SetLocateModuleCallback(
+ [this, &callback_call_count](const ModuleSpec &module_spec,
+ FileSpec &module_file_spec,
+ FileSpec &symbol_file_spec) {
+CheckCallbackArgsWithUUID(module_spec, module_file_spec,
+ symbol_file_spec, ++callback_call_count);
+return Status("The locate module callback failed");
+ });
m_module_sp = m_target_sp->GetOrCreateModule(m_module_spec,
[Lldb-commits] [PATCH] D156066: [lldb][LocateModuleCallback] Call locate module callback in Platform too
splhack updated this revision to Diff 543329.
splhack added a comment.
Remove wrong comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156066/new/
https://reviews.llvm.org/D156066
Files:
lldb/include/lldb/Target/Platform.h
lldb/include/lldb/Target/Target.h
lldb/source/Target/Platform.cpp
lldb/source/Target/Target.cpp
lldb/unittests/Target/LocateModuleCallbackTest.cpp
Index: lldb/unittests/Target/LocateModuleCallbackTest.cpp
===
--- lldb/unittests/Target/LocateModuleCallbackTest.cpp
+++ lldb/unittests/Target/LocateModuleCallbackTest.cpp
@@ -231,6 +231,7 @@
EXPECT_TRUE(m_process_sp);
m_module_spec = GetTestModuleSpec();
+m_module_spec_without_uuid = ModuleSpec(GetRemotePath(), ArchSpec(k_arch));
}
void TearDown() override {
@@ -244,15 +245,33 @@
}
void CheckCallbackArgs(const ModuleSpec &module_spec,
- FileSpec &module_file_spec,
- FileSpec &symbol_file_spec) {
-EXPECT_TRUE(m_module_spec.Matches(module_spec,
- /*exact_arch_match=*/true));
+ FileSpec &module_file_spec, FileSpec &symbol_file_spec,
+ const ModuleSpec &expected_module_spec,
+ int expected_callback_call_count) {
+EXPECT_TRUE(expected_module_spec.Matches(module_spec,
+ /*exact_arch_match=*/true));
EXPECT_FALSE(module_file_spec);
EXPECT_FALSE(symbol_file_spec);
-EXPECT_EQ(m_callback_call_count, 0);
-m_callback_call_count++;
+EXPECT_EQ(++m_callback_call_count, expected_callback_call_count);
+ }
+
+ void CheckCallbackArgsWithUUID(const ModuleSpec &module_spec,
+ FileSpec &module_file_spec,
+ FileSpec &symbol_file_spec,
+ int expected_callback_call_count) {
+CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec,
+ m_module_spec, expected_callback_call_count);
+EXPECT_TRUE(module_spec.GetUUID().IsValid());
+ }
+
+ void CheckCallbackArgsWithoutUUID(const ModuleSpec &module_spec,
+FileSpec &module_file_spec,
+FileSpec &symbol_file_spec,
+int expected_callback_call_count) {
+CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec,
+ m_module_spec_without_uuid, expected_callback_call_count);
+EXPECT_FALSE(module_spec.GetUUID().IsValid());
}
protected:
@@ -262,6 +281,7 @@
TargetSP m_target_sp;
ProcessSP m_process_sp;
ModuleSpec m_module_spec;
+ ModuleSpec m_module_spec_without_uuid;
ModuleSP m_module_sp;
int m_callback_call_count = 0;
};
@@ -331,14 +351,18 @@
// download the module and fails.
BuildEmptyCacheDir(m_test_dir);
- m_platform_sp->SetLocateModuleCallback([this](const ModuleSpec &module_spec,
-FileSpec &module_file_spec,
-FileSpec &symbol_file_spec) {
-CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec);
-return Status("The locate module callback failed");
- });
+ int callback_call_count = 0;
+ m_platform_sp->SetLocateModuleCallback(
+ [this, &callback_call_count](const ModuleSpec &module_spec,
+ FileSpec &module_file_spec,
+ FileSpec &symbol_file_spec) {
+CheckCallbackArgsWithUUID(module_spec, module_file_spec,
+ symbol_file_spec, ++callback_call_count);
+return Status("The locate module callback failed");
+ });
m_module_sp = m_target_sp->GetOrCreateModule(m_module_spec, /*notify=*/false);
+ ASSERT_EQ(callback_call_count, 2);
ASSERT_FALSE(m_module_sp);
}
@@ -348,14 +372,18 @@
// some reason.
FileSpec uuid_view = BuildCacheDir(m_test_dir);
- m_platform_sp->SetLocateModuleCallback([this](const ModuleSpec &module_spec,
-FileSpec &module_file_spec,
-FileSpec &symbol_file_spec) {
-CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec);
-return Status("The locate module callback failed");
- });
+ int callback_call_count = 0;
+ m_platform_sp->SetLocateModuleCallback(
+ [this, &callback_call_count](const ModuleSpec &module_spec,
+ FileSpec &module_file_spec,
+ FileSpec &symbol_file_spec) {
+CheckCallbackArgsWithUUID(module_spec, module_file_spec,
+ symbol_file_spec, ++callback_call_count);
+return Status("The locate module callback failed");
+ });
