* Small code cleanup in dwfl_segment_report_module.c (Was: [commit] [PATCHv2 1/2] Add is_executable to Dwfl_Module.)
@ 2014-09-23 12:50 Mark Wielaard
0 siblings, 0 replies; 4+ messages in thread
From: Mark Wielaard @ 2014-09-23 12:50 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 7222 bytes --]
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Small code cleanup in dwfl_segment_report_module.c (Was: [commit] [PATCHv2 1/2] Add is_executable to Dwfl_Module.)
@ 2014-09-23 15:23 Jan Kratochvil
0 siblings, 0 replies; 4+ messages in thread
From: Jan Kratochvil @ 2014-09-23 15:23 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 197 bytes --]
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Small code cleanup in dwfl_segment_report_module.c (Was: [commit] [PATCHv2 1/2] Add is_executable to Dwfl_Module.)
@ 2014-09-23 15:18 Mark Wielaard
0 siblings, 0 replies; 4+ messages in thread
From: Mark Wielaard @ 2014-09-23 15:18 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 6634 bytes --]
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Small code cleanup in dwfl_segment_report_module.c (Was: [commit] [PATCHv2 1/2] Add is_executable to Dwfl_Module.)
@ 2014-09-23 12:58 Jan Kratochvil
0 siblings, 0 replies; 4+ messages in thread
From: Jan Kratochvil @ 2014-09-23 12:58 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 813 bytes --]
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-23 15:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-23 12:50 Small code cleanup in dwfl_segment_report_module.c (Was: [commit] [PATCHv2 1/2] Add is_executable to Dwfl_Module.) Mark Wielaard
2014-09-23 12:58 Jan Kratochvil
2014-09-23 15:18 Mark Wielaard
2014-09-23 15:23 Jan Kratochvil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).