Remove nested functions from readelf.c

Remove nested functions from readelf.c

[tbaeder]

Hi,

here another round for src/readelf.c. I think they are simple, but I'm
not happy with the advance_pc() commit. I'm open for suggestions here
but I can't come up with something better right now. I'll keep looking
in to that in the meantime.


- Timm

[PATCH 1/5] readelf: Pull add_dump_section() into file scope

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Get rid of a nested function this way.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/readelf.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/readelf.c b/src/readelf.c
index 829a418d..a95fc0aa 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -381,24 +381,26 @@ main (int argc, char *argv[])
   return error_message_count != 0;
 }
 
+static void
+add_dump_section (const char *name,
+		  int key,
+		  bool implicit)
+{
+  struct section_argument *a = xmalloc (sizeof *a);
+  a->arg = name;
+  a->next = NULL;
+  a->implicit = implicit;
+  struct section_argument ***tailp
+    = key == 'x' ? &dump_data_sections_tail : &string_sections_tail;
+  **tailp = a;
+  *tailp = &a->next;
+}
 
 /* Handle program arguments.  */
 static error_t
 parse_opt (int key, char *arg,
 	   struct argp_state *state __attribute__ ((unused)))
 {
-  void add_dump_section (const char *name, bool implicit)
-  {
-    struct section_argument *a = xmalloc (sizeof *a);
-    a->arg = name;
-    a->next = NULL;
-    a->implicit = implicit;
-    struct section_argument ***tailp
-      = key == 'x' ? &dump_data_sections_tail : &string_sections_tail;
-    **tailp = a;
-    *tailp = &a->next;
-  }
-
   switch (key)
     {
     case 'a':
@@ -414,9 +416,9 @@ parse_opt (int key, char *arg,
       print_arch = true;
       print_notes = true;
       implicit_debug_sections |= section_exception;
-      add_dump_section (".strtab", true);
-      add_dump_section (".dynstr", true);
-      add_dump_section (".comment", true);
+      add_dump_section (".strtab", key, true);
+      add_dump_section (".dynstr", key, true);
+      add_dump_section (".comment", key, true);
       any_control_option = true;
       break;
     case 'A':
@@ -562,7 +564,7 @@ parse_opt (int key, char *arg,
 	}
       FALLTHROUGH;
     case 'x':
-      add_dump_section (arg, false);
+      add_dump_section (arg, key, false);
       any_control_option = true;
       break;
     case 'N':
-- 
2.26.2

Re: [PATCH 1/5] readelf: Pull add_dump_section() into file scope

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:16:29AM +0100, Timm Bäder via Elfutils-devel wrote:
> Get rid of a nested function this way.

OK. Adding the key as argument allows to select either the data or
string sections to add. Added a ChangeLog entry and pushed.

Thanks,

Mark

[PATCH 2/5] readelf: Pull same_set() info file scope

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Get rid of a nested function this way

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/readelf.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/readelf.c b/src/readelf.c
index a95fc0aa..0157f8a2 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -12072,6 +12072,18 @@ compare_register_sets (const void *a, const void *b)
   return compare_sets_by_info (*p1, *p2);
 }
 
+static inline bool
+same_set (const struct register_info *a,
+	  const struct register_info *b,
+	  const struct register_info *regs,
+	  size_t maxnreg)
+{
+  return (a < &regs[maxnreg] && a->regloc != NULL
+	  && b < &regs[maxnreg] && b->regloc != NULL
+	  && a->bits == b->bits
+	  && (a->set == b->set || !strcmp (a->set, b->set)));
+}
+
 static unsigned int
 handle_core_registers (Ebl *ebl, Elf *core, const void *desc,
 		       const Ebl_Register_Location *reglocs, size_t nregloc)
@@ -12110,19 +12122,11 @@ handle_core_registers (Ebl *ebl, Elf *core, const void *desc,
   qsort (regs, maxreg + 1, sizeof regs[0], &compare_registers);
 
   /* Collect the unique sets and sort them.  */
-  inline bool same_set (const struct register_info *a,
-			const struct register_info *b)
-  {
-    return (a < &regs[maxnreg] && a->regloc != NULL
-	    && b < &regs[maxnreg] && b->regloc != NULL
-	    && a->bits == b->bits
-	    && (a->set == b->set || !strcmp (a->set, b->set)));
-  }
   struct register_info *sets[maxreg + 1];
   sets[0] = &regs[0];
   size_t nsets = 1;
   for (int i = 1; i <= maxreg; ++i)
-    if (regs[i].regloc != NULL && !same_set (&regs[i], &regs[i - 1]))
+    if (regs[i].regloc != NULL && !same_set (&regs[i], &regs[i - 1], regs, maxnreg))
       sets[nsets++] = &regs[i];
   qsort (sets, nsets, sizeof sets[0], &compare_register_sets);
 
@@ -12133,7 +12137,7 @@ handle_core_registers (Ebl *ebl, Elf *core, const void *desc,
       /* Find the longest name of a register in this set.  */
       size_t maxname = 0;
       const struct register_info *end;
-      for (end = sets[i]; same_set (sets[i], end); ++end)
+      for (end = sets[i]; same_set (sets[i], end, regs, maxnreg); ++end)
 	{
 	  size_t len = strlen (end->name);
 	  if (len > maxname)
-- 
2.26.2

Re: [PATCH 2/5] readelf: Pull same_set() info file scope

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:16:30AM +0100, Timm Bäder via Elfutils-devel wrote:
> Get rid of a nested function this way

OK, with the 2 extra arguments this works identically.
I did put one same_set call on its own line because it became too long.
Added a ChangeLog entry and pushed.

Thanks,

Mark

[PATCH 3/5] readelf: Pull left() info file scope

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Get rid of a nested function this way.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/readelf.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/readelf.c b/src/readelf.c
index 0157f8a2..99e90c34 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -3571,6 +3571,13 @@ print_liblist (Ebl *ebl)
     }
 }
 
+static inline size_t
+left (Elf_Data *data,
+      const unsigned char *p)
+{
+  return (const unsigned char *) data->d_buf + data->d_size - p;
+}
+
 static void
 print_attributes (Ebl *ebl, const GElf_Ehdr *ehdr)
 {
@@ -3615,13 +3622,8 @@ print_attributes (Ebl *ebl, const GElf_Ehdr *ehdr)
 
       fputs_unlocked (_("  Owner          Size\n"), stdout);
 
-      inline size_t left (void)
-      {
-	return (const unsigned char *) data->d_buf + data->d_size - p;
-      }
-
       /* Loop over the sections.  */
-      while (left () >= 4)
+      while (left (data, p) >= 4)
 	{
 	  /* Section length.  */
 	  uint32_t len;
@@ -3630,7 +3632,7 @@ print_attributes (Ebl *ebl, const GElf_Ehdr *ehdr)
 	  if (MY_ELFDATA != ehdr->e_ident[EI_DATA])
 	    CONVERT (len);
 
-	  if (unlikely (len > left ()))
+	  if (unlikely (len > left (data, p)))
 	    break;
 
 	  /* Section vendor name.  */
-- 
2.26.2

Re: [PATCH 3/5] readelf: Pull left() info file scope

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:16:31AM +0100, Timm Bäder via Elfutils-devel wrote:
> Get rid of a nested function this way.

I would probably simply inlined that line of code into the two callers
instead of introducing a new function with two arguments. But this
also works. Added a ChangeLog entry and pushed.

Cheers,

Mark

[PATCH 4/5] readelf: Pull regname() into file scope

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Get rid of a nested function this way.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/readelf.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/src/readelf.c b/src/readelf.c
index 99e90c34..e0163891 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -6206,6 +6206,13 @@ read_encoded (unsigned int encoding, const unsigned char *readp,
   return readp;
 }
 
+static const char *
+regname (Ebl *ebl, unsigned int regno, char *regnamebuf)
+{
+  register_info (ebl, regno, NULL, regnamebuf, NULL, NULL);
+
+  return regnamebuf;
+}
 
 static void
 print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
@@ -6216,11 +6223,6 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 		   Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr, Dwarf *dbg)
 {
   char regnamebuf[REGNAMESZ];
-  const char *regname (unsigned int regno)
-  {
-    register_info (ebl, regno, NULL, regnamebuf, NULL, NULL);
-    return regnamebuf;
-  }
 
   puts ("\n   Program:");
   Dwarf_Word pc = vma_base;
@@ -6277,26 +6279,28 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	    get_uleb128 (op2, readp, endp);
 	    printf ("     offset_extended r%" PRIu64 " (%s) at cfa%+" PRId64
 		    "\n",
-		    op1, regname (op1), op2 * data_align);
+		    op1, regname (ebl, op1, regnamebuf), op2 * data_align);
 	    break;
 	  case DW_CFA_restore_extended:
 	    if ((uint64_t) (endp - readp) < 1)
 	      goto invalid;
 	    get_uleb128 (op1, readp, endp);
 	    printf ("     restore_extended r%" PRIu64 " (%s)\n",
-		    op1, regname (op1));
+		    op1, regname (ebl, op1, regnamebuf));
 	    break;
 	  case DW_CFA_undefined:
 	    if ((uint64_t) (endp - readp) < 1)
 	      goto invalid;
 	    get_uleb128 (op1, readp, endp);
-	    printf ("     undefined r%" PRIu64 " (%s)\n", op1, regname (op1));
+	    printf ("     undefined r%" PRIu64 " (%s)\n", op1,
+		    regname (ebl, op1, regnamebuf));
 	    break;
 	  case DW_CFA_same_value:
 	    if ((uint64_t) (endp - readp) < 1)
 	      goto invalid;
 	    get_uleb128 (op1, readp, endp);
-	    printf ("     same_value r%" PRIu64 " (%s)\n", op1, regname (op1));
+	    printf ("     same_value r%" PRIu64 " (%s)\n", op1,
+		    regname (ebl, op1, regnamebuf));
 	    break;
 	  case DW_CFA_register:
 	    if ((uint64_t) (endp - readp) < 1)
@@ -6306,7 +6310,8 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	      goto invalid;
 	    get_uleb128 (op2, readp, endp);
 	    printf ("     register r%" PRIu64 " (%s) in r%" PRIu64 " (%s)\n",
-		    op1, regname (op1), op2, regname (op2));
+		    op1, regname (ebl, op1, regnamebuf), op2,
+		    regname (ebl, op2, regnamebuf));
 	    break;
 	  case DW_CFA_remember_state:
 	    puts ("     remember_state");
@@ -6322,14 +6327,14 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	      goto invalid;
 	    get_uleb128 (op2, readp, endp);
 	    printf ("     def_cfa r%" PRIu64 " (%s) at offset %" PRIu64 "\n",
-		    op1, regname (op1), op2);
+		    op1, regname (ebl, op1, regnamebuf), op2);
 	    break;
 	  case DW_CFA_def_cfa_register:
 	    if ((uint64_t) (endp - readp) < 1)
 	      goto invalid;
 	    get_uleb128 (op1, readp, endp);
 	    printf ("     def_cfa_register r%" PRIu64 " (%s)\n",
-		    op1, regname (op1));
+		    op1, regname (ebl, op1, regnamebuf));
 	    break;
 	  case DW_CFA_def_cfa_offset:
 	    if ((uint64_t) (endp - readp) < 1)
@@ -6360,7 +6365,7 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	      goto invalid;
 	    get_uleb128 (op2, readp, endp);	/* Length of DW_FORM_block.  */
 	    printf ("     expression r%" PRIu64 " (%s) \n",
-		    op1, regname (op1));
+		    op1, regname (ebl, op1, regnamebuf));
 	    if ((uint64_t) (endp - readp) < op2)
 	      goto invalid;
 	    print_ops (dwflmod, dbg, 10, 10, version, ptr_size, 0, NULL,
@@ -6376,7 +6381,7 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	    get_sleb128 (sop2, readp, endp);
 	    printf ("     offset_extended_sf r%" PRIu64 " (%s) at cfa%+"
 		    PRId64 "\n",
-		    op1, regname (op1), sop2 * data_align);
+		    op1, regname (ebl, op1, regnamebuf), sop2 * data_align);
 	    break;
 	  case DW_CFA_def_cfa_sf:
 	    if ((uint64_t) (endp - readp) < 1)
@@ -6386,7 +6391,7 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	      goto invalid;
 	    get_sleb128 (sop2, readp, endp);
 	    printf ("     def_cfa_sf r%" PRIu64 " (%s) at offset %" PRId64 "\n",
-		    op1, regname (op1), sop2 * data_align);
+		    op1, regname (ebl, op1, regnamebuf), sop2 * data_align);
 	    break;
 	  case DW_CFA_def_cfa_offset_sf:
 	    if ((uint64_t) (endp - readp) < 1)
@@ -6422,7 +6427,7 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	      goto invalid;
 	    get_uleb128 (op2, readp, endp);	/* Length of DW_FORM_block.  */
 	    printf ("     val_expression r%" PRIu64 " (%s)\n",
-		    op1, regname (op1));
+		    op1, regname (ebl, op1, regnamebuf));
 	    if ((uint64_t) (endp - readp) < op2)
 	      goto invalid;
 	    print_ops (dwflmod, dbg, 10, 10, version, ptr_size, 0,
@@ -6462,11 +6467,12 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
 	    goto invalid;
 	  get_uleb128 (offset, readp, endp);
 	  printf ("     offset r%u (%s) at cfa%+" PRId64 "\n",
-		  opcode & 0x3f, regname (opcode & 0x3f), offset * data_align);
+		  opcode & 0x3f, regname (ebl, opcode & 0x3f, regnamebuf),
+		  offset * data_align);
 	}
       else
 	printf ("     restore r%u (%s)\n",
-		opcode & 0x3f, regname (opcode & 0x3f));
+		opcode & 0x3f, regname (ebl, opcode & 0x3f, regnamebuf));
     }
 }
 
-- 
2.26.2

Re: [PATCH 4/5] readelf: Pull regname() into file scope

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:16:32AM +0100, Timm Bäder via Elfutils-devel wrote:
> Get rid of a nested function this way.

Right. Adding two arguments allows it as file scope function.
Added a ChangeLog entry and pushed.

Cheers,

Mark

[PATCH 5/5] readelf: Pull advance_pc() into file scope

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Get rid of a nested function this way

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/readelf.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/readelf.c b/src/readelf.c
index e0163891..9758d338 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -8356,6 +8356,23 @@ print_form_data (Dwarf *dbg, int form, const unsigned char *readp,
   return readp;
 }
 
+static inline void
+advance_pc (unsigned int op_advance,
+	    uint_fast8_t max_ops_per_instr,
+	    uint_fast8_t minimum_instr_len,
+	    Dwarf_Word *address,
+	    unsigned int *op_addr_advance,
+	    unsigned int *op_index,
+	    bool *show_op_index)
+{
+  *op_addr_advance = minimum_instr_len * ((*op_index + op_advance)
+					 / max_ops_per_instr);
+  (*address) += *op_addr_advance;
+  *show_op_index = (*op_index > 0 ||
+		   (*op_index + op_advance) % max_ops_per_instr > 0);
+  *op_index = (*op_index + op_advance) % max_ops_per_instr;
+}
+
 static void
 print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr,
 			  Elf_Scn *scn, GElf_Shdr *shdr, Dwarf *dbg)
@@ -8747,15 +8764,6 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr,
 	 or DW_LNS_advance_pc (as per DWARF4 6.2.5.1).  */
       unsigned int op_addr_advance;
       bool show_op_index;
-      inline void advance_pc (unsigned int op_advance)
-      {
-	op_addr_advance = minimum_instr_len * ((op_index + op_advance)
-					       / max_ops_per_instr);
-	address += op_addr_advance;
-	show_op_index = (op_index > 0 ||
-			 (op_index + op_advance) % max_ops_per_instr > 0);
-	op_index = (op_index + op_advance) % max_ops_per_instr;
-      }
 
       if (max_ops_per_instr == 0)
 	{
@@ -8792,7 +8800,9 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr,
 
 	      /* Perform the increments.  */
 	      line += line_increment;
-	      advance_pc ((opcode - opcode_base) / line_range);
+	      advance_pc ((opcode - opcode_base) / line_range,
+			  max_ops_per_instr, minimum_instr_len, &address,
+			  &op_addr_advance, &op_index, &show_op_index);
 
 	      printf (_(" special opcode %u: address+%u = "),
 		      opcode, op_addr_advance);
@@ -8910,7 +8920,9 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr,
 		  if (lineendp - linep < 1)
 		    goto invalid_unit;
 		  get_uleb128 (u128, linep, lineendp);
-		  advance_pc (u128);
+		  advance_pc (u128, max_ops_per_instr, minimum_instr_len,
+			      &address, &op_addr_advance, &op_index,
+			      &show_op_index);
 		  {
 		    printf (_(" advance address by %u to "),
 			    op_addr_advance);
@@ -8971,7 +8983,10 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr,
 		  if (unlikely (line_range == 0))
 		    goto invalid_unit;
 
-		  advance_pc ((255 - opcode_base) / line_range);
+		  advance_pc ((255 - opcode_base) / line_range,
+			      max_ops_per_instr, minimum_instr_len,
+			      &address, &op_addr_advance, &op_index,
+			      &show_op_index);
 		  {
 		    printf (_(" advance address by constant %u to "),
 			    op_addr_advance);
-- 
2.26.2

Re: Remove nested functions from readelf.c

[Mark Wielaard]

On Fri, 2021-01-08 at 09:16 +0100, Timm Bäder via Elfutils-devel wrote:
> here another round for src/readelf.c. I think they are simple, but I'm
> not happy with the advance_pc() commit. I'm open for suggestions here
> but I can't come up with something better right now. I'll keep looking
> in to that in the meantime.

Yeah, the advance_pc function is really why you want nested functions
in the first place IMHO. Passing around the captured state as 6 extra
arguments seems to not really make the code much clearer. Especially
because on its own it isn't immediately clear what the code is about or
that it is to be used as part of the special opcode or
DW_LNS_advance_pc opcode (where DW_LNS_const_add_pc acts like special
opcode 255) handling.

It might be helpful to spell out in a comment to the function which
part of the DWARF4 6.2.5.1 Special Opcodes handling the advance_pc
function is responsible for.

But honestly in this case I think just keeping the nested function is
the cleanest way to handle this.

Cheers,

Mark

Re: Remove nested functions from readelf.c

[Timm Bäder]

On 01/02/2021 15:21, Mark Wielaard wrote:
> On Fri, 2021-01-08 at 09:16 +0100, Timm Bäder via Elfutils-devel wrote:
>> here another round for src/readelf.c. I think they are simple, but I'm
>> not happy with the advance_pc() commit. I'm open for suggestions here
>> but I can't come up with something better right now. I'll keep looking
>> in to that in the meantime.
> 
> Yeah, the advance_pc function is really why you want nested functions
> in the first place IMHO. Passing around the captured state as 6 extra
> arguments seems to not really make the code much clearer. Especially
> because on its own it isn't immediately clear what the code is about or
> that it is to be used as part of the special opcode or
> DW_LNS_advance_pc opcode (where DW_LNS_const_add_pc acts like special
> opcode 255) handling.
> 
> It might be helpful to spell out in a comment to the function which
> part of the DWARF4 6.2.5.1 Special Opcodes handling the advance_pc
> function is responsible for.
> 
> But honestly in this case I think just keeping the nested function is
> the cleanest way to handle this.
> 
> Cheers,
> 
> Mark
> 

That's what I was worried about as well of course. But I think I can
work on this a bit and solve it in a cleaner way, with some additional
refactorings. I'll try to come up with some patches.


- Timm


-- 
Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Michael O'Neill, Tom Savage, Eric 
Shander