From patchwork Thu Dec 27 14:22:37 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 30863 Received: (qmail 126941 invoked by alias); 27 Dec 2018 14:22:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 126922 invoked by uid 89); 27 Dec 2018 14:22:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=quickly, framework, straight, asked X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Dec 2018 14:22:43 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5D5D5116332; Thu, 27 Dec 2018 09:22:42 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 8rR-I5YWXK6g; Thu, 27 Dec 2018 09:22:42 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id DDB97116347; Thu, 27 Dec 2018 09:22:41 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9348F86753; Thu, 27 Dec 2018 18:22:37 +0400 (+04) Date: Thu, 27 Dec 2018 18:22:37 +0400 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Remove cleanups from coffread.c Message-ID: <20181227142237.GH21851@adacore.com> References: <20181107050135.30993-1-tom@tromey.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181107050135.30993-1-tom@tromey.com> User-Agent: Mutt/1.9.4 (2018-02-28) Hi Tom, > gdb/ChangeLog > 2018-11-05 Tom Tromey > > * 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) : 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 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::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 *); 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 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 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. From 790455205108588cd3290493d929a092cb7b79f7 Mon Sep 17 00:00:00 2001 From: Joel Brobecker 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