[RFC] Remove cleanups from coffread.c
Joel Brobecker
brobecker@adacore.com
Wed Nov 21 21:34:00 GMT 2018
Hi Tom,
> This removes the remaining cleanups from coffread.c.
>
> Tested by the buildbot. However, I don't think the buildbot really
> tests these code paths, and TBH I am not sure how to do it manually.
> I did manage to crash an earlier version of the patch using a mingw
> .exe file, though I would have thought that was only going to test
> coff-pe-read.c.
>
> gdb/ChangeLog
> 2018-11-05 Tom Tromey <tom@tromey.com>
>
> * stabsread.h (struct stab_section_list): Remove.
> (coffstab_build_psymtabs): Update.
> * dbxread.c (symbuf_sections): Now a std::vector.
> (sect_idx): New global.
> (fill_symbuf): Update.
> (coffstab_build_psymtabs): Change type of stabsects parameter.
> Update.
> * coffread.c (struct coff_symfile_info) <stabsects>: Now a
> std::vector.
> (linetab, linetab_offset, linetab_size, stringtab): Move earlier.
> (coff_locate_sections): Update.
> (coff_symfile_read): Remove cleanups. Update.
> (init_stringtab): Add storage parameter.
> (free_stringtab, free_stringtab_cleanup): Remove.
> (init_lineno): Add storage parameter.
> (free_linetab, free_linetab_cleanup): Remove.
First of all, thanks for the patch!
I've been meaning to try to review this patch since you posted it,
and unfortunately I haven't had the time. So I decided to at least
give it a quick test -- hoping it would only take a few minutes and
still be better than nothing. And unfortunately, it seems to be causing
some regressions. With a simple program, we can see:
$ gdb -q --nh simple_main.exe
Reading symbols from simple_main.exe...
The debugging information in `C:\tmp\brobecke\ex\simple\simple_main.exe' is corrupted.
The file has a `.stabs' section, but no `.stabstr' section.
(gdb) run
Starting program: C:\tmp\gdb-TS-kdpzkw\simple\simple_main.exe
Error while reading shared library symbols for C:\Windows\SYSTEM32\ntdll.dll:
The debugging information in `C:\Windows\SYSTEM32\ntdll.dll' is corrupted.
The file has a `.stabs' section, but no `.stabstr' section.
Error while reading shared library symbols for C:\Windows\System32\kernel32.dll:
The debugging information in `C:\Windows\System32\kernel32.dll' is corrupted.
The file has a `.stabs' section, but no `.stabstr' section.
[etc]
I'll keep this one in my list to investigate further when I have
a moment; Looking at the error message, I don't think it is necessarily
very complicated, but I probably won't have time before mid December :-(.
> ---
> gdb/ChangeLog | 19 ++++++++
> gdb/coffread.c | 113 ++++++++++++++----------------------------------
> gdb/dbxread.c | 32 +++++++-------
> gdb/stabsread.h | 14 +-----
> 4 files changed, 70 insertions(+), 108 deletions(-)
>
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index a473b78245..4feb449948 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -58,7 +58,7 @@ struct coff_symfile_info
>
> CORE_ADDR textaddr; /* Addr of .text section. */
> unsigned int textsize; /* Size of .text section. */
> - struct stab_section_list *stabsects; /* .stab sections. */
> + std::vector<asection *> *stabsects; /* .stab sections. */
> asection *stabstrsect; /* Section pointer for .stab section. */
> char *stabstrdata;
> };
> @@ -155,6 +155,12 @@ static int type_vector_length;
>
> #define INITIAL_TYPE_VECTOR_LENGTH 160
>
> +static char *linetab = NULL;
> +static long linetab_offset;
> +static unsigned long linetab_size;
> +
> +static char *stringtab = NULL;
> +
> extern void stabsread_clear_cache (void);
>
> static struct type *coff_read_struct_type (int, int, int,
> @@ -185,21 +191,13 @@ static void patch_opaque_types (struct symtab *);
>
> static void enter_linenos (long, int, int, struct objfile *);
>
> -static void free_linetab (void);
> -
> -static void free_linetab_cleanup (void *ignore);
> -
> -static int init_lineno (bfd *, long, int);
> +static int init_lineno (bfd *, long, int, gdb::unique_xmalloc_ptr<char> *);
>
> static char *getsymname (struct internal_syment *);
>
> static const char *coff_getfilename (union internal_auxent *);
>
> -static void free_stringtab (void);
> -
> -static void free_stringtab_cleanup (void *ignore);
> -
> -static int init_stringtab (bfd *, long);
> +static int init_stringtab (bfd *, long, gdb::unique_xmalloc_ptr<char> *);
>
> static void read_one_sym (struct coff_symbol *,
> struct internal_syment *,
> @@ -249,21 +247,7 @@ coff_locate_sections (bfd *abfd, asection *sectp, void *csip)
> if (!isdigit (*s))
> break;
> if (*s == '\0')
> - {
> - struct stab_section_list *n, **pn;
> -
> - n = XNEW (struct stab_section_list);
> - n->section = sectp;
> - n->next = NULL;
> - for (pn = &csi->stabsects; *pn != NULL; pn = &(*pn)->next)
> - ;
> - *pn = n;
> -
> - /* This will be run after coffstab_build_psymtabs is called
> - in coff_symfile_read, at which point we no longer need
> - the information. */
> - make_cleanup (xfree, n);
> - }
> + csi->stabsects->push_back (sectp);
> }
> }
>
> @@ -570,13 +554,16 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
> unsigned int num_symbols;
> int symtab_offset;
> int stringtab_offset;
> - struct cleanup *back_to;
> int stabstrsize;
>
> info = (struct coff_symfile_info *) objfile_data (objfile,
> coff_objfile_data_key);
> symfile_bfd = abfd; /* Kludge for swap routines. */
>
> + std::vector<asection *> stabsects;
> + scoped_restore restore_stabsects
> + = make_scoped_restore (&info->stabsects, &stabsects);
> +
> /* 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 */
> @@ -595,10 +582,10 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>
> /* Allocate space for raw symbol and aux entries, based on their
> space requirements as reported by BFD. */
> - temp_sym = (char *) xmalloc
> - (cdata->local_symesz + cdata->local_auxesz);
> + gdb::def_vector<char> temp_storage (cdata->local_symesz
> + + cdata->local_auxesz);
> + temp_sym = temp_storage.data ();
> temp_aux = temp_sym + cdata->local_symesz;
> - back_to = make_cleanup (free_current_contents, &temp_sym);
>
> /* We need to know whether this is a PE file, because in PE files,
> unlike standard COFF files, symbol values are stored as offsets
> @@ -628,22 +615,25 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
> can avoid spurious error messages (and maybe run a little
> faster!) by not even reading the line number table unless we have
> symbols. */
> + scoped_restore restore_linetab = make_scoped_restore (&linetab);
> + gdb::unique_xmalloc_ptr<char> linetab_storage;
> if (num_symbols > 0)
> {
> /* Read the line number table, all at once. */
> bfd_map_over_sections (abfd, find_linenos, (void *) info);
>
> - make_cleanup (free_linetab_cleanup, 0 /*ignore*/);
> val = init_lineno (abfd, info->min_lineno_offset,
> - info->max_lineno_offset - info->min_lineno_offset);
> + info->max_lineno_offset - info->min_lineno_offset,
> + &linetab_storage);
> if (val < 0)
> error (_("\"%s\": error reading line numbers."), filename);
> }
>
> /* Now read the string table, all at once. */
>
> - make_cleanup (free_stringtab_cleanup, 0 /*ignore*/);
> - val = init_stringtab (abfd, stringtab_offset);
> + scoped_restore restore_stringtab = make_scoped_restore (&stringtab);
> + gdb::unique_xmalloc_ptr<char> stringtab_storage;
> + val = init_stringtab (abfd, stringtab_offset, &stringtab_storage);
> if (val < 0)
> error (_("\"%s\": can't get string table"), filename);
>
> @@ -720,7 +710,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>
> coffstab_build_psymtabs (objfile,
> info->textaddr, info->textsize,
> - info->stabsects,
> + *info->stabsects,
> info->stabstrsect->filepos, stabstrsize);
> }
> if (dwarf2_has_info (objfile, NULL))
> @@ -747,8 +737,6 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
> symfile_flags, objfile);
> }
> }
> -
> - do_cleanups (back_to);
> }
>
> static void
> @@ -1298,17 +1286,13 @@ read_one_sym (struct coff_symbol *cs,
>
> /* Support for string table handling. */
>
> -static char *stringtab = NULL;
> -
> static int
> -init_stringtab (bfd *abfd, long offset)
> +init_stringtab (bfd *abfd, long offset, gdb::unique_xmalloc_ptr<char> *storage)
> {
> long length;
> int val;
> unsigned char lengthbuf[4];
>
> - free_stringtab ();
> -
> /* If the file is stripped, the offset might be zero, indicating no
> string table. Just return with `stringtab' set to null. */
> if (offset == 0)
> @@ -1325,7 +1309,8 @@ init_stringtab (bfd *abfd, long offset)
> if (val != sizeof lengthbuf || length < sizeof lengthbuf)
> return 0;
>
> - stringtab = (char *) xmalloc (length);
> + storage->reset ((char *) xmalloc (length));
> + stringtab = storage->get ();
> /* This is in target format (probably not very useful, and not
> currently used), not host format. */
> memcpy (stringtab, lengthbuf, sizeof lengthbuf);
> @@ -1340,20 +1325,6 @@ init_stringtab (bfd *abfd, long offset)
> return 0;
> }
>
> -static void
> -free_stringtab (void)
> -{
> - if (stringtab)
> - xfree (stringtab);
> - stringtab = NULL;
> -}
> -
> -static void
> -free_stringtab_cleanup (void *ignore)
> -{
> - free_stringtab ();
> -}
> -
> static char *
> getsymname (struct internal_syment *symbol_entry)
> {
> @@ -1407,24 +1378,19 @@ coff_getfilename (union internal_auxent *aux_entry)
>
> /* Support for line number handling. */
>
> -static char *linetab = NULL;
> -static long linetab_offset;
> -static unsigned long linetab_size;
> -
> /* Read in all the line numbers for fast lookups later. Leave them in
> external (unswapped) format in memory; we'll swap them as we enter
> them into GDB's data structures. */
>
> static int
> -init_lineno (bfd *abfd, long offset, int size)
> +init_lineno (bfd *abfd, long offset, int size,
> + gdb::unique_xmalloc_ptr<char> *storage)
> {
> int val;
>
> linetab_offset = offset;
> linetab_size = size;
>
> - free_linetab ();
> -
> if (size == 0)
> return 0;
>
> @@ -1432,9 +1398,10 @@ init_lineno (bfd *abfd, long offset, int size)
> return -1;
>
> /* Allocate the desired table, plus a sentinel. */
> - linetab = (char *) xmalloc (size + local_linesz);
> + storage->reset ((char *) xmalloc (size + local_linesz));
> + linetab = storage->get ();
>
> - val = bfd_bread (linetab, size, abfd);
> + val = bfd_bread (storage->get (), size, abfd);
> if (val != size)
> return -1;
>
> @@ -1444,20 +1411,6 @@ init_lineno (bfd *abfd, long offset, int size)
> return 0;
> }
>
> -static void
> -free_linetab (void)
> -{
> - if (linetab)
> - xfree (linetab);
> - linetab = NULL;
> -}
> -
> -static void
> -free_linetab_cleanup (void *ignore)
> -{
> - free_linetab ();
> -}
> -
> #if !defined (L_LNNO32)
> #define L_LNNO32(lp) ((lp)->l_lnno)
> #endif
> diff --git a/gdb/dbxread.c b/gdb/dbxread.c
> index e004e8cb93..d2357c8c10 100644
> --- a/gdb/dbxread.c
> +++ b/gdb/dbxread.c
> @@ -751,7 +751,8 @@ static char *stringtab_global;
> /* These variables are used to control fill_symbuf when the stabs
> symbols are not contiguous (as may be the case when a COFF file is
> linked using --split-by-reloc). */
> -static struct stab_section_list *symbuf_sections;
> +static const std::vector<asection *> *symbuf_sections;
> +static size_t sect_idx;
> static unsigned int symbuf_left;
> static unsigned int symbuf_read;
>
> @@ -787,13 +788,13 @@ fill_symbuf (bfd *sym_bfd)
> {
> if (symbuf_left <= 0)
> {
> - file_ptr filepos = symbuf_sections->section->filepos;
> + file_ptr filepos = (*symbuf_sections)[sect_idx]->filepos;
>
> if (bfd_seek (sym_bfd, filepos, SEEK_SET) != 0)
> perror_with_name (bfd_get_filename (sym_bfd));
> - symbuf_left = bfd_section_size (sym_bfd, symbuf_sections->section);
> + symbuf_left = bfd_section_size (sym_bfd, (*symbuf_sections)[sect_idx]);
> symbol_table_offset = filepos - symbuf_read;
> - symbuf_sections = symbuf_sections->next;
> + ++sect_idx;
> }
>
> count = symbuf_left;
> @@ -2962,7 +2963,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
> void
> coffstab_build_psymtabs (struct objfile *objfile,
> CORE_ADDR textaddr, unsigned int textsize,
> - struct stab_section_list *stabsects,
> + const std::vector<asection *> &stabsects,
> file_ptr stabstroffset, unsigned int stabstrsize)
> {
> int val;
> @@ -3001,27 +3002,28 @@ coffstab_build_psymtabs (struct objfile *objfile,
> /* In a coff file, we've already installed the minimal symbols that came
> from the coff (non-stab) symbol table, so always act like an
> incremental load here. */
> - if (stabsects->next == NULL)
> + scoped_restore save_symbuf_sections
> + = make_scoped_restore (&symbuf_sections);
> + if (stabsects.size () == 1)
> {
> - stabsize = bfd_section_size (sym_bfd, stabsects->section);
> + stabsize = bfd_section_size (sym_bfd, stabsects[0]);
> DBX_SYMCOUNT (objfile) = stabsize / DBX_SYMBOL_SIZE (objfile);
> - DBX_SYMTAB_OFFSET (objfile) = stabsects->section->filepos;
> + DBX_SYMTAB_OFFSET (objfile) = stabsects[0]->filepos;
> }
> else
> {
> - struct stab_section_list *stabsect;
> -
> DBX_SYMCOUNT (objfile) = 0;
> - for (stabsect = stabsects; stabsect != NULL; stabsect = stabsect->next)
> + for (asection *section : stabsects)
> {
> - stabsize = bfd_section_size (sym_bfd, stabsect->section);
> + stabsize = bfd_section_size (sym_bfd, section);
> DBX_SYMCOUNT (objfile) += stabsize / DBX_SYMBOL_SIZE (objfile);
> }
>
> - DBX_SYMTAB_OFFSET (objfile) = stabsects->section->filepos;
> + DBX_SYMTAB_OFFSET (objfile) = stabsects[0]->filepos;
>
> - symbuf_sections = stabsects->next;
> - symbuf_left = bfd_section_size (sym_bfd, stabsects->section);
> + sect_idx = 1;
> + symbuf_sections = &stabsects;
> + symbuf_left = bfd_section_size (sym_bfd, stabsects[0]);
> symbuf_read = 0;
> }
>
> diff --git a/gdb/stabsread.h b/gdb/stabsread.h
> index b0194e0c69..3376f9eaaf 100644
> --- a/gdb/stabsread.h
> +++ b/gdb/stabsread.h
> @@ -173,18 +173,6 @@ extern void end_stabs (void);
>
> extern void finish_global_stabs (struct objfile *objfile);
>
> -/* COFF files can have multiple .stab sections, if they are linked
> - using --split-by-reloc. This linked list is used to pass the
> - information into the functions in dbxread.c. */
> -struct stab_section_list
> - {
> - /* Next in list. */
> - struct stab_section_list *next;
> -
> - /* Stab section. */
> - asection *section;
> - };
> -
> /* Functions exported by dbxread.c. These are not in stabsread.c because
> they are only used by some stabs readers. */
>
> @@ -207,7 +195,7 @@ extern void elfstab_build_psymtabs (struct objfile *objfile,
> extern void coffstab_build_psymtabs
> (struct objfile *objfile,
> CORE_ADDR textaddr, unsigned int textsize,
> - struct stab_section_list *stabs,
> + const std::vector<asection *> &stabs,
> file_ptr stabstroffset, unsigned int stabstrsize);
>
> extern void stabsect_build_psymtabs (struct objfile *objfile, char *stab_name,
> --
> 2.17.2
--
Joel
More information about the Gdb-patches
mailing list