Message ID | 83sgyo8brg.fsf@gnu.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 73352 invoked by alias); 23 Dec 2018 16:10:46 -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 73216 invoked by uid 89); 23 Dec 2018 16:10:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.2 spammy=Based, stepping, continuing X-HELO: eggs.gnu.org Received: from eggs.gnu.org (HELO eggs.gnu.org) (208.118.235.92) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 23 Dec 2018 16:10:41 +0000 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from <eliz@gnu.org>) id 1gb6Kt-0003Ox-89 for gdb-patches@sourceware.org; Sun, 23 Dec 2018 11:10:40 -0500 Received: from fencepost.gnu.org ([2001:4830:134:3::e]:56159) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <eliz@gnu.org>) id 1gb6Kt-0003Od-4i; Sun, 23 Dec 2018 11:10:39 -0500 Received: from [176.228.60.248] (port=3025 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from <eliz@gnu.org>) id 1gb6Ks-0006NL-OE; Sun, 23 Dec 2018 11:10:39 -0500 Date: Sun, 23 Dec 2018 18:10:27 +0200 Message-Id: <83sgyo8brg.fsf@gnu.org> From: Eli Zaretskii <eliz@gnu.org> To: Simon Marchi <simon.marchi@polymtl.ca> CC: gdb-patches@sourceware.org In-reply-to: <c11ed1e96e5fc8cdca8dd8d417de880f@polymtl.ca> (message from Simon Marchi on Sat, 22 Dec 2018 11:47:07 -0500) Subject: Re: GDB internal error in pc_in_thread_step_range References: <83h8kjr8r6.fsf@gnu.org> <100001f1b27aa7d90902a75d5db37710@polymtl.ca> <83a7m6tk92.fsf@gnu.org> <e1065324-72b2-1a80-fccd-b5624ed9b37c@polymtl.ca> <8336qxfpjo.fsf@gnu.org> <bd38227d-30c4-36de-f12b-f41d11c28027@polymtl.ca> <83tvjde68l.fsf@gnu.org> <f52ef14d-448b-aecd-3714-e3d87c9bc57b@polymtl.ca> <83ftutcy7p.fsf@gnu.org> <659d33b5e4af35aea6c3aaef08559f31@polymtl.ca> <837eg4cick.fsf@gnu.org> <988ca92d2c5c976fbea57c2381eb6279@polymtl.ca> <834lb6ar3g.fsf@gnu.org> <c11ed1e96e5fc8cdca8dd8d417de880f@polymtl.ca> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-IsSubscribed: yes |
Commit Message
Eli Zaretskii
Dec. 23, 2018, 4:10 p.m. UTC
> Date: Sat, 22 Dec 2018 11:47:07 -0500 > From: Simon Marchi <simon.marchi@polymtl.ca> > Cc: gdb-patches@sourceware.org > > >> Huh, interesting. I looked at elfread, and similar undefined symbols > >> are skipped: > >> > >> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/elfread.c;h=71e6fcca6ec62ec57f93f06d8a9913612be6f9e2;hb=HEAD#l270 > > > > So maybe GDB should skip them as well? > > Yes. Can you please give it a try it? Like this? > >> Note that if we implement the solution of rejecting the symbols with > >> section == -1, those mst_abs symbols won't be there anymore. > > > > Fine by me. Should we push such a change? > > Based on what we saw, I would be for it. But you'll need to make the > change and test it for regression, as I don't have the necessary setup > (and knowledge) to do that on Windows. I'm sorry, I don't have such a setup, either. Can anyone else run the regression tests? If not, how are MinGW related changes tested when submitted here? > >> So it kind of works for your use case, but it's not right, IMO. If > >> the > >> process did not exit as it does here, the behavior would be erratic. > > > > I don't think it would be erratic, we will just see the same > > > > 0x00401nnn in __register_frame_info () > > > > for several steps. Is that so bad? > > Well, first thing, I think it's wrong that we show that it's in > __register_frame_info. If this was an actual resolved .text symbol, it > wouldn't be so bad, but here it's not even a function in the program, it > doesn't make sense. The change above causes the following behavior, when stepping out of 'main' with "next": 6 return 0; (gdb) n 7 } (gdb) 0x00401288 in ?? () (gdb) n Cannot find bounds of current function (gdb) c Continuing. [Inferior 1 (process 9228) exited normally] (gdb) q If this is fine with you, it's fine with me. Thanks.
Comments
On 2018-12-23 11:10 a.m., Eli Zaretskii wrote: > Like this? > > --- gdb/coffread.c~1 2018-07-04 18:41:59.000000000 +0300 > +++ gdb/coffread.c 2018-12-23 10:24:15.758116900 +0200 > @@ -874,8 +874,10 @@ coff_symtab_read (minimal_symbol_reader > int section = cs_to_section (cs, objfile); > > tmpaddr = cs->c_value; > - record_minimal_symbol (reader, cs, tmpaddr, mst_text, > - section, objfile); > + /* Don't record unresolved symbols. */ > + if (!(cs->c_secnum <= 0 && cs->c_value == 0)) > + record_minimal_symbol (reader, cs, tmpaddr, mst_text, > + section, objfile); > > fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr; > fcn_start_addr = tmpaddr; That looks good to me. > I'm sorry, I don't have such a setup, either. Can anyone else run the > regression tests? If not, how are MinGW related changes tested when > submitted here? I thought you were the MinGW reference around here :). I guess the changes should be validated the same way we do on GNU/Linux, run the testsuite without and with the patch applied, diff the gdb.sum files to see if there's any regression. I don't know what's the state of the testsuite on Windows though... Running the testsuite is done using "make check". There are some tips on this page: https://sourceware.org/gdb/wiki/TestingGDB > The change above causes the following behavior, when stepping out of > 'main' with "next": > > 6 return 0; > (gdb) n > 7 } > (gdb) > 0x00401288 in ?? () > (gdb) n > Cannot find bounds of current function > (gdb) c > Continuing. > [Inferior 1 (process 9228) exited normally] > (gdb) q > > If this is fine with you, it's fine with me. That seems good to me, given the information GDB works with. Simon
> Cc: gdb-patches@sourceware.org > From: Simon Marchi <simon.marchi@polymtl.ca> > Date: Sun, 23 Dec 2018 18:31:45 -0500 > > On 2018-12-23 11:10 a.m., Eli Zaretskii wrote: > > Like this? > > > > --- gdb/coffread.c~1 2018-07-04 18:41:59.000000000 +0300 > > +++ gdb/coffread.c 2018-12-23 10:24:15.758116900 +0200 > > @@ -874,8 +874,10 @@ coff_symtab_read (minimal_symbol_reader > > int section = cs_to_section (cs, objfile); > > > > tmpaddr = cs->c_value; > > - record_minimal_symbol (reader, cs, tmpaddr, mst_text, > > - section, objfile); > > + /* Don't record unresolved symbols. */ > > + if (!(cs->c_secnum <= 0 && cs->c_value == 0)) > > + record_minimal_symbol (reader, cs, tmpaddr, mst_text, > > + section, objfile); > > > > fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr; > > fcn_start_addr = tmpaddr; > > That looks good to me. Should I push it to the master branch? > > I'm sorry, I don't have such a setup, either. Can anyone else run the > > regression tests? If not, how are MinGW related changes tested when > > submitted here? > > I thought you were the MinGW reference around here :). I'm happy to help with this, but setting up the GDB test suite needs a Cygwin installation as well, and I don't have that. > > The change above causes the following behavior, when stepping out of > > 'main' with "next": > > > > 6 return 0; > > (gdb) n > > 7 } > > (gdb) > > 0x00401288 in ?? () > > (gdb) n > > Cannot find bounds of current function > > (gdb) c > > Continuing. > > [Inferior 1 (process 9228) exited normally] > > (gdb) q > > > > If this is fine with you, it's fine with me. > > That seems good to me, given the information GDB works with. I agree, so I'd like to install this patch.
On 2018-12-24 11:13 a.m., Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org >> From: Simon Marchi <simon.marchi@polymtl.ca> >> Date: Sun, 23 Dec 2018 18:31:45 -0500 >> >> On 2018-12-23 11:10 a.m., Eli Zaretskii wrote: >>> Like this? >>> >>> --- gdb/coffread.c~1 2018-07-04 18:41:59.000000000 +0300 >>> +++ gdb/coffread.c 2018-12-23 10:24:15.758116900 +0200 >>> @@ -874,8 +874,10 @@ coff_symtab_read (minimal_symbol_reader >>> int section = cs_to_section (cs, objfile); >>> >>> tmpaddr = cs->c_value; >>> - record_minimal_symbol (reader, cs, tmpaddr, mst_text, >>> - section, objfile); >>> + /* Don't record unresolved symbols. */ >>> + if (!(cs->c_secnum <= 0 && cs->c_value == 0)) >>> + record_minimal_symbol (reader, cs, tmpaddr, mst_text, >>> + section, objfile); >>> >>> fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr; >>> fcn_start_addr = tmpaddr; >> >> That looks good to me. > > Should I push it to the master branch? Yes, just make sure to include the relevant info in the commit log. It would be a good idea to link back to this thread, so someone doing archeology can find why we added this. > I'm happy to help with this, but setting up the GDB test suite needs a > Cygwin installation as well, and I don't have that. Ok I see. Simon
> Cc: gdb-patches@sourceware.org > From: Simon Marchi <simon.marchi@polymtl.ca> > Date: Mon, 24 Dec 2018 11:29:36 -0500 > > >>> --- gdb/coffread.c~1 2018-07-04 18:41:59.000000000 +0300 > >>> +++ gdb/coffread.c 2018-12-23 10:24:15.758116900 +0200 > >>> @@ -874,8 +874,10 @@ coff_symtab_read (minimal_symbol_reader > >>> int section = cs_to_section (cs, objfile); > >>> > >>> tmpaddr = cs->c_value; > >>> - record_minimal_symbol (reader, cs, tmpaddr, mst_text, > >>> - section, objfile); > >>> + /* Don't record unresolved symbols. */ > >>> + if (!(cs->c_secnum <= 0 && cs->c_value == 0)) > >>> + record_minimal_symbol (reader, cs, tmpaddr, mst_text, > >>> + section, objfile); > >>> > >>> fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr; > >>> fcn_start_addr = tmpaddr; > >> > >> That looks good to me. > > > > Should I push it to the master branch? > > Yes, just make sure to include the relevant info in the commit log. > It would be a good idea to link back to this thread, so someone doing > archeology can find why we added this. Done. Thanks for your help with investigating this issue and with finding the right solution for it.
> > >>> --- gdb/coffread.c~1 2018-07-04 18:41:59.000000000 +0300 > > >>> +++ gdb/coffread.c 2018-12-23 10:24:15.758116900 +0200 > > >>> @@ -874,8 +874,10 @@ coff_symtab_read (minimal_symbol_reader > > >>> int section = cs_to_section (cs, objfile); > > >>> > > >>> tmpaddr = cs->c_value; > > >>> - record_minimal_symbol (reader, cs, tmpaddr, mst_text, > > >>> - section, objfile); > > >>> + /* Don't record unresolved symbols. */ > > >>> + if (!(cs->c_secnum <= 0 && cs->c_value == 0)) > > >>> + record_minimal_symbol (reader, cs, tmpaddr, mst_text, > > >>> + section, objfile); > > >>> > > >>> fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr; > > >>> fcn_start_addr = tmpaddr; > > >> > > >> That looks good to me. > > > > > > Should I push it to the master branch? > > > > Yes, just make sure to include the relevant info in the commit log. > > It would be a good idea to link back to this thread, so someone doing > > archeology can find why we added this. > > Done. > > Thanks for your help with investigating this issue and with finding > the right solution for it. FWIW, I ran this patch through AdaCore's testsuite, on both x86 and x86_64 Windows, and detected do regressions.
> Date: Fri, 28 Dec 2018 14:09:48 +0400 > From: Joel Brobecker <brobecker@adacore.com> > Cc: Simon Marchi <simon.marchi@polymtl.ca>, gdb-patches@sourceware.org > > FWIW, I ran this patch through AdaCore's testsuite, on both > x86 and x86_64 Windows, and detected do regressions. Thank you.
--- gdb/coffread.c~1 2018-07-04 18:41:59.000000000 +0300 +++ gdb/coffread.c 2018-12-23 10:24:15.758116900 +0200 @@ -874,8 +874,10 @@ coff_symtab_read (minimal_symbol_reader int section = cs_to_section (cs, objfile); tmpaddr = cs->c_value; - record_minimal_symbol (reader, cs, tmpaddr, mst_text, - section, objfile); + /* Don't record unresolved symbols. */ + if (!(cs->c_secnum <= 0 && cs->c_value == 0)) + record_minimal_symbol (reader, cs, tmpaddr, mst_text, + section, objfile); fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr; fcn_start_addr = tmpaddr;