| Message ID | 83sgyo8brg.fsf@gnu.org |
|---|---|
| State | New |
| Headers | show |
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;