<HTML><BODY><div>Hi!</div><div> </div><div>Thanks for the comments!</div><div> </div><div>I have a question regarding tests: is it OK to bring a test .so file for each case</div><div>or should I do it in a different way?</div><div data-signature-widget="container"><div spellcheck="false" data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16475862161426697876_BODY">Hi, Maxim!<br><br>Thanks for the fixes!<br><br>Please consider my comments below.<br><br>NB: Please add tests for this patch. I suggest tests that will cover 3<br>branches:<br>* ELF reading and parsing<br>* Dumping functions only from PT_DYNAMIC segment (includes 2 types of<br> hashes)<br>* Dumping only .so name, as last fallback<br><br>Also, regarding code style: please, use quater-half tabs (8 whitespacses<br>should be replaced with 1 tab, and indentation level is 2) for all<br>LuaJIT C sources.<br><br>On 04.03.22, Maxim Kokryashkin wrote:<br>> This commit enriches memprof's symbol table with information<br><br><snipped><br><br>><br>> Makefile.original | 2 +-<br>> src/lj_memprof.c | 330 ++++++++++++++++++<br>> src/lj_memprof.h | 7 +-<br>> test/tarantool-tests/tools-utils-avl.test.lua | 59 ++++<br>> tools/CMakeLists.txt | 2 +<br>> tools/utils/avl.lua | 118 +++++++<br><br>May be we can add avl.lua utils module with the corresponding tests in<br>the separate commit and mark this commit as "Needed for"?<br>Just trying to use the most atomic commit way.<br>Feel free to ignore.<br><br>> tools/utils/symtab.lua | 24 +-<br>> 7 files changed, 537 insertions(+), 5 deletions(-)<br>> create mode 100644 test/tarantool-tests/tools-utils-avl.test.lua<br>> create mode 100644 tools/utils/avl.lua<br>><br>> diff --git a/Makefile.original b/Makefile.original<br>> index 33dc2ed5..0c92df9e 100644<br>> --- a/Makefile.original<br>> +++ b/Makefile.original<br><br><snipped><br><br>> diff --git a/src/lj_memprof.c b/src/lj_memprof.c<br>> index 2d779983..71c1da7f 100644<br>> --- a/src/lj_memprof.c<br>> +++ b/src/lj_memprof.c<br>> @@ -8,11 +8,22 @@<br>> #define lj_memprof_c<br>> #define LUA_CORE<br>><br>> +#define _GNU_SOURCE<br>> +<br>> +#include <assert.h><br><br>This include is excess.<br><br>> #include <errno.h><br>> +#include <stdio.h><br>> +#include <stdlib.h><br>> +#include <string.h><br><br><string.h> and <stdlib.h> are included inside lj_def.h, so this include is excess.<br><br>><br>> #include "lj_arch.h"<br>> #include "lj_memprof.h"<br>><br>> +#if LUAJIT_OS != LUAJIT_OS_OSX<br>> +#include <elf.h><br>> +#include <link.h><br>> +#include <sys/auxv.h><br>> +#endif<br>> #if LJ_HASMEMPROF<br>><br>> #include "lj_obj.h"<br>> @@ -66,12 +77,327 @@ static void dump_symtab_trace(struct lj_wbuf *out, const GCtrace *trace)<br>><br>> #endif<br>><br>> +#if LUAJIT_OS != LUAJIT_OS_OSX<br>> +<br>> +struct ghashtab_header {<br>> + uint32_t nbuckets;<br>> + uint32_t symoffset;<br>> + uint32_t bloom_size;<br>> + uint32_t bloom_shift;<br>> +};<br>> +<br>> +uint32_t ghashtab_size(ElfW(Addr) ghashtab)<br><br>Please use `static` as far as this function shouldn't be public.<br>Here and below.<br><br>> +{<br>> + /*<br>> + ** There is no easy way to get count of symbols in GNU hashtable, so the<br>> + ** only way to do this is to take highest possible non-empty bucket and<br>> + ** iterate through its symbols until the last chain is over.<br>> + */<br>> + uint32_t last_entry = 0;<br>> + uint32_t *cur_bucket = NULL;<br>> +<br>> + const uint32_t *chain = NULL;<br>> + struct ghashtab_header *header = (struct ghashtab_header*)ghashtab;<br>> + /*<br>> + ** sizeof(size_t) returns 8, if compiled with 64-bit compiler, and 4 if<br>> + ** compiled with 32-bit compiler. It is the best option to determine which<br>> + ** kind of CPU we are running on.<br>> + */<br>> + const char *buckets = (char*)ghashtab + sizeof(struct ghashtab_header) +<br><br>Nit: Please, use `char *` instead `char*`.<br>Here and below for all casts.<br><br>> + sizeof(size_t) * header->bloom_size;<br>> +<br>> + cur_bucket = (uint32_t*)buckets;<br><br>AFAICS, `cur_bucket` can be declared here.<br><br>> + for (uint32_t i = 0; i < header->nbuckets; ++i) {<br>> + if (last_entry < *cur_bucket)<br>> + last_entry = *cur_bucket;<br>> + cur_bucket++;<br>> + }<br>> +<br>> + if (last_entry < header->symoffset)<br>> + return header->symoffset;<br>> +<br>> + chain = (uint32_t*)(buckets + sizeof(uint32_t) * header->nbuckets);<br>> + /* The chain ends with the lowest bit set to 1. */<br>> + while (!(chain[last_entry - header->symoffset] & 1)) {<br>> + last_entry++;<br>> + }<br><br>Nit: { } are excess.<br><br>> +<br>> + return ++last_entry;<br>> +}<br>> +<br>> +struct symbol_resolver_conf {<br>> + struct lj_wbuf *buf;<br>> + const uint8_t header;<br><br>I'm a little bit confused about this field: do we need it?<br>AFAICS below, it is always SYMTAB_CFUNC. So we can just use this<br>constant in the dump. Also, it reduces amount of parameters in all these<br>functions, doesn't it?<br><br>> +};<br><br>Nit: please move this declaration below (nearby its usage).<br><br>> +<br>> +void write_c_symtab(ElfW(Sym*) sym, char *strtab, ElfW(Addr) so_addr,<br>> + size_t sym_cnt, const uint8_t header, struct lj_wbuf *buf) {<br><br>Nit: please to format this line (it should starts after opening<br>"(" with parameters declarations.<br>Also, please move `{` to the next line.<br><br>| static void write_c_symtab(ElfW(Sym*) sym, char *strtab, ElfW(Addr) so_addr,<br>| size_t sym_cnt, const uint8_t header,<br>| struct lj_wbuf *buf)<br>| {<br><br>Here and below for other functions.<br><br>> + char *sym_name = NULL;<br>> +<br>> + /*<br>> + ** Index 0 in ELF symtab is used to<br><br>Nit: something strange with line break here.<br><br>> + ** represent undefined symbols. Hence, we can just<br>> + ** start with index 1.<br>> + **<br>> + ** For more information, see:<br>> + ** <a href="https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-79797.html" target="_blank">https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-79797.html</a><br>> + */<br>> +<br>> + for (ElfW(Word) sym_index = 1; sym_index < sym_cnt; sym_index++) {<br>> + /*<br>> + ** ELF32_ST_TYPE and ELF64_ST_TYPE are the same, so we can use<br>> + ** ELF32_ST_TYPE for both 64-bit and 32-bit ELFs.<br>> + **<br>> + ** For more, see <a href="https://github.com/torvalds/linux/blob/9137eda53752ef73148e42b0d7640a00f1bc96b1/include/uapi/linux/elf.h#L135" target="_blank">https://github.com/torvalds/linux/blob/9137eda53752ef73148e42b0d7640a00f1bc96b1/include/uapi/linux/elf.h#L135</a><br>> + */<br>> + if (ELF32_ST_TYPE(sym[sym_index].st_info) == STT_FUNC) {<br>> + if (sym[sym_index].st_name == 0)<br><br>Nit: I suppose that these conditions can be joined via &&.<br><br>> + /* Symbol has no name. */<br>> + continue;<br>> + sym_name = &strtab[sym[sym_index].st_name];<br><br>Nit: Also, after fixing the previous comment you can move declaration for<br>`sym_name` here.<br><br>> + lj_wbuf_addbyte(buf, header);<br>> + lj_wbuf_addu64(buf, sym[sym_index].st_value + so_addr);<br>> + lj_wbuf_addstring(buf, sym_name);<br>> + }<br>> + }<br>> +}<br>> +<br>> +int dump_sht_symtab(const char *elf_name, struct lj_wbuf *buf,<br>> + const uint8_t header, const ElfW(Addr) so_addr) {<br>> + int status = 0;<br>> +<br>> + char *strtab = NULL;<br>> + ElfW(Shdr*) section_headers = NULL;<br>> + ElfW(Sym*) sym = NULL;<br>> + ElfW(Ehdr) elf_header = {};<br>> +<br>> + ElfW(Off) sym_off = 0;<br>> + ElfW(Off) strtab_off = 0;<br>> +<br>> + size_t sym_cnt = 0;<br>> + size_t symtab_size = 0;<br>> + size_t strtab_size = 0;<br>> + size_t strtab_index = 0;<br>> +<br>> + size_t shoff = 0; /* Section headers offset. */<br>> + size_t shnum = 0; /* Section headers number. */<br>> + size_t shentsize = 0; /* Section header entry size. */<br>> +<br>> + FILE *elf_file = fopen(elf_name, "rb");<br>> +<br>> + if (elf_file == NULL)<br>> + return -1;<br>> +<br>> + fread(&elf_header, sizeof(elf_header), 1, elf_file);<br>> + if (ferror(elf_file) != 0)<br><br>Nit: I suppose we should check amount of readen bytes and call<br>`ferrror()` only if we read not enough bytes.<br>Here and below.<br><br>> + goto error;<br>> + if (memcmp(elf_header.e_ident, ELFMAG, SELFMAG) != 0)<br>> + /* Not a valid ELF file. */<br>> + goto error;<br>> +<br>> + shoff = elf_header.e_shoff;<br>> + shnum = elf_header.e_shnum;<br>> + shentsize = elf_header.e_shentsize;<br>> +<br>> + if (shoff == 0 || shnum == 0 || shentsize == 0)<br>> + /* No sections in ELF. */<br>> + goto error;<br>> +<br>> + /*<br>> + ** Memory occupied by section headers is unlikely to be more than 160B, but<br>> + ** 32-bit and 64-bit ELF files may have sections of different sizes and some<br>> + ** of the sections may duiplicate, so we need to take that into account.<br>> + */<br>> + section_headers = calloc(shnum, shentsize);<br><br>It's better to use LuaJIT allocator here and below.<br><br>> + if (section_headers == NULL)<br>> + goto error;<br>> +<br>> + if (fseek(elf_file, shoff, SEEK_SET) != 0)<br>> + goto error;<br>> +<br>> + fread(section_headers, shentsize, shnum, elf_file);<br>> + if (ferror(elf_file) != 0)<br>> + goto error;<br>> +<br>> + for (size_t header_index = 0; header_index < shnum; ++header_index) {<br>> + if(section_headers[header_index].sh_type == SHT_SYMTAB) {<br><br>Nit: Pattern `section_headers[header_index]` is repeaten often below. I<br>suggest to use additional variable for it.<br><br>> + sym_off = section_headers[header_index].sh_offset;<br>> + symtab_size = section_headers[header_index].sh_size;<br>> + sym_cnt = symtab_size / section_headers[header_index].sh_entsize;<br>> +<br>> + strtab_index = section_headers[header_index].sh_link;<br>> +<br>> + strtab_off = section_headers[strtab_index].sh_offset;<br>> + strtab_size = section_headers[strtab_index].sh_size;<br>> + break;<br>> + }<br>> + }<br>> +<br>> + if (sym_off == 0 || strtab_off == 0 || sym_cnt == 0)<br>> + goto error;<br>> +<br>> + /* Load strtab and symtab into memory. */<br><br>Nit: I would like to see this comments separated -- current format is<br>too hard-readable due to similar structure. It will be nice to join into<br>one block loading of symtab and in the other one loading of strtab.<br><br>> + sym = calloc(sym_cnt, sizeof(ElfW(Sym)));<br>> + if (sym == NULL)<br>> + goto error;<br>> +<br>> + strtab = calloc(strtab_size, sizeof(char));<br>> + if (strtab == NULL)<br>> + goto error;<br>> +<br>> + if (fseek(elf_file, sym_off, SEEK_SET) != 0)<br>> + goto error;<br>> +<br>> + fread(sym, sizeof(ElfW(Sym)), sym_cnt, elf_file);<br>> + if (ferror(elf_file) != 0)<br>> + goto error;<br>> +<br>> + if (fseek(elf_file, strtab_off, SEEK_SET) != 0)<br>> + goto error;<br>> +<br>> + fread(strtab, sizeof(char), strtab_size, elf_file);<br>> + if (ferror(elf_file) != 0)<br>> + goto error;<br>> +<br>> + write_c_symtab(sym, strtab, so_addr, sym_cnt, header, buf);<br>> +<br>> + goto end;<br>> +<br>> + error:<br>> + status = -1;<br>> +<br>> + end:<br>> + free(sym);<br><br>`sym` may be not allocated when this line is reached (for example on<br>fread error earlier). Same for `strtab`.<br><br>> + free(strtab);<br>> + free(section_headers);<br>> + fclose(elf_file);<br>> +<br>> + return status;<br>> +}<br>> +<br>> +int dump_dyn_symtab(struct dl_phdr_info *info, const uint8_t header,<br>> + struct lj_wbuf *buf) {<br>> + for (size_t header_index = 0; header_index < info->dlpi_phnum; ++header_index) {<br><br>Nit: linewidth should be 120 symbols max.<br><br>> + if (info->dlpi_phdr[header_index].p_type == PT_DYNAMIC) {<br>> + ElfW(Dyn*) dyn = NULL;<br>> + ElfW(Sym*) sym = NULL;<br>> + ElfW(Word*) hashtab = NULL;<br>> + ElfW(Addr) ghashtab = 0;<br>> + ElfW(Word) sym_cnt = 0;<br>> +<br>> + char *strtab = 0;<br>> +<br>> + dyn = (ElfW(Dyn)*)(info->dlpi_addr + info->dlpi_phdr[header_index].p_vaddr);<br><br>Ditto.<br><br>> +<br>> + for(; dyn->d_tag != DT_NULL; dyn++) {<br>> + switch(dyn->d_tag) {<br>> + case DT_HASH:<br><br>Nit: swich case should have same indent.<br><br>> + hashtab = (ElfW(Word*))dyn->d_un.d_ptr;<br>> + break;<br>> + case DT_GNU_HASH:<br>> + ghashtab = dyn->d_un.d_ptr;<br>> + break;<br>> + case DT_STRTAB:<br>> + strtab = (char*)dyn->d_un.d_ptr;<br>> + break;<br>> + case DT_SYMTAB:<br>> + sym = (ElfW(Sym*))dyn->d_un.d_ptr;<br>> + break;<br>> + default:<br>> + break;<br>> + }<br>> + }<br>> +<br>> + if ((hashtab == NULL && ghashtab == 0)<br>> + || strtab == NULL || sym == NULL)<br><br>Nit: Looks like these lines may be joined.<br><br>> + /* Not enough data to resolve symbols. */<br>> + return 1;<br>> +<br>> + /*<br>> + ** A hash table consists of Elf32_Word or Elf64_Word objects that provide for<br>> + ** symbol table access. Hash table has the following organization:<br>> + ** +-------------------+<br><br>Nit: something strange with formating of table:).<br><br>> + ** | nbucket |<br>> + ** +-------------------+<br>> + ** | nchain |<br>> + ** +-------------------+<br>> + ** | bucket[0] |<br>> + ** | ... |<br>> + ** | bucket[nbucket-1] |<br>> + ** +-------------------+<br>> + ** | chain[0] |<br>> + ** | ... |<br>> + ** | chain[nchain-1] |<br>> + ** +-------------------+<br>> + ** Chain table entries parallel the symbol table. The number of symbol<br>> + ** table entries should equal nchain, so symbol table indexes also select<br>> + ** chain table entries. Since the chain array values are indexes for not only<br>> + ** the chain array itself, but also for the symbol table, the chain array must<br>> + ** be the same size as the symbol table. This makes nchain equal to the length<br>> + ** of the symbol table.<br><br>Nit: Please format this comment to fit linewidth 120 symbols.<br><br>> + **<br>> + ** For more, see <a href="https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-48031.html" target="_blank">https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-48031.html</a><br>> + */<br>> + sym_cnt = ghashtab == 0 ? hashtab[1] : ghashtab_size(ghashtab);<br>> + write_c_symtab(sym, strtab, info->dlpi_addr, sym_cnt, header, buf);<br>> + return 0;<br>> + }<br>> + }<br>> +<br>> + return 1;<br>> +}<br>> +<br>> +int resolve_symbolnames(struct dl_phdr_info *info, size_t info_size, void *data)<br>> +{<br>> + struct symbol_resolver_conf *conf = data;<br>> + const uint8_t header = conf->header;<br>> + struct lj_wbuf *buf = conf->buf;<br>> +<br>> + UNUSED(info_size);<br>> +<br>> + /* Skip vDSO library. */<br><br>Nit: something strange with indentation here.<br><br>> + if (info->dlpi_addr == getauxval(AT_SYSINFO_EHDR))<br>> + return 0;<br>> +<br>> + /*<br>> + ** Main way: try to open ELF and read SHT_SYMTAB, SHT_STRTAB and SHT_HASH<br>> + ** sections from it.<br>> + */<br>> + if (dump_sht_symtab(info->dlpi_name, buf, header, info->dlpi_addr) == 0) {<br>> + return 0;<br>> + }<br>> +<br>> + /* First fallback: dump functions only from PT_DYNAMIC segment. */<br>> + if(dump_dyn_symtab(info, header, buf) == 0) {<br>> + return 0;<br>> + }<br><br>Nit: I suggest to refactor this section like the following:<br>| if () {<br>| /* ... */<br>| } elseif () {<br>| /* ... */<br>| } else {<br>| /* ... */<br>| }<br>| return 0;<br><br>Feel free to ignore.<br><br>> +<br>> + /*<br>> + ** Last resort: dump ELF size and address to show .so name for its functions<br>> + ** in memprof output.<br>> + */<br>> + lj_wbuf_addbyte(buf, header);<br>> + lj_wbuf_addu64(buf, info->dlpi_addr);<br>> + lj_wbuf_addstring(buf, info->dlpi_name);<br>> +<br>> + return 0;<br>> +}<br>> +<br>> +#endif<br><br>Nit: Please, add inline comment that this is `LUAJIT_OS != LUAJIT_OS_OSX`<br><br>> +<br>> static void dump_symtab(struct lj_wbuf *out, const struct global_State *g)<br>> {<br>> const GCRef *iter = &g->gc.root;<br>> const GCobj *o;<br>> const size_t ljs_header_len = sizeof(ljs_header) / sizeof(ljs_header[0]);<br>><br>> +#if LUAJIT_OS != LUAJIT_OS_OSX<br>> + struct symbol_resolver_conf conf = {<br>> + out,<br>> + SYMTAB_CFUNC,<br>> + };<br>> +#endif<br>> +<br>> /* Write prologue. */<br>> lj_wbuf_addn(out, ljs_header, ljs_header_len);<br>><br>> @@ -95,6 +421,10 @@ static void dump_symtab(struct lj_wbuf *out, const struct global_State *g)<br>> iter = &o->gch.nextgc;<br>> }<br>><br>> +#if LUAJIT_OS != LUAJIT_OS_OSX<br>> + /* Write symbols. */<br><br>Typo? Do you mean C symbols?<br><br>> + dl_iterate_phdr(resolve_symbolnames, &conf);<br>> +#endif<br>> lj_wbuf_addbyte(out, SYMTAB_FINAL);<br>> }<br>><br>> diff --git a/src/lj_memprof.h b/src/lj_memprof.h<br>> index 395fb429..0327a205 100644<br>> --- a/src/lj_memprof.h<br>> +++ b/src/lj_memprof.h<br><br><snipped><br><br>> diff --git a/test/tarantool-tests/tools-utils-avl.test.lua b/test/tarantool-tests/tools-utils-avl.test.lua<br>> new file mode 100644<br>> index 00000000..17cc7a85<br>> --- /dev/null<br>> +++ b/test/tarantool-tests/tools-utils-avl.test.lua<br>> @@ -0,0 +1,59 @@<br>> +local avl = require "utils.avl"<br><br>Nit: Please, use require("modulename") for consistency.<br>Also, for our tests we are using '' instead "" for strings.<br><br>> +local tap = require("tap")<br>> +<br>> +local test = tap.test("tools-utils-avl")<br>> +test:plan(7)<br>> +<br>> +local function traverse(node, result)<br>> + if node ~= nil then<br>> + table.insert(result, node.key)<br>> + traverse(node.left, result)<br>> + traverse(node.right, result)<br>> + end<br>> + return result<br>> +end<br>> +<br>> +local function batch_insert(root, values)<br>> + for i = 1, #values do<br>> + root = avl.insert(root, values[i])<br>> + end<br>> +<br>> + return root<br>> +end<br>> +<br>> +local function compare(arr1, arr2)<br>> + for i, v in pairs(arr1) do<br><br>Nit: this tab looks confusing.<br><br>> + if v ~= arr2[i] then<br>> + return false<br>> + end<br>> + end<br>> + return true<br>> +end<br>> +<br>> +-- 1L rotation test.<br>> +local root = batch_insert(nil, {1, 2, 3})<br>> +test:ok(compare(traverse(root, {}), {2, 1, 3}))<br>> +<br>> +-- 1R rotation test.<br>> +root = batch_insert(nil, {3, 2, 1})<br>> +test:ok(compare(traverse(root, {}), {2, 1, 3}))<br>> +<br>> +-- 2L rotation test.<br>> +root = batch_insert(nil, {1, 3, 2})<br>> +test:ok(compare(traverse(root, {}), {2, 1, 3}))<br>> +<br>> +-- 2R rotation test.<br>> +root = batch_insert(nil, {3, 1, 2})<br>> +test:ok(compare(traverse(root, {}), {2, 1, 3}))<br>> +<br>> +-- Exact upper bound.<br>> +test:ok(avl.upper_bound(root, 1) == 1)<br>> +<br>> +-- No upper bound.<br>> +test:ok(avl.upper_bound(root, -10) == nil)<br>> +<br>> +-- Not exact upper bound.<br>> +test:ok(avl.upper_bound(root, 2.75) == 2)<br>> +<br>> +<br>> +<br><br>Nit: Please, remove unnecessary empty lines.<br>Nit: Please, and os.exit() like it is done in other tests.<br><br>> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt<br>> index 61830e44..c6803d00 100644<br>> --- a/tools/CMakeLists.txt<br>> +++ b/tools/CMakeLists.txt<br>> @@ -30,6 +30,7 @@ else()<br>> memprof/humanize.lua<br>> memprof/parse.lua<br>> memprof.lua<br>> + utils/avl.lua<br>> utils/bufread.lua<br>> utils/symtab.lua<br>> )<br>> @@ -46,6 +47,7 @@ else()<br>> COMPONENT tools-parse-memprof<br>> )<br>> install(FILES<br>> + ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua<br>> ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua<br>> ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua<br>> DESTINATION ${LUAJIT_DATAROOTDIR}/utils<br>> diff --git a/tools/utils/avl.lua b/tools/utils/avl.lua<br>> new file mode 100644<br>> index 00000000..98c15bd7<br>> --- /dev/null<br>> +++ b/tools/utils/avl.lua<br>> @@ -0,0 +1,118 @@<br>> +local math = require'math'<br><br>Nit: this require is excess.<br><br>> +<br>> +local M = {}<br>> +local max = math.max<br><br><snipped><br><br>> +function M.upper_bound(node, key)<br>> + if node == nil then<br>> + return nil, nil<br>> + end<br>> + -- Explicit match.<br>> + if key == node.key then<br>> + return node.key, node.value<br>> + elseif key < node.key then<br>> + return M.upper_bound(node.left, key)<br>> + elseif key > node.key then<br>> + local right_key, value = M.upper_bound(node.right, key)<br>> + right_key = right_key or node.key<br>> + value = value or node.value<br>> +<br>> + return right_key, value<br>> + end<br>> +end<br>> +<br><br>Nit: This empty line is excess.<br><br>> +<br>> +return M<br>> +<br><br>Nit: This empty line is excess.<br><br>> diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua<br>> index c7fcf77c..aa66269c 100644<br>> --- a/tools/utils/symtab.lua<br>> +++ b/tools/utils/symtab.lua<br>> @@ -6,6 +6,8 @@<br>><br>> local bit = require "bit"<br>><br>> +local avl = require "utils.avl"<br>> +<br>> local band = bit.band<br>> local string_format = string.format<br>><br>> @@ -15,7 +17,8 @@ local LJS_EPILOGUE_HEADER = 0x80<br>> local LJS_SYMTYPE_MASK = 0x03<br>><br>> local SYMTAB_LFUNC = 0<br>> -local SYMTAB_TRACE = 1<br>> +local SYMTAB_CFUNC = 1<br>> +local SYMTAB_TRACE = 2<br>><br>> local M = {}<br>><br>> @@ -50,15 +53,27 @@ local function parse_sym_trace(reader, symtab)<br>> }<br>> end<br>><br>> +-- Parse a single entry in a symtab: .so library<br>> +local function parse_sym_cfunc(reader, symtab)<br>> + local addr = reader:read_uleb128()<br>> + local name = reader:read_string()<br>> +<br>> + symtab.cfunc = avl.insert(symtab.cfunc, addr, {<br>> + name = name<br>> + })<br>> +end<br><br>Side note: Does it sufficient for all 3 cases mentioned above as far as<br>format is the same?<br><br>> +<br>> local parsers = {<br>> [SYMTAB_LFUNC] = parse_sym_lfunc,<br>> [SYMTAB_TRACE] = parse_sym_trace,<br>> + [SYMTAB_CFUNC] = parse_sym_cfunc<br>> }<br>><br>> function M.parse(reader)<br>> local symtab = {<br>> lfunc = {},<br>> trace = {},<br>> + cfunc = nil,<br>> }<br>> local magic = reader:read_octets(3)<br>> local version = reader:read_octets(1)<br>> @@ -93,7 +108,6 @@ function M.parse(reader)<br>> parsers[sym_type](reader, symtab)<br>> end<br>> end<br>> -<br><br>Nit: This change is unnecessary.<br><br>> return symtab<br>> end<br>><br>> @@ -135,6 +149,12 @@ function M.demangle(symtab, loc)<br>> return string_format("%s:%d", symtab.lfunc[addr].source, loc.line)<br>> end<br>><br>> + local key, value = avl.upper_bound(symtab.cfunc, addr)<br>> +<br>> + if key then<br>> + return string_format("%s:%#x", value.name, key)<br>> + end<br>> +<br>> return string_format("CFUNC %#x", addr)<br>> end<br>><br>> --<br>> 2.35.1<br>><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>