jankratochvil created this revision.
jankratochvil added reviewers: aadsm, labath, xiaobai, clayborg.
jankratochvil added a project: LLDB.
Herald added a subscriber: abidh.
jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
jankratochvil retitled this revision from "Unify+fix remote XML libraries
handling with legacy one" to "Unify+fix remote XML libraries handling with the
legacy one".
================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:185
- // If we can't get the SO info from the remote, return failure.
- if (fromRemote && m_process->LoadModules(module_list) == 0)
+ if (m_current.map_addr == 0)
return false;
----------------
This change is not really required but I do not see why the `map_addr` check
should differ when the remote XML protocol is in use.
D62503 <https://reviews.llvm.org/D62503> broke Fedora 30 x86_64
<https://reviews.llvm.org/D62503#1549874>. That is because it fixed failing
`ReadMemory` there and thus enabling to really use the XML libraries protocol
which broke LLDB because then `DynamicLoaderPOSIXDYLD::RendezvousBreakpointHit`
happens only once and any further library updates are ignored.
So D62503 <https://reviews.llvm.org/D62503> should be reapplied after this fix
as it is safe then.
In this patch I do not understand why there should be any special add/remove
handling in `ProcessGDBRemote::LoadModules`, it is just a new retrieved list of
libraries and loading their modules is handled by later code the same way as it
always worked with legacy memory reading from the list at `_r_debug`.
There are some more opportunities how to unify the XML protocol vs. legacy
libraries handling but I wanted to first fix this pending regression.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D63868
Files:
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4853,77 +4853,7 @@
if (GetLoadedModuleList(module_list).Fail())
return 0;
- // get a list of all the modules
- ModuleList new_modules;
-
- for (LoadedModuleInfoList::LoadedModuleInfo &modInfo : module_list.m_list) {
- std::string mod_name;
- lldb::addr_t mod_base;
- lldb::addr_t link_map;
- bool mod_base_is_offset;
-
- bool valid = true;
- valid &= modInfo.get_name(mod_name);
- valid &= modInfo.get_base(mod_base);
- valid &= modInfo.get_base_is_offset(mod_base_is_offset);
- if (!valid)
- continue;
-
- if (!modInfo.get_link_map(link_map))
- link_map = LLDB_INVALID_ADDRESS;
-
- FileSpec file(mod_name);
- FileSystem::Instance().Resolve(file);
- lldb::ModuleSP module_sp =
- LoadModuleAtAddress(file, link_map, mod_base, mod_base_is_offset);
-
- if (module_sp.get())
- new_modules.Append(module_sp);
- }
-
- if (new_modules.GetSize() > 0) {
- ModuleList removed_modules;
- Target &target = GetTarget();
- ModuleList &loaded_modules = m_process->GetTarget().GetImages();
-
- for (size_t i = 0; i < loaded_modules.GetSize(); ++i) {
- const lldb::ModuleSP loaded_module = loaded_modules.GetModuleAtIndex(i);
-
- bool found = false;
- for (size_t j = 0; j < new_modules.GetSize(); ++j) {
- if (new_modules.GetModuleAtIndex(j).get() == loaded_module.get())
- found = true;
- }
-
- // The main executable will never be included in libraries-svr4, don't
- // remove it
- if (!found &&
- loaded_module.get() != target.GetExecutableModulePointer()) {
- removed_modules.Append(loaded_module);
- }
- }
-
- loaded_modules.Remove(removed_modules);
- m_process->GetTarget().ModulesDidUnload(removed_modules, false);
-
- new_modules.ForEach([&target](const lldb::ModuleSP module_sp) -> bool {
- lldb_private::ObjectFile *obj = module_sp->GetObjectFile();
- if (!obj)
- return true;
-
- if (obj->GetType() != ObjectFile::Type::eTypeExecutable)
- return true;
-
- lldb::ModuleSP module_copy_sp = module_sp;
- target.SetExecutableModule(module_copy_sp, eLoadDependentsNo);
- return false;
- });
-
- loaded_modules.AppendIfNeeded(new_modules);
- m_process->GetTarget().ModulesDidLoad(new_modules);
- }
-
- return new_modules.GetSize();
+ return module_list.m_list.size();
}
size_t ProcessGDBRemote::LoadModules() {
Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
===================================================================
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
@@ -182,11 +182,11 @@
SOEntry entry;
LoadedModuleInfoList module_list;
- // If we can't get the SO info from the remote, return failure.
- if (fromRemote && m_process->LoadModules(module_list) == 0)
+ if (m_current.map_addr == 0)
return false;
- if (!fromRemote && m_current.map_addr == 0)
+ // If we can't get the SO info from the remote, return failure.
+ if (fromRemote && m_process->LoadModules(module_list) == 0)
return false;
// When the previous and current states are consistent this is the first time
@@ -207,12 +207,10 @@
return false;
m_soentries.clear();
- if (fromRemote)
- return SaveSOEntriesFromRemote(module_list);
-
m_added_soentries.clear();
m_removed_soentries.clear();
- return TakeSnapshot(m_soentries);
+ return fromRemote ? SaveSOEntriesFromRemote(module_list)
+ : TakeSnapshot(m_soentries);
}
assert(m_current.state == eConsistent);
@@ -288,8 +286,10 @@
return false;
// Only add shared libraries and not the executable.
- if (!SOEntryIsMainExecutable(entry))
+ if (!SOEntryIsMainExecutable(entry)) {
m_soentries.push_back(entry);
+ m_added_soentries.push_back(entry);
+ }
}
m_loaded_modules = module_list;
@@ -321,6 +321,7 @@
return false;
m_soentries.erase(pos);
+ m_removed_soentries.push_back(entry);
}
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
