diff mbox

GDB internal error in pc_in_thread_step_range

Message ID 83sgyo8brg.fsf@gnu.org
State New, archived
Headers

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

Simon Marchi Dec. 23, 2018, 11:31 p.m. UTC | #1 
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
Eli Zaretskii Dec. 24, 2018, 4:13 p.m. UTC | #2 
> 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.
Simon Marchi Dec. 24, 2018, 4:29 p.m. UTC | #3 
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
Eli Zaretskii Dec. 28, 2018, 7:09 a.m. UTC | #4 
> 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.
Joel Brobecker Dec. 28, 2018, 10:09 a.m. UTC | #5 
> > >>> --- 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.
Eli Zaretskii Dec. 28, 2018, 10:14 a.m. UTC | #6 
> 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.
diff mbox

Patch

--- 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;