[RFA] Change read_alphacoff_dynamic_symtab to use gdb::def_vector

Message ID 20180401162622.15065-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Change read_alphacoff_dynamic_symtab to use gdb::def_vector
Related show

Commit Message

Tom Tromey April 1, 2018, 4:26 p.m.
This changes read_alphacoff_dynamic_symtab to use gdb::def_vector.
This allows for the removal of some cleanups.

Tested by the buildbot; though I don't know whether this code path is
ever actually run.

gdb/ChangeLog
2018-03-31  Tom Tromey  <tom@tromey.com>

	* mipsread.c (read_alphacoff_dynamic_symtab): Use
	gdb::def_vector.
---
 gdb/ChangeLog  |  5 +++++
 gdb/mipsread.c | 64 ++++++++++++++++++----------------------------------------
 2 files changed, 25 insertions(+), 44 deletions(-)

-- 
2.13.6

Comments

Simon Marchi April 1, 2018, 6:39 p.m. | #1
> @@ -214,43 +209,28 @@ read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,

>    str_secsize = bfd_get_section_size (si.str_sect);

>    dyninfo_secsize = bfd_get_section_size (si.dyninfo_sect);

>    got_secsize = bfd_get_section_size (si.got_sect);

> -  sym_secptr = (char *) xmalloc (sym_secsize);

> -  cleanups = make_cleanup (xfree, sym_secptr);

> -  str_secptr = (char *) xmalloc (str_secsize);

> -  make_cleanup (xfree, str_secptr);

> -  dyninfo_secptr = (char *) xmalloc (dyninfo_secsize);

> -  make_cleanup (xfree, dyninfo_secptr);

> -  got_secptr = (char *) xmalloc (got_secsize);

> -  make_cleanup (xfree, got_secptr);

> -

> -  if (!bfd_get_section_contents (abfd, si.sym_sect, sym_secptr,

> +  gdb::def_vector<char> sym_secptr (sym_secsize);

> +  gdb::def_vector<char> str_secptr (str_secsize);

> +  gdb::def_vector<char> dyninfo_secptr (dyninfo_secsize);

> +  gdb::def_vector<char> got_secptr (got_secsize);


I am pretty sure that you could use gdb::byte_vector here and it would be fine.

Could you (if it's not too much trouble) remove the size variables (sym_secsize and
friends)?  This way, there's only one source of truth for the allocated size (the
vector size).  Also, the "ptr" suffix probably made sense when the variables were
pointers.  It would make sense to rename them, sym_secptr -> sym_sec, and so forth.

Simon
Tom Tromey April 3, 2018, 5:29 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> I am pretty sure that you could use gdb::byte_vector here and it
Simon> would be fine.

It needed one extra cast, but yeah.

Simon> Could you (if it's not too much trouble) remove the size variables (sym_secsize and
Simon> friends)?  This way, there's only one source of truth for the allocated size (the
Simon> vector size).  Also, the "ptr" suffix probably made sense when the variables were
Simon> pointers.  It would make sense to rename them, sym_secptr -> sym_sec, and so forth.

How's this?

Tom

commit 5f17c7b0f85ebb78d38c75884fbf45ae4b217546
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Nov 10 19:52:33 2017 -0700

    Change read_alphacoff_dynamic_symtab to use gdb::byte_vector
    
    This changes read_alphacoff_dynamic_symtab to use gdb::byte_vector.
    This allows for the removal of some cleanups.
    
    Tested by the buildbot; though I don't know whether this code path is
    ever actually run.
    
    gdb/ChangeLog
    2018-04-03  Tom Tromey  <tom@tromey.com>
    
            * mipsread.c (read_alphacoff_dynamic_symtab): Use
            gdb::byte_vector.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7e220ef895..5b5bed5720 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-03  Tom Tromey  <tom@tromey.com>
+
+	* mipsread.c (read_alphacoff_dynamic_symtab): Use
+	gdb::byte_vector.
+
 2018-04-01  Tom Tromey  <tom@tromey.com>
 
 	* rs6000-nat.c (rs6000_ptrace_ldinfo): Return a byte_vector.
diff --git a/gdb/mipsread.c b/gdb/mipsread.c
index af339e2e1f..7b6ec2e48d 100644
--- a/gdb/mipsread.c
+++ b/gdb/mipsread.c
@@ -180,24 +180,15 @@ read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 {
   bfd *abfd = objfile->obfd;
   struct alphacoff_dynsecinfo si;
-  char *sym_secptr;
-  char *str_secptr;
-  char *dyninfo_secptr;
-  char *got_secptr;
-  bfd_size_type sym_secsize;
-  bfd_size_type str_secsize;
-  bfd_size_type dyninfo_secsize;
-  bfd_size_type got_secsize;
   int sym_count;
   int i;
   int stripped;
   Elfalpha_External_Sym *x_symp;
-  char *dyninfo_p;
-  char *dyninfo_end;
+  gdb_byte *dyninfo_p;
+  gdb_byte *dyninfo_end;
   int got_entry_size = 8;
   int dt_mips_local_gotno = -1;
   int dt_mips_gotsym = -1;
-  struct cleanup *cleanups;
 
   /* We currently only know how to handle alpha dynamic symbols.  */
   if (bfd_get_arch (abfd) != bfd_arch_alpha)
@@ -210,47 +201,28 @@ read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
       || si.dyninfo_sect == NULL || si.got_sect == NULL)
     return;
 
-  sym_secsize = bfd_get_section_size (si.sym_sect);
-  str_secsize = bfd_get_section_size (si.str_sect);
-  dyninfo_secsize = bfd_get_section_size (si.dyninfo_sect);
-  got_secsize = bfd_get_section_size (si.got_sect);
-  sym_secptr = (char *) xmalloc (sym_secsize);
-  cleanups = make_cleanup (xfree, sym_secptr);
-  str_secptr = (char *) xmalloc (str_secsize);
-  make_cleanup (xfree, str_secptr);
-  dyninfo_secptr = (char *) xmalloc (dyninfo_secsize);
-  make_cleanup (xfree, dyninfo_secptr);
-  got_secptr = (char *) xmalloc (got_secsize);
-  make_cleanup (xfree, got_secptr);
-
-  if (!bfd_get_section_contents (abfd, si.sym_sect, sym_secptr,
-				 (file_ptr) 0, sym_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
-  if (!bfd_get_section_contents (abfd, si.str_sect, str_secptr,
-				 (file_ptr) 0, str_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
-  if (!bfd_get_section_contents (abfd, si.dyninfo_sect, dyninfo_secptr,
-				 (file_ptr) 0, dyninfo_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
-  if (!bfd_get_section_contents (abfd, si.got_sect, got_secptr,
-				 (file_ptr) 0, got_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
+  gdb::byte_vector sym_sec (bfd_get_section_size (si.sym_sect));
+  gdb::byte_vector str_sec (bfd_get_section_size (si.str_sect));
+  gdb::byte_vector dyninfo_sec (bfd_get_section_size (si.dyninfo_sect));
+  gdb::byte_vector got_sec (bfd_get_section_size (si.got_sect));
+
+  if (!bfd_get_section_contents (abfd, si.sym_sect, sym_sec.data (),
+				 (file_ptr) 0, sym_sec.size ()))
+    return;
+  if (!bfd_get_section_contents (abfd, si.str_sect, str_sec.data (),
+				 (file_ptr) 0, str_sec.size ()))
+    return;
+  if (!bfd_get_section_contents (abfd, si.dyninfo_sect, dyninfo_sec.data (),
+				 (file_ptr) 0, dyninfo_sec.size ()))
+    return;
+  if (!bfd_get_section_contents (abfd, si.got_sect, got_sec.data (),
+				 (file_ptr) 0, got_sec.size ()))
+    return;
 
   /* Find the number of local GOT entries and the index for the
      first dynamic symbol in the GOT.  */
-  for (dyninfo_p = dyninfo_secptr, dyninfo_end = dyninfo_p + dyninfo_secsize;
+  for ((dyninfo_p = dyninfo_sec.data (),
+	dyninfo_end = dyninfo_p + dyninfo_sec.size ());
        dyninfo_p < dyninfo_end;
        dyninfo_p += sizeof (Elfalpha_External_Dyn))
     {
@@ -274,18 +246,15 @@ read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 	}
     }
   if (dt_mips_local_gotno < 0 || dt_mips_gotsym < 0)
-    {
-      do_cleanups (cleanups);
-      return;
-    }
+    return;
 
   /* Scan all dynamic symbols and enter them into the minimal symbol
      table if appropriate.  */
-  sym_count = sym_secsize / sizeof (Elfalpha_External_Sym);
+  sym_count = sym_sec.size () / sizeof (Elfalpha_External_Sym);
   stripped = (bfd_get_symcount (abfd) == 0);
 
   /* Skip first symbol, which is a null dummy.  */
-  for (i = 1, x_symp = (Elfalpha_External_Sym *) sym_secptr + 1;
+  for (i = 1, x_symp = (Elfalpha_External_Sym *) sym_sec.data () + 1;
        i < sym_count;
        i++, x_symp++)
     {
@@ -298,9 +267,9 @@ read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
       enum minimal_symbol_type ms_type;
 
       strx = bfd_h_get_32 (abfd, (bfd_byte *) x_symp->st_name);
-      if (strx >= str_secsize)
+      if (strx >= str_sec.size ())
 	continue;
-      name = str_secptr + strx;
+      name = (char *) (str_sec.data () + strx);
       if (*name == '\0' || *name == '.')
 	continue;
 
@@ -341,11 +310,13 @@ read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 	      int got_entry_offset =
 		(i - dt_mips_gotsym + dt_mips_local_gotno) * got_entry_size;
 
-	      if (got_entry_offset < 0 || got_entry_offset >= got_secsize)
+	      if (got_entry_offset < 0
+		  || got_entry_offset >= got_sec.size ())
 		continue;
 	      sym_value =
 		bfd_h_get_64 (abfd,
-			      (bfd_byte *) (got_secptr + got_entry_offset));
+			      (bfd_byte *) (got_sec.data ()
+					    + got_entry_offset));
 	      if (sym_value == 0)
 		continue;
 	    }
@@ -392,8 +363,6 @@ read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 
       reader.record (name, sym_value, ms_type);
     }
-
-  do_cleanups (cleanups);
 }
 
 /* Initialization.  */
Simon Marchi April 3, 2018, 5:38 p.m. | #3
On 2018-04-03 13:29, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> 

> Simon> I am pretty sure that you could use gdb::byte_vector here and it

> Simon> would be fine.

> 

> It needed one extra cast, but yeah.

> 

> Simon> Could you (if it's not too much trouble) remove the size

> variables (sym_secsize and

> Simon> friends)?  This way, there's only one source of truth for the

> allocated size (the

> Simon> vector size).  Also, the "ptr" suffix probably made sense when

> the variables were

> Simon> pointers.  It would make sense to rename them, sym_secptr ->

> sym_sec, and so forth.

> 

> How's this?

> 

> Tom


LGTM, thanks!

Simon

Patch

diff --git a/gdb/mipsread.c b/gdb/mipsread.c
index af339e2e1f..046a25bd50 100644
--- a/gdb/mipsread.c
+++ b/gdb/mipsread.c
@@ -180,10 +180,6 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 {
   bfd *abfd = objfile->obfd;
   struct alphacoff_dynsecinfo si;
-  char *sym_secptr;
-  char *str_secptr;
-  char *dyninfo_secptr;
-  char *got_secptr;
   bfd_size_type sym_secsize;
   bfd_size_type str_secsize;
   bfd_size_type dyninfo_secsize;
@@ -197,7 +193,6 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
   int got_entry_size = 8;
   int dt_mips_local_gotno = -1;
   int dt_mips_gotsym = -1;
-  struct cleanup *cleanups;
 
   /* We currently only know how to handle alpha dynamic symbols.  */
   if (bfd_get_arch (abfd) != bfd_arch_alpha)
@@ -214,43 +209,28 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
   str_secsize = bfd_get_section_size (si.str_sect);
   dyninfo_secsize = bfd_get_section_size (si.dyninfo_sect);
   got_secsize = bfd_get_section_size (si.got_sect);
-  sym_secptr = (char *) xmalloc (sym_secsize);
-  cleanups = make_cleanup (xfree, sym_secptr);
-  str_secptr = (char *) xmalloc (str_secsize);
-  make_cleanup (xfree, str_secptr);
-  dyninfo_secptr = (char *) xmalloc (dyninfo_secsize);
-  make_cleanup (xfree, dyninfo_secptr);
-  got_secptr = (char *) xmalloc (got_secsize);
-  make_cleanup (xfree, got_secptr);
-
-  if (!bfd_get_section_contents (abfd, si.sym_sect, sym_secptr,
+  gdb::def_vector<char> sym_secptr (sym_secsize);
+  gdb::def_vector<char> str_secptr (str_secsize);
+  gdb::def_vector<char> dyninfo_secptr (dyninfo_secsize);
+  gdb::def_vector<char> got_secptr (got_secsize);
+
+  if (!bfd_get_section_contents (abfd, si.sym_sect, sym_secptr.data (),
 				 (file_ptr) 0, sym_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
-  if (!bfd_get_section_contents (abfd, si.str_sect, str_secptr,
+    return;
+  if (!bfd_get_section_contents (abfd, si.str_sect, str_secptr.data (),
 				 (file_ptr) 0, str_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
-  if (!bfd_get_section_contents (abfd, si.dyninfo_sect, dyninfo_secptr,
+    return;
+  if (!bfd_get_section_contents (abfd, si.dyninfo_sect, dyninfo_secptr.data (),
 				 (file_ptr) 0, dyninfo_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
-  if (!bfd_get_section_contents (abfd, si.got_sect, got_secptr,
+    return;
+  if (!bfd_get_section_contents (abfd, si.got_sect, got_secptr.data (),
 				 (file_ptr) 0, got_secsize))
-    {
-      do_cleanups (cleanups);
-      return;
-    }
+    return;
 
   /* Find the number of local GOT entries and the index for the
      first dynamic symbol in the GOT.  */
-  for (dyninfo_p = dyninfo_secptr, dyninfo_end = dyninfo_p + dyninfo_secsize;
+  for ((dyninfo_p = dyninfo_secptr.data (),
+	dyninfo_end = dyninfo_p + dyninfo_secsize);
        dyninfo_p < dyninfo_end;
        dyninfo_p += sizeof (Elfalpha_External_Dyn))
     {
@@ -274,10 +254,7 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 	}
     }
   if (dt_mips_local_gotno < 0 || dt_mips_gotsym < 0)
-    {
-      do_cleanups (cleanups);
-      return;
-    }
+    return;
 
   /* Scan all dynamic symbols and enter them into the minimal symbol
      table if appropriate.  */
@@ -285,7 +262,7 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
   stripped = (bfd_get_symcount (abfd) == 0);
 
   /* Skip first symbol, which is a null dummy.  */
-  for (i = 1, x_symp = (Elfalpha_External_Sym *) sym_secptr + 1;
+  for (i = 1, x_symp = (Elfalpha_External_Sym *) sym_secptr.data () + 1;
        i < sym_count;
        i++, x_symp++)
     {
@@ -300,7 +277,7 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
       strx = bfd_h_get_32 (abfd, (bfd_byte *) x_symp->st_name);
       if (strx >= str_secsize)
 	continue;
-      name = str_secptr + strx;
+      name = str_secptr.data () + strx;
       if (*name == '\0' || *name == '.')
 	continue;
 
@@ -345,7 +322,8 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 		continue;
 	      sym_value =
 		bfd_h_get_64 (abfd,
-			      (bfd_byte *) (got_secptr + got_entry_offset));
+			      (bfd_byte *) (got_secptr.data ()
+					    + got_entry_offset));
 	      if (sym_value == 0)
 		continue;
 	    }
@@ -392,8 +370,6 @@  read_alphacoff_dynamic_symtab (minimal_symbol_reader &reader,
 
       reader.record (name, sym_value, ms_type);
     }
-
-  do_cleanups (cleanups);
 }
 
 /* Initialization.  */