Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665581 > unrolled thread

Re: [RFC PATCH 04/10] objtool: add undwarf debuginfo generation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 04/10] objtool: add undwarf debuginfo generation Jiri Slaby <jslaby@suse.cz> - 2017-06-14 10:50 +0200
    Re: [RFC PATCH 04/10] objtool: add undwarf debuginfo generation Josh Poimboeuf <jpoimboe@redhat.com> - 2017-06-14 15:30 +0200

#1665581 — Re: [RFC PATCH 04/10] objtool: add undwarf debuginfo generation

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;
> +}

thanks,
-- 
js
suse labs

[toc] | [next] | [standalone]

#1665832

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web