[Lldb-commits] [lldb] [lldb] Keep addr for Memory Modules separate (PR #199810)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

<details>
<summary>Changes</summary>

When we read a Module out of memory, we keep the address of the module in Module's m_object_name field as a string.  This is normally the name of a file in a ranlib/static library/.a archive like the "main.o" in "foo.a(main.o)".  The address is most often seen in the "image list" output, and is the only easy way to distinguish in that output which binaries were read out of memory, versus found on local disk.  The "name" of the Module ends up being the combination of the FileSpec plus this m_object_name.

Reading a binary out of memory is expensive, primarily because of reading the symbol table.  The DataFileCache feature that Greg introduced five years ago can cache the Symbol Table for a binary locally, and when we see the same binary loaded again in a future debug session/lldb session, we can skip parsing the symbol table (or in the case of Memory Module, reading it from memory).

Unfortunately because the "name" of the Module is the combination of the FileSpec PLUS the address, if this is a system where binaries can load at different addresses, we may not be able to use the DataFileCache for a memory module.  The address should not be part of the name for this.

In Module, I created a new optional ivar to track the memory module address, if this is one, instead of putting the string representation in m_object_name.  In all places (except the DataFileCache name construction), I append the address to the name just like we do with m_object_name.

There were a few logging points that were manually constructing the name when a method would do it for them already; I changed them to call the method instead.

Probably the least clean part is in CommandObjectTarget (the image list output), we add the ObjectName to the FileSpec here as well. For now, instead of trying to leverage one of the methods in Module that combine them already, I changed it to use the same change.

I used a std::optional for this address instead of the LLDB_INVALID_ADDRESS convention because it was a lot cleaner to read, I thought, even if it is different than most often used in the codebase.  Open to using the older coding style if other people think consistency is more important.

---
Full diff: https://github.com/llvm/llvm-project/pull/199810.diff


3 Files Affected:

- (modified) lldb/include/lldb/Core/Module.h (+7) 
- (modified) lldb/source/Commands/CommandObjectTarget.cpp (+3) 
- (modified) lldb/source/Core/Module.cpp (+15-15) 


``````````diff
diff --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index b605a9e3b4edc..af73898519ae0 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -522,6 +522,10 @@ class Module : public std::enable_shared_from_this<Module>,
 
   ConstString GetObjectName() const;
 
+  std::optional<lldb::addr_t> GetMemoryModuleAddress() const {
+    return m_memory_module_addr;
+  }
+
   uint64_t GetObjectOffset() const { return m_object_offset; }
 
   /// Get the object file representation for the current architecture.
@@ -1062,6 +1066,9 @@ class Module : public std::enable_shared_from_this<Module>,
   ConstString m_object_name; ///< The name an object within this module that is
                              /// selected, or empty of the module is represented
                              /// by \a m_file.
+  std::optional<lldb::addr_t>
+      m_memory_module_addr; ///< For a Module read from memory,
+                            ///  the address it was read from.
   uint64_t m_object_offset = 0;
   llvm::sys::TimePoint<> m_object_mod_time;
 
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index 471e8473e84c6..56e0dfef34444 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -3453,6 +3453,9 @@ class CommandObjectTargetModulesList : public CommandObjectParsed {
       const char *object_name = module->GetObjectName().GetCString();
       if (object_name)
         strm.Printf("(%s)", object_name);
+      std::optional<addr_t> memory_addr = module->GetMemoryModuleAddress();
+      if (memory_addr.has_value())
+        strm.Printf("(0x%" PRIx64 ")", memory_addr.value());
     }
     strm.EOL();
   }
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index d857fd500c662..709b78f62062a 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -249,10 +249,9 @@ Module::Module(const FileSpec &file_spec, const ArchSpec &arch,
   }
 
   Log *log(GetLog(LLDBLog::Object | LLDBLog::Modules));
-  LLDB_LOGF(log, "%p Module::Module((%s) '%s%s%s%s')",
-            static_cast<void *>(this), m_arch.GetArchitectureName(),
-            m_file.GetPath().c_str(), m_object_name.IsEmpty() ? "" : "(",
-            m_object_name.AsCString(""), m_object_name.IsEmpty() ? "" : ")");
+  LLDB_LOGF(log, "%p Module::Module((%s) '%s')", static_cast<void *>(this),
+            m_arch.GetArchitectureName(),
+            GetSpecificationDescription().c_str());
 }
 
 Module::Module()
@@ -278,10 +277,9 @@ Module::~Module() {
     modules.erase(pos);
   }
   Log *log(GetLog(LLDBLog::Object | LLDBLog::Modules));
-  LLDB_LOGF(log, "%p Module::~Module((%s) '%s%s%s%s')",
-            static_cast<void *>(this), m_arch.GetArchitectureName(),
-            m_file.GetPath().c_str(), m_object_name.IsEmpty() ? "" : "(",
-            m_object_name.AsCString(""), m_object_name.IsEmpty() ? "" : ")");
+  LLDB_LOGF(log, "%p Module::~Module((%s) '%s')", static_cast<void *>(this),
+            m_arch.GetArchitectureName(),
+            GetSpecificationDescription().c_str());
   // Release any auto pointers before we start tearing down our member
   // variables since the object file and symbol files might need to make
   // function calls back into this module object. The ordering is important
@@ -313,9 +311,7 @@ ObjectFile *Module::GetMemoryObjectFile(const lldb::ProcessSP &process_sp,
         m_objfile_sp = ObjectFile::FindPlugin(shared_from_this(), process_sp,
                                               header_addr, data_sp);
         if (m_objfile_sp) {
-          StreamString s;
-          s.Printf("0x%16.16" PRIx64, header_addr);
-          m_object_name.SetString(s.GetString());
+          m_memory_module_addr = header_addr;
 
           // Once we get the object file, update our module with the object
           // file's architecture since it might differ in vendor/os if some
@@ -1029,6 +1025,11 @@ std::string Module::GetSpecificationDescription() const {
     spec += m_object_name.GetCString();
     spec += ')';
   }
+  if (m_memory_module_addr.has_value()) {
+    StreamString s;
+    s.Printf("(0x%" PRIx64 ")", m_memory_module_addr.value());
+    spec += s.GetData();
+  }
   return spec;
 }
 
@@ -1052,6 +1053,8 @@ void Module::GetDescription(llvm::raw_ostream &s,
   const char *object_name = m_object_name.GetCString();
   if (object_name)
     s << llvm::formatv("({0})", object_name);
+  if (m_memory_module_addr.has_value())
+    s << llvm::formatv("({0})", m_memory_module_addr.value());
 }
 
 bool Module::FileHasChanged() const {
@@ -1159,10 +1162,7 @@ void Module::Dump(Stream *s) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   // s->Printf("%.*p: ", (int)sizeof(void*) * 2, this);
   s->Indent();
-  s->Printf("Module %s%s%s%s\n", m_file.GetPath().c_str(),
-            m_object_name ? "(" : "",
-            m_object_name ? m_object_name.GetCString() : "",
-            m_object_name ? ")" : "");
+  s->Printf("Module %s\n", GetSpecificationDescription().c_str());
 
   s->IndentMore();
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/199810