Re: [Lldb-commits] [PATCH] D19230: Properly unload modules from target image list when using svr4 packets
This revision was automatically updated to reflect the committed changes. Closed by commit rL267467: Properly unload modules from target image list when using svr4 packets (authored by fjricci). Changed prior to commit: http://reviews.llvm.org/D19230?vs=54083=54902#toc Repository: rL LLVM http://reviews.llvm.org/D19230 Files: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Index: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp === --- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4879,7 +4879,31 @@ if (new_modules.GetSize() > 0) { +ModuleList removed_modules; Target = GetTarget(); +ModuleList _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; +} + +if (!found) +{ +lldb_private::ObjectFile * obj = loaded_module->GetObjectFile (); +if (obj && obj->GetType () != ObjectFile::Type::eTypeExecutable) +removed_modules.Append (loaded_module); +} +} + +loaded_modules.Remove (removed_modules); +m_process->GetTarget().ModulesDidUnload (removed_modules, false); new_modules.ForEach ([](const lldb::ModuleSP module_sp) -> bool { @@ -4895,13 +4919,11 @@ return false; }); -ModuleList _modules = m_process->GetTarget().GetImages(); loaded_modules.AppendIfNeeded (new_modules); m_process->GetTarget().ModulesDidLoad (new_modules); } return new_modules.GetSize(); - } size_t Index: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp === --- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4879,7 +4879,31 @@ if (new_modules.GetSize() > 0) { +ModuleList removed_modules; Target = GetTarget(); +ModuleList _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; +} + +if (!found) +{ +lldb_private::ObjectFile * obj = loaded_module->GetObjectFile (); +if (obj && obj->GetType () != ObjectFile::Type::eTypeExecutable) +removed_modules.Append (loaded_module); +} +} + +loaded_modules.Remove (removed_modules); +m_process->GetTarget().ModulesDidUnload (removed_modules, false); new_modules.ForEach ([](const lldb::ModuleSP module_sp) -> bool { @@ -4895,13 +4919,11 @@ return false; }); -ModuleList _modules = m_process->GetTarget().GetImages(); loaded_modules.AppendIfNeeded (new_modules); m_process->GetTarget().ModulesDidLoad (new_modules); } return new_modules.GetSize(); - } size_t ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19230: Properly unload modules from target image list when using svr4 packets
ADodds accepted this revision. ADodds added a comment. Looks fine to me. http://reviews.llvm.org/D19230 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19230: Properly unload modules from target image list when using svr4 packets
tfiala accepted this revision. tfiala added a comment. This revision is now accepted and ready to land. LGTM. Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4907 @@ -4883,2 +4906,3 @@ +m_process->GetTarget().ModulesDidUnload (removed_modules, false); new_modules.ForEach ([](const lldb::ModuleSP module_sp) -> bool fjricci wrote: > tfiala wrote: > > It looks like this code will unload any modules currently listed as loaded > > via m_process->GetTarget().GetImages(), if they do not appear in the > > module_list argument to this function. Is that correct behavior? (It > > might be, but that's not what I would have guessed without digging deeper). > > > > I might be not following the flow here correctly, though. Can you clarify? > > Did the full test suite run with this change? Thanks! > So yes, this will remove any existing modules that are not in the svr4 > packet, provided that there were modules listed in the svr4 packet > (indicating that the remote is using the packet) - if `new_modules.GetSize() > == 0`, we won't remove anything. > > As far as I can tell from the gdb protocol specs, the svr4 packet should > contain a list of all loaded libraries, so as long as the svr4 packet > contains libraries, I believe that removing modules which are no longer > listed is the correct behavior. > > I did run the full suite on linux (with lldb-server), and it passes with this > change. > > As a side note, the module_list argument is actually an output parameter, > filled by GetLoadedModuleList(). Oh of course I see the flow. http://reviews.llvm.org/D19230 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19230: Properly unload modules from target image list when using svr4 packets
fjricci added inline comments. Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4907 @@ -4883,2 +4906,3 @@ +m_process->GetTarget().ModulesDidUnload (removed_modules, false); new_modules.ForEach ([](const lldb::ModuleSP module_sp) -> bool tfiala wrote: > It looks like this code will unload any modules currently listed as loaded > via m_process->GetTarget().GetImages(), if they do not appear in the > module_list argument to this function. Is that correct behavior? (It might > be, but that's not what I would have guessed without digging deeper). > > I might be not following the flow here correctly, though. Can you clarify? > Did the full test suite run with this change? Thanks! So yes, this will remove any existing modules that are not in the svr4 packet, provided that there were modules listed in the svr4 packet (indicating that the remote is using the packet) - if `new_modules.GetSize() == 0`, we won't remove anything. As far as I can tell from the gdb protocol specs, the svr4 packet should contain a list of all loaded libraries, so as long as the svr4 packet contains libraries, I believe that removing modules which are no longer listed is the correct behavior. I did run the full suite on linux (with lldb-server), and it passes with this change. As a side note, the module_list argument is actually an output parameter, filled by GetLoadedModuleList(). http://reviews.llvm.org/D19230 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19230: Properly unload modules from target image list when using svr4 packets
tfiala added inline comments. Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4907 @@ -4883,2 +4906,3 @@ +m_process->GetTarget().ModulesDidUnload (removed_modules, false); new_modules.ForEach ([](const lldb::ModuleSP module_sp) -> bool It looks like this code will unload any modules currently listed as loaded via m_process->GetTarget().GetImages(), if they do not appear in the module_list argument to this function. Is that correct behavior? (It might be, but that's not what I would have guessed without digging deeper). I might be not following the flow here correctly, though. Can you clarify? Did the full test suite run with this change? Thanks! http://reviews.llvm.org/D19230 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D19230: Properly unload modules from target image list when using svr4 packets
fjricci created this revision. fjricci added reviewers: ADodds, zturner, tfiala. fjricci added subscribers: sas, lldb-commits. When we receive an svr4 packet from the remote, we check for new modules and add them to the list of images in the target. However, we did not do the same for modules which have been removed. This was causing TestLoadUnload to fail when using ds2, which uses svr4 packets to communicate all library info on Linux. This patch fixes the failing test. http://reviews.llvm.org/D19230 Files: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp === --- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4879,7 +4879,31 @@ if (new_modules.GetSize() > 0) { +ModuleList removed_modules; Target = GetTarget(); +ModuleList _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; +} + +if (!found) +{ +lldb_private::ObjectFile * obj = loaded_module->GetObjectFile (); +if (obj && obj->GetType () != ObjectFile::Type::eTypeExecutable) +removed_modules.Append (loaded_module); +} +} + +loaded_modules.Remove (removed_modules); +m_process->GetTarget().ModulesDidUnload (removed_modules, false); new_modules.ForEach ([](const lldb::ModuleSP module_sp) -> bool { @@ -4895,13 +4919,11 @@ return false; }); -ModuleList _modules = m_process->GetTarget().GetImages(); loaded_modules.AppendIfNeeded (new_modules); m_process->GetTarget().ModulesDidLoad (new_modules); } return new_modules.GetSize(); - } size_t Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp === --- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4879,7 +4879,31 @@ if (new_modules.GetSize() > 0) { +ModuleList removed_modules; Target = GetTarget(); +ModuleList _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; +} + +if (!found) +{ +lldb_private::ObjectFile * obj = loaded_module->GetObjectFile (); +if (obj && obj->GetType () != ObjectFile::Type::eTypeExecutable) +removed_modules.Append (loaded_module); +} +} + +loaded_modules.Remove (removed_modules); +m_process->GetTarget().ModulesDidUnload (removed_modules, false); new_modules.ForEach ([](const lldb::ModuleSP module_sp) -> bool { @@ -4895,13 +4919,11 @@ return false; }); -ModuleList _modules = m_process->GetTarget().GetImages(); loaded_modules.AppendIfNeeded (new_modules); m_process->GetTarget().ModulesDidLoad (new_modules); } return new_modules.GetSize(); - } size_t ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits