Message ID | d266f2a4-5482-6178-f4fd-9280e27757b4@imgtec.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 74373 invoked by alias); 28 Apr 2017 23:44:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 73365 invoked by uid 89); 28 Apr 2017 23:44:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:10.20.2, our X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 28 Apr 2017 23:44:35 +0000 Received: from hhmail02.hh.imgtec.org (unknown [10.100.10.20]) by Forcepoint Email with ESMTPS id DE140C8B3F5E6; Sat, 29 Apr 2017 00:44:29 +0100 (IST) Received: from HHMAIL-X.hh.imgtec.org (10.100.10.113) by hhmail02.hh.imgtec.org (10.100.10.20) with Microsoft SMTP Server (TLS) id 14.3.294.0; Sat, 29 Apr 2017 00:44:34 +0100 Received: from BAMAIL02.ba.imgtec.org (10.20.40.28) by HHMAIL-X.hh.imgtec.org (10.100.10.113) with Microsoft SMTP Server (TLS) id 14.3.294.0; Sat, 29 Apr 2017 00:44:34 +0100 Received: from [10.20.2.42] (10.20.2.42) by bamail02.ba.imgtec.org (10.20.40.28) with Microsoft SMTP Server id 14.3.266.1; Fri, 28 Apr 2017 16:44:31 -0700 Subject: Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging. To: Simon Marchi <simon.marchi@polymtl.ca> References: <d7619e0c-38f4-b547-8117-8e5315313b71@imgtec.com> <a869bc5c-a685-d9f1-1f28-aafcde8c3e1d@codesourcery.com> <20511c76-c816-d31d-5144-749eac9fc470@imgtec.com> <3c5ce0a0-72e5-4460-5555-ad2214866260@imgtec.com> <5c494cc147f71dd8246572aa0b815c9f@polymtl.ca> <fddf3219-1de7-09ca-517c-7d68b34e878d@imgtec.com> <7e9595026acbfd2f1a7bff321fa255e1@polymtl.ca> CC: Luis Machado <lgustavo@codesourcery.com>, <gdb-patches@sourceware.org> From: Doug Gilmore <Doug.Gilmore@imgtec.com> Message-ID: <d266f2a4-5482-6178-f4fd-9280e27757b4@imgtec.com> Date: Fri, 28 Apr 2017 16:44:31 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <7e9595026acbfd2f1a7bff321fa255e1@polymtl.ca> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit |
Commit Message
Doug Gilmore
April 28, 2017, 11:44 p.m. UTC
> ... > Hi Doug, > > I've ran this in my head many times, but I still don't understand which field exactly is stale and causes the segfault. > > According to the backtrace, the line of the crash is: > > if (pc < obj_section_addr (section)) > > That macro expands to > > #define obj_section_addr(s) \ > (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section) \ > + obj_section_offset (s)) > > which further expands to > > #define obj_section_offset(s) \ > (((s)->objfile->section_offsets)->offsets[gdb_bfd_section_index ((s)->objfile->obfd, (s)->the_bfd_section)]) > > > Could you point out which dereferencing operator/memory access causes the crash? At first I thought it would be ->section_offsets, but that field is set properly before calling read_symbols: > > /* We use the same section offsets as from last time. I'm not > sure whether that is always correct for shared libraries. */ > objfile->section_offsets = (struct section_offsets *) > obstack_alloc (&objfile->objfile_obstack, > SIZEOF_N_SECTION_OFFSETS (num_offsets)); > memcpy (objfile->section_offsets, offsets, > SIZEOF_N_SECTION_OFFSETS (num_offsets)); > objfile->num_sections = num_offsets; > > so it should not be the culprit... The s variable itself should point to an instance of obj_section, contained in the objfile_pspace_info::sections array. This one is allocated with XNEWVEC, so it shouldn't be affected by the fact that we clear the obstack. > > The objfile object itself doesn't change address, so the pointers to it should still be valid... > > I find the obj_section_addr and obj_section_offset very bad for readability and debuggability. Could you change them for inline functions that are not one liners? Then it will be obvious in the backtrace which operation causes the crash. > > Thanks, > > Simon Hi Simon, After thinking about it my comment and code placement wasn't particularly good. Something along the line's of Luis's change is better. Does Luis's comment address the question you have? If so, Luis: Should is it OK we incorporate your changes in the patch? I attached a diff for the change. Thanks, Doug
Comments
On 04/28/2017 06:44 PM, Doug Gilmore wrote: >> ... >> /* We use the same section offsets as from last time. I'm not >> sure whether that is always correct for shared libraries. */ >> objfile->section_offsets = (struct section_offsets *) >> obstack_alloc (&objfile->objfile_obstack, >> SIZEOF_N_SECTION_OFFSETS (num_offsets)); >> memcpy (objfile->section_offsets, offsets, >> SIZEOF_N_SECTION_OFFSETS (num_offsets)); >> objfile->num_sections = num_offsets; >> >> so it should not be the culprit... The s variable itself should point to an instance of obj_section, contained in the objfile_pspace_info::sections array. This one is allocated with XNEWVEC, so it shouldn't be affected by the fact that we clear the obstack. >> >> The objfile object itself doesn't change address, so the pointers to it should still be valid... >> >> I find the obj_section_addr and obj_section_offset very bad for readability and debuggability. Could you change them for inline functions that are not one liners? Then it will be obvious in the backtrace which operation causes the crash. >> >> Thanks, >> >> Simon > Hi Simon, > > After thinking about it my comment and code placement wasn't > particularly good. Something along the line's of Luis's change > is better. > > Does Luis's comment address the question you have? > > If so, Luis: Should is it OK we incorporate your changes in the patch? Sure. I have no problems with that.
On 2017-04-28 19:44, Doug Gilmore wrote: > Hi Simon, > > After thinking about it my comment and code placement wasn't > particularly good. Something along the line's of Luis's change > is better. > > Does Luis's comment address the question you have? > > If so, Luis: Should is it OK we incorporate your changes in the patch? > > I attached a diff for the change. > > Thanks, > > Doug Hi Doug, The comment certainly helps, but in the commit log I'd like to see a more detailed list of events that leads to the crash. Now that I look into it again, I think I understand. The objfile_pspace_info::sections array/vector is a list of obj_section pointers (in C++ we'd probably use an std::vector<obj_section*>). That list contains pointers to all the sections from all the objfiles sorted in order of increasing address. They point directly to the sections allocated by the objfile in their obstacks (and accessible through objfile::sections). So when the obstack is freed in reread_symbols, the sorted list contains stale pointers. Is that it? If that's what's happening, then I'm more convinced the fix is right. Is this behaviour caught by a test? If not, could you write one? > > diff --git a/gdb/symfile.c b/gdb/symfile.c > index 846aabe..aff4341 100644 > --- a/gdb/symfile.c > +++ b/gdb/symfile.c > @@ -2628,6 +2628,20 @@ reread_symbols (void) > clear_complaints (&symfile_complaints, 1, 1); > > objfile->flags &= ~OBJF_PSYMTABS_READ; > + > + /* We are about to read new symbols and potentially also DWARF > + information. Some targets may want to pass addresses read from > + DWARF DIE's through an adjustment function before saving them, > like > + MIPS, which may call into "find_pc_section". When called, that > + function will make use of per-objfile program space data. If you are talking about objfile_pspace_info, it's not per-objfile. There's one instance of it per program space, so it's actually "objfiles-related per-program-space data". It contains the sorted list of all sections of all objfiles loaded in the pspace. > + > + Since we discarded our section information above, we have > dangling > + pointers in the per-objfile program space data structure. Force again > + GDB to update the section mapping information by letting it know > + the objfile has changed, making the dangling pointers point to > + correct data again. */ > + objfiles_changed (); > + > read_symbols (objfile, 0); > > if (!objfile_has_symbols (objfile)) > @@ -2660,9 +2674,6 @@ reread_symbols (void) > > if (!new_objfiles.empty ()) > { > - /* Notify objfiles that we've modified objfile sections. */ > - objfiles_changed (); > - > clear_symtab_users (0); > > /* clear_objfile_data for each objfile was called before freeing > it and Thanks, Simon
On 04/28/17 18:41, Simon Marchi wrote: > On 2017-04-28 19:44, Doug Gilmore wrote: >> Hi Simon, >> >> After thinking about it my comment and code placement wasn't >> particularly good. Something along the line's of Luis's change >> is better. >> >> Does Luis's comment address the question you have? >> >> If so, Luis: Should is it OK we incorporate your changes in the patch? >> >> I attached a diff for the change. >> >> Thanks, >> >> Doug > > Hi Doug, > > The comment certainly helps, but in the commit log I'd like to see a > more detailed list of events that leads to the crash. > > Now that I look into it again, I think I understand. The > objfile_pspace_info::sections array/vector is a list of obj_section > pointers (in C++ we'd probably use an std::vector<obj_section*>). > That list contains pointers to all the sections from all the > objfiles sorted in order of increasing address. They point directly > to the sections allocated by the objfile in their obstacks (and > accessible through objfile::sections). So when the obstack is freed > in reread_symbols, the sorted list contains stale pointers. Is that > it? Right. > > If that's what's happening, then I'm more convinced the fix is > right. Is this behaviour caught by a test? If not, could you write > one? > ... I'll need to take a look. Last time I tried I it was more difficult to expose the problem on the native build of GDB. Thanks, Doug
On 2017-04-29 13:12, Doug Gilmore wrote: > On 04/28/17 18:41, Simon Marchi wrote: >> On 2017-04-28 19:44, Doug Gilmore wrote: >>> Hi Simon, >>> >>> After thinking about it my comment and code placement wasn't >>> particularly good. Something along the line's of Luis's change >>> is better. >>> >>> Does Luis's comment address the question you have? >>> >>> If so, Luis: Should is it OK we incorporate your changes in the >>> patch? >>> >>> I attached a diff for the change. >>> >>> Thanks, >>> >>> Doug >> >> Hi Doug, >> >> The comment certainly helps, but in the commit log I'd like to see a >> more detailed list of events that leads to the crash. >> >> Now that I look into it again, I think I understand. The >> objfile_pspace_info::sections array/vector is a list of obj_section >> pointers (in C++ we'd probably use an std::vector<obj_section*>). >> That list contains pointers to all the sections from all the >> objfiles sorted in order of increasing address. They point directly >> to the sections allocated by the objfile in their obstacks (and >> accessible through objfile::sections). So when the obstack is freed >> in reread_symbols, the sorted list contains stale pointers. Is that >> it? > Right. >> >> If that's what's happening, then I'm more convinced the fix is >> right. Is this behaviour caught by a test? If not, could you write >> one? >> ... > I'll need to take a look. Last time I tried I it was more difficult > to expose the problem on the native build of GDB. reread_symbols is called when using the run (run_command_1), attach (attach_post_wait which then calls setup_inferior) and load (load_command) commands. So maybe something like this would reproduce it? - compile test program - launch gdb with test program - touch test program - run Simon
>>> ... >> I'll need to take a look. Last time I tried I it was more difficult >> to expose the problem on the native build of GDB. > > reread_symbols is called when using the run (run_command_1), attach (attach_post_wait which then calls setup_inferior) and load (load_command) commands. So maybe something like this would reproduce it? > > - compile test program > - launch gdb with test program > - touch test program > - run > > Simon > Hi Simon, What I meant was that when I previously did tests with the native build, for some reason the freed data was not being overwritten, or possible written with the same data, at the time read_symbols was called in reread_symbols. Thus problem wasn't exposed before the objfiles_changed is eventually called in reread_symbols. I just did a test with a native build of gdb (7.9.1) and the problem was exposed, so chances are it will be also be exposed with a ToT build. Doug
diff --git a/gdb/symfile.c b/gdb/symfile.c index 846aabe..aff4341 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2628,6 +2628,20 @@ reread_symbols (void) clear_complaints (&symfile_complaints, 1, 1); objfile->flags &= ~OBJF_PSYMTABS_READ; + + /* We are about to read new symbols and potentially also DWARF + information. Some targets may want to pass addresses read from + DWARF DIE's through an adjustment function before saving them, like + MIPS, which may call into "find_pc_section". When called, that + function will make use of per-objfile program space data. + + Since we discarded our section information above, we have dangling + pointers in the per-objfile program space data structure. Force + GDB to update the section mapping information by letting it know + the objfile has changed, making the dangling pointers point to + correct data again. */ + objfiles_changed (); + read_symbols (objfile, 0); if (!objfile_has_symbols (objfile)) @@ -2660,9 +2674,6 @@ reread_symbols (void) if (!new_objfiles.empty ()) { - /* Notify objfiles that we've modified objfile sections. */ - objfiles_changed (); - clear_symtab_users (0); /* clear_objfile_data for each objfile was called before freeing it and