From d7ad082afa80e3ab5d40f90d777c3163ca9afd71 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 25 Dec 2020 00:59:15 +0100 Subject: [PATCH] Kernel+LibELF: Stop doing ELF symbolication in the kernel Now that the CrashDaemon symbolicates crashes in userspace, let's take this one step further and stop trying to symbolicate userspace programs in the kernel at all. --- Kernel/KSyms.cpp | 16 +++----------- Kernel/Process.cpp | 20 ------------------ Kernel/Process.h | 10 --------- Kernel/Profiling.cpp | 1 - Kernel/Thread.cpp | 15 +++---------- Libraries/LibELF/Loader.cpp | 42 +++---------------------------------- Libraries/LibELF/Loader.h | 5 ++--- 7 files changed, 11 insertions(+), 98 deletions(-) diff --git a/Kernel/KSyms.cpp b/Kernel/KSyms.cpp index f49ce0cb442..7d10e91ecfb 100644 --- a/Kernel/KSyms.cpp +++ b/Kernel/KSyms.cpp @@ -30,7 +30,6 @@ #include #include #include -#include namespace Kernel { @@ -129,11 +128,6 @@ NEVER_INLINE static void dump_backtrace_impl(FlatPtr base_pointer, bool use_ksym return; } - OwnPtr elf_bundle; - auto current_process = Process::current(); - if (current_process) - elf_bundle = current_process->elf_bundle(); - struct RecognizedSymbol { FlatPtr address; const KernelSymbol* symbol { nullptr }; @@ -170,18 +164,14 @@ NEVER_INLINE static void dump_backtrace_impl(FlatPtr base_pointer, bool use_ksym if (!symbol.address) break; if (!symbol.symbol) { - if (elf_bundle && elf_bundle->elf_loader->has_symbols()) { - dbg() << String::format("%p", symbol.address) << " " << elf_bundle->elf_loader->symbolicate(symbol.address); - } else { - dbg() << String::format("%p", symbol.address) << " (no ELF symbols for process)"; - } + dbgln("{:p}", symbol.address); continue; } size_t offset = symbol.address - symbol.symbol->address; if (symbol.symbol->address == g_highest_kernel_symbol_address && offset > 4096) - dbg() << String::format("%p", symbol.address); + dbgln("{:p}", symbol.address); else - dbg() << String::format("%p", symbol.address) << " " << demangle(symbol.symbol->name) << " +" << offset; + dbgln("{:p} {} +{}", symbol.address, demangle(symbol.symbol->name), offset); } } diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 8f85d2f0991..26a19f25cb6 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -52,7 +52,6 @@ #include #include #include -#include //#define DEBUG_IO //#define DEBUG_POLL_SELECT @@ -462,8 +461,6 @@ void Process::crash(int signal, u32 eip, bool out_of_memory) if (eip >= 0xc0000000 && g_kernel_symbols_available) { auto* symbol = symbolicate_kernel_address(eip); dbg() << "\033[31;1m" << String::format("%p", eip) << " " << (symbol ? demangle(symbol->name) : "(k?)") << " +" << (symbol ? eip - symbol->address : 0) << "\033[0m\n"; - } else if (auto elf_bundle = this->elf_bundle()) { - dbg() << "\033[31;1m" << String::format("%p", eip) << " " << elf_bundle->elf_loader->symbolicate(eip) << "\033[0m\n"; } else { dbg() << "\033[31;1m" << String::format("%p", eip) << " (?)\033[0m\n"; } @@ -891,23 +888,6 @@ void Process::set_tty(TTY* tty) m_tty = tty; } -OwnPtr Process::elf_bundle() const -{ - if (!m_executable) - return nullptr; - auto bundle = make(); - if (!m_executable->inode().shared_vmobject()) { - return nullptr; - } - ASSERT(m_executable->inode().shared_vmobject()); - auto& vmobject = *m_executable->inode().shared_vmobject(); - bundle->region = MM.allocate_kernel_region_with_vmobject(const_cast(vmobject), vmobject.size(), "ELF bundle", Region::Access::Read); - if (!bundle->region) - return nullptr; - bundle->elf_loader = ELF::Loader::create(bundle->region->vaddr().as_ptr(), bundle->region->size()); - return bundle; -} - void Process::start_tracing_from(ProcessID tracer) { m_tracer = ThreadTracer::create(tracer); diff --git a/Kernel/Process.h b/Kernel/Process.h index 7632d003501..dd454358c18 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -48,10 +48,6 @@ #include #include -namespace ELF { -class Loader; -} - namespace Kernel { timeval kgettimeofday(); @@ -471,12 +467,6 @@ public: return m_big_lock; } - struct ELFBundle { - OwnPtr region; - RefPtr elf_loader; - }; - OwnPtr elf_bundle() const; - int icon_id() const { return m_icon_id; diff --git a/Kernel/Profiling.cpp b/Kernel/Profiling.cpp index 18fff9aadc8..64d84de5623 100644 --- a/Kernel/Profiling.cpp +++ b/Kernel/Profiling.cpp @@ -31,7 +31,6 @@ #include #include #include -#include namespace Kernel { diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index ddc8e8b0012..0e1a5be6928 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -40,7 +40,6 @@ #include #include #include -#include //#define SIGNAL_DEBUG //#define THREAD_DEBUG @@ -1039,7 +1038,7 @@ struct RecognizedSymbol { const KernelSymbol* symbol { nullptr }; }; -static bool symbolicate(const RecognizedSymbol& symbol, const Process& process, StringBuilder& builder, Process::ELFBundle* elf_bundle) +static bool symbolicate(const RecognizedSymbol& symbol, const Process& process, StringBuilder& builder) { if (!symbol.address) return false; @@ -1049,10 +1048,7 @@ static bool symbolicate(const RecognizedSymbol& symbol, const Process& process, if (!is_user_address(VirtualAddress(symbol.address))) { builder.append("0xdeadc0de\n"); } else { - if (elf_bundle && elf_bundle->elf_loader->has_symbols()) - builder.appendf("%p %s\n", symbol.address, elf_bundle->elf_loader->symbolicate(symbol.address).characters()); - else - builder.appendf("%p\n", symbol.address); + builder.appendff("{:p}\n", symbol.address); } return true; } @@ -1070,11 +1066,6 @@ String Thread::backtrace_impl() Vector recognized_symbols; auto& process = const_cast(this->process()); - OwnPtr elf_bundle; - if (!Processor::current().in_irq()) { - // If we're handling IRQs we can't really safely symbolicate - elf_bundle = process.elf_bundle(); - } auto stack_trace = Processor::capture_stack_trace(*this); ASSERT(!g_scheduler_lock.own_lock()); ProcessPagingScope paging_scope(process); @@ -1088,7 +1079,7 @@ String Thread::backtrace_impl() StringBuilder builder; for (auto& symbol : recognized_symbols) { - if (!symbolicate(symbol, process, builder, elf_bundle.ptr())) + if (!symbolicate(symbol, process, builder)) break; } return builder.to_string(); diff --git a/Libraries/LibELF/Loader.cpp b/Libraries/LibELF/Loader.cpp index d585dc4b05a..52df1f38f32 100644 --- a/Libraries/LibELF/Loader.cpp +++ b/Libraries/LibELF/Loader.cpp @@ -151,22 +151,6 @@ Optional Loader::find_symbol(u32 address, u32* out_offset) const return {}; SortedSymbol* sorted_symbols = nullptr; -# ifdef KERNEL - if (!m_sorted_symbols_region) { - m_sorted_symbols_region = MM.allocate_kernel_region(PAGE_ROUND_UP(m_symbol_count * sizeof(SortedSymbol)), "Sorted symbols", Kernel::Region::Access::Read | Kernel::Region::Access::Write); - sorted_symbols = (SortedSymbol*)m_sorted_symbols_region->vaddr().as_ptr(); - size_t index = 0; - m_image.for_each_symbol([&](auto& symbol) { - sorted_symbols[index++] = { symbol.value(), symbol.name() }; - return IterationDecision::Continue; - }); - quick_sort(sorted_symbols, sorted_symbols + m_symbol_count, [](auto& a, auto& b) { - return a.address < b.address; - }); - } else { - sorted_symbols = (SortedSymbol*)m_sorted_symbols_region->vaddr().as_ptr(); - } -# else if (m_sorted_symbols.is_empty()) { m_sorted_symbols.ensure_capacity(m_symbol_count); m_image.for_each_symbol([this](auto& symbol) { @@ -178,7 +162,6 @@ Optional Loader::find_symbol(u32 address, u32* out_offset) const }); } sorted_symbols = m_sorted_symbols.data(); -# endif for (size_t i = 0; i < m_symbol_count; ++i) { if (sorted_symbols[i].address > address) { @@ -192,7 +175,6 @@ Optional Loader::find_symbol(u32 address, u32* out_offset) const } return {}; } -#endif String Loader::symbolicate(u32 address, u32* out_offset) const { @@ -202,22 +184,7 @@ String Loader::symbolicate(u32 address, u32* out_offset) const return "??"; } SortedSymbol* sorted_symbols = nullptr; -#ifdef KERNEL - if (!m_sorted_symbols_region) { - m_sorted_symbols_region = MM.allocate_kernel_region(PAGE_ROUND_UP(m_symbol_count * sizeof(SortedSymbol)), "Sorted symbols", Kernel::Region::Access::Read | Kernel::Region::Access::Write); - sorted_symbols = (SortedSymbol*)m_sorted_symbols_region->vaddr().as_ptr(); - size_t index = 0; - m_image.for_each_symbol([&](auto& symbol) { - sorted_symbols[index++] = { symbol.value(), symbol.name() }; - return IterationDecision::Continue; - }); - quick_sort(sorted_symbols, sorted_symbols + m_symbol_count, [](auto& a, auto& b) { - return a.address < b.address; - }); - } else { - sorted_symbols = (SortedSymbol*)m_sorted_symbols_region->vaddr().as_ptr(); - } -#else + if (m_sorted_symbols.is_empty()) { m_sorted_symbols.ensure_capacity(m_symbol_count); m_image.for_each_symbol([this](auto& symbol) { @@ -229,7 +196,6 @@ String Loader::symbolicate(u32 address, u32* out_offset) const }); } sorted_symbols = m_sorted_symbols.data(); -#endif for (size_t i = 0; i < m_symbol_count; ++i) { if (sorted_symbols[i].address > address) { @@ -240,14 +206,10 @@ String Loader::symbolicate(u32 address, u32* out_offset) const } auto& symbol = sorted_symbols[i - 1]; -#ifdef KERNEL - auto demangled_name = demangle(symbol.name); -#else auto& demangled_name = symbol.demangled_name; if (demangled_name.is_null()) { demangled_name = demangle(symbol.name); } -#endif if (out_offset) { *out_offset = address - symbol.address; @@ -261,4 +223,6 @@ String Loader::symbolicate(u32 address, u32* out_offset) const return "??"; } +#endif + } // end namespace ELF diff --git a/Libraries/LibELF/Loader.h b/Libraries/LibELF/Loader.h index cafbcdca6c2..109a63cae4e 100644 --- a/Libraries/LibELF/Loader.h +++ b/Libraries/LibELF/Loader.h @@ -87,9 +87,8 @@ private: Optional symbol; #endif }; -#ifdef KERNEL - mutable OwnPtr m_sorted_symbols_region; -#else + +#ifndef KERNEL mutable Vector m_sorted_symbols; #endif };