Re: [RFA/stabs] Fix for line table problems (was: Re: [RFC] Gdb line table implementation tweak)

[Jim Blandy <jimb at redhat dot com>]

This looks sound to me.  Let's put it in and see what happens.

Daniel Jacobowitz <drow@mvista.com> writes:

> [This is long.  Feel free to skip to the patch at the very end, and
> stop to take a gander at the testsuite numbers in the middle :)]
> 
> On Wed, Feb 27, 2002 at 04:20:05PM -0700, Fred Fish wrote:
> > > Fred, could you look into these failures?
> > 
> > Yes, but I would need access to a host system that exhibits
> > the failures.  Given a pointer and access to such a system,
> > I can try and see what is going on.
> 
> I've got it.  It's almost exactly like the bug I fixed in binutils
> addr2line a few hours ago.  We do not have an N_SLINE at the beginning
> of the function.  Our starting lines for functions have, as a result,
> always been a little odd...
> 
> So how do we figure out where we are?  There's two ways.  GCC gives us
> the line number of the start of the function in the N_FUN, but other
> compilers don't.  Also, every function should contain at least one
> line number note.  We could assume we have the GCC-style line numbering
> information if we had an ending marker for the function, but that's an
> assumption.  Or we could do it this way: If we are inside a function,
> and we found our first line note, use the start address of the function
> instead of the address on the line note.
> 
> If we do it the first way, we also fix various problems with inlining
> (reported to this list a couple of weeks ago).  But there's an
> assumption in there I really dislike making.  Perhaps we could trigger
> that off processing_gcc_compilation, hackish as it is, and gain a few
> more improvements in addition to this patch.  For now, I went with the
> second way instead.
> 
> Can anyone think of a possible problem?  I suppose this might cause
> more badness with the section-jumping creativity that the Linux kernel
> is so fond of, if it happened to be the very first thing in the
> function, but I believe we should always have a line note before any of
> that happens.
> 
> The results from this patch are very good.  In fact, I was astonished. 
> I'm considering adapting a similar method for DWARF-2; it's less often
> necessary but definitely applicable, because the line table does not
> indicate function boundaries.
> 
> I got the lowest number of unexpected failures I've ever seen with this
> patch.  I've some way to go with C++/Java, and there's the heap of MI
> tests that have been failing as long as I've been running regular
> testsuites, and we still haven't resolved the MAYBE_PROTOTYPED and
> related changes for the eight or ten failures that causes across my
> testsuite runs; but hitting 0 is in sight again now.
> 
> Testsuite numbers (across {gcc 2.95.3, gcc 3.0.4} {-gstabs+, -gdwarf-2}):
> 
> Current CVS, with Fred's original patch reverted:
>                 === gdb Summary ===
> 
> # of expected passes            31991
> # of unexpected failures        358
> # of unexpected successes       108
> # of expected failures          399
> # of unresolved testcases       200
> # of untested testcases         24
> # of unsupported tests          8
> 
> Current CVS, which includes Fred's original patch:
>                 === gdb Summary ===
> 
> # of expected passes            31939
> # of unexpected failures        408
> # of unexpected successes       108
> # of expected failures          392
> # of unresolved testcases       212
> # of untested testcases         24
> # of unsupported tests          8
> 
> With the attached patch:
>                 === gdb Summary ===
> 
> # of expected passes            32120
> # of unexpected failures        270
> # of unexpected successes       108
> # of expected failures          399
> # of unresolved testcases       100
> # of untested testcases         24
> # of unsupported tests          8
> 
> 
> The only worrysome results were (2.95.3,stabs):
> +FAIL: gdb.base/reread.exp: second pass: breakpoint foo in first file
> 
> (2.95.3, dwarf-2)
> +FAIL: gdb.base/scope.exp: running to foo in runto
> +FAIL: gdb.base/scope.exp: running to bar in runto
> +FAIL: gdb.base/scope.exp: setting breakpoint at localscopes
> 
> The reread failure is present with Fred's patch or with Fred's and
> mine; it looks like:
>  Reading symbols from /opt/src/binutils/x86-as/gdb/testsuite/gdb.base/reread...done.
> -Breakpoint 1 at 0x80483df: file ../../../src/gdb/testsuite/gdb.base/reread1.c, line 14.
> +Breakpoint 1 at 0x80483dc
>  (gdb) break foo
> -Note: breakpoint 1 also set at pc 0x80483df.
> -Breakpoint 2 at 0x80483df: file ../../../src/gdb/testsuite/gdb.base/reread1.c, line 14.
> -(gdb) PASS: gdb.base/reread.exp: second pass: breakpoint foo in first file
> +Note: breakpoint 1 also set at pc 0x80483dc.
> +Breakpoint 2 at 0x80483dc
> +(gdb) FAIL: gdb.base/reread.exp: second pass: breakpoint foo in first file
>  run 
>  The program being debugged has been started already.
>  Start it from the beginning? (y or n) y
>  Starting program: /opt/src/binutils/x86-as/gdb/testsuite/gdb.base/reread 
> +Breakpoint 1 at 0x80483df: file ../../../src/gdb/testsuite/gdb.base/reread1.c, line 14.
> +Breakpoint 2 at 0x80483df: file ../../../src/gdb/testsuite/gdb.base/reread1.c, line 14.
> 
> 
> That is, after rereading, we think that 0x80483dc is out of function -
> and we break there rather than at 0x80483df, where we used to.
> 
> 
> scope.exp replaces hitting breakpoint 2 with:
> +Program terminated with signal SIGKILL, Killed.
> +The program no longer exists.
> +You can't do that without a process to debug.
> +(gdb) FAIL: gdb.base/scope.exp: running to foo in runto
> 
> Which appears to have been temporary, since I can't reproduce it any
> more with the exact same binaries.
> 
> We seem to be much better at stopping after the prologue with this
> patch.  And some other breakpoints get earlier.  This is because of
> changes like:
> -Line 13 of "../../../src/gdb/testsuite/gdb.base/ending-run.c" is at
>   address 0x8048496 <callee+6> but contains no code.
> +Line 13 of "../../../src/gdb/testsuite/gdb.base/ending-run.c" starts
>   at address 0x8048490 <callee> and ends at 0x8048496 <callee+6>.
> 
> i.e. the first N_SLINE was placed too late and we now correctly move
> it.  No breakpoints changed PC in the entire testsuite with GCC 3.x, so
> I'm satisfied as to the correctness of the change.  I'd appreciate
> testsuite results with a non-GCC compiler.
> 
> 
> 
> And after all that - OK to commit?
> 
> 
> -- 
> Daniel Jacobowitz                           Carnegie Mellon University
> MontaVista Software                         Debian GNU/Linux Developer
> 
> 2002-03-17  Daniel Jacobowitz  <drow@mvista.com>
> 
> 	* dbxread.c (process_one_symbol): Extend the first N_SLINE
> 	in a function to cover the entire beginning of the function
> 	as well if it does not already.
> 
> Index: dbxread.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dbxread.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 dbxread.c
> --- dbxread.c	2002/02/22 00:17:13	1.30
> +++ dbxread.c	2002/03/17 06:41:21
> @@ -2707,6 +2707,15 @@ process_one_symbol (int type, int desc, 
>       used to relocate these symbol types rather than SECTION_OFFSETS.  */
>    static CORE_ADDR function_start_offset;
>  
> +  /* This holds the address of the start of a function, without the system
> +     peculiarities of function_start_offset.  */
> +  static CORE_ADDR last_function_start;
> +
> +  /* If this is nonzero, we've seen an N_SLINE since the start of the current
> +     function.  Initialized to nonzero to assure that last_function_start
> +     is never used uninitialized.  */
> +  static int sline_found_in_function = 1;
> +
>    /* If this is nonzero, we've seen a non-gcc N_OPT symbol for this source
>       file.  Used to detect the SunPRO solaris compiler.  */
>    static int n_opt_found;
> @@ -2758,9 +2767,13 @@ process_one_symbol (int type, int desc, 
>  	  break;
>  	}
>  
> +      sline_found_in_function = 0;
> +
>        /* Relocate for dynamic loading */
>        valu += ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
>        valu = SMASH_TEXT_ADDRESS (valu);
> +      last_function_start = valu;
> +
>        goto define_a_symbol;
>  
>      case N_LBRAC:
> @@ -2953,7 +2966,15 @@ process_one_symbol (int type, int desc, 
>  #ifdef SUN_FIXED_LBRAC_BUG
>        last_pc_address = valu;	/* Save for SunOS bug circumcision */
>  #endif
> -      record_line (current_subfile, desc, valu);
> +      /* If this is the first SLINE note in the function, record it at
> +	 the start of the function instead of at the listed location.  */
> +      if (within_function && sline_found_in_function == 0)
> +	{
> +	  record_line (current_subfile, desc, last_function_start);
> +	  sline_found_in_function = 1;
> +	}
> +      else
> +	record_line (current_subfile, desc, valu);
>        break;
>  
>      case N_BCOMM: