[PATCH 0/4] Minor cleanups in coff-pe-read.c

[PATCH 0/4] Minor cleanups in coff-pe-read.c

[Tom Tromey]

I looked a little into having coff-pe-read.c reuse minimal symbols
without re-reading.  This turned out to be difficult because this code
is only called from coffread.c, which does more complicated work
first, so I didn't do this.

However, while looking at this code I found a few things that could be
tidied.  This series is the result.  I tested this using AdaCore's
internal test suite on Windows.

Tom

[PATCH 1/4] Simplify BFD section iteration in coff-pe-read.c

[Tom Tromey]

coff-pe-read.c iterates over BFD sections using bfd_map_over_sections,
but it's much simpler to use a for-each loop.  This allows for the
removal of helper functions and types.
---
 gdb/coff-pe-read.c | 55 +++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 42 deletions(-)

diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
index 3ae2a7ffb13..32d0a9ef649 100644
--- a/gdb/coff-pe-read.c
+++ b/gdb/coff-pe-read.c
@@ -100,45 +100,14 @@ read_pe_section_index (const char *section_name)
 
 static int
 get_pe_section_index (const char *section_name,
-		      struct read_pe_section_data *sections,
-		      int nb_sections)
+		      const std::vector<read_pe_section_data> &sections)
 {
-  int i;
-
-  for (i = 0; i < nb_sections; i++)
+  for (int i = 0; i < sections.size (); i++)
     if (sections[i].section_name == section_name)
       return i;
   return PE_SECTION_INDEX_INVALID;
 }
 
-/* Structure used by get_section_vmas function below
-   to access section_data array and the size of the array
-   stored in nb_sections field.  */
-struct pe_sections_info
-{
-  int nb_sections;
-  struct read_pe_section_data *sections;
-};
-
-/* Record the virtual memory address of a section.  */
-
-static void
-get_section_vmas (bfd *abfd, asection *sectp, void *context)
-{
-  struct pe_sections_info *data = (struct pe_sections_info *) context;
-  struct read_pe_section_data *sections = data->sections;
-  int sectix = get_pe_section_index (sectp->name, sections,
-				     data->nb_sections);
-
-  if (sectix != PE_SECTION_INDEX_INVALID)
-    {
-      /* Data within the section start at rva_start in the pe and at
-	 bfd_get_section_vma() within memory.  Store the offset.  */
-
-      sections[sectix].vma_offset
-	= bfd_section_vma (sectp) - sections[sectix].rva_start;
-    }
-}
 \f
 /* Create a minimal symbol entry for an exported symbol.
    SYM_NAME contains the exported name or NULL if exported by ordinal,
@@ -347,11 +316,6 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
   int is_pe64 = 0;
   int is_pe32 = 0;
 
-  /* Array elements are for text, data and bss in that order
-     Initialization with RVA_START > RVA_END guarantees that
-     unused sections won't be matched.  */
-  struct pe_sections_info pe_sections_info;
-
   char const *target = bfd_get_target (objfile->obfd);
 
   std::vector<struct read_pe_section_data> section_data
@@ -522,10 +486,17 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
   /* Use internal dll name instead of full pathname.  */
   dll_name = (char *) (pe_as32 (expdata + 12) + erva);
 
-  pe_sections_info.nb_sections = otherix;
-  pe_sections_info.sections = section_data.data ();
-
-  bfd_map_over_sections (dll, get_section_vmas, &pe_sections_info);
+  for (asection *sectp : gdb_bfd_sections (dll))
+    {
+      int sectix = get_pe_section_index (sectp->name, section_data);
+      if (sectix != PE_SECTION_INDEX_INVALID)
+	{
+	  /* Data within the section start at rva_start in the pe and at
+	     bfd_get_section_vma() within memory.  Store the offset.  */
+	  section_data[sectix].vma_offset
+	    = bfd_section_vma (sectp) - section_data[sectix].rva_start;
+	}
+    }
 
   /* Truncate name at first dot. Should maybe also convert to all
      lower case for convenience on Windows.  */
-- 
2.34.1

[PATCH 2/4] Remove a const-removing cast from coff-pe-read.c

[Tom Tromey]

coff-pe-read.c casts away const at one spot, but this is easily
replaced by calling bfd_get_filename directly in a couple of debugging
prints.
---
 gdb/coff-pe-read.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
index 32d0a9ef649..ae4ca5435b6 100644
--- a/gdb/coff-pe-read.c
+++ b/gdb/coff-pe-read.c
@@ -311,7 +311,6 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
   unsigned long exp_funcbase;
   unsigned char *expdata, *erva;
   unsigned long name_rvas, ordinals, nexp, ordbase;
-  char *dll_name = (char *) bfd_get_filename (dll);
   int otherix = PE_SECTION_TABLE_SIZE;
   int is_pe64 = 0;
   int is_pe32 = 0;
@@ -394,12 +393,12 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
 	      if (debug_coff_pe_read)
 		gdb_printf (gdb_stdlog, _("Export RVA for dll "
 					  "\"%s\" is in section \"%s\"\n"),
-			    dll_name, sname);
+			    bfd_get_filename (dll), sname);
 	    }
 	  else if (export_opthdrrva != vaddr && debug_coff_pe_read)
 	    gdb_printf (gdb_stdlog, _("Wrong value of export RVA"
 				      " for dll \"%s\": 0x%lx instead of 0x%lx\n"),
-			dll_name, export_opthdrrva, vaddr);
+			bfd_get_filename (dll), export_opthdrrva, vaddr);
 	  expptr = fptr + (export_opthdrrva - vaddr);
 	  break;
 	}
@@ -484,7 +483,7 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
   exp_funcbase = pe_as32 (expdata + 28);
 
   /* Use internal dll name instead of full pathname.  */
-  dll_name = (char *) (pe_as32 (expdata + 12) + erva);
+  char *dll_name = (char *) (pe_as32 (expdata + 12) + erva);
 
   for (asection *sectp : gdb_bfd_sections (dll))
     {
-- 
2.34.1

[PATCH 3/4] Use std::string in coff-pe-read.c

[Tom Tromey]

coff-pe-read.c uses xsnprintf and alloca, but using std::string is
better, and just as easy.  In general I think alloca is something to
be avoided, and unbounded uses especially so.
---
 gdb/coff-pe-read.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
index ae4ca5435b6..5bf0e2dc7f0 100644
--- a/gdb/coff-pe-read.c
+++ b/gdb/coff-pe-read.c
@@ -178,16 +178,13 @@ add_pe_forwarded_sym (minimal_symbol_reader &reader,
   struct bound_minimal_symbol msymbol;
   enum minimal_symbol_type msymtype;
   int forward_dll_name_len = strlen (forward_dll_name);
-  int forward_func_name_len = strlen (forward_func_name);
-  int forward_len = forward_dll_name_len + forward_func_name_len + 2;
-  char *forward_qualified_name = (char *) alloca (forward_len);
   short section;
 
-  xsnprintf (forward_qualified_name, forward_len, "%s!%s", forward_dll_name,
-	     forward_func_name);
+  std::string forward_qualified_name = string_printf ("%s!%s",
+						      forward_dll_name,
+						      forward_func_name);
 
-
-  msymbol = lookup_bound_minimal_symbol (forward_qualified_name);
+  msymbol = lookup_bound_minimal_symbol (forward_qualified_name.c_str ());
 
   if (!msymbol.minsym)
     {
@@ -195,7 +192,7 @@ add_pe_forwarded_sym (minimal_symbol_reader &reader,
 
       for (i = 0; i < forward_dll_name_len; i++)
 	forward_qualified_name[i] = tolower (forward_qualified_name[i]);
-      msymbol = lookup_bound_minimal_symbol (forward_qualified_name);
+      msymbol = lookup_bound_minimal_symbol (forward_qualified_name.c_str ());
     }
 
   if (!msymbol.minsym)
-- 
2.34.1

[PATCH 4/4] More const use and alloca avoidance in coff-pe-read.c

[Tom Tromey]

This changes another function in coff-pe-read.c to use 'const' more,
and to avoid the use of alloca by instead using std::string.
---
 gdb/coff-pe-read.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
index 5bf0e2dc7f0..d533a96ef6d 100644
--- a/gdb/coff-pe-read.c
+++ b/gdb/coff-pe-read.c
@@ -525,20 +525,20 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
       /* First handle forward cases.  */
       if (func_rva >= export_rva && func_rva < export_rva + export_size)
 	{
-	  char *forward_name = (char *) (erva + func_rva);
-	  char *funcname = (char *) (erva + name_rva);
-	  char *forward_dll_name = forward_name;
-	  char *forward_func_name = forward_name;
-	  char *sep = strrchr (forward_name, '.');
-
-	  if (sep)
+	  const char *forward_name = (const char *) (erva + func_rva);
+	  const char *funcname = (const char *) (erva + name_rva);
+	  const char *forward_dll_name = forward_name;
+	  const char *forward_func_name = forward_name;
+	  const char *sep = strrchr (forward_name, '.');
+
+	  std::string name_storage;
+	  if (sep != nullptr)
 	    {
 	      int len = (int) (sep - forward_name);
 
-	      forward_dll_name = (char *) alloca (len + 1);
-	      strncpy (forward_dll_name, forward_name, len);
-	      forward_dll_name[len] = '\0';
-	      forward_func_name = ++sep;
+	      name_storage = std::string (forward_name, len);
+	      forward_dll_name = name_storage.c_str ();
+	      forward_func_name = sep + 1;
 	    }
 	  if (add_pe_forwarded_sym (reader, funcname, forward_dll_name,
 				    forward_func_name, ordinal,
@@ -552,7 +552,7 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
 	  if ((func_rva >= section_data[sectix].rva_start)
 	      && (func_rva < section_data[sectix].rva_end))
 	    {
-	      char *sym_name = (char *) (erva + name_rva);
+	      const char *sym_name = (const char *) (erva + name_rva);
 
 	      section_found = 1;
 	      add_pe_exported_sym (reader, sym_name, func_rva, ordinal,
@@ -563,7 +563,7 @@ read_pe_exported_syms (minimal_symbol_reader &reader,
 	}
       if (!section_found)
 	{
-	  char *funcname = (char *) (erva + name_rva);
+	  const char *funcname = (const char *) (erva + name_rva);
 
 	  if (name_rva == 0)
 	    {
-- 
2.34.1

Re: [PATCH 0/4] Minor cleanups in coff-pe-read.c

[Pedro Alves]

On 2022-04-22 14:56, Tom Tromey via Gdb-patches wrote:
> I looked a little into having coff-pe-read.c reuse minimal symbols
> without re-reading.  This turned out to be difficult because this code
> is only called from coffread.c, which does more complicated work
> first, so I didn't do this.
> 
> However, while looking at this code I found a few things that could be
> tidied.  This series is the result.  I tested this using AdaCore's
> internal test suite on Windows.

The series looks fine to me, but now I'm curious on what you mean by "reuse
minimal symbols without re-reading".

Re: [PATCH 0/4] Minor cleanups in coff-pe-read.c

[Tom Tromey]

Pedro> The series looks fine to me, but now I'm curious on what you mean
Pedro> by "reuse minimal symbols without re-reading".

Minimal symbols are normally stored on the per-BFD object.  So, a savvy
symbol reader can simply reuse them without doing any additional work.
For less savvy symbol readers, the minimal_symbol_reader object itself
knows how to do this -- it simply discards symbols when they are being
re-read.  Only elfread.c was ever converted to do this sharing the best
way:

  /* If we already have minsyms, then we can skip some work here.
     However, if there were stabs or mdebug sections, we go ahead and
     redo all the work anyway, because the psym readers for those
     kinds of debuginfo need extra information found here.  This can
     go away once all types of symbols are in the per-BFD object.  */
  if (objfile->per_bfd->minsyms_read
      && ei->stabsect == NULL
      && ei->mdebugsect == NULL
      && ei->ctfsect == NULL)
    {
      if (symtab_create_debug)
	gdb_printf (gdb_stdlog,
		    "... minimal symbols previously read\n");
      return;
    }

The problem for other readers is that they tend not to be "pure", in the
sense that they mix minimal symbol reading with other kinds of
reading -- and the other kinds may not be shared or even shareable.

That's why the elfread.c change has so many conditions.  For coffread.c,
coff_symfile_read may create full symbols, and it wasn't immediately
obvious to me that this could easily be skipped for PE.

Making everything be shared would be great.  Then we could hoist this
check into generic code and re-using a BFD would be simple.  However,
this is the objfile splitting project, which I've attempted and failed
at several times now.  (It's still a good project but it needs a
strategy other than one giant patch series.)


Now that I look at this again, though, maybe it's easier than I thought.

  if ((nsyms == 0) && (pe_file))
    {
      /* We've got no debugging symbols, but it's a portable
	 executable, so try to read the export table.  */
      read_pe_exported_syms (reader, objfile);
    }

Maybe this can be checked earlier.  I'll look at it more.  I had thought
that coff_symtab_read installing minsyms would be a problem, but that
seems to happen in a loop that's checking nsyms.


The symbol readers are all headache-inducing.  For example just glancing
at coffread.c makes me think there are memory leaks:

		sym->type ()->set_name (xstrdup (sym->linkage_name ()));

... type names must be allocated on an obstack.  Here there's probably
just no need to copy at all.  No idea how to test this though.

They also tend to be a mess of global variables and other bad practices.

Tom