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

Jim Blandy
2002-03-21T19:03:48Z

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