From acabc37c242ab2769eaa2b8bd1ac133f97c9bbf0 Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Thu, 4 Feb 2021 23:33:27 +0100
Subject: [PATCH] SymbolServer+LibSymbolClient: Just do one symbol per IPC
 message

I originally wanted to batch the symbolication requests but that just
makes the client logic significantly more complicated with no real
benefit other than architectural feelgood points.
---
 Userland/Libraries/LibSymbolClient/Client.cpp | 32 +++++++++----------
 Userland/Libraries/LibSymbolClient/Client.h   |  2 +-
 .../SymbolServer/ClientConnection.cpp         | 16 +++-------
 .../Services/SymbolServer/SymbolServer.ipc    |  2 +-
 4 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/Userland/Libraries/LibSymbolClient/Client.cpp b/Userland/Libraries/LibSymbolClient/Client.cpp
index 3ef06b44a4..2c1835e5f6 100644
--- a/Userland/Libraries/LibSymbolClient/Client.cpp
+++ b/Userland/Libraries/LibSymbolClient/Client.cpp
@@ -47,19 +47,19 @@ void Client::handle(const Messages::SymbolClient::Dummy&)
 {
 }
 
-Vector<Symbol> Client::symbolicate(const String& path, const Vector<FlatPtr>& addresses)
+Optional<Symbol> Client::symbolicate(const String& path, FlatPtr address)
 {
-    auto response = send_sync<Messages::SymbolServer::Symbolicate>(path, addresses);
+    auto response = send_sync<Messages::SymbolServer::Symbolicate>(path, address);
     if (!response->success())
         return {};
 
-    Vector<Symbol> symbols;
-    for (auto& symbol : response->symbols()) {
-        Symbol s;
-        s.name = symbol;
-        symbols.append(move(s));
-    }
-    return symbols;
+    return Symbol {
+        .address = address,
+        .name = response->name(),
+        .offset = response->offset(),
+        .filename = response->filename(),
+        .line_number = response->line()
+    };
 }
 
 Vector<Symbol> symbolicate_thread(pid_t pid, pid_t tid)
@@ -159,23 +159,21 @@ Vector<Symbol> symbolicate_thread(pid_t pid, pid_t tid)
             continue;
         }
 
-        Vector<FlatPtr> addresses;
+        FlatPtr adjusted_address;
         if (found_region->is_relative)
-            addresses.append(address - found_region->base);
+            adjusted_address = address - found_region->base;
         else
-            addresses.append(address);
+            adjusted_address = address;
 
-        auto result = client->symbolicate(found_region->path, addresses);
-        if (result.is_empty()) {
+        auto result = client->symbolicate(found_region->path, adjusted_address);
+        if (!result.has_value()) {
             symbols.append(Symbol {
                 .address = address,
             });
             continue;
         }
 
-        symbols.append(Symbol {
-            .address = address,
-            .name = result[0].name });
+        symbols.append(result.value());
     }
     return symbols;
 }
diff --git a/Userland/Libraries/LibSymbolClient/Client.h b/Userland/Libraries/LibSymbolClient/Client.h
index 8fa1cf75b9..ef99240e9f 100644
--- a/Userland/Libraries/LibSymbolClient/Client.h
+++ b/Userland/Libraries/LibSymbolClient/Client.h
@@ -50,7 +50,7 @@ class Client
 public:
     virtual void handshake() override;
 
-    Vector<Symbol> symbolicate(const String& path, const Vector<FlatPtr>& addresses);
+    Optional<Symbol> symbolicate(const String& path, FlatPtr address);
 
 private:
     Client();
diff --git a/Userland/Services/SymbolServer/ClientConnection.cpp b/Userland/Services/SymbolServer/ClientConnection.cpp
index b958807afd..fee6bfb02e 100644
--- a/Userland/Services/SymbolServer/ClientConnection.cpp
+++ b/Userland/Services/SymbolServer/ClientConnection.cpp
@@ -66,12 +66,12 @@ OwnPtr<Messages::SymbolServer::SymbolicateResponse> ClientConnection::handle(con
         auto mapped_file = MappedFile::map(path);
         if (mapped_file.is_error()) {
             dbgln("Failed to map {}: {}", path, mapped_file.error().string());
-            return make<Messages::SymbolServer::SymbolicateResponse>(false, Vector<String> {});
+            return make<Messages::SymbolServer::SymbolicateResponse>(false, String {}, 0, String {}, 0);
         }
         auto elf = ELF::Image(mapped_file.value()->bytes());
         if (!elf.is_valid()) {
             dbgln("ELF not valid: {}", path);
-            return make<Messages::SymbolServer::SymbolicateResponse>(false, Vector<String> {});
+            return make<Messages::SymbolServer::SymbolicateResponse>(false, String {}, 0, String {}, 0);
         }
         auto cached_elf = CachedELF { mapped_file.release_value(), move(elf) };
         s_cache.set(path, move(cached_elf));
@@ -81,16 +81,10 @@ OwnPtr<Messages::SymbolServer::SymbolicateResponse> ClientConnection::handle(con
     ASSERT(it != s_cache.end());
     auto& cached_elf = it->value;
 
-    Vector<String> symbols;
-    symbols.ensure_capacity(message.addresses().size());
+    u32 offset = 0;
+    auto symbol = cached_elf.elf.symbolicate(message.address(), &offset);
 
-    for (auto address : message.addresses()) {
-        u32 offset = 0;
-        auto symbol = cached_elf.elf.symbolicate(address, &offset);
-        symbols.append(move(symbol));
-    }
-
-    return make<Messages::SymbolServer::SymbolicateResponse>(true, move(symbols));
+    return make<Messages::SymbolServer::SymbolicateResponse>(true, symbol, offset, String {}, 0);
 }
 
 }
diff --git a/Userland/Services/SymbolServer/SymbolServer.ipc b/Userland/Services/SymbolServer/SymbolServer.ipc
index 10e4ed51b7..a578b01809 100644
--- a/Userland/Services/SymbolServer/SymbolServer.ipc
+++ b/Userland/Services/SymbolServer/SymbolServer.ipc
@@ -2,5 +2,5 @@ endpoint SymbolServer = 4541510
 {
     Greet() => ()
 
-    Symbolicate(String path, Vector<u32> addresses) => (bool success, Vector<String> symbols)
+    Symbolicate(String path, u32 address) => (bool success, String name, u32 offset, String filename, u32 line)
 }