Skip to content

Commit

Permalink
LibCoreDump: Change Backtrace debug info cache to member variable
Browse files Browse the repository at this point in the history
This changes the previously static s_debug_info_cache to a member
variable. This is required so the cache is not kept alive if the
Backtrace object is destroyed.

Previously, the cache object would keep alive MappedFile objects and
other data, resulting in CrashReporter and CrashDaemon using more than
100 MB of memory even after the Backtrace objects have been destroyed
(and the data is thus no longer needed). This was especially the case
when handling crashes from Browser (due to libweb.so and libjs.so).

Due to this change, object_info_for_region has been promoted to a
instance method. It has also been cleaned up somewhat.
  • Loading branch information
MaxWipfli authored and awesomekling committed Jun 30, 2021
1 parent fe2716d commit c1fbfdc
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 17 deletions.
26 changes: 9 additions & 17 deletions Userland/Libraries/LibCoreDump/Backtrace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,17 @@

namespace CoreDump {

// FIXME: This cache has to be invalidated when libraries/programs are re-compiled.
// We can store the last-modified timestamp of the elf files in ELFObjectInfo to invalidate cache entries.
static HashMap<String, NonnullOwnPtr<ELFObjectInfo>> s_debug_info_cache;

static const ELFObjectInfo* object_info_for_region(const ELF::Core::MemoryRegionInfo& region)
ELFObjectInfo const* Backtrace::object_info_for_region(ELF::Core::MemoryRegionInfo const& region)
{
auto name = region.object_name();

String path;
if (name.contains(".so"))
path = String::formatted("/usr/lib/{}", name);
else {
path = name;
}
auto path = region.object_name();
if (!path.starts_with('/') && path.ends_with(".so"sv))
path = LexicalPath::join("/usr/lib", path).string();

if (auto it = s_debug_info_cache.find(path); it != s_debug_info_cache.end())
return it->value.ptr();
auto maybe_ptr = m_debug_info_cache.get(path);
if (maybe_ptr.has_value())
return *maybe_ptr;

if (!Core::File::exists(path.characters()))
if (!Core::File::exists(path))
return nullptr;

auto file_or_error = MappedFile::map(path);
Expand All @@ -49,7 +41,7 @@ static const ELFObjectInfo* object_info_for_region(const ELF::Core::MemoryRegion
#endif
auto info = make<ELFObjectInfo>(file_or_error.release_value(), make<Debug::DebugInfo>(move(image)));
auto* info_ptr = info.ptr();
s_debug_info_cache.set(path, move(info));
m_debug_info_cache.set(path, move(info));
return info_ptr;
}

Expand Down
2 changes: 2 additions & 0 deletions Userland/Libraries/LibCoreDump/Backtrace.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ class Backtrace {

private:
void add_entry(const Reader&, FlatPtr eip);
ELFObjectInfo const* object_info_for_region(ELF::Core::MemoryRegionInfo const&);

ELF::Core::ThreadInfo m_thread_info;
Vector<Entry> m_entries;
HashMap<String, NonnullOwnPtr<ELFObjectInfo>> m_debug_info_cache;
};

}

0 comments on commit c1fbfdc

Please sign in to comment.