From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752570AbdFNN1h (ORCPT ); Wed, 14 Jun 2017 09:27:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60384 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752528AbdFNN1g (ORCPT ); Wed, 14 Jun 2017 09:27:36 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 93C58C0587D3 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jpoimboe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 93C58C0587D3 Date: Wed, 14 Jun 2017 08:27:32 -0500 From: Josh Poimboeuf To: Jiri Slaby Cc: x86@kernel.org, linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, Linus Torvalds , Andy Lutomirski , Ingo Molnar , "H. Peter Anvin" , Peter Zijlstra Subject: Re: [RFC PATCH 04/10] objtool: add undwarf debuginfo generation Message-ID: <20170614132732.52etdpbvtbv7os6m@treble> References: <848a1c6a1384ff5dd40ff204e1ace7e07559fde0.1496293620.git.jpoimboe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 14 Jun 2017 13:27:35 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 14, 2017 at 10:42:19AM +0200, Jiri Slaby wrote: > On 06/01/2017, 07:44 AM, Josh Poimboeuf wrote: > ... > > index 3fb0747..1ca5d9a 100644 > > --- a/tools/objtool/elf.c > > +++ b/tools/objtool/elf.c > ... > > +int elf_write_to_file(struct elf *elf, char *outfile) > > +{ > > + int fd; > > + struct section *sec; > > + Elf *elfout; > > + GElf_Ehdr eh, ehout; > > + Elf_Scn *scn; > > + Elf_Data *data; > > + GElf_Shdr sh; > > + > > + fd = creat(outfile, 0777); > > 0755 even though it is umasked? > > > + if (fd == -1) { > > + perror("creat"); > > + return -1; > > + } > > + > > + elfout = elf_begin(fd, ELF_C_WRITE, NULL); > > + if (!elfout) { > > + perror("elf_begin"); > > + return -1; > > + } > > + > > + if (!gelf_newehdr(elfout, gelf_getclass(elf->elf))) { > > + perror("gelf_newehdr"); > > + return -1; > > + } > > + > > + if (!gelf_getehdr(elfout, &ehout)) { > > This does not make much sense to do. You memset(0) it below. > > > + perror("gelf_getehdr"); > > + return -1; > > + } > > + > > + if (!gelf_getehdr(elf->elf, &eh)) { > > + perror("gelf_getehdr"); > > + return -1; > > + } > > + > > + memset(&ehout, 0, sizeof(ehout)); > > + ehout.e_ident[EI_DATA] = eh.e_ident[EI_DATA]; > > + ehout.e_machine = eh.e_machine; > > + ehout.e_type = eh.e_type; > > + ehout.e_version = EV_CURRENT; > > + ehout.e_shstrndx = find_section_by_name(elf, ".shstrtab")->idx; > > + > > + list_for_each_entry(sec, &elf->sections, list) { > > + if (sec->idx == 0) > > + continue; > > + > > + scn = elf_newscn(elfout); > > + if (!scn) { > > + perror("elf_newscn"); > > + return -1; > > + } > > + > > + data = elf_newdata(scn); > > + if (!data) { > > + perror("elf_newdata"); > > + return -1; > > + } > > + > > + if (!elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY)) { > > + perror("elf_flagdata"); > > + return -1; > > + } > > There is not much point setting DIRTY flag here. elf_newdata does so. > > > + data->d_type = sec->data->d_type; > > + data->d_buf = sec->data->d_buf; > > + data->d_size = sec->data->d_size; > > + > > + if(!gelf_getshdr(scn, &sh)) { > > + perror("gelf_getshdr"); > > + return -1; > > + } > > This does not make much sense to do again. You overwrite the content > right away: > > > + sh = sec->sh; > > + > > + if (!gelf_update_shdr(scn, &sh)) { > > + perror("gelf_update_shdr"); > > + return -1; > > + } > > + } > > + > > + if (!gelf_update_ehdr(elfout, &ehout)) { > > + perror("gelf_update_ehdr"); > > + return -1; > > + } > > + > > + if (elf_update(elfout, ELF_C_WRITE) < 0) { > > + perror("elf_update"); > > + return -1; > > + } > > elf_end() + close() ? > > > + > > + return 0; > > +} > > + > > void elf_close(struct elf *elf) > > { > > struct section *sec, *tmpsec; > > ... > > > --- /dev/null > > +++ b/tools/objtool/undwarf.c > > @@ -0,0 +1,308 @@ > ... > > +int undwarf_dump(const char *_objname) > > +{ > > + struct elf *elf; > > + struct section *sec; > > + struct rela *rela; > > + struct undwarf *undwarf; > > + int nr, i; > > + > > + objname = _objname; > > + > > + elf = elf_open(objname); > > + if (!elf) { > > + WARN("error reading elf file %s\n", objname); > > + return 1; > > + } > > + > > + sec = find_section_by_name(elf, ".undwarf"); > > + if (!sec || !sec->rela) > > + return 0; > > + > > + nr = sec->len / sizeof(*undwarf); > > + for (i = 0; i < nr; i++) { > ... > > + } > > elf_close() ? > > > + > > + return 0; > > +} I agree with all your comments, will fix them all. Thanks for the review. -- Josh