[PATCH 00/10] More coffread cleanups

[PATCH 00/10] More coffread cleanups

[Tom Tromey]

After Simon removed a huge chunk of coffread.c, I noticed there were
more cleanups that could be done.  This series is the result.

I'm not totally certain how to test this, but I did at least run the
resulting gdb on some Windows object files to make sure nothing
untoward happens.

Signed-off-by: Tom Tromey <tromey@adacore.com>
---
Tom Tromey (10):
      Constify coffread.c:getsymname
      Clean up a comment in coffread.c
      Fix indentation in coffread.c
      Change coffread.c:pe_file to bool
      Remove redundant nlist_bfd_global
      Use symfile_bfd in more places
      Use coffread_objfile throughout coffread.c
      Remove coff_symfile_init
      Remove all globals from coffread.c
      Change is_import_fixup_symbol to return bool

 gdb/coffread.c | 327 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 158 insertions(+), 169 deletions(-)
---
base-commit: 03375540de733c30feae892db8075eebef946383
change-id: 20260116-coffread-cleanups-4bf0b45c8b04

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 01/10] Constify coffread.c:getsymname

[Tom Tromey]

I noticed that the symbol name in coffread.c can be const.
---
 gdb/coffread.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index 18641aafd18..dc5550af4a2 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -71,7 +71,7 @@ static int pe_file;
 
 struct coff_symbol
   {
-    char *c_name;
+    const char *c_name;
     int c_naux;			/* 0 if syment only, 1 if syment +
 				   auxent, etc.  */
     CORE_ADDR c_value;
@@ -86,7 +86,7 @@ static long stringtab_length = 0;
 /* Used when reading coff symbols.  */
 static int symnum;
 
-static char *getsymname (struct internal_syment *);
+static const char *getsymname (struct internal_syment *);
 
 static int init_stringtab (bfd *, file_ptr, gdb::unique_xmalloc_ptr<char> *);
 
@@ -636,11 +636,11 @@ init_stringtab (bfd *abfd, file_ptr offset, gdb::unique_xmalloc_ptr<char> *stora
   return 0;
 }
 
-static char *
+static const char *
 getsymname (struct internal_syment *symbol_entry)
 {
   static char buffer[SYMNMLEN + 1];
-  char *result;
+  const char *result;
 
   if (symbol_entry->_n._n_n._n_zeroes == 0)
     {

-- 
2.52.0

[PATCH 02/10] Clean up a comment in coffread.c

[Tom Tromey]

This comment in coffread.c refers to some things that are no longer
used.
---
 gdb/coffread.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index dc5550af4a2..b0c4a073df3 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -46,8 +46,7 @@ static char *temp_aux;
 /* Local variables that hold the shift and mask values for the
    COFF file that we are currently reading.  These come back to us
    from BFD, and are referenced by their macro names, as well as
-   internally to the BTYPE, ISPTR, ISFCN, ISARY, ISTAG, and DECREF
-   macros from include/coff/internal.h .  */
+   internally to the ISFCN macro from include/coff/internal.h .  */
 
 static unsigned local_n_btshft;
 static unsigned local_n_tmask;

-- 
2.52.0

[PATCH 03/10] Fix indentation in coffread.c

[Tom Tromey]

A couple of spots in coffread.c were indented incorrectly.
---
 gdb/coffread.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index b0c4a073df3..eaaebd55326 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -69,15 +69,15 @@ static int pe_file;
 /* Simplified internal version of coff symbol table information.  */
 
 struct coff_symbol
-  {
-    const char *c_name;
-    int c_naux;			/* 0 if syment only, 1 if syment +
+{
+  const char *c_name;
+  int c_naux;			/* 0 if syment only, 1 if syment +
 				   auxent, etc.  */
-    CORE_ADDR c_value;
-    int c_sclass;
-    int c_secnum;
-    unsigned int c_type;
-  };
+  CORE_ADDR c_value;
+  int c_sclass;
+  int c_secnum;
+  unsigned int c_type;
+};
 
 static char *stringtab = NULL;
 static long stringtab_length = 0;
@@ -298,7 +298,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 
   symfile_bfd = abfd;		/* Kludge for swap routines.  */
 
-/* WARNING WILL ROBINSON!  ACCESSING BFD-PRIVATE DATA HERE!  FIXME!  */
+  /* WARNING WILL ROBINSON!  ACCESSING BFD-PRIVATE DATA HERE!  FIXME!  */
   num_symbols = bfd_get_symcount (abfd);	/* How many syms */
   symtab_offset = cdata->sym_filepos;	/* Symbol table file offset */
   stringtab_offset = symtab_offset +	/* String table file offset */

-- 
2.52.0

[PATCH 04/10] Change coffread.c:pe_file to bool

[Tom Tromey]

This changes coffread.c:pe_file to use bool.
---
 gdb/coffread.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index eaaebd55326..c8c9c6409ea 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -64,7 +64,7 @@ static unsigned local_auxesz;
 
 /* This is set if this is a PE format file.  */
 
-static int pe_file;
+static bool pe_file;
 
 /* Simplified internal version of coff symbol table information.  */
 
@@ -323,9 +323,8 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
      from the section address, rather than as absolute addresses.
      FIXME: We should use BFD to read the symbol table, and thus avoid
      this problem.  */
-  pe_file =
-    startswith (bfd_get_target (objfile->obfd.get ()), "pe")
-    || startswith (bfd_get_target (objfile->obfd.get ()), "epoc-pe");
+  pe_file = (startswith (bfd_get_target (objfile->obfd.get ()), "pe")
+	     || startswith (bfd_get_target (objfile->obfd.get ()), "epoc-pe"));
 
   /* End of warning.  */
 

-- 
2.52.0

[PATCH 05/10] Remove redundant nlist_bfd_global

[Tom Tromey]

coffread.c has two BFD globals: nlist_bfd_global and symfile_bfd.
There's no need for both, and since symfile_bfd is set early (in
coff_symfile_read, the entry point), this removes nlist_bfd_global and
replaces all the uses.
---
 gdb/coffread.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index c8c9c6409ea..b75721d2577 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -32,10 +32,10 @@
 
 static struct objfile *coffread_objfile;
 
-/* The addresses of the symbol table stream and number of symbols
-   of the object file we are reading (as copied into core).  */
+/* The BFD for this file -- only good while we're actively reading
+   symbols into a psymtab or a symtab.  */
 
-static bfd *nlist_bfd_global;
+static bfd *symfile_bfd;
 
 /* Pointers to scratch storage, used for reading raw symbols and
    auxents.  */
@@ -278,11 +278,6 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms,
     }
 }
 
-/* The BFD for this file -- only good while we're actively reading
-   symbols into a psymtab or a symtab.  */
-
-static bfd *symfile_bfd;
-
 /* Read a symbol file, after initialization by coff_symfile_init.  */
 
 static void
@@ -394,7 +389,6 @@ coff_symtab_read (minimal_symbol_reader &reader,
 	   objfile_name (objfile), bfd_errmsg (bfd_get_error ()));
 
   coffread_objfile = objfile;
-  nlist_bfd_global = objfile->obfd.get ();
 
   while (symnum < nsyms)
     {
@@ -538,7 +532,7 @@ read_one_sym (struct coff_symbol *cs)
   bfd_size_type bytes;
   internal_syment sym;
 
-  bytes = bfd_read (temp_sym, local_symesz, nlist_bfd_global);
+  bytes = bfd_read (temp_sym, local_symesz, symfile_bfd);
   if (bytes != local_symesz)
     error (_("%s: error reading symbols"), objfile_name (coffread_objfile));
   bfd_coff_swap_sym_in (symfile_bfd, temp_sym, &sym);
@@ -548,7 +542,7 @@ read_one_sym (struct coff_symbol *cs)
       /* We don't need aux entries, read past them.  */
       for (i = 0; i < cs->c_naux; i++)
 	{
-	  bytes = bfd_read (temp_aux, local_auxesz, nlist_bfd_global);
+	  bytes = bfd_read (temp_aux, local_auxesz, symfile_bfd);
 	  if (bytes != local_auxesz)
 	    error (_("%s: error reading symbols"),
 		   objfile_name (coffread_objfile));

-- 
2.52.0

[PATCH 06/10] Use symfile_bfd in more places

[Tom Tromey]

Since coffread.c is setting symfile_bfd, it might as well use it
everywhere.  This changes all other BFD references in coffread.c to
use the global.
---
 gdb/coffread.c | 50 +++++++++++++++++++++++---------------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index b75721d2577..5cbbf490937 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -87,7 +87,7 @@ static int symnum;
 
 static const char *getsymname (struct internal_syment *);
 
-static int init_stringtab (bfd *, file_ptr, gdb::unique_xmalloc_ptr<char> *);
+static int init_stringtab (file_ptr, gdb::unique_xmalloc_ptr<char> *);
 
 static void read_one_sym (struct coff_symbol *);
 
@@ -97,9 +97,9 @@ static void coff_symtab_read (minimal_symbol_reader &,
 /* Return the BFD section that CS points to.  */
 
 static asection *
-cs_to_bfd_section (struct coff_symbol *cs, bfd *abfd)
+cs_to_bfd_section (struct coff_symbol *cs)
 {
-  for (asection *sect : gdb_bfd_sections (abfd))
+  for (asection *sect : gdb_bfd_sections (symfile_bfd))
     if (sect->target_index == cs->c_secnum)
       return sect;
 
@@ -110,19 +110,19 @@ cs_to_bfd_section (struct coff_symbol *cs, bfd *abfd)
 static int
 cs_to_section (struct coff_symbol *cs, struct objfile *objfile)
 {
-  asection *sect = cs_to_bfd_section (cs, objfile->obfd.get ());
+  asection *sect = cs_to_bfd_section (cs);
 
   if (sect == NULL)
     return SECT_OFF_TEXT (objfile);
-  return gdb_bfd_section_index (objfile->obfd.get (), sect);
+  return gdb_bfd_section_index (symfile_bfd, sect);
 }
 
 /* Return the address of the section of a COFF symbol.  */
 
 static CORE_ADDR
-cs_section_address (struct coff_symbol *cs, bfd *abfd)
+cs_section_address (struct coff_symbol *cs)
 {
-  asection *sect = cs_to_bfd_section (cs, abfd);
+  asection *sect = cs_to_bfd_section (cs);
 
   if (sect == nullptr)
     return 0;
@@ -256,8 +256,7 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms,
 		name1 = name + 6;
 	      if (name1 != NULL)
 		{
-		  int lead
-		    = bfd_get_symbol_leading_char (objfile->obfd.get ());
+		  int lead = bfd_get_symbol_leading_char (symfile_bfd);
 
 		  if (lead != '\0' && *name1 == lead)
 		    name1 += 1;
@@ -283,18 +282,16 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms,
 static void
 coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 {
-  bfd *abfd = objfile->obfd.get ();
-  coff_data_type *cdata = coff_data (abfd);
-  const char *filename = bfd_get_filename (abfd);
+  symfile_bfd = objfile->obfd.get ();
+  coff_data_type *cdata = coff_data (symfile_bfd);
+  const char *filename = bfd_get_filename (symfile_bfd);
   int val;
   unsigned int num_symbols;
   file_ptr symtab_offset;
   file_ptr stringtab_offset;
 
-  symfile_bfd = abfd;		/* Kludge for swap routines.  */
-
   /* WARNING WILL ROBINSON!  ACCESSING BFD-PRIVATE DATA HERE!  FIXME!  */
-  num_symbols = bfd_get_symcount (abfd);	/* How many syms */
+  num_symbols = bfd_get_symcount (symfile_bfd); /* How many syms */
   symtab_offset = cdata->sym_filepos;	/* Symbol table file offset */
   stringtab_offset = symtab_offset +	/* String table file offset */
     num_symbols * cdata->local_symesz;
@@ -318,8 +315,8 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
      from the section address, rather than as absolute addresses.
      FIXME: We should use BFD to read the symbol table, and thus avoid
      this problem.  */
-  pe_file = (startswith (bfd_get_target (objfile->obfd.get ()), "pe")
-	     || startswith (bfd_get_target (objfile->obfd.get ()), "epoc-pe"));
+  pe_file = (startswith (bfd_get_target (symfile_bfd), "pe")
+	     || startswith (bfd_get_target (symfile_bfd), "epoc-pe"));
 
   /* End of warning.  */
 
@@ -327,7 +324,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 
   scoped_restore restore_stringtab = make_scoped_restore (&stringtab);
   gdb::unique_xmalloc_ptr<char> stringtab_storage;
-  val = init_stringtab (abfd, stringtab_offset, &stringtab_storage);
+  val = init_stringtab (stringtab_offset, &stringtab_storage);
   if (val < 0)
     error (_("\"%s\": can't get string table"), filename);
 
@@ -337,7 +334,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
     {
       bool found_stab_section = false;
 
-      for (asection *sect : gdb_bfd_sections (abfd))
+      for (asection *sect : gdb_bfd_sections (symfile_bfd))
 	if (startswith (bfd_section_name (sect), ".stab"))
 	  {
 	    found_stab_section = true;
@@ -383,7 +380,7 @@ coff_symtab_read (minimal_symbol_reader &reader,
   symnum = 0;
 
   /* Position to read the symbol table.  */
-  val = bfd_seek (objfile->obfd.get (), symtab_offset, 0);
+  val = bfd_seek (symfile_bfd, symtab_offset, 0);
   if (val < 0)
     error (_("Error reading symbols from %s: %s"),
 	   objfile_name (objfile), bfd_errmsg (bfd_get_error ()));
@@ -476,8 +473,7 @@ coff_symtab_read (minimal_symbol_reader &reader,
 	      }
 	    else
 	      {
-		asection *bfd_section
-		  = cs_to_bfd_section (cs, objfile->obfd.get ());
+		asection *bfd_section = cs_to_bfd_section (cs);
 
 		sec = cs_to_section (cs, objfile);
 		tmpaddr = cs->c_value;
@@ -579,7 +575,7 @@ read_one_sym (struct coff_symbol *cs)
 	case C_FCN:
 	case C_EFCN:
 	  if (cs->c_secnum != 0)
-	    cs->c_value += cs_section_address (cs, symfile_bfd);
+	    cs->c_value += cs_section_address (cs);
 	  break;
 	}
     }
@@ -588,7 +584,7 @@ read_one_sym (struct coff_symbol *cs)
 /* Support for string table handling.  */
 
 static int
-init_stringtab (bfd *abfd, file_ptr offset, gdb::unique_xmalloc_ptr<char> *storage)
+init_stringtab (file_ptr offset, gdb::unique_xmalloc_ptr<char> *storage)
 {
   long length;
   int val;
@@ -599,10 +595,10 @@ init_stringtab (bfd *abfd, file_ptr offset, gdb::unique_xmalloc_ptr<char> *stora
   if (offset == 0)
     return 0;
 
-  if (bfd_seek (abfd, offset, 0) < 0)
+  if (bfd_seek (symfile_bfd, offset, 0) < 0)
     return -1;
 
-  val = bfd_read (lengthbuf, sizeof lengthbuf, abfd);
+  val = bfd_read (lengthbuf, sizeof lengthbuf, symfile_bfd);
   /* If no string table is needed, then the file may end immediately
      after the symbols.  Just return with `stringtab' set to null.  */
   if (val != sizeof lengthbuf)
@@ -621,7 +617,7 @@ init_stringtab (bfd *abfd, file_ptr offset, gdb::unique_xmalloc_ptr<char> *stora
     return 0;
 
   val = bfd_read (stringtab + sizeof lengthbuf,
-		  length - sizeof lengthbuf, abfd);
+		  length - sizeof lengthbuf, symfile_bfd);
   if (val != length - sizeof lengthbuf || stringtab[length - 1] != '\0')
     return -1;
 

-- 
2.52.0

[PATCH 07/10] Use coffread_objfile throughout coffread.c

[Tom Tromey]

coffread.c:coff_symtab_read sets a global objfile while working.  It
seemed nicer to me to hoist this so that it is set by the main entry
point, and then used throughout the file.

Normally I wouldn't encourage the use of globals, but shortly we'll be
replacing these with an object.

Note that while coff_symtab_read cleared coffread_objfile when it was
finished, I haven't preserved this in the new patch.  First, this
should have been done with a scoped_restore.  Second, this is the only
global to get this treatment.  Third, it's not necessary at all.  And,
finally, this will be moot anyway when the globals are removed.
---
 gdb/coffread.c | 63 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index 5cbbf490937..67fe6483087 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -91,8 +91,7 @@ static int init_stringtab (file_ptr, gdb::unique_xmalloc_ptr<char> *);
 
 static void read_one_sym (struct coff_symbol *);
 
-static void coff_symtab_read (minimal_symbol_reader &,
-			      file_ptr, unsigned int, struct objfile *);
+static void coff_symtab_read (minimal_symbol_reader &, file_ptr, unsigned int);
 
 /* Return the BFD section that CS points to.  */
 
@@ -108,12 +107,12 @@ cs_to_bfd_section (struct coff_symbol *cs)
 
 /* Return the section number (SECT_OFF_*) that CS points to.  */
 static int
-cs_to_section (struct coff_symbol *cs, struct objfile *objfile)
+cs_to_section (struct coff_symbol *cs)
 {
   asection *sect = cs_to_bfd_section (cs);
 
   if (sect == NULL)
-    return SECT_OFF_TEXT (objfile);
+    return SECT_OFF_TEXT (coffread_objfile);
   return gdb_bfd_section_index (symfile_bfd, sect);
 }
 
@@ -166,8 +165,7 @@ is_import_fixup_symbol (struct coff_symbol *cs,
 static struct minimal_symbol *
 record_minimal_symbol (minimal_symbol_reader &reader,
 		       struct coff_symbol *cs, unrelocated_addr address,
-		       enum minimal_symbol_type type, int section,
-		       struct objfile *objfile)
+		       enum minimal_symbol_type type, int section)
 {
   /* We don't want TDESC entry points in the minimal symbol table.  */
   if (cs->c_name[0] == '@')
@@ -206,29 +204,28 @@ coff_symfile_init (struct objfile *objfile)
    symbols.  It may also read other forms of symbol as well.  */
 
 static void
-coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms,
-		   struct objfile *objfile)
+coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms)
 
 {
   /* If minimal symbols were already read, and if we know we aren't
      going to read any other kind of symbol here, then we can just
      return.  */
-  if (objfile->per_bfd->minsyms_read && pe_file && nsyms == 0)
+  if (coffread_objfile->per_bfd->minsyms_read && pe_file && nsyms == 0)
     return;
 
-  minimal_symbol_reader reader (objfile);
+  minimal_symbol_reader reader (coffread_objfile);
 
   if (pe_file && nsyms == 0)
     {
       /* 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);
+      read_pe_exported_syms (reader, coffread_objfile);
     }
   else
     {
       /* Now that the executable file is positioned at symbol table,
 	 process it and define symbols accordingly.  */
-      coff_symtab_read (reader, symtab_offset, nsyms, objfile);
+      coff_symtab_read (reader, symtab_offset, nsyms);
     }
 
   /* Install any minimal symbols that have been collected as the
@@ -238,7 +235,7 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms,
 
   if (pe_file)
     {
-      for (minimal_symbol *msym : objfile->msymbols ())
+      for (minimal_symbol *msym : coffread_objfile->msymbols ())
 	{
 	  const char *name = msym->linkage_name ();
 
@@ -263,7 +260,7 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms,
 
 		  bound_minimal_symbol found
 		    = lookup_minimal_symbol (current_program_space, name1,
-					     objfile);
+					     coffread_objfile);
 
 		  /* If found, there are symbols named "_imp_foo" and "foo"
 		     respectively in OBJFILE.  Set the type of symbol "foo"
@@ -282,6 +279,7 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms,
 static void
 coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 {
+  coffread_objfile = objfile;
   symfile_bfd = objfile->obfd.get ();
   coff_data_type *cdata = coff_data (symfile_bfd);
   const char *filename = bfd_get_filename (symfile_bfd);
@@ -328,9 +326,9 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
   if (val < 0)
     error (_("\"%s\": can't get string table"), filename);
 
-  coff_read_minsyms (symtab_offset, num_symbols, objfile);
+  coff_read_minsyms (symtab_offset, num_symbols);
 
-  if (!(objfile->flags & OBJF_READNEVER))
+  if (!(coffread_objfile->flags & OBJF_READNEVER))
     {
       bool found_stab_section = false;
 
@@ -345,18 +343,18 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 	warning (_ ("stabs debug information is not supported."));
     }
 
-  if (dwarf2_initialize_objfile (objfile))
+  if (dwarf2_initialize_objfile (coffread_objfile))
     {
       /* Nothing.  */
     }
 
   /* Try to add separate debug file if no symbols table found.   */
-  else if (!objfile->has_partial_symbols ()
-	   && objfile->separate_debug_objfile == NULL
-	   && objfile->separate_debug_objfile_backlink == NULL)
+  else if (!coffread_objfile->has_partial_symbols ()
+	   && coffread_objfile->separate_debug_objfile == NULL
+	   && coffread_objfile->separate_debug_objfile_backlink == NULL)
     {
-      if (objfile->find_and_add_separate_symbol_file (symfile_flags))
-	gdb_assert (objfile->separate_debug_objfile != nullptr);
+      if (coffread_objfile->find_and_add_separate_symbol_file (symfile_flags))
+	gdb_assert (coffread_objfile->separate_debug_objfile != nullptr);
     }
 }
 
@@ -367,10 +365,9 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 
 static void
 coff_symtab_read (minimal_symbol_reader &reader,
-		  file_ptr symtab_offset, unsigned int nsyms,
-		  struct objfile *objfile)
+		  file_ptr symtab_offset, unsigned int nsyms)
 {
-  struct gdbarch *gdbarch = objfile->arch ();
+  struct gdbarch *gdbarch = coffread_objfile->arch ();
   struct coff_symbol coff_symbol;
   struct coff_symbol *cs = &coff_symbol;
   int val;
@@ -383,9 +380,7 @@ coff_symtab_read (minimal_symbol_reader &reader,
   val = bfd_seek (symfile_bfd, symtab_offset, 0);
   if (val < 0)
     error (_("Error reading symbols from %s: %s"),
-	   objfile_name (objfile), bfd_errmsg (bfd_get_error ()));
-
-  coffread_objfile = objfile;
+	   objfile_name (coffread_objfile), bfd_errmsg (bfd_get_error ()));
 
   while (symnum < nsyms)
     {
@@ -398,14 +393,14 @@ coff_symtab_read (minimal_symbol_reader &reader,
 	{
 	  /* Record all functions -- external and static -- in
 	     minsyms.  */
-	  int section = cs_to_section (cs, objfile);
+	  int section = cs_to_section (cs);
 
 	  tmpaddr = cs->c_value;
 	  /* Don't record unresolved symbols.  */
 	  if (!(cs->c_secnum <= 0 && cs->c_value == 0))
 	    record_minimal_symbol (reader, cs,
 				   unrelocated_addr (tmpaddr),
-				   mst_text, section, objfile);
+				   mst_text, section);
 
 	  continue;
 	}
@@ -468,14 +463,14 @@ coff_symtab_read (minimal_symbol_reader &reader,
 		/* Use the correct minimal symbol type (and don't
 		   relocate) for absolute values.  */
 		ms_type = mst_abs;
-		sec = cs_to_section (cs, objfile);
+		sec = cs_to_section (cs);
 		tmpaddr = cs->c_value;
 	      }
 	    else
 	      {
 		asection *bfd_section = cs_to_bfd_section (cs);
 
-		sec = cs_to_section (cs, objfile);
+		sec = cs_to_section (cs);
 		tmpaddr = cs->c_value;
 
 		if (bfd_section->flags & SEC_CODE)
@@ -505,7 +500,7 @@ coff_symtab_read (minimal_symbol_reader &reader,
 
 	    msym = record_minimal_symbol (reader, cs,
 					  unrelocated_addr (tmpaddr),
-					  ms_type, sec, objfile);
+					  ms_type, sec);
 	    if (msym)
 	      gdbarch_coff_make_msymbol_special (gdbarch,
 						 cs->c_sclass, msym);
@@ -513,8 +508,6 @@ coff_symtab_read (minimal_symbol_reader &reader,
 	  break;
 	}
     }
-
-  coffread_objfile = NULL;
 }
 
 /* Routines for reading headers and symbols from executable.  */

-- 
2.52.0

[PATCH 08/10] Remove coff_symfile_init

[Tom Tromey]

coff_symfile_init is an empty function, and rather than give it a
name, just use a lambda in coff_sym_fns.

I've occasionally considered adding a do_nothing template function for
all our empty function needs, but still haven't bothered.
---
 gdb/coffread.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index 67fe6483087..5ea2d940faa 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -184,22 +184,6 @@ record_minimal_symbol (minimal_symbol_reader &reader,
   return reader.record_full (cs->c_name, true, address, type, section);
 }
 \f
-/* coff_symfile_init ()
-   is the coff-specific initialization routine for reading symbols.
-   It is passed a struct objfile which contains, among other things,
-   the BFD for the file whose symbols are being read, and a slot for
-   a pointer to "private data" which we fill with cookies and other
-   treats for coff_symfile_read ().
-
-   We will only be called if this is a COFF or COFF-like file.  BFD
-   handles figuring out the format of the file, and code in symtab.c
-   uses BFD's determination to vector to us.  */
-
-static void
-coff_symfile_init (struct objfile *objfile)
-{
-}
-
 /* A helper function for coff_symfile_read that reads minimal
    symbols.  It may also read other forms of symbol as well.  */
 
@@ -643,7 +627,7 @@ getsymname (struct internal_syment *symbol_entry)
 
 static const struct sym_fns coff_sym_fns =
 {
-  coff_symfile_init,		/* sym_init: read initial info, setup
+  [] (objfile *) { },		/* sym_init: read initial info, setup
 				   for sym_read() */
   coff_symfile_read,		/* sym_read: read a symbol file into
 				   symtab */

-- 
2.52.0

[PATCH 09/10] Remove all globals from coffread.c

[Tom Tromey]

This patch changes coffread.c to add a reader object type.  All
globals are moved into this type, and the majority of functions in
coffread.c are changed to be methods.
---
 gdb/coffread.c | 178 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 101 insertions(+), 77 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index 5ea2d940faa..9716bdceaec 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -28,44 +28,6 @@
 #include "dwarf2/public.h"
 #include "coff-pe-read.h"
 
-/* The objfile we are currently reading.  */
-
-static struct objfile *coffread_objfile;
-
-/* The BFD for this file -- only good while we're actively reading
-   symbols into a psymtab or a symtab.  */
-
-static bfd *symfile_bfd;
-
-/* Pointers to scratch storage, used for reading raw symbols and
-   auxents.  */
-
-static char *temp_sym;
-static char *temp_aux;
-
-/* Local variables that hold the shift and mask values for the
-   COFF file that we are currently reading.  These come back to us
-   from BFD, and are referenced by their macro names, as well as
-   internally to the ISFCN macro from include/coff/internal.h .  */
-
-static unsigned local_n_btshft;
-static unsigned local_n_tmask;
-
-#define	N_BTSHFT	local_n_btshft
-#define	N_TMASK		local_n_tmask
-
-/* Local variables that hold the sizes in the file of various COFF
-   structures.  (We only need to know this to read them from the file
-   -- BFD will then translate the data in them, into `internal_xxx'
-   structs in the right byte order, alignment, etc.)  */
-
-static unsigned local_symesz;
-static unsigned local_auxesz;
-
-/* This is set if this is a PE format file.  */
-
-static bool pe_file;
-
 /* Simplified internal version of coff symbol table information.  */
 
 struct coff_symbol
@@ -79,24 +41,79 @@ struct coff_symbol
   unsigned int c_type;
 };
 
-static char *stringtab = NULL;
-static long stringtab_length = 0;
+/* A class that reads symbols from a COFF file.  */
+struct coff_reader
+{
+  explicit coff_reader (objfile *objfile)
+    : coffread_objfile (objfile),
+      symfile_bfd (objfile->obfd.get ())
+  {
+  }
+
+  /* Do the work of reading.  */
+  void symfile_read (symfile_add_flags symfile_flags);
 
-/* Used when reading coff symbols.  */
-static int symnum;
+private:
 
-static const char *getsymname (struct internal_syment *);
+  /* The objfile we are currently reading.  */
+  objfile *coffread_objfile;
 
-static int init_stringtab (file_ptr, gdb::unique_xmalloc_ptr<char> *);
+  /* The BFD for this file.  */
+  bfd *symfile_bfd;
 
-static void read_one_sym (struct coff_symbol *);
+  /* Pointers to scratch storage, used for reading raw symbols and
+     auxents.  */
+  char *temp_sym;
+  char *temp_aux;
 
-static void coff_symtab_read (minimal_symbol_reader &, file_ptr, unsigned int);
+  /* Local variables that hold the shift and mask values for the
+     COFF file that we are currently reading.  These come back to us
+     from BFD, and are referenced by their macro names, as well as
+     internally to the ISFCN macro from include/coff/internal.h .  */
+  unsigned local_n_btshft;
+  unsigned local_n_tmask;
+
+#define	N_BTSHFT	local_n_btshft
+#define	N_TMASK		local_n_tmask
+
+  /* Local variables that hold the sizes in the file of various COFF
+     structures.  (We only need to know this to read them from the file
+     -- BFD will then translate the data in them, into `internal_xxx'
+     structs in the right byte order, alignment, etc.)  */
+  unsigned local_symesz;
+  unsigned local_auxesz;
+
+  /* This is set if this is a PE format file.  */
+  bool pe_file;
+
+  char *stringtab = NULL;
+  long stringtab_length = 0;
+
+  /* Used when reading coff symbols.  */
+  int symnum;
+
+  asection *cs_to_bfd_section (coff_symbol *cs);
+  int cs_to_section (coff_symbol *cs);
+  CORE_ADDR cs_section_address (coff_symbol *cs);
+  int is_import_fixup_symbol (coff_symbol *cs, minimal_symbol_type type);
+  minimal_symbol *record_minimal_symbol (minimal_symbol_reader &reader,
+					 coff_symbol *cs,
+					 unrelocated_addr address,
+					 minimal_symbol_type type,
+					 int section);
+  void read_minsyms (file_ptr symtab_offset, unsigned int nsyms);
+  void symtab_read (minimal_symbol_reader &reader, file_ptr symtab_offset,
+		    unsigned int nsyms);
+  void read_one_sym (coff_symbol *cs);
+  int init_stringtab (file_ptr offset,
+		      gdb::unique_xmalloc_ptr<char> *storage);
+  const char *getsymname (struct internal_syment *symbol_entry);
+};
 
 /* Return the BFD section that CS points to.  */
 
-static asection *
-cs_to_bfd_section (struct coff_symbol *cs)
+asection *
+coff_reader::cs_to_bfd_section (struct coff_symbol *cs)
 {
   for (asection *sect : gdb_bfd_sections (symfile_bfd))
     if (sect->target_index == cs->c_secnum)
@@ -106,8 +123,8 @@ cs_to_bfd_section (struct coff_symbol *cs)
 }
 
 /* Return the section number (SECT_OFF_*) that CS points to.  */
-static int
-cs_to_section (struct coff_symbol *cs)
+int
+coff_reader::cs_to_section (struct coff_symbol *cs)
 {
   asection *sect = cs_to_bfd_section (cs);
 
@@ -118,8 +135,8 @@ cs_to_section (struct coff_symbol *cs)
 
 /* Return the address of the section of a COFF symbol.  */
 
-static CORE_ADDR
-cs_section_address (struct coff_symbol *cs)
+CORE_ADDR
+coff_reader::cs_section_address (struct coff_symbol *cs)
 {
   asection *sect = cs_to_bfd_section (cs);
 
@@ -133,9 +150,9 @@ cs_section_address (struct coff_symbol *cs)
    functions referencing variables imported from another DLL.
    Return nonzero if the given symbol corresponds to one of them.  */
 
-static int
-is_import_fixup_symbol (struct coff_symbol *cs,
-			enum minimal_symbol_type type)
+int
+coff_reader::is_import_fixup_symbol (struct coff_symbol *cs,
+				     enum minimal_symbol_type type)
 {
   /* The following is a bit of a heuristic using the characteristics
      of these fixup symbols, but should work well in practice...  */
@@ -162,10 +179,12 @@ is_import_fixup_symbol (struct coff_symbol *cs,
   return 1;
 }
 
-static struct minimal_symbol *
-record_minimal_symbol (minimal_symbol_reader &reader,
-		       struct coff_symbol *cs, unrelocated_addr address,
-		       enum minimal_symbol_type type, int section)
+struct minimal_symbol *
+coff_reader::record_minimal_symbol (minimal_symbol_reader &reader,
+				    struct coff_symbol *cs,
+				    unrelocated_addr address,
+				    enum minimal_symbol_type type,
+				    int section)
 {
   /* We don't want TDESC entry points in the minimal symbol table.  */
   if (cs->c_name[0] == '@')
@@ -187,9 +206,8 @@ record_minimal_symbol (minimal_symbol_reader &reader,
 /* A helper function for coff_symfile_read that reads minimal
    symbols.  It may also read other forms of symbol as well.  */
 
-static void
-coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms)
-
+void
+coff_reader::read_minsyms (file_ptr symtab_offset, unsigned int nsyms)
 {
   /* If minimal symbols were already read, and if we know we aren't
      going to read any other kind of symbol here, then we can just
@@ -209,7 +227,7 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms)
     {
       /* Now that the executable file is positioned at symbol table,
 	 process it and define symbols accordingly.  */
-      coff_symtab_read (reader, symtab_offset, nsyms);
+      symtab_read (reader, symtab_offset, nsyms);
     }
 
   /* Install any minimal symbols that have been collected as the
@@ -260,11 +278,9 @@ coff_read_minsyms (file_ptr symtab_offset, unsigned int nsyms)
 
 /* Read a symbol file, after initialization by coff_symfile_init.  */
 
-static void
-coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
+void
+coff_reader::symfile_read (symfile_add_flags symfile_flags)
 {
-  coffread_objfile = objfile;
-  symfile_bfd = objfile->obfd.get ();
   coff_data_type *cdata = coff_data (symfile_bfd);
   const char *filename = bfd_get_filename (symfile_bfd);
   int val;
@@ -310,7 +326,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
   if (val < 0)
     error (_("\"%s\": can't get string table"), filename);
 
-  coff_read_minsyms (symtab_offset, num_symbols);
+  read_minsyms (symtab_offset, num_symbols);
 
   if (!(coffread_objfile->flags & OBJF_READNEVER))
     {
@@ -342,14 +358,21 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
     }
 }
 
+static void
+coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
+{
+  coff_reader reader (objfile);
+  reader.symfile_read (symfile_flags);
+}
+
 /* Given pointers to a symbol table in coff style exec file,
    analyze them and create struct symtab's describing the symbols.
    NSYMS is the number of symbols in the symbol table.
    We read them one at a time using read_one_sym ().  */
 
-static void
-coff_symtab_read (minimal_symbol_reader &reader,
-		  file_ptr symtab_offset, unsigned int nsyms)
+void
+coff_reader::symtab_read (minimal_symbol_reader &reader,
+			  file_ptr symtab_offset, unsigned int nsyms)
 {
   struct gdbarch *gdbarch = coffread_objfile->arch ();
   struct coff_symbol coff_symbol;
@@ -498,8 +521,8 @@ coff_symtab_read (minimal_symbol_reader &reader,
 
 /* Read the next symbol into CS.  */
 
-static void
-read_one_sym (struct coff_symbol *cs)
+void
+coff_reader::read_one_sym (struct coff_symbol *cs)
 {
   int i;
   bfd_size_type bytes;
@@ -560,8 +583,9 @@ read_one_sym (struct coff_symbol *cs)
 
 /* Support for string table handling.  */
 
-static int
-init_stringtab (file_ptr offset, gdb::unique_xmalloc_ptr<char> *storage)
+int
+coff_reader::init_stringtab (file_ptr offset,
+			     gdb::unique_xmalloc_ptr<char> *storage)
 {
   long length;
   int val;
@@ -601,8 +625,8 @@ init_stringtab (file_ptr offset, gdb::unique_xmalloc_ptr<char> *storage)
   return 0;
 }
 
-static const char *
-getsymname (struct internal_syment *symbol_entry)
+const char *
+coff_reader::getsymname (struct internal_syment *symbol_entry)
 {
   static char buffer[SYMNMLEN + 1];
   const char *result;

-- 
2.52.0

[PATCH 10/10] Change is_import_fixup_symbol to return bool

[Tom Tromey]

This changes the is_import_fixup_symbol in coffread.c to return bool
rather than int.
---
 gdb/coffread.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index 9716bdceaec..fdfc67c8b3a 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -95,7 +95,7 @@ struct coff_reader
   asection *cs_to_bfd_section (coff_symbol *cs);
   int cs_to_section (coff_symbol *cs);
   CORE_ADDR cs_section_address (coff_symbol *cs);
-  int is_import_fixup_symbol (coff_symbol *cs, minimal_symbol_type type);
+  bool is_import_fixup_symbol (coff_symbol *cs, minimal_symbol_type type);
   minimal_symbol *record_minimal_symbol (minimal_symbol_reader &reader,
 					 coff_symbol *cs,
 					 unrelocated_addr address,
@@ -148,9 +148,9 @@ coff_reader::cs_section_address (struct coff_symbol *cs)
 
 /* The linker sometimes generates some non-function symbols inside
    functions referencing variables imported from another DLL.
-   Return nonzero if the given symbol corresponds to one of them.  */
+   Return true if the given symbol corresponds to one of them.  */
 
-int
+bool
 coff_reader::is_import_fixup_symbol (struct coff_symbol *cs,
 				     enum minimal_symbol_type type)
 {
@@ -160,23 +160,23 @@ coff_reader::is_import_fixup_symbol (struct coff_symbol *cs,
 
   /* Must be a non-static text symbol.  */
   if (type != mst_text)
-    return 0;
+    return false;
 
   /* Must be a non-function symbol.  */
   if (ISFCN (cs->c_type))
-    return 0;
+    return false;
 
   /* The name must start with "__fu<digits>__".  */
   if (!startswith (cs->c_name, "__fu"))
-    return 0;
+    return false;
   if (! c_isdigit (cs->c_name[4]))
-    return 0;
+    return false;
   for (i = 5; cs->c_name[i] != '\0' && c_isdigit (cs->c_name[i]); i++)
     /* Nothing, just incrementing index past all digits.  */;
   if (cs->c_name[i] != '_' || cs->c_name[i + 1] != '_')
-    return 0;
+    return false;
 
-  return 1;
+  return true;
 }
 
 struct minimal_symbol *

-- 
2.52.0

Re: [PATCH 03/10] Fix indentation in coffread.c

[Simon Marchi]

On 2026-01-16 14:11, Tom Tromey wrote:
> A couple of spots in coffread.c were indented incorrectly.
> ---
>  gdb/coffread.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index b0c4a073df3..eaaebd55326 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -69,15 +69,15 @@ static int pe_file;
>  /* Simplified internal version of coff symbol table information.  */
>  
>  struct coff_symbol
> -  {
> -    const char *c_name;
> -    int c_naux;			/* 0 if syment only, 1 if syment +
> +{
> +  const char *c_name;
> +  int c_naux;			/* 0 if syment only, 1 if syment +
>  				   auxent, etc.  */

While at it, I think you could move the comment so it is above the field
it is describing.  We don't really do this style of comment to the right
nowadays.

Simon

Re: [PATCH 06/10] Use symfile_bfd in more places

[Simon Marchi]

On 2026-01-16 14:11, Tom Tromey wrote:
> Since coffread.c is setting symfile_bfd, it might as well use it
> everywhere.  This changes all other BFD references in coffread.c to
> use the global.

I think you should just remove symfile_bfd too.

 - add a bfd parameter to read_one_sym
 - coff_symtab_read can pass objfile->obfd to read_one_sym
 - init_stringtab refers to symfile_bfd, but it already receives the
   same bfd by parameter, so it can just use the parameter

Simon

Re: [PATCH 06/10] Use symfile_bfd in more places

[Simon Marchi]

On 2026-01-16 14:11, Tom Tromey wrote:
> Since coffread.c is setting symfile_bfd, it might as well use it
> everywhere.  This changes all other BFD references in coffread.c to
> use the global.

Oh well, my suggestion on the previous patch kind of goes against this.

Simon

Re: [PATCH 08/10] Remove coff_symfile_init

[Simon Marchi]

On 2026-01-16 14:11, Tom Tromey wrote:
> coff_symfile_init is an empty function, and rather than give it a
> name, just use a lambda in coff_sym_fns.
> 
> I've occasionally considered adding a do_nothing template function for
> all our empty function needs, but still haven't bothered.

I think it would also be fine to change sym_fns to allow for this
callback to be nullptr.

Simon

Re: [PATCH 06/10] Use symfile_bfd in more places

[Simon Marchi]

On 2026-01-16 23:09, Simon Marchi wrote:
> 
> 
> On 2026-01-16 14:11, Tom Tromey wrote:
>> Since coffread.c is setting symfile_bfd, it might as well use it
>> everywhere.  This changes all other BFD references in coffread.c to
>> use the global.
> 
> I think you should just remove symfile_bfd too.
> 
>  - add a bfd parameter to read_one_sym
>  - coff_symtab_read can pass objfile->obfd to read_one_sym
>  - init_stringtab refers to symfile_bfd, but it already receives the
>    same bfd by parameter, so it can just use the parameter

Heh, this should have been in reply to patch 5.  Anyway, you can
disregard this comment, given that this is all replaced with a reader
object at the end.  I should really read whole series before commenting
on individual patches to know what the end goal is.

Simon

Re: [PATCH 00/10] More coffread cleanups

[Simon Marchi]

On 2026-01-16 14:11, Tom Tromey wrote:
> After Simon removed a huge chunk of coffread.c, I noticed there were
> more cleanups that could be done.  This series is the result.
> 
> I'm not totally certain how to test this, but I did at least run the
> resulting gdb on some Windows object files to make sure nothing
> untoward happens.
> 
> Signed-off-by: Tom Tromey <tromey@adacore.com>

This all looks fine to me, thanks.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 03/10] Fix indentation in coffread.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> While at it, I think you could move the comment so it is above the field
Simon> it is describing.  We don't really do this style of comment to the right
Simon> nowadays.

I made this change.

Tom

Re: [PATCH 08/10] Remove coff_symfile_init

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> On 2026-01-16 14:11, Tom Tromey wrote:
>> coff_symfile_init is an empty function, and rather than give it a
>> name, just use a lambda in coff_sym_fns.
>> 
>> I've occasionally considered adding a do_nothing template function for
>> all our empty function needs, but still haven't bothered.

Simon> I think it would also be fine to change sym_fns to allow for this
Simon> callback to be nullptr.

At some point I think we should replace these with real objects.

Tom