diff mbox

[RFC] Remove cleanups from coffread.c

Message ID 20181227142237.GH21851@adacore.com
State New
Headers show

Commit Message

Joel Brobecker Dec. 27, 2018, 2:22 p.m. UTC
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.
diff mbox

Patch

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