Browse Source

Profiler: Fix disassembling objects with a non-zero `.text` vaddr

Previously, we assumed that the `.text` segment was loaded at vaddr 0 in
shared object, which is not the case with `-z separate-code` enabled.
Because we didn't do the right calculations to translate an address from
a performance event into its value within the ELF file, Profiler would
try to disassemble out-of-bounds memory locations, leading to a crash.

This commit also changes `LibraryMetadata` to apply to a loaded library
as a whole, not just to one of its segments (like .text or .data). This
lets us simplify the interface, as we no longer have to worry about
`text_base`.

Fixes #10628
Daniel Bertalan 3 years ago
parent
commit
ac1cac286b
2 changed files with 34 additions and 29 deletions
  1. 34 28
      Userland/DevTools/Profiler/Process.cpp
  2. 0 1
      Userland/DevTools/Profiler/Process.h

+ 34 - 28
Userland/DevTools/Profiler/Process.cpp

@@ -68,36 +68,42 @@ static MappedObject* get_or_create_mapped_object(const String& path)
 
 void LibraryMetadata::handle_mmap(FlatPtr base, size_t size, const String& name)
 {
-    String path;
-    if (name.contains("Loader.so"))
-        path = "Loader.so";
-    else if (!name.contains(":"))
+    StringView path;
+    if (name.contains("Loader.so"sv))
+        path = "Loader.so"sv;
+    else if (!name.contains(':'))
         return;
     else
-        path = name.substring(0, name.view().find(':').value());
-
-    String full_path;
-    if (name.contains(".so"))
-        full_path = String::formatted("/usr/lib/{}", path);
-    else
-        full_path = path;
-
-    auto* mapped_object = get_or_create_mapped_object(full_path);
-    if (!mapped_object) {
-        full_path = String::formatted("/usr/local/lib/{}", path);
-        mapped_object = get_or_create_mapped_object(full_path);
-        if (!mapped_object)
-            return;
+        path = name.substring_view(0, name.view().find(':').value());
+
+    // Each loaded object has at least 4 segments associated with it: .rodata, .text, .relro, .data.
+    // We only want to create a single LibraryMetadata object for each library, so we need to update the
+    // associated base address and size as new regions are discovered.
+
+    // We don't allocate a temporary String object if an entry already exists.
+    // This assumes that String::hash and StringView::hash return the same result.
+    auto string_view_compare = [&path](auto& entry) { return path == entry.key.view(); };
+    if (auto existing_it = m_libraries.find(path.hash(), string_view_compare); existing_it != m_libraries.end()) {
+        auto& entry = *existing_it->value;
+        entry.base = min(entry.base, base);
+        entry.size = max(entry.size + size, base - entry.base + size);
+    } else {
+        String path_string = path.to_string();
+        String full_path;
+        if (name.contains(".so"sv))
+            full_path = String::formatted("/usr/lib/{}", path);
+        else
+            full_path = path_string;
+
+        auto* mapped_object = get_or_create_mapped_object(full_path);
+        if (!mapped_object) {
+            full_path = String::formatted("/usr/local/lib/{}", path);
+            mapped_object = get_or_create_mapped_object(full_path);
+            if (!mapped_object)
+                return;
+        }
+        m_libraries.set(path_string, adopt_own(*new Library { base, size, path_string, mapped_object }));
     }
-
-    FlatPtr text_base {};
-    mapped_object->elf.for_each_program_header([&](const ELF::Image::ProgramHeader& ph) {
-        if (ph.is_executable())
-            text_base = ph.vaddr().get();
-        return IterationDecision::Continue;
-    });
-
-    m_libraries.set(name, adopt_own(*new Library { base, size, name, text_base, mapped_object }));
 }
 
 String LibraryMetadata::Library::symbolicate(FlatPtr ptr, u32* offset) const
@@ -105,7 +111,7 @@ String LibraryMetadata::Library::symbolicate(FlatPtr ptr, u32* offset) const
     if (!object)
         return String::formatted("?? <{:p}>", ptr);
 
-    return object->elf.symbolicate(ptr - base + text_base, offset);
+    return object->elf.symbolicate(ptr - base, offset);
 }
 
 const LibraryMetadata::Library* LibraryMetadata::library_containing(FlatPtr ptr) const

+ 0 - 1
Userland/DevTools/Profiler/Process.h

@@ -28,7 +28,6 @@ public:
         FlatPtr base;
         size_t size;
         String name;
-        FlatPtr text_base;
         MappedObject* object { nullptr };
 
         String symbolicate(FlatPtr, u32* offset) const;