aadsm created this revision.
aadsm added reviewers: labath, jankratochvil, clayborg.
Herald added subscribers: lldb-commits, JDevlieghere, srhines.
Herald added a project: LLDB.
aadsm added a reviewer: xiaobai.
Here's a replacement for D62504 <https://reviews.llvm.org/D62504>. I thought I
could use LoadModules to implement this but in reality I can't because there
are at few issues with it:
- The LoadModules assumes that the list returned by GetLoadedModuleList is
comprehensive in the sense that reflects all the mapped segments, however, this
is not true, for instance VDSO entry is not there since it's loaded manually by
LoadVDSO using GetMemoryRegionInfo and it doesn't represent a specific shared
object in disk. Because of this LoadModules will unload the VDSO module.
- The loader (interpreter) module might have also been loaded using
GetMemoryRegionInfo, this is true when we launch the process and the rendezvous
structure is not yet available (done through LoadInterpreterModule()). The
problem here is that this entry will point to the same file name as the one
found in /proc/pid/maps, however, when we read the same module from the
r_debug.link_map structure it might be under a different name. This is true at
least on CentOS where the loader is a symlink. Because of this LoadModules will
unload and load the module in a way where the rendezvous breakpoint is
unresolved but not resolved again (because we add the new module first and
remove the old one after).
The symlink issue might be fixable by first unloading the old and loading the
news (but sounds super brittle), however, I'm not sure how to fix the VDSO
issue.
Since I can't trust it I'm just going to use GetLoadedModuleList directly with
the same logic that we use today for when we read the linked list in lldb. The
only safe thing to do here is to only calculate differences between different
snapshots of the svr4 packet itself. This will also cut the dependency this
plugin has from LoadModules.
I separated the 2 logics into 2 different functions (remote and not remote)
because I don't like mixing 2 different logics in the same function with
if/else's. Two different functions makes it easier to reason with I believe.
However, I did abstract away the logic that decides if we should take a
snapshot or add/remove modules so both functions could reuse it.
The other difference between the two is that on the UpdateSOEntriesFromRemote I
take the snapshot only once when state = Consistent because I didn't find a
good reason to always update that, as we already got the list from state = Add
| Remove. I probably should use the same logic on UpdateSOEntries though I
don't see a reason not to since it's really using the same data, just read in
different places. Any thoughts here?
It might also be worthwhile to add a test to make sure we don't unload modules
that were not actually "unloaded" like the vdso. I haven't done this yet though.
This diff is also missing the option for svr4 like proposed in
https://reviews.llvm.org/D62503#1564296, I'll start working on this but wanted
to have this up first.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D64013
Files:
lldb/include/lldb/Target/Process.h
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -200,9 +200,9 @@
llvm::VersionTuple GetHostOSVersion() override;
- size_t LoadModules(LoadedModuleInfoList &module_list) override;
+ Status LoadModules() override;
- size_t LoadModules() override;
+ llvm::Expected<LoadedModuleInfoList> GetLoadedModuleList() override;
Status GetFileLoadAddress(const FileSpec &file, bool &is_loaded,
lldb::addr_t &load_addr) override;
@@ -391,9 +391,6 @@
// Query remote GDBServer for register information
bool GetGDBServerRegisterInfo(ArchSpec &arch);
- // Query remote GDBServer for a detailed loaded library list
- Status GetLoadedModuleList(LoadedModuleInfoList &);
-
lldb::ModuleSP LoadModuleAtAddress(const FileSpec &file,
lldb::addr_t link_map,
lldb::addr_t base_addr,
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
@@ -2728,9 +2728,9 @@
// the loaded module list can also provides a link map address
if (addr == LLDB_INVALID_ADDRESS) {
- LoadedModuleInfoList list;
- if (GetLoadedModuleList(list).Success())
- addr = list.m_link_map;
+ llvm::Expected<LoadedModuleInfoList> list = GetLoadedModuleList();
+ if (list)
+ addr = list->m_link_map;
}
return addr;
@@ -4671,28 +4671,29 @@
return m_register_info.GetNumRegisters() > 0;
}
-Status ProcessGDBRemote::GetLoadedModuleList(LoadedModuleInfoList &list) {
+llvm::Expected<LoadedModuleInfoList> ProcessGDBRemote::GetLoadedModuleList() {
// Make sure LLDB has an XML parser it can use first
if (!XMLDocument::XMLEnabled())
- return Status(0, ErrorType::eErrorTypeGeneric);
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "XML parsing not available");
Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS);
if (log)
log->Printf("ProcessGDBRemote::%s", __FUNCTION__);
+ LoadedModuleInfoList list;
GDBRemoteCommunicationClient &comm = m_gdb_comm;
// check that we have extended feature read support
if (comm.GetQXferLibrariesSVR4ReadSupported()) {
- list.clear();
-
// request the loaded library list
std::string raw;
lldb_private::Status lldberr;
if (!comm.ReadExtFeature(ConstString("libraries-svr4"), ConstString(""),
raw, lldberr))
- return Status(0, ErrorType::eErrorTypeGeneric);
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Error in libraries-svr4 packet");
// parse the xml file in memory
if (log)
@@ -4700,11 +4701,14 @@
XMLDocument doc;
if (!doc.ParseMemory(raw.c_str(), raw.size(), "noname.xml"))
- return Status(0, ErrorType::eErrorTypeGeneric);
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Error reading noname.xml");
XMLNode root_element = doc.GetRootElement("library-list-svr4");
if (!root_element)
- return Status();
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "Error finding library-list-svr4 xml element");
// main link map structure
llvm::StringRef main_lm = root_element.GetAttributeValue("main-lm");
@@ -4770,27 +4774,29 @@
if (log)
log->Printf("found %" PRId32 " modules in total",
(int)list.m_list.size());
+ return list;
} else if (comm.GetQXferLibrariesReadSupported()) {
- list.clear();
-
// request the loaded library list
std::string raw;
lldb_private::Status lldberr;
if (!comm.ReadExtFeature(ConstString("libraries"), ConstString(""), raw,
lldberr))
- return Status(0, ErrorType::eErrorTypeGeneric);
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Error in libraries packet");
if (log)
log->Printf("parsing: %s", raw.c_str());
XMLDocument doc;
if (!doc.ParseMemory(raw.c_str(), raw.size(), "noname.xml"))
- return Status(0, ErrorType::eErrorTypeGeneric);
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Error reading noname.xml");
XMLNode root_element = doc.GetRootElement("library-list");
if (!root_element)
- return Status();
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Error finding library-list xml element");
root_element.ForEachChildElementWithName(
"library", [log, &list](const XMLNode &library) -> bool {
@@ -4830,11 +4836,11 @@
if (log)
log->Printf("found %" PRId32 " modules in total",
(int)list.m_list.size());
+ return list;
} else {
- return Status(0, ErrorType::eErrorTypeGeneric);
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Remote libraries not supported");
}
-
- return Status();
}
lldb::ModuleSP ProcessGDBRemote::LoadModuleAtAddress(const FileSpec &file,
@@ -4849,17 +4855,18 @@
value_is_offset);
}
-size_t ProcessGDBRemote::LoadModules(LoadedModuleInfoList &module_list) {
+Status ProcessGDBRemote::LoadModules() {
using lldb_private::process_gdb_remote::ProcessGDBRemote;
// request a list of loaded libraries from GDBServer
- if (GetLoadedModuleList(module_list).Fail())
- return 0;
+ llvm::Expected<LoadedModuleInfoList> module_list = GetLoadedModuleList();
+ if (!module_list)
+ return Status(module_list.takeError());
// get a list of all the modules
ModuleList new_modules;
- for (LoadedModuleInfoList::LoadedModuleInfo &modInfo : module_list.m_list) {
+ for (LoadedModuleInfoList::LoadedModuleInfo &modInfo : module_list->m_list) {
std::string mod_name;
lldb::addr_t mod_base;
lldb::addr_t link_map;
@@ -4926,12 +4933,7 @@
m_process->GetTarget().ModulesDidLoad(new_modules);
}
- return new_modules.GetSize();
-}
-
-size_t ProcessGDBRemote::LoadModules() {
- LoadedModuleInfoList module_list;
- return LoadModules(module_list);
+ return Status();
}
Status ProcessGDBRemote::GetFileLoadAddress(const FileSpec &file,
Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
===================================================================
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
@@ -222,16 +222,26 @@
/// Updates the current set of SOEntries, the set of added entries, and the
/// set of removed entries.
- bool UpdateSOEntries(bool fromRemote = false);
+ bool UpdateSOEntries();
+
+ /// Same as UpdateSOEntries but it gets the list of loaded modules from the
+ /// remote debug server (faster when supported).
+ bool UpdateSOEntriesFromRemote();
+
+ bool StateIsTakeSnapshot();
+
+ bool StateIsAddModules();
+
+ bool StateIsRemoveModules();
bool FillSOEntryFromModuleInfo(
LoadedModuleInfoList::LoadedModuleInfo const &modInfo, SOEntry &entry);
- bool SaveSOEntriesFromRemote(LoadedModuleInfoList &module_list);
+ bool SaveSOEntriesFromRemote(const LoadedModuleInfoList &module_list);
- bool AddSOEntriesFromRemote(LoadedModuleInfoList &module_list);
+ bool AddSOEntriesFromRemote(const LoadedModuleInfoList &module_list);
- bool RemoveSOEntriesFromRemote(LoadedModuleInfoList &module_list);
+ bool RemoveSOEntriesFromRemote(const LoadedModuleInfoList &module_list);
bool AddSOEntries();
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
@@ -168,7 +168,10 @@
m_previous = m_current;
m_current = info;
- if (UpdateSOEntries(true))
+ if (m_current.map_addr == 0)
+ return false;
+
+ if (UpdateSOEntriesFromRemote())
return true;
return UpdateSOEntries();
@@ -178,51 +181,75 @@
return m_rendezvous_addr != LLDB_INVALID_ADDRESS;
}
-bool DYLDRendezvous::UpdateSOEntries(bool fromRemote) {
- SOEntry entry;
- LoadedModuleInfoList module_list;
+bool DYLDRendezvous::StateIsTakeSnapshot() {
+ return (
+ // When the previous and current states are consistent this is the first
+ // time we have been asked to update. Just take a snapshot of the
+ // currently loaded modules.
+ (m_previous.state == eConsistent && m_current.state == eConsistent) ||
+ // If we are about to add or remove a shared object clear out the current
+ // state and take a snapshot of the currently loaded images.
+ ((m_current.state == eAdd || m_current.state == eDelete) &&
+ // Some versions of the android dynamic linker might send two
+ // notifications with state == eAdd back to back. Ignore them until we
+ // get an eConsistent notification.
+ (m_previous.state == eConsistent ||
+ (m_previous.state == eAdd && m_current.state == eDelete))));
+}
- // If we can't get the SO info from the remote, return failure.
- if (fromRemote && m_process->LoadModules(module_list) == 0)
- return false;
+bool DYLDRendezvous::StateIsAddModules() {
+ return m_previous.state == eAdd && m_current.state == eConsistent;
+}
- if (!fromRemote && m_current.map_addr == 0)
- return false;
+bool DYLDRendezvous::StateIsRemoveModules() {
+ return m_previous.state == eDelete && m_current.state == eConsistent;
+}
- // When the previous and current states are consistent this is the first time
- // we have been asked to update. Just take a snapshot of the currently
- // loaded modules.
- if (m_previous.state == eConsistent && m_current.state == eConsistent)
- return fromRemote ? SaveSOEntriesFromRemote(module_list)
- : TakeSnapshot(m_soentries);
-
- // If we are about to add or remove a shared object clear out the current
- // state and take a snapshot of the currently loaded images.
- if (m_current.state == eAdd || m_current.state == eDelete) {
- // Some versions of the android dynamic linker might send two notifications
- // with state == eAdd back to back. Ignore them until we get an eConsistent
- // notification.
- if (!(m_previous.state == eConsistent ||
- (m_previous.state == eAdd && m_current.state == eDelete)))
- return false;
+bool DYLDRendezvous::UpdateSOEntriesFromRemote() {
+ Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
+
+ if (StateIsTakeSnapshot()) {
+ LLDB_LOG(log, "Clear Added/Removed");
+ m_added_soentries.clear();
+ m_removed_soentries.clear();
+ // We already have the loaded list from the previous update so no need to
+ // refetch.
+ if (!m_loaded_modules.m_list.empty())
+ return true;
+ }
+ llvm::Expected<LoadedModuleInfoList> module_list =
+ m_process->GetLoadedModuleList();
+ if (!module_list)
+ return false;
+
+ if (StateIsTakeSnapshot()) {
m_soentries.clear();
- if (fromRemote)
- return SaveSOEntriesFromRemote(module_list);
+ return SaveSOEntriesFromRemote(*module_list);
+ }
+ if (StateIsAddModules()) {
+ return AddSOEntriesFromRemote(*module_list);
+ }
+ if (StateIsRemoveModules()) {
+ return RemoveSOEntriesFromRemote(*module_list);
+ }
+ return false;
+}
+
+bool DYLDRendezvous::UpdateSOEntries() {
+ if (StateIsTakeSnapshot()) {
+ m_soentries.clear();
m_added_soentries.clear();
m_removed_soentries.clear();
return TakeSnapshot(m_soentries);
}
- assert(m_current.state == eConsistent);
-
- // Otherwise check the previous state to determine what to expect and update
- // accordingly.
- if (m_previous.state == eAdd)
- return fromRemote ? AddSOEntriesFromRemote(module_list) : AddSOEntries();
- else if (m_previous.state == eDelete)
- return fromRemote ? RemoveSOEntriesFromRemote(module_list)
- : RemoveSOEntries();
+ if (StateIsAddModules()) {
+ return AddSOEntries();
+ }
+ if (StateIsRemoveModules()) {
+ return RemoveSOEntries();
+ }
return false;
}
@@ -255,7 +282,7 @@
}
bool DYLDRendezvous::SaveSOEntriesFromRemote(
- LoadedModuleInfoList &module_list) {
+ const LoadedModuleInfoList &module_list) {
for (auto const &modInfo : module_list.m_list) {
SOEntry entry;
if (!FillSOEntryFromModuleInfo(modInfo, entry))
@@ -270,7 +297,8 @@
return true;
}
-bool DYLDRendezvous::AddSOEntriesFromRemote(LoadedModuleInfoList &module_list) {
+bool DYLDRendezvous::AddSOEntriesFromRemote(
+ const LoadedModuleInfoList &module_list) {
for (auto const &modInfo : module_list.m_list) {
bool found = false;
for (auto const &existing : m_loaded_modules.m_list) {
@@ -288,8 +316,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;
@@ -297,7 +327,7 @@
}
bool DYLDRendezvous::RemoveSOEntriesFromRemote(
- LoadedModuleInfoList &module_list) {
+ const LoadedModuleInfoList &module_list) {
for (auto const &existing : m_loaded_modules.m_list) {
bool found = false;
for (auto const &modInfo : module_list.m_list) {
@@ -321,6 +351,7 @@
return false;
m_soentries.erase(pos);
+ m_removed_soentries.push_back(entry);
}
}
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -680,10 +680,16 @@
/// shared library load state.
///
/// \return
- /// The number of shared libraries that were loaded
- virtual size_t LoadModules() { return 0; }
+ /// A status object indicating if the operation was sucessful or not.
+ virtual Status LoadModules() { return Status("Not implemented"); }
- virtual size_t LoadModules(LoadedModuleInfoList &) { return 0; }
+ /// Query remote GDBServer for a detailed loaded library list
+ /// \return
+ /// The list of modules currently loaded by the process, or an error.
+ virtual llvm::Expected<LoadedModuleInfoList> GetLoadedModuleList() {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Not implemented");
+ }
protected:
virtual JITLoaderList &GetJITLoaders();
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
