* [RFC] Remove cleanups from coffread.c
@ 2018-11-07 5:01 Tom Tromey
2018-11-21 21:34 ` Joel Brobecker
2018-12-27 14:22 ` Joel Brobecker
0 siblings, 2 replies; 3+ messages in thread
From: Tom Tromey @ 2018-11-07 5:01 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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.
---
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,
\f
/* 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)
\f
/* 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);
\f
-/* 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;
- };
-\f
/* 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC] Remove cleanups from coffread.c
2018-11-07 5:01 [RFC] Remove cleanups from coffread.c Tom Tromey
@ 2018-11-21 21:34 ` Joel Brobecker
2018-12-27 14:22 ` Joel Brobecker
1 sibling, 0 replies; 3+ messages in thread
From: Joel Brobecker @ 2018-11-21 21:34 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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,
> \f
> /* 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)
> \f
> /* 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);
> \f
> -/* 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;
> - };
> -\f
> /* 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC] Remove cleanups from coffread.c
2018-11-07 5:01 [RFC] Remove cleanups from coffread.c Tom Tromey
2018-11-21 21:34 ` Joel Brobecker
@ 2018-12-27 14:22 ` Joel Brobecker
1 sibling, 0 replies; 3+ messages in thread
From: Joel Brobecker @ 2018-12-27 14:22 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 5295 bytes --]
Hi Tom,
> 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.
As mentioned earlier, this patch causes a regression while processing
the objfile. With any program, we now get:
(gdb) file dummy
Reading symbols from dummy...
The debugging information in `C:\tmp\brobecke\ex\tbegin\dummy.exe' is corrupted.
The file has a `.stabs' section, but no `.stabstr' section.
The cause wasn't difficult to spot. The spot that generates the error
message looks like this:
| if (info->stabsects)
| {
| if (!info->stabstrsect)
| {
| error (_("The debugging information in `%s' is corrupted.\nThe "
| "file has a `.stabs' section, but no `.stabstr' section."),
| filename);
| }
In my cases, what happens is that my executable has no stabs debug
info in it. But the code above thought so, because INFO->STABSECTS
is not NULL. The reason it is not NULL is because we explicitly
set it to the address of a local vector via:
| + std::vector<asection *> stabsects;
| + scoped_restore restore_stabsects
| + = make_scoped_restore (&info->stabsects, &stabsects);
The obvious fix is to change the test to check the size of
the vector instead.
One question I asked myself, however, was why the pointer to
the vector instead of a straight vector? I even started the attempt
to fix the issue doing exactly that. However, this caused a compilation
failure on this statement:
| coff = XCNEW (struct coff_symfile_info);
The error message was:
| /[...]/gdb/common/poison.h:120:18: error: static assertion failed: Trying to use XCNEW with a non-POD data type. Use operator new instead.
| static_assert (IsMallocable<T>::value, "Trying to use XCNEW with a non-POD \
| ^~~~~~~~~~~~~~~
My guess is that this was the reason. When I saw this, it seemed
to me that switching this field to a straight vector would mean
having to switch the allocation policy of coff_symfile_info objects,
which means having a look at the deallocation as well. Considering
that this was purely for stabs support, I quickly reatreated from
the idea, and just made the obvious fix.
I tested the patch combined with my fix, and found no regression.
However, we use DWARF by default, so my suspicion is that the changes
are barely covered. I do not have a good stabs testing framework
anymore (haven't for a long time). Nevertheless, I ran our testsuite
with -gstabs before and after. As expected, there were a lot of
failures, making the results obtained only marginally useful; but
the good news is that the results obtained before and after the patch
are identical. So, not the absolute best testing methodology, but
at least no obvious regression with stabs either.
I took a look at the rest of the patch. Overall, the changes made
sense to me, although I have to say that I find the code quite obscure,
expecially the code in dbxread.c. I'll add my thoughts while going
through the patch, as a way to explain how I understood this patch,
and how I convinced myself the changes are probably good. See comments
below.
> -static int init_lineno (bfd *, long, int);
> +static int init_lineno (bfd *, long, int, gdb::unique_xmalloc_ptr<char> *);
Out of curiosity, why a pointer, here, rather than a reference?
(same for init_stringtab)
> @@ -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;
This corresponds to the removal of the free_linetab_cleanup
make_cleanup just below. The goal of the cleanup was to reset
tinetab to NULL at the end, so this restore assumes that we start
with a NULL linetab.
> - 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);
Same idea as above.
> diff --git a/gdb/dbxread.c b/gdb/dbxread.c
> index e004e8cb93..d2357c8c10 100644
> --- a/gdb/dbxread.c
> +++ b/gdb/dbxread.c
At this point, it was a lot harder for me to follow how the code
(old and new) is supposed to work. But the changes looked to me
like they are preserving the original behavior.
--
Joel
[-- Attachment #2: 0001-Windows-erroneous-error-message-about-stabstr-sectio.patch --]
[-- Type: text/x-diff, Size: 943 bytes --]
From 790455205108588cd3290493d929a092cb7b79f7 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 27 Dec 2018 07:01:46 +0100
Subject: [PATCH] (Windows) erroneous error message about stabstr section
missing
(gdb) file dummy
Reading symbols from dummy...
The debugging information in `C:\tmp\brobecke\ex\tbegin\dummy.exe' is corrupted.
The file has a `.stabs' section, but no `.stabstr' section.
---
gdb/coffread.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb/coffread.c b/gdb/coffread.c
index 4feb449..7491919 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -693,7 +693,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
if (!(objfile->flags & OBJF_READNEVER))
bfd_map_over_sections (abfd, coff_locate_sections, (void *) info);
- if (info->stabsects)
+ if (! info->stabsects->empty())
{
if (!info->stabstrsect)
{
--
2.8.3
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-27 14:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 5:01 [RFC] Remove cleanups from coffread.c Tom Tromey
2018-11-21 21:34 ` Joel Brobecker
2018-12-27 14:22 ` Joel Brobecker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).