Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

> 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!

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

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

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

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

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 3033 bytes --]

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 */

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libdwfl-dwfl_segment_report_module-use-ei_class-ei_d.patch --]
[-- Type: text/x-patch, Size: 6430 bytes --]

From d25ff20853035a8aaf7bf517db8e5c082770e83f Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 10 Sep 2014 21:57:36 +0200
Subject: [PATCH] libdwfl: dwfl_segment_report_module use ei_class, ei_data and
 e_type.

To make it easier to review 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>
---
 libdwfl/ChangeLog                    |  5 +++++
 libdwfl/dwfl_segment_report_module.c | 41 ++++++++++++++++++------------------
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 3de772e..bb039b5 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2014-09-10  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-08-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* dwfl_module_getdwarf.c (find_offsets): Add parameter main_bias, use
diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
index dfecb51..572f15b 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;
@@ -710,7 +711,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);
-- 
1.8.3.1

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

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

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

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

Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

>    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.

[PATCH 1/2] Add is_executable to Dwfl_Module.

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: attachment.mht --]
[-- Type: message/rfc822, Size: 47 bytes --]


WzxlbWFpbC5tZXNzYWdlLk1lc3NhZ2UgaW5zdGFuY2UgYXQgMHgyYjc2N2EwPl0=