From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 964BE385B831 for ; Mon, 23 Mar 2020 03:13:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 964BE385B831 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 1D7AD1E4A4; Sun, 22 Mar 2020 23:13:25 -0400 (EDT) Subject: Re: [PATCH 7/7] [gdb/testsuite] add jit-elf-util.h and run jit function To: Mihails Strasuns , gdb-patches@sourceware.org References: <20200218124339.11270-1-mihails.strasuns@intel.com> <20200218124339.11270-8-mihails.strasuns@intel.com> From: Simon Marchi Message-ID: <32519038-95ed-d46e-28cd-0670e0798a02@simark.ca> Date: Sun, 22 Mar 2020 23:13:24 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200218124339.11270-8-mihails.strasuns@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US-large Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-16.9 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Mar 2020 03:13:27 -0000 On 2020-02-18 7:43 a.m., Mihails Strasuns wrote: > +/* Looks into string tables for entry > + "jit_function_XXXX" and updates it to use `rename_num`. */ > +static void > +update_name (ElfW (Addr) addr, int rename_num) > +{ > + const ElfW (Ehdr) *const ehdr = (ElfW (Ehdr) *) addr; > + ElfW (Shdr) *const shdr = (ElfW (Shdr) *) ((char *) addr + ehdr->e_shoff); > + ElfW (Phdr) *const phdr = (ElfW (Phdr) *) ((char *) addr + ehdr->e_phoff); > + > + for (int i = 0; i < ehdr->e_shnum; ++i) > + { > + if (shdr[i].sh_type == SHT_STRTAB) > + { > + /* Note: we update both .strtab and .dynstr. The latter would > + not be correct if this were a regular shared library (.hash > + would be wrong), but this is a simulation -- the library is > + never exposed to the dynamic loader, so it all ends up ok. */ > + char *const strtab = (char *) (addr + shdr[i].sh_offset); > + char *const strtab_end = strtab + shdr[i].sh_size; > + char *p; > + > + for (p = strtab; p < strtab_end; p += strlen (p) + 1) > + if (strcmp (p, "jit_function_XXXX") == 0) > + sprintf (p, "jit_function_%04d", rename_num); > + } > + } > +} I was wondering about this function. It updates the function name in the ELF string table, but not in the DWARF debug info, so what is GDB going to display if the debug info says that the function is called `jit_function_XXXX`? In fact, since we generate multiple shared objects, could we just generate them with the right function names directly? The .exp would compile the first one with -DJIT_FUNCTION_NAME=jit_function_0001 the second one with -DJIT_FUNCTION_NAME=jit_function_0002 and so forth. We wouldn't need any of this name munging. > + > +/* Find symbol with the name `sym_name`. */ > +static ElfW (Addr) > +load_symbol (ElfW (Addr) addr, const char *sym_name) > +{ > + const ElfW (Ehdr) *const ehdr = (ElfW (Ehdr) *) addr; > + ElfW (Shdr) *const shdr = (ElfW (Shdr) *) ((char *) addr + ehdr->e_shoff); > + > + ElfW (Addr) sym_old_addr = 0; > + ElfW (Addr) sym_new_addr = 0; > + > + /* Find `func_name` in symbol_table and adjust it from the addr. */ > + > + for (int i = 0; i < ehdr->e_shnum; ++i) > + { > + if (shdr[i].sh_type == SHT_SYMTAB) > + { > + ElfW (Sym) *symtab = (ElfW (Sym) *) (addr + shdr[i].sh_offset); > + ElfW (Sym) *symtab_end > + = (ElfW (Sym) *) (addr + shdr[i].sh_offset + shdr[i].sh_size); > + char *const strtab > + = (char *) (addr + shdr[shdr[i].sh_link].sh_offset); > + > + for (ElfW (Sym) *p = symtab; p < symtab_end; ++p) > + { > + const char *s = strtab + p->st_name; > + if (strcmp (s, sym_name) == 0) > + return p->st_value; > + } > + } > + } > + > + fprintf (stderr, "symbol '%s' not found\n", sym_name); > + exit (1); > + return 0; > +} > + > +/* Open an elf binary file and memory map it with execution flag enabled. */ > +ElfW (Addr) > +load_elf (const char *libname, size_t *size, ElfW (Addr) load_addr) I may not matter here, but since this is in a header, please make this function static. > +{ > + int fd; > + struct stat st; > + > + if ((fd = open (libname, O_RDONLY)) == -1) > + { > + fprintf (stderr, "open (\"%s\", O_RDONLY): %s\n", libname, > + strerror (errno)); > + exit (1); > + } > + > + if (fstat (fd, &st) != 0) > + { > + fprintf (stderr, "fstat (\"%d\"): %s\n", fd, strerror (errno)); > + exit (1); > + } > + > + void *addr = mmap ((void *) load_addr, st.st_size, > + PROT_READ | PROT_WRITE | PROT_EXEC, > + load_addr ? MAP_PRIVATE | MAP_FIXED : MAP_PRIVATE, fd, 0); I see that you've used MAP_FIXED here, which means you changed the code in addition to moving the function. This makes it difficult to review, since I can't really look at the diff for this function. When moving code around, we try to avoid changing it at the same time (other than the necessary adjustments). Either make those changes part of the previous patch (it seems to me like they are related enough), or make a separate patch for modifying the code vs moving it. Simon