From f1d53eb2fd250f85563b420d8ec9f0ead0b4d1c8 Mon Sep 17 00:00:00 2001 From: Stephen Brennan Date: Mon, 18 Mar 2024 00:20:32 -0700 Subject: [PATCH] libdrgn: add lifetime to drgn_symbol, reduce copying The SymbolIndex structure owns the memory for symbols and names, and once added to the Program, it cannot be removed. Making copies of any of these symbols is purely a waste: we should be able to treat them as static. So add a lifetime and allow the SymbolIndex to specify static, avoiding unnecessary copies and frees. Signed-off-by: Stephen Brennan --- libdrgn/kallsyms.c | 1 + libdrgn/symbol.c | 29 ++++++++++------------------- libdrgn/symbol.h | 1 + 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/libdrgn/kallsyms.c b/libdrgn/kallsyms.c index 68e186464..771738c4e 100644 --- a/libdrgn/kallsyms.c +++ b/libdrgn/kallsyms.c @@ -298,6 +298,7 @@ static void symbol_from_kallsyms(uint64_t address, char *name, char kind, ret->kind = DRGN_SYMBOL_KIND_UNKNOWN; } ret->name_lifetime = DRGN_LIFETIME_STATIC; + ret->lifetime = DRGN_LIFETIME_STATIC; // avoid copying } /** Compute an address via the CONFIG_KALLSYMS_ABSOLUTE_PERCPU method*/ diff --git a/libdrgn/symbol.c b/libdrgn/symbol.c index 619886f51..46e21a6b0 100644 --- a/libdrgn/symbol.c +++ b/libdrgn/symbol.c @@ -15,6 +15,8 @@ DEFINE_VECTOR_FUNCTIONS(symbol_vector); LIBDRGN_PUBLIC void drgn_symbol_destroy(struct drgn_symbol *sym) { + if (sym && sym->lifetime == DRGN_LIFETIME_STATIC) + return; if (sym && sym->name_lifetime == DRGN_LIFETIME_OWNED) /* Cast here is necessary - we want symbol users to * never modify sym->name, but when we own the name, @@ -36,6 +38,7 @@ void drgn_symbol_from_elf(const char *name, uint64_t address, { ret->name = name; ret->name_lifetime = DRGN_LIFETIME_STATIC; + ret->lifetime = DRGN_LIFETIME_OWNED; ret->address = address; ret->size = elf_sym->st_size; int binding = GELF_ST_BIND(elf_sym->st_info); @@ -83,6 +86,7 @@ drgn_symbol_create(const char *name, uint64_t address, uint64_t size, sym->binding = binding; sym->kind = kind; sym->name_lifetime = name_lifetime; + sym->lifetime = DRGN_LIFETIME_OWNED; *ret = sym; return NULL; } @@ -330,21 +334,6 @@ static void address_search_range(struct drgn_symbol_index *index, uint64_t addre *start_ret = lo; } -/** Allocate a copy of the symbol and add to it the builder */ -static bool add_symbol_result(struct drgn_symbol_result_builder *builder, - struct drgn_symbol *symbol) -{ - struct drgn_symbol *copy = malloc(sizeof(*copy)); - if (!copy) - return false; - *copy = *symbol; - if (!drgn_symbol_result_builder_add(builder, copy)) { - free(copy); - return false; - } - return true; -} - struct drgn_error * drgn_symbol_index_find(const char *name, uint64_t address, enum drgn_find_symbol_flags flags, void *arg, @@ -368,7 +357,7 @@ drgn_symbol_index_find(const char *name, uint64_t address, if ((flags & DRGN_FIND_SYMBOL_NAME) && strcmp(s->name, name) != 0) continue; - if (!add_symbol_result(builder, s)) + if (!drgn_symbol_result_builder_add(builder, s)) return &drgn_enomem; if (flags & DRGN_FIND_SYMBOL_ONE) break; @@ -380,7 +369,7 @@ drgn_symbol_index_find(const char *name, uint64_t address, return NULL; for (uint32_t i = it.entry->value.start; i < it.entry->value.end; i++) { struct drgn_symbol *s = &index->symbols[index->name_sort[i]]; - if (!add_symbol_result(builder, s)) + if (!drgn_symbol_result_builder_add(builder, s)) return &drgn_enomem; if (flags & DRGN_FIND_SYMBOL_ONE) break; @@ -388,7 +377,7 @@ drgn_symbol_index_find(const char *name, uint64_t address, } else { for (int i = 0; i < index->num_syms; i++) { struct drgn_symbol *s = &index->symbols[i]; - if (!add_symbol_result(builder, s)) + if (!drgn_symbol_result_builder_add(builder, s)) return &drgn_enomem; if (flags & DRGN_FIND_SYMBOL_ONE) break; @@ -444,11 +433,13 @@ drgn_symbol_index_init_from_builder(struct drgn_symbol_index *index, // Now that the name array is finalized, resolve the names to real // pointers. Update the name lifetime to static, reflecting that the // symbol name is owned by the finder whose lifetime is bound to the - // program's once it is attached. + // program's once it is attached. The same goes for the symbol. Using + // static lifetimes helps avoid unnecessary copying. for (size_t i = 0; i < num_syms; i++) { size_t string_index = (size_t)symbols[i].name; symbols[i].name = &names[string_index]; symbols[i].name_lifetime = DRGN_LIFETIME_STATIC; + symbols[i].lifetime = DRGN_LIFETIME_STATIC; } if (num_syms > UINT32_MAX) { diff --git a/libdrgn/symbol.h b/libdrgn/symbol.h index 410ebe2a3..c3dd75ca7 100644 --- a/libdrgn/symbol.h +++ b/libdrgn/symbol.h @@ -21,6 +21,7 @@ struct drgn_symbol { enum drgn_symbol_binding binding; enum drgn_symbol_kind kind; enum drgn_lifetime name_lifetime; + enum drgn_lifetime lifetime; }; struct drgn_symbol_finder {