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

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

[Michael Elizabeth Chastain]

My test bed likes the new gdb HEAD a lot.  I think it would be good to
import into the 5.2 branch.  Report coming soon.

Michael C

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

[Andrew Cagney]

> My test bed likes the new gdb HEAD a lot.  I think it would be good to
> import into the 5.2 branch.  Report coming soon.
> 
> Michael C

Sounds like it is clear for the branch?


Andrew

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

[Daniel Jacobowitz]

On Thu, Mar 28, 2002 at 12:17:34AM -0500, Andrew Cagney wrote:
> >My test bed likes the new gdb HEAD a lot.  I think it would be good to
> >import into the 5.2 branch.  Report coming soon.
> >
> >Michael C
> 
> Sounds like it is clear for the branch?

I think so.  Want me to wait for Michael C's report first?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

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

[Jim Blandy]

Daniel Jacobowitz <drow@mvista.com> writes:
> I think so.  Want me to wait for Michael C's report first?

If doing so wouldn't delay 5.2 too much, yes.

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

[Andrew Cagney]

> Daniel Jacobowitz <drow@mvista.com> writes:
> 
>> I think so.  Want me to wait for Michael C's report first?
> 
> 
> If doing so wouldn't delay 5.2 too much, yes.

It won't - this is on the list of things I'm trying to get fixed.  Check 
the current list of high priority PRs of which MichaelC created three.

enjoy,
Andrew

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

[Jim Blandy]

Daniel Jacobowitz <dmj+@andrew.cmu.edu> writes:
> On Fri, Mar 29, 2002 at 03:59:13PM -0500, Jim Blandy wrote:
> > 
> > Daniel Jacobowitz <drow@mvista.com> writes:
> > > I think so.  Want me to wait for Michael C's report first?
> > 
> > If doing so wouldn't delay 5.2 too much, yes.
> 
> OK, I will.  Could you look over:
>   http://sources.redhat.com/ml/gdb-patches/2002-03/msg00415.html
> (the equivalent patch for DWARF-2), if you get a chance?

So, it's not that the first line number marker is *missing*, it's that
it's *misplaced*.  So repositioning the line is sufficient --- we
don't need to make up an extra entry.  Is that right?

If so, it seems fine, some minor comments:

Could you move the code that initializes the function range list and
the code that adds a new entry to the function range list into their
own functions?

Could check_cu_functions complain when it has to back up a line entry?

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

[Daniel Jacobowitz]

On Thu, Apr 04, 2002 at 04:51:27PM -0500, Jim Blandy wrote:
> 
> Daniel Jacobowitz <dmj+@andrew.cmu.edu> writes:
> > On Fri, Mar 29, 2002 at 03:59:13PM -0500, Jim Blandy wrote:
> > > 
> > > Daniel Jacobowitz <drow@mvista.com> writes:
> > > > I think so.  Want me to wait for Michael C's report first?
> > > 
> > > If doing so wouldn't delay 5.2 too much, yes.
> > 
> > OK, I will.  Could you look over:
> >   http://sources.redhat.com/ml/gdb-patches/2002-03/msg00415.html
> > (the equivalent patch for DWARF-2), if you get a chance?
> 
> So, it's not that the first line number marker is *missing*, it's that
> it's *misplaced*.  So repositioning the line is sufficient --- we
> don't need to make up an extra entry.  Is that right?

Yes, that's right.  It's much easier to add an extra entry - but that
causes problems all over GDB.  Trust me, I tried it :)

> If so, it seems fine, some minor comments:
> 
> Could you move the code that initializes the function range list and
> the code that adds a new entry to the function range list into their
> own functions?

Sure.

> Could check_cu_functions complain when it has to back up a line entry?

Sure.

I've committed this version, thanks!  We'll see if we have time to move
this one to the branch later.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2002-04-04  Daniel Jacobowitz  <drow@mvista.com>

	* dwarf2read.c (struct function_range): New.
	(cu_first_fn, cu_last_fn, cu_cached_fn): New.
	(check_cu_functions): New.
	(read_file_scope): Initialize global function lists.
	Call dwarf_decode_line after processing children.
	(read_func_scope): Add to global function list.
	(dwarf_decode_lines): Call check_cu_functions everywhere
	record_line is called.  Call record_line with a linenumber
	of 0 to mark sequence ends.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.51
diff -u -p -r1.51 dwarf2read.c
--- dwarf2read.c	2002/03/21 00:53:44	1.51
+++ dwarf2read.c	2002/04/04 22:23:19
@@ -256,6 +256,16 @@ struct attribute
     u;
   };
 
+struct function_range
+{
+  const char *name;
+  CORE_ADDR lowpc, highpc;
+  int seen_line;
+  struct function_range *next;
+};
+
+static struct function_range *cu_first_fn, *cu_last_fn, *cu_cached_fn;
+
 /* Get at parts of an attribute structure */
 
 #define DW_STRING(attr)    ((attr)->u.str)
@@ -560,6 +570,10 @@ static struct complaint dwarf2_unsupport
 {
   "unsupported const value attribute form: '%s'", 0, 0
 };
+static struct complaint dwarf2_misplaced_line_number =
+{
+  "misplaced first line number at 0x%lx for '%s'", 0, 0
+};
 
 /* local function prototypes */
 
@@ -794,6 +808,10 @@ static struct abbrev_info *dwarf_alloc_a
 
 static struct die_info *dwarf_alloc_die (void);
 
+static void initialize_cu_func_list (void);
+
+static void add_to_cu_func_list (const char *, CORE_ADDR, CORE_ADDR);
+
 /* Try to locate the sections we need for DWARF 2 debugging
    information and return true if we have enough to do something.  */
 
@@ -1542,6 +1560,12 @@ process_die (struct die_info *die, struc
 }
 
 static void
+initialize_cu_func_list (void)
+{
+  cu_first_fn = cu_last_fn = cu_cached_fn = NULL;
+}
+
+static void
 read_file_scope (struct die_info *die, struct objfile *objfile,
 		 const struct comp_unit_head *cu_header)
 {
@@ -1633,13 +1657,7 @@ read_file_scope (struct die_info *die, s
   start_symtab (name, comp_dir, lowpc);
   record_debugformat ("DWARF 2");
 
-  /* Decode line number information if present.  */
-  attr = dwarf_attr (die, DW_AT_stmt_list);
-  if (attr)
-    {
-      line_offset = DW_UNSND (attr);
-      dwarf_decode_lines (line_offset, comp_dir, abfd, cu_header);
-    }
+  initialize_cu_func_list ();
 
   /* Process all dies in compilation unit.  */
   if (die->has_children)
@@ -1651,9 +1669,38 @@ read_file_scope (struct die_info *die, s
 	  child_die = sibling_die (child_die);
 	}
     }
+
+  /* Decode line number information if present.  */
+  attr = dwarf_attr (die, DW_AT_stmt_list);
+  if (attr)
+    {
+      line_offset = DW_UNSND (attr);
+      dwarf_decode_lines (line_offset, comp_dir, abfd, cu_header);
+    }
 }
 
 static void
+add_to_cu_func_list (const char *name, CORE_ADDR lowpc, CORE_ADDR highpc)
+{
+  struct function_range *thisfn;
+
+  thisfn = (struct function_range *)
+    obstack_alloc (&dwarf2_tmp_obstack, sizeof (struct function_range));
+  thisfn->name = name;
+  thisfn->lowpc = lowpc;
+  thisfn->highpc = highpc;
+  thisfn->seen_line = 0;
+  thisfn->next = NULL;
+
+  if (cu_last_fn == NULL)
+      cu_first_fn = thisfn;
+  else
+      cu_last_fn->next = thisfn;
+
+  cu_last_fn = thisfn;
+}
+
+static void
 read_func_scope (struct die_info *die, struct objfile *objfile,
 		 const struct comp_unit_head *cu_header)
 {
@@ -1674,6 +1721,9 @@ read_func_scope (struct die_info *die, s
   lowpc += baseaddr;
   highpc += baseaddr;
 
+  /* Record the function range for dwarf_decode_lines.  */
+  add_to_cu_func_list (name, lowpc, highpc);
+
   if (objfile->ei.entry_point >= lowpc &&
       objfile->ei.entry_point < highpc)
     {
@@ -3908,6 +3958,51 @@ struct directories
     char **dirs;
   };
 
+/* This function exists to work around a bug in certain compilers
+   (particularly GCC 2.95), in which the first line number marker of a
+   function does not show up until after the prologue, right before
+   the second line number marker.  This function shifts ADDRESS down
+   to the beginning of the function if necessary, and is called on
+   addresses passed to record_line.  */
+
+static CORE_ADDR
+check_cu_functions (CORE_ADDR address)
+{
+  struct function_range *fn;
+
+  /* Find the function_range containing address.  */
+  if (!cu_first_fn)
+    return address;
+
+  if (!cu_cached_fn)
+    cu_cached_fn = cu_first_fn;
+
+  fn = cu_cached_fn;
+  while (fn)
+    if (fn->lowpc <= address && fn->highpc > address)
+      goto found;
+    else
+      fn = fn->next;
+
+  fn = cu_first_fn;
+  while (fn && fn != cu_cached_fn)
+    if (fn->lowpc <= address && fn->highpc > address)
+      goto found;
+    else
+      fn = fn->next;
+
+  return address;
+
+ found:
+  if (fn->seen_line)
+    return address;
+  if (address != fn->lowpc)
+    complain (&dwarf2_misplaced_line_number,
+	      (unsigned long) address, fn->name);
+  fn->seen_line = 1;
+  return fn->lowpc;
+}
+
 static void
 dwarf_decode_lines (unsigned int offset, char *comp_dir, bfd *abfd,
 		    const struct comp_unit_head *cu_header)
@@ -4048,6 +4143,7 @@ dwarf_decode_lines (unsigned int offset,
 		* lh.minimum_instruction_length;
 	      line += lh.line_base + (adj_opcode % lh.line_range);
 	      /* append row to matrix using current values */
+	      address = check_cu_functions (address);
 	      record_line (current_subfile, line, address);
 	      basic_block = 1;
 	    }
@@ -4061,12 +4157,7 @@ dwarf_decode_lines (unsigned int offset,
 		{
 		case DW_LNE_end_sequence:
 		  end_sequence = 1;
-		  /* Don't call record_line here.  The end_sequence
-		     instruction provides the address of the first byte
-		     *after* the last line in the sequence; it's not the
-		     address of any real source line.  However, the GDB
-		     linetable structure only records the starts of lines,
-		     not the ends.  This is a weakness of GDB.  */
+		  record_line (current_subfile, 0, address);
 		  break;
 		case DW_LNE_set_address:
 		  address = read_address (abfd, line_ptr, cu_header, &bytes_read);
@@ -4103,6 +4194,7 @@ dwarf_decode_lines (unsigned int offset,
 		}
 	      break;
 	    case DW_LNS_copy:
+	      address = check_cu_functions (address);
 	      record_line (current_subfile, line, address);
 	      basic_block = 0;
 	      break;

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

[Daniel Jacobowitz]

On Wed, Apr 03, 2002 at 04:19:11PM -0500, Andrew Cagney wrote:
> >Daniel Jacobowitz <drow@mvista.com> writes:
> >
> >>I think so.  Want me to wait for Michael C's report first?
> >
> >
> >If doing so wouldn't delay 5.2 too much, yes.
> 
> It won't - this is on the list of things I'm trying to get fixed.  Check 
> the current list of high priority PRs of which MichaelC created three.

Since Michael C's report looked pretty good, and the patch hasn't done
anyone any harm yet, I've moved it to the release branch.

2002-04-04  Daniel Jacobowitz  <drow@mvista.com>

        Merge from trunk:

        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.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

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

[Michael Elizabeth Chastain]

I'll do my weekly pull this evening (2002-03-29) and post the
analysis and tables tomorrow afternoon.

If the "step" fixes are in the branch then I will post a detailed
5.1.1 versus 5.2 branch analysis on Sunday.

I've been busy at my day job so I haven't kept up with the
gdb mailing lists lately.

Michael C

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

[Jim Blandy]

Andrew Cagney <ac131313@cygnus.com> writes:
> > This looks sound to me.  Let's put it in and see what happens.
> 
> Was that trunk or branch?  I can understand a suck-it-and-see approach
> for the trunk but not for the branch :-/

I have to admit, changes to this area of code have further-reaching
effects than I usually expect.  Definitely for the trunk.  Perhaps if
someone tracks down the regressions that Daniel noted, we could
include it in the branch, too.

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

[Andrew Cagney]

> On Thu, Mar 21, 2002 at 02:03:44PM -0500, Jim Blandy wrote:
> 
>> 
>> This looks sound to me.  Let's put it in and see what happens.
> 
> 
> Committed.  If everything seems OK in a little while (a week, say?)
> I'll move it to the branch.
> 
> As I said, I was going to do the exact same thing for DWARF-2. 
> Unfortunately... this is hard to do efficiently.  We don't process
> functions at the same time as line numbers.  For working around a
> compiler bug, it's probably not worthwhile.

It needs to at least get past MichaelC's testing :-)

I think I'm going to slip 5.2 by a week (to 2002-04-14ish).  Mainly 
because one weekend before (2002-03-31) - when I should seeing that this 
sort of stuff gets pulled in - I'll be on holidays :-)

Andrew

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

[Daniel Jacobowitz]

On Thu, Mar 21, 2002 at 02:03:44PM -0500, Jim Blandy wrote:
> 
> This looks sound to me.  Let's put it in and see what happens.

Committed.  If everything seems OK in a little while (a week, say?)
I'll move it to the branch.

As I said, I was going to do the exact same thing for DWARF-2. 
Unfortunately... this is hard to do efficiently.  We don't process
functions at the same time as line numbers.  For working around a
compiler bug, it's probably not worthwhile.

I could do the equivalent of Fred's original patch for DWARF-2, at
DW_LNS_end_sequence; there's a comment there saying we can not record
the end of a line, but now we can.  On the other hand, it will
introduce the same problem with broken debug output that we observed in
stabs.  So I'm not going to do that until I think of an efficient way
to handle it.  Despite the clutter, I'd prefer to deal with this error
gracefully, now that it's "out there" in quantity.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

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

[Andrew Cagney]

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

Was that trunk or branch?  I can understand a suck-it-and-see approach 
for the trunk but not for the branch :-/

Andrew

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

[Jim Blandy]

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:

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

[Jim Blandy]

Andrew Cagney <ac131313@cygnus.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...
> 
> Jim, would you be able to bump this one up a bit on your things to do
> list?  I believe it addresses the regressions MichaelC detected just
> prior to 5.2 being branched and is on the high priority list.

Yes, I've been sort of panicing this week, but I think this might be
valuable to the project I'm currenty panicing about.  I'll probably
get to it today.

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

[Andrew Cagney]

> [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...

Jim, would you be able to bump this one up a bit on your things to do 
list?  I believe it addresses the regressions MichaelC detected just 
prior to 5.2 being branched and is on the high priority list.

Andrew

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

[Daniel Jacobowitz]

On Sun, Mar 17, 2002 at 01:48:16AM -0500, Daniel Jacobowitz wrote:
> 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.

By the way, this fixes PRs 379, 380, 381.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

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

[Daniel Jacobowitz]

[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: