int segment; /* Index of first segment table entry. */ bool gc; /* Mark/sweep flag. */
- bool is_executable : 1; /* Use Dwfl::executable_for_core? */
If you're going to use a bitfield, then make every other bool in that struct a bitfield too. But it's not usually worthwhile. It's premature microoptimization that privileges memory over CPU, which might not even be the right tradeoff any more.
On Tue, 09 Sep 2014 23:55:34 +0200, Roland McGrath wrote:
int segment; /* Index of first segment table entry. */ bool gc; /* Mark/sweep flag. */
- bool is_executable : 1; /* Use Dwfl::executable_for_core? */
If you're going to use a bitfield, then make every other bool in that struct a bitfield too. But it's not usually worthwhile. It's premature microoptimization that privileges memory over CPU, which might not even be the right tradeoff any more.
I really do not mind, up to the maintainer how it all should be.
Jan
On Wed, 2014-09-10 at 09:21 +0200, Jan Kratochvil wrote:
On Tue, 09 Sep 2014 23:55:34 +0200, Roland McGrath wrote:
int segment; /* Index of first segment table entry. */ bool gc; /* Mark/sweep flag. */
- bool is_executable : 1; /* Use Dwfl::executable_for_core? */
If you're going to use a bitfield, then make every other bool in that struct a bitfield too. But it's not usually worthwhile. It's premature microoptimization that privileges memory over CPU, which might not even be the right tradeoff any more.
I really do not mind, up to the maintainer how it all should be.
I don't think it is very helpful or productive to refuse to have a technical opinion on a fair question about a code change you are proposing. But if you need a "ruling" to move forward then I am with Roland that the usage of booleans in a structure should be consistent. Don't introduce a one-off bool bitfield unless you group the other bools in the struct together and make them all bitfields. If you are not doing that, then just make it a regular bool field.
Thanks,
Mark
On Wed, 10 Sep 2014 20:56:48 +0200, Mark Wielaard wrote:
I don't think it is very helpful or productive to refuse to have a technical opinion on a fair question about a code change you are proposing.
By ": 1" I give a promise to compiler I will use only its single bit. Smart compiler with -fwhole-program, -flto etc. could make it 'unsigned char' when the struct is not externally visible and therefore ABI-constrained.
But I see in real world the -O2 x86_64 'bool:1' code is much more horrible compared to plain 'bool' than I expected, so when we already discuss it I have changed it to 'bool'. 400406: 88 44 24 e9 mov %dl,-0x17(%rsp) vs. 400400: 0f b6 44 24 e8 movzbl -0x18(%rsp),%eax 40040b: c1 e2 03 shl $0x3,%edx 40040e: 83 e0 f7 and $0xfffffff7,%eax 400411: 09 d0 or %edx,%eax 400413: 88 44 24 e8 mov %al,-0x18(%rsp)
Thanks, Jan
On Wed, 2014-09-10 at 21:22 +0200, Jan Kratochvil wrote:
On Wed, 10 Sep 2014 20:56:48 +0200, Mark Wielaard wrote:
I don't think it is very helpful or productive to refuse to have a technical opinion on a fair question about a code change you are proposing.
By ": 1" I give a promise to compiler I will use only its single bit. Smart compiler with -fwhole-program, -flto etc. could make it 'unsigned char' when the struct is not externally visible and therefore ABI-constrained.
Yeah, I understood why you proposed it for the new field. It was just pointed out that it was an inconsistent choice with respect to the existing code/struct.
libdwfl/ 2014-09-10 Jan Kratochvil jan.kratochvil@redhat.com
* dwfl_build_id_find_elf.c (dwfl_build_id_find_elf): Use IS_EXECUTABLE. * dwfl_segment_report_module.c (dwfl_segment_report_module): Set IS_EXECUTABLE. * libdwflP.h (struct Dwfl_Module): New field is_executable.
I like this cleanup (modulo the already existing e32/e64 confusion in the code).
Thanks,
Mark
On Wed, 10 Sep 2014 22:33:50 +0200, Mark Wielaard wrote:
On Wed, 2014-09-10 at 21:22 +0200, Jan Kratochvil wrote:
libdwfl/ 2014-09-10 Jan Kratochvil jan.kratochvil@redhat.com
* dwfl_build_id_find_elf.c (dwfl_build_id_find_elf): Use IS_EXECUTABLE. * dwfl_segment_report_module.c (dwfl_segment_report_module): Set IS_EXECUTABLE. * libdwflP.h (struct Dwfl_Module): New field is_executable.I like this cleanup (modulo the already existing e32/e64 confusion in the code).
Checked in as: 6097c00a539873e9baa22e10f9387b9c36c4fa25
In the form as posted, without any cleanups being discussed.
Thanks, Jan
On Thu, 2014-09-18 at 18:23 +0200, Jan Kratochvil wrote:
On Wed, 10 Sep 2014 22:33:50 +0200, Mark Wielaard wrote:
On Wed, 2014-09-10 at 21:22 +0200, Jan Kratochvil wrote:
libdwfl/ 2014-09-10 Jan Kratochvil jan.kratochvil@redhat.com
* dwfl_build_id_find_elf.c (dwfl_build_id_find_elf): Use IS_EXECUTABLE. * dwfl_segment_report_module.c (dwfl_segment_report_module): Set IS_EXECUTABLE. * libdwflP.h (struct Dwfl_Module): New field is_executable.I like this cleanup (modulo the already existing e32/e64 confusion in the code).
Checked in as: 6097c00a539873e9baa22e10f9387b9c36c4fa25
In the form as posted, without any cleanups being discussed.
Thanks. Updated cleanup based on current master attached. I didn't take the new union member since I think that introduces the same kind of confusion of which union member to use that we are trying to fix in the first place.
Cheers,
Mark
commit c3fd359171dc2af4d034df4a8e511a1737702e0e Author: Mark Wielaard mjw@redhat.com Date: Tue Sep 23 14:48:38 2014 +0200
libdwfl: dwfl_segment_report_module use ei_class, ei_data and e_type.
To make it easier to see that the code is using the correct fields of the ehdr e32/e64 union extract ei_class, ei_data and e_type early and use them directly.
Signed-off-by: Mark Wielaard mjw@redhat.com
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index bfbc1f7..12bfa21 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,3 +1,8 @@ +2014-09-23 Mark Wielaard mjw@redhat.com + + * dwfl_segment_report_module.c (dwfl_segment_report_module): + Extract ei_class, ei_data and e_type early and use the result. + 2014-09-18 Jan Kratochvil jan.kratochvil@redhat.com
* dwfl_build_id_find_elf.c (dwfl_build_id_find_elf): Use IS_EXECUTABLE. diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 3393b08..6441aed 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -1,5 +1,5 @@ /* Sniff out modules from ELF headers visible in memory segments. - Copyright (C) 2008-2012 Red Hat, Inc. + Copyright (C) 2008-2012, 2014 Red Hat, Inc. This file is part of elfutils.
This file is free software; you can redistribute it and/or modify @@ -161,6 +161,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, }
/* Extract the information we need from the file header. */ + unsigned char ei_class; + unsigned char ei_data; + uint16_t e_type; union { Elf32_Ehdr e32; @@ -183,13 +186,15 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, .d_size = sizeof ehdr, .d_version = EV_CURRENT, }; - switch (((const unsigned char *) buffer)[EI_CLASS]) + ei_class = ((const unsigned char *) buffer)[EI_CLASS]; + ei_data = ((const unsigned char *) buffer)[EI_DATA]; + switch (ei_class) { case ELFCLASS32: xlatefrom.d_size = sizeof (Elf32_Ehdr); - if (elf32_xlatetom (&xlateto, &xlatefrom, - ((const unsigned char *) buffer)[EI_DATA]) == NULL) + if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) return finish (); + e_type = ehdr.e32.e_type; phoff = ehdr.e32.e_phoff; phnum = ehdr.e32.e_phnum; phentsize = ehdr.e32.e_phentsize; @@ -200,9 +205,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
case ELFCLASS64: xlatefrom.d_size = sizeof (Elf64_Ehdr); - if (elf64_xlatetom (&xlateto, &xlatefrom, - ((const unsigned char *) buffer)[EI_DATA]) == NULL) + if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) return finish (); + e_type = ehdr.e64.e_type; phoff = ehdr.e64.e_phoff; phnum = ehdr.e64.e_phnum; phentsize = ehdr.e64.e_phentsize; @@ -281,7 +286,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
void *notes; - if (ehdr.e32.e_ident[EI_DATA] == MY_ELFDATA) + if (ei_data == MY_ELFDATA) notes = data; else { @@ -397,10 +402,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, break; } } - if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32) + if (ei_class == ELFCLASS32) { - if (elf32_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) == NULL) + if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) found_bias = false; /* Trigger error check. */ else for (uint_fast16_t i = 0; i < phnum; ++i) @@ -411,8 +415,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, } else { - if (elf64_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) == NULL) + if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) found_bias = false; /* Trigger error check. */ else for (uint_fast16_t i = 0; i < phnum; ++i) @@ -568,7 +571,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, return soname_stroff != 0 && dynstr_vaddr != 0 && dynstrsz != 0; }
- const size_t dyn_entsize = (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32 + const size_t dyn_entsize = (ei_class == ELFCLASS32 ? sizeof (Elf32_Dyn) : sizeof (Elf64_Dyn)); void *dyn_data = NULL; size_t dyn_data_size = 0; @@ -587,18 +590,16 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, xlateto.d_buf = &dyn; xlateto.d_size = sizeof dyn;
- if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32) + if (ei_class == ELFCLASS32) { - if (elf32_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) != NULL) + if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL) for (size_t i = 0; i < dyn_filesz / sizeof dyn.d32[0]; ++i) if (consider_dyn (dyn.d32[i].d_tag, dyn.d32[i].d_un.d_val)) break; } else { - if (elf64_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) != NULL) + if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL) for (size_t i = 0; i < dyn_filesz / sizeof dyn.d64[0]; ++i) if (consider_dyn (dyn.d64[i].d_tag, dyn.d64[i].d_un.d_val)) break; @@ -608,7 +609,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
/* We'll use the name passed in or a stupid default if not DT_SONAME. */ if (name == NULL) - name = ehdr.e32.e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]"; + name = e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]";
void *soname = NULL; size_t soname_size = 0; @@ -716,7 +717,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, final_read (offset, vaddr + bias, filesz); }
- if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32) + if (ei_class == ELFCLASS32) for (uint_fast16_t i = 0; i < phnum; ++i) read_phdr (phdrs.p32[i].p_type, phdrs.p32[i].p_vaddr, phdrs.p32[i].p_offset, phdrs.p32[i].p_filesz);
On Tue, 23 Sep 2014 14:50:13 +0200, Mark Wielaard wrote:
Updated cleanup based on current master attached.
FYI I find that patch rather hiding what the code really does, in summary it has done:
- if (ehdr.e32.e_ident[EI_DATA] == MY_ELFDATA) + ei_data = ((const unsigned char *) buffer)[EI_DATA]; + if (ei_data == MY_ELFDATA)
- if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32) + ei_class = ((const unsigned char *) buffer)[EI_CLASS]; + if (ei_class == ELFCLASS32)
By jumping on the 'e_ident' tag one could say what is being referenced. But now 'buffer' is 'void *' and it is being cast to 'const unsigned char *'. And neither 'EI_DATA' nor 'EI_CLASS' reference 'e_ident' in their comment.
Not that it matters much but when there has been already some effort put into it.
Jan
On Tue, 2014-09-23 at 14:58 +0200, Jan Kratochvil wrote:
By jumping on the 'e_ident' tag one could say what is being referenced. But now 'buffer' is 'void *' and it is being cast to 'const unsigned char *'. And neither 'EI_DATA' nor 'EI_CLASS' reference 'e_ident' in their comment.
Sure, that avoids one cast, which is always nice. Patch updated.
Thanks,
Mark
commit f2f7bd73ef09da833ff5116aca6237b86cefc7df Author: Mark Wielaard mjw@redhat.com Date: Tue Sep 23 14:48:38 2014 +0200
libdwfl: dwfl_segment_report_module use ei_class, ei_data and e_type.
To make it easier to see that the code is using the correct fields of the ehdr e32/e64 union extract ei_class, ei_data and e_type early and use them directly.
Signed-off-by: Mark Wielaard mjw@redhat.com
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index bfbc1f7..12bfa21 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,3 +1,8 @@ +2014-09-23 Mark Wielaard mjw@redhat.com + + * dwfl_segment_report_module.c (dwfl_segment_report_module): + Extract ei_class, ei_data and e_type early and use the result. + 2014-09-18 Jan Kratochvil jan.kratochvil@redhat.com
* dwfl_build_id_find_elf.c (dwfl_build_id_find_elf): Use IS_EXECUTABLE. diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 3393b08..a4fbb08 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -1,5 +1,5 @@ /* Sniff out modules from ELF headers visible in memory segments. - Copyright (C) 2008-2012 Red Hat, Inc. + Copyright (C) 2008-2012, 2014 Red Hat, Inc. This file is part of elfutils.
This file is free software; you can redistribute it and/or modify @@ -161,6 +161,10 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, }
/* Extract the information we need from the file header. */ + const unsigned char *e_ident; + unsigned char ei_class; + unsigned char ei_data; + uint16_t e_type; union { Elf32_Ehdr e32; @@ -183,13 +187,16 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, .d_size = sizeof ehdr, .d_version = EV_CURRENT, }; - switch (((const unsigned char *) buffer)[EI_CLASS]) + e_ident = ((const unsigned char *) buffer); + ei_class = e_ident[EI_CLASS]; + ei_data = e_ident[EI_DATA]; + switch (ei_class) { case ELFCLASS32: xlatefrom.d_size = sizeof (Elf32_Ehdr); - if (elf32_xlatetom (&xlateto, &xlatefrom, - ((const unsigned char *) buffer)[EI_DATA]) == NULL) + if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) return finish (); + e_type = ehdr.e32.e_type; phoff = ehdr.e32.e_phoff; phnum = ehdr.e32.e_phnum; phentsize = ehdr.e32.e_phentsize; @@ -200,9 +207,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
case ELFCLASS64: xlatefrom.d_size = sizeof (Elf64_Ehdr); - if (elf64_xlatetom (&xlateto, &xlatefrom, - ((const unsigned char *) buffer)[EI_DATA]) == NULL) + if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) return finish (); + e_type = ehdr.e64.e_type; phoff = ehdr.e64.e_phoff; phnum = ehdr.e64.e_phnum; phentsize = ehdr.e64.e_phentsize; @@ -281,7 +288,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
void *notes; - if (ehdr.e32.e_ident[EI_DATA] == MY_ELFDATA) + if (ei_data == MY_ELFDATA) notes = data; else { @@ -397,10 +404,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, break; } } - if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32) + if (ei_class == ELFCLASS32) { - if (elf32_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) == NULL) + if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) found_bias = false; /* Trigger error check. */ else for (uint_fast16_t i = 0; i < phnum; ++i) @@ -411,8 +417,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, } else { - if (elf64_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) == NULL) + if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) found_bias = false; /* Trigger error check. */ else for (uint_fast16_t i = 0; i < phnum; ++i) @@ -568,7 +573,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, return soname_stroff != 0 && dynstr_vaddr != 0 && dynstrsz != 0; }
- const size_t dyn_entsize = (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32 + const size_t dyn_entsize = (ei_class == ELFCLASS32 ? sizeof (Elf32_Dyn) : sizeof (Elf64_Dyn)); void *dyn_data = NULL; size_t dyn_data_size = 0; @@ -587,18 +592,16 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, xlateto.d_buf = &dyn; xlateto.d_size = sizeof dyn;
- if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32) + if (ei_class == ELFCLASS32) { - if (elf32_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) != NULL) + if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL) for (size_t i = 0; i < dyn_filesz / sizeof dyn.d32[0]; ++i) if (consider_dyn (dyn.d32[i].d_tag, dyn.d32[i].d_un.d_val)) break; } else { - if (elf64_xlatetom (&xlateto, &xlatefrom, - ehdr.e32.e_ident[EI_DATA]) != NULL) + if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL) for (size_t i = 0; i < dyn_filesz / sizeof dyn.d64[0]; ++i) if (consider_dyn (dyn.d64[i].d_tag, dyn.d64[i].d_un.d_val)) break; @@ -608,7 +611,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
/* We'll use the name passed in or a stupid default if not DT_SONAME. */ if (name == NULL) - name = ehdr.e32.e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]"; + name = e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]";
void *soname = NULL; size_t soname_size = 0; @@ -716,7 +719,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, final_read (offset, vaddr + bias, filesz); }
- if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32) + if (ei_class == ELFCLASS32) for (uint_fast16_t i = 0; i < phnum; ++i) read_phdr (phdrs.p32[i].p_type, phdrs.p32[i].p_vaddr, phdrs.p32[i].p_offset, phdrs.p32[i].p_filesz);
On Tue, 23 Sep 2014 17:18:05 +0200, Mark Wielaard wrote:
Sure, that avoids one cast, which is always nice. Patch updated.
That looks more OK with me to keep ctags working.
Thanks, Jan
This is somewhat separate from the rest of your proposed patch, but...
- // !execlike && ET_EXEC is PIE.
- // execlike && !ET_EXEC is a static executable.
- if (mod != NULL && (execlike || ehdr.e32.e_type == ET_EXEC))
- mod->is_executable = true;
This made me look in the function and there are a couple more places that just use ehdr.e32 fields without checking whether we are dealing with a ELFCLASS32/Elf32_Ehdr or ELFCLASS64/Elf64_Ehdr (or worse use the e32 variants even when we just checked it is Elf64). ehdr is a union of those two types. It happens to work for the first few struct members since they have the same underlying base type (but different type names). But that makes it somewhat hard to review this code is correct. You have to double check every time the field offsets really match up.
So I propose a cleanup like the attached first.
Cheers,
Mark
On Wed, 10 Sep 2014 22:15:42 +0200, Mark Wielaard wrote:
So I propose a cleanup like the attached first.
While I find that as an improvement in general IMO on top of your patch the changes could be done a bit differently.
Patricularly I at least miss there that 'e_ident'.
Jan
diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 572f15b..be6950d 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -163,11 +163,11 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, /* Extract the information we need from the file header. */ unsigned char ei_class; unsigned char ei_data; - uint16_t e_type; union { Elf32_Ehdr e32; Elf64_Ehdr e64; + Elf_Ehdr e; } ehdr; GElf_Off phoff; uint_fast16_t phnum; @@ -186,15 +186,14 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, .d_size = sizeof ehdr, .d_version = EV_CURRENT, }; - ei_class = ((const unsigned char *) buffer)[EI_CLASS]; - ei_data = ((const unsigned char *) buffer)[EI_DATA]; + ei_class = ((const Elf_Ehdr *) buffer)->e_ident[EI_CLASS]; + ei_data = ((const Elf_Ehdr *) buffer)->e_ident[EI_DATA]; switch (ei_class) { case ELFCLASS32: xlatefrom.d_size = sizeof (Elf32_Ehdr); if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) return finish (); - e_type = ehdr.e32.e_type; phoff = ehdr.e32.e_phoff; phnum = ehdr.e32.e_phnum; phentsize = ehdr.e32.e_phentsize; @@ -207,7 +206,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, xlatefrom.d_size = sizeof (Elf64_Ehdr); if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) return finish (); - e_type = ehdr.e64.e_type; phoff = ehdr.e64.e_phoff; phnum = ehdr.e64.e_phnum; phentsize = ehdr.e64.e_phentsize; @@ -609,7 +607,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
/* We'll use the name passed in or a stupid default if not DT_SONAME. */ if (name == NULL) - name = e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]"; + name = ehdr.e.e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]";
void *soname = NULL; size_t soname_size = 0; diff --git a/libelf/elf.h b/libelf/elf.h index 40e87b2..47e7bc7 100644 --- a/libelf/elf.h +++ b/libelf/elf.h @@ -67,6 +67,14 @@ typedef Elf64_Half Elf64_Versym; typedef struct { unsigned char e_ident[EI_NIDENT]; /* Magic number and other info */ + uint16_t e_type; /* Object file type */ + uint16_t e_machine; /* Architecture */ + uint32_t e_version; /* Object file version */ +} Elf_Ehdr; + +typedef struct +{ + unsigned char e_ident[EI_NIDENT]; /* Magic number and other info */ Elf32_Half e_type; /* Object file type */ Elf32_Half e_machine; /* Architecture */ Elf32_Word e_version; /* Object file version */
ping, so that the patchset can move forward
On Thu, 11 Sep 2014 21:00:49 +0200, Jan Kratochvil wrote:
On Wed, 10 Sep 2014 22:15:42 +0200, Mark Wielaard wrote:
So I propose a cleanup like the attached first.
While I find that as an improvement in general IMO on top of your patch the changes could be done a bit differently.
Patricularly I at least miss there that 'e_ident'.
On Wed, 2014-09-17 at 22:31 +0200, Jan Kratochvil wrote:
ping, so that the patchset can move forward
BTW. Please feel free to push the already approved patch that actually does the code change. The cleanup can be done separately (since it doesn't really change the code, just makes it more readable).
Cheers,
Mark
On Thu, 2014-09-11 at 21:00 +0200, Jan Kratochvil wrote:
On Wed, 10 Sep 2014 22:15:42 +0200, Mark Wielaard wrote:
So I propose a cleanup like the attached first.
While I find that as an improvement in general IMO on top of your patch the changes could be done a bit differently.
Clever. You create another struct in the union for just those fields that do overlap. That does indeed solve the issue of documenting what can/cannot be directly accessed. But I am afraid that might be a little too subtle. You are kind of back to the original issue that you have to check why this is allowed. I like the direct approach better, where the union accesses are only done when the type is explicitly known.
Also we shouldn't add such a partial type struct to the public gelf.h header since that is kind of a shared standard type, so if other libelf/gelf implementations don't support it we shouldn't export it IMHO.
Cheers,
Mark
On Thu, 18 Sep 2014 16:56:59 +0200, Mark Wielaard wrote:
Also we shouldn't add such a partial type struct to the public gelf.h header since that is kind of a shared standard type, so if other libelf/gelf implementations don't support it we shouldn't export it IMHO.
BTW there could be #ifdef _GNU_SOURCE for such case.
Jan
On Thu, 18 Sep 2014 16:56:59 +0200, Mark Wielaard wrote:
Also we shouldn't add such a partial type struct to the public gelf.h header since that is kind of a shared standard type, so if other libelf/gelf implementations don't support it we shouldn't export it IMHO.
BTW there could be #ifdef _GNU_SOURCE for such case.
No!
elfutils-devel@lists.fedorahosted.org
-
Jan Kratochvil -
Mark Wielaard -
Roland McGrath