[RFA] Fix "break foo" when `foo's prologue ends before line table

[RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

With the following test program (tchset.c)

    #include <stdio.h>
    #include <libcharset.h>

    int main (void)
    {
      const char *cset = locale_charset ();

      if (cset)
	printf ("locale_charset: `%s'\n", cset);
      else
	printf ("locale_charset: NULL\n");

      return 0;
    }

and the DJGPP build of a recent snapshot, trying to set a breakpoint
in `main' makes GDB think `main' has no source line info, if I compile
the program with COFF debug info ("gcc -gcoff -o tchset44c.exe tchset.c").

Observe:

  D:\usr\djgpp\gnu\gdb-68.410\gdb>.\gdb.exe --nx ./tchset44c.exe
  GNU gdb (GDB) 6.8.50.20090410
  Copyright (C) 2009 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "--host=i586-pc-msdosdjgpp --target=djgpp".
  For bug reporting instructions, please see:
  <http://www.gnu.org/software/gdb/bugs/>...
  (gdb) break main
  Breakpoint 1 at 0x1732
  (gdb) 

With GDB 6.1, it does work:

  GNU gdb 6.1
  Copyright 2004 Free Software Foundation, Inc.
  GDB is free software, covered by the GNU General Public License, and you are
  welcome to change it and/or distribute copies of it under certain conditions.
  Type "show copying" to see the conditions.
  There is absolutely no warranty for GDB.  Type "show warranty" for details.
  This GDB was configured as "--host=i386-pc-msdosdjgpp --target=djgpp"...
  (gdb) break main
  Breakpoint 1 at 0x1748: file tchset.c, line 6.

The reason is this change:

  2006-11-28  Daniel Jacobowitz  <dan@codesourcery.com>

	  * symtab.c (find_pc_sect_line): Do not return a line before
	  the start of a symtab.

(No, I'm not suggesting to revert that change.  I want to show my
analysis of this problem and suggest a solution.)

Using "maint print symbols", I see this:

  Symtab for file tchset.c
  Read from object file d:/usr/djgpp/gnu/gdb-68.410/gdb/./tchset44c.exe (0x43be20)
  Language: c

  Line table:

   line 6 at 0x1748
   line 8 at 0x1750
   line 9 at 0x1756
   line 11 at 0x176b
   line 13 at 0x177b
   line 14 at 0x1780

  Blockvector:

  block #000, object at 0x4403fc, 1 syms/buckets in 0x1700..0x1782
   int main(); block object 0x440394, 0x172c..0x1782 section .text
    block #001, object at 0x4403c4 under 0x4403fc, 2 syms/buckets in 0x1700..0x1782
  [...]
      block #002, object at 0x440394 under 0x4403c4, 0 syms/buckets in 0x172c..0x1782, function main

Note that the lineinfo table for the program starts at line 6 at PC
0x1748, whereas the function `main' begins at PC 0x172c.

When I type "break main", we eventually call find_function_start_sal,
which skips the prologue, and then calls find_pc_sect_line to find the
sal for the PC after the prologue.  But skipping the prologue in this
cases gets us to PC 0x1732, which is still outside the lineinfo
table.  So find_pc_sect_line fails to find a suitable source line,
returns a sal without a symtab, and GDB then thinks `main' is a
function with no source line information.

Here's the disassembly of `main', in case someone thinks that
skip_prologue function didn't do a good job (I think it did a
reasonable job, but I'm far from being an expert on prologue
analysis):

    Dump of assembler code for function main:
    0x0000172c <main+0>:    push   %ebp
    0x0000172d <main+1>:    mov    %esp,%ebp
    0x0000172f <main+3>:    sub    $0x8,%esp
    0x00001732 <main+6>:    and    $0xfffffff0,%esp
    0x00001735 <main+9>:    mov    $0x0,%eax
    0x0000173a <main+14>:   add    $0xf,%eax
    0x0000173d <main+17>:   add    $0xf,%eax
    0x00001740 <main+20>:   shr    $0x4,%eax
    0x00001743 <main+23>:   shl    $0x4,%eax
    0x00001746 <main+26>:   sub    %eax,%esp
    0x00001748 <main+28>:   call   0x17e0 <locale_charset>
    0x0000174d <main+33>:   mov    %eax,0xfffffffc(%ebp)
    0x00001750 <main+36>:   cmpl   $0x0,0xfffffffc(%ebp)
    0x00001754 <main+40>:   je     0x176b <main+63>
    0x00001756 <main+42>:   sub    $0x8,%esp
    0x00001759 <main+45>:   pushl  0xfffffffc(%ebp)
    0x0000175c <main+48>:   push   $0x1700
    0x00001761 <main+53>:   call   0x3950 <printf>
    0x00001766 <main+58>:   add    $0x10,%esp
    0x00001769 <main+61>:   jmp    0x177b <main+79>
    0x0000176b <main+63>:   sub    $0xc,%esp
    0x0000176e <main+66>:   push   $0x1716
    0x00001773 <main+71>:   call   0x3950 <printf>
    0x00001778 <main+76>:   add    $0x10,%esp
    0x0000177b <main+79>:   mov    $0x0,%eax
    0x00001780 <main+84>:   leave
    0x00001781 <main+85>:   ret

If I compile the same program with -gdwarf-2, the problem does not
happen because the lineinfo table now includes one more entry, for
line 5 of the file.  But I cannot deprecate usage of COFF debug info
in DJGPP, because that is the only way to build the DJGPP port of
Emacs (the code in Emacs's unexec.c was never updated to handle
DWARF-2, and probably never will).

My suggestion for solving this is in the patch below.  The idea is
that we make one more attempt to find the first line of the function's
body, using the lineinfo table.  We loop over all symtabs for the
function's file, looking for an entry in a lineinfo table whose PC is
in the range [FUNC_START..FUNC_END] and whose line number is the
smallest.  FUNC_START and FUNC_END are obtained via a call to
find_pc_partial_function.

Is this idea reasonable?  Is the patch okay to commit?

2009-05-09  Eli Zaretskii  <eliz@gnu.org>

	* symtab.c (skip_prologue_using_lineinfo): New function.
	(find_function_start_sal): Use it to get to the first line of
	function's body that has an entry in the lineinfo table.

--- symtab.c~0	2009-04-01 02:21:07.000000000 +0300
+++ symtab.c	2009-05-09 16:40:36.442027000 +0300
@@ -2599,6 +2599,68 @@
   return pc;
 }
 
+/* Given a function start address FUNC_ADDR and SYMTAB, find the first
+   address for that function that has an entry in any line info table
+   that comes from any symtab for the same file as SYMTAB.  If such an
+   entry cannot be found, return FUNC_ADDR unaltered.  */
+CORE_ADDR
+skip_prologue_using_lineinfo (CORE_ADDR func_addr, struct symtab *symtab)
+{
+  CORE_ADDR func_start, func_end;
+  struct objfile *objfile;
+  struct partial_symtab *p;
+  struct symtab *s;
+  int best_lineno = 0;
+  CORE_ADDR best_pc = func_addr;
+
+  /* Get the range for the function's PC values, or give up if we
+     cannot, for some reason.  */
+  if (!find_pc_partial_function (func_addr, NULL, &func_start, &func_end))
+    return func_addr;
+
+  ALL_PSYMTABS (objfile, p)
+    {
+      if (FILENAME_CMP (symtab->filename, p->filename) != 0)
+	continue;
+      PSYMTAB_TO_SYMTAB (p);
+    }
+
+  /* Loop over all symtabs for the function's file, looking for an
+     entry in a lineinfo table whose PC is in the range
+     [FUNC_START..FUNC_END] and whose line number is the smallest.  */
+  ALL_SYMTABS (objfile, s)
+    {
+      struct linetable *l;
+      int ind, i, len;
+
+      if (FILENAME_CMP (symtab->filename, s->filename) != 0)
+	continue;
+      l = LINETABLE (s);
+      if (l == NULL)
+	continue;
+
+      len = l->nitems;
+      for (i = 0; i < len; i++)
+	{
+	  struct linetable_entry *item = &(l->item[i]);
+
+	  /* Don't use line numbers of zero, they mark special entries
+	     in the table.  See the commentary on symtab.h before the
+	     definition of struct linetable.  */
+	  if (item->line > 0
+	      && func_start <= item->pc && item->pc <= func_end)
+	    {
+	      if (!best_lineno || item->line < best_lineno)
+		{
+		  best_lineno = item->line;
+		  best_pc = item->pc;
+		}
+	    }
+	}
+    }
+  return best_pc;
+}
+
 /* Given a function symbol SYM, find the symtab and line for the start
    of the function.
    If the argument FUNFIRSTLINE is nonzero, we want the first line
@@ -2649,6 +2711,15 @@
       sal = find_pc_sect_line (pc, SYMBOL_OBJ_SECTION (sym), 0);
     }
 
+  /* If we still don't have a valid source line, try to find the first
+     line in the lineinfo table that belongs to the same function.  */
+  if (funfirstline && sal.symtab == NULL)
+    {
+      pc = skip_prologue_using_lineinfo (pc, SYMBOL_SYMTAB (sym));
+      /* Recalculate the line number.  */
+      sal = find_pc_sect_line (pc, SYMBOL_OBJ_SECTION (sym), 0);
+    }
+
   sal.pc = pc;
 
   return sal;

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Joel Brobecker]

>   Line table:
> 
>    line 6 at 0x1748
>    line 8 at 0x1750
>    line 9 at 0x1756
>    line 11 at 0x176b
>    line 13 at 0x177b
>    line 14 at 0x1780
[...]
> Here's the disassembly of `main', in case someone thinks that
> skip_prologue function didn't do a good job (I think it did a
> reasonable job, but I'm far from being an expert on prologue
> analysis):

Here is what the prologue looks like:

>     Dump of assembler code for function main:
>     0x0000172c <main+0>:    push   %ebp
>     0x0000172d <main+1>:    mov    %esp,%ebp
>     0x0000172f <main+3>:    sub    $0x8,%esp
>     0x00001732 <main+6>:    and    $0xfffffff0,%esp
[this is where GDB thinks the prologue stops]
>     0x00001735 <main+9>:    mov    $0x0,%eax
>     0x0000173a <main+14>:   add    $0xf,%eax
>     0x0000173d <main+17>:   add    $0xf,%eax
>     0x00001740 <main+20>:   shr    $0x4,%eax
>     0x00001743 <main+23>:   shl    $0x4,%eax
>     0x00001746 <main+26>:   sub    %eax,%esp
[this is the last instruction before the first line of code
according to the line table]

Regarding your question above, it really depends on whether you
consider the second part of the code above as prologue or not.
What the code above does is making sure that the stack is properly
aligned. I am sure that some people would want to consider this
as part of the prologue, and I wouldn't disagree, but others
could argue the opposite, and I wouldn't disagree either.

Regardless of that, however, we should really look at what
"break FUNCTION" is supposed to be doing. And looking at the doc,
it says: ``Specifies the line that begins the body of the function''
(this matches what I thought it should be doing intuitively).
So, regardless of what GDB should be doing in terms of prologue
analysis, I think we should still try to find the first line
as you're doing in your patch.

> +  ALL_PSYMTABS (objfile, p)
> +    {
> +      if (FILENAME_CMP (symtab->filename, p->filename) != 0)
> +	continue;
> +      PSYMTAB_TO_SYMTAB (p);
> +    }
> +
> +  /* Loop over all symtabs for the function's file, looking for an
> +     entry in a lineinfo table whose PC is in the range
> +     [FUNC_START..FUNC_END] and whose line number is the smallest.  */
> +  ALL_SYMTABS (objfile, s)

I am wondering if this looping over all PSYMTAB and SYMTABs is really
necessary. Is the symtab associated to your symbol not sufficient?

Also, instead of returning the line whose number is the smallest,
I would return the smallest PC, as we're trying to skip the minimum
before inserting the breakpoint. This means that your iteration on
the line table can stop as soon as you've found a non-zero line
that's inside your function address range.
-- 
Joel

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

> Date: Mon, 11 May 2009 14:56:44 +0200
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> Regardless of that, however, we should really look at what
> "break FUNCTION" is supposed to be doing. And looking at the doc,
> it says: ``Specifies the line that begins the body of the function''
> (this matches what I thought it should be doing intuitively).
> So, regardless of what GDB should be doing in terms of prologue
> analysis, I think we should still try to find the first line
> as you're doing in your patch.

Thanks for the feedback.  That's two in favor, none against. ;-)

> > +  ALL_PSYMTABS (objfile, p)
> > +    {
> > +      if (FILENAME_CMP (symtab->filename, p->filename) != 0)
> > +	continue;
> > +      PSYMTAB_TO_SYMTAB (p);
> > +    }
> > +
> > +  /* Loop over all symtabs for the function's file, looking for an
> > +     entry in a lineinfo table whose PC is in the range
> > +     [FUNC_START..FUNC_END] and whose line number is the smallest.  */
> > +  ALL_SYMTABS (objfile, s)
> 
> I am wondering if this looping over all PSYMTAB and SYMTABs is really
> necessary. Is the symtab associated to your symbol not sufficient?

I must admit that I have only a very basic knowledge of symbol tables.
In particular, I'm only vaguely familiar with the possible intricacies
of symtabs in the presence of included files and such likes.  I simply
saw that find_line_symtab, which does a similar job, loops like that,
so I used the same paradigm.

> Also, instead of returning the line whose number is the smallest,
> I would return the smallest PC, as we're trying to skip the minimum
> before inserting the breakpoint.

But the smallest PC could come from some source line that is further
down in the function's body, source-wise, if the compiler rearranged
code, couldn't it?  What I'm trying to do is find the first source
line of the body of the function, not the first PC of the body.  I
think the former is more in line with the semantics of "break FOO".

> This means that your iteration on the line table can stop as soon as
> you've found a non-zero line that's inside your function address
> range.

Is it guaranteed that the line table is always sorted by PC?

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Joel Brobecker]

> > I am wondering if this looping over all PSYMTAB and SYMTABs is really
> > necessary. Is the symtab associated to your symbol not sufficient?
> 
> I must admit that I have only a very basic knowledge of symbol tables.
> In particular, I'm only vaguely familiar with the possible intricacies
> of symtabs in the presence of included files and such likes.  I simply
> saw that find_line_symtab, which does a similar job, loops like that,
> so I used the same paradigm.

I think that the situation in this case is a little bit different,
since we're using the symtab coming from the symbol itself. You'd
expect that the associated linetable would be in that symtab...
I'm always reluctant to introduce code I don't understand, and usually
leave it out until I see a bug - that's why I was asking. Have you
tried without this part?

> > Also, instead of returning the line whose number is the smallest,
> > I would return the smallest PC, as we're trying to skip the minimum
> > before inserting the breakpoint.
> 
> But the smallest PC could come from some source line that is further
> down in the function's body, source-wise, if the compiler rearranged
> code, couldn't it?  What I'm trying to do is find the first source
> line of the body of the function, not the first PC of the body.  I
> think the former is more in line with the semantics of "break FOO".

I would disagree with that. Imagine that you have function like this:

   foo ()
   {
     a ();
     b ();
   }

If for some reason the optimizer rearanged the code like this:

   foo ()
   {
     b ();
     a ();
   }

Do you really want "break foo" to break on the line where a () is
called? The problem with that is that, by the time we reach the
breakpoint, b would have already been called. My expection is that
the debugger should find the first line of code, not the line
whose number is the smallest. In other words, when I break on
a function, I expect that by the time I reach that function breakpoint,
none of the real code has been executed yet - I want to debug the
function :-).

> > This means that your iteration on the line table can stop as soon as
> > you've found a non-zero line that's inside your function address
> > range.
> 
> Is it guaranteed that the line table is always sorted by PC?

Yep:

/* The order of entries in the linetable is significant.  They should
   be sorted by increasing values of the pc field.

-- 
Joel

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Daniel Jacobowitz]

On Sat, May 09, 2009 at 05:26:16PM +0300, Eli Zaretskii wrote:
> Note that the lineinfo table for the program starts at line 6 at PC
> 0x1748, whereas the function `main' begins at PC 0x172c.

Is this really what the coff file says?  If so, it is a bug in GCC.
If we're at 0x172c, GDB should be able to show which line we're on,
and we're definitely on some line of main.

Is it possible to patch this up in the coff line table reader?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

> Date: Mon, 11 May 2009 15:42:47 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> On Sat, May 09, 2009 at 05:26:16PM +0300, Eli Zaretskii wrote:
> > Note that the lineinfo table for the program starts at line 6 at PC
> > 0x1748, whereas the function `main' begins at PC 0x172c.
> 
> Is this really what the coff file says?

I would think so.  What I show in my mail comes from "maint print
symbols", so I have no reason to believe the lineinfo table does not
reflect the COFF debug info.

Here's the relevant part of "objdump -t", as another data point:

  [ 56](sec  1)(fl 0x00)(ty  24)(scl   2) (nx 1) 0x0000172c _main
  AUX tagndx 0 ttlsiz 0x56 lnnos 66672 next 64
  _main :
     2 : 00001748
     4 : 00001750
     5 : 00001756
     7 : 0000176b
     9 : 0000177b
    10 : 00001780
  [ 58](sec  1)(fl 0x00)(ty   0)(scl 101) (nx 1) 0x0000172c .bf
  AUX lnno 5 size 0x0 tagndx 0

> If so, it is a bug in GCC.  If we're at 0x172c, GDB should be able
> to show which line we're on, and we're definitely on some line of
> main.

Maybe.  But the lines before that are just decorations, from the
source code POV, they are not function body.  The code generated by
them is actually the prologue, isn't it?

> Is it possible to patch this up in the coff line table reader?

What, by inventing extra entries in the table?  That could be
dangerous, since we would be doing that without any clear idea of the
code between 0x172c and 0x1748.  Even if we could, is that really
better than the approach I suggested?  We already skip the prologue
when looking for the first line of the function's body, even before
looking in the lineinfo table; what I suggest simply uses yet another
possible definition of that first line, when the other definitions
fail.

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

> Date: Mon, 11 May 2009 21:27:09 +0200
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> I'm always reluctant to introduce code I don't understand, and usually
> leave it out until I see a bug - that's why I was asking. Have you
> tried without this part?

No, but I will.  Keep in mind, though, that the program which I was
using as my test case is just a toy test case.  Even if it works
there, I cannot be sure it will work in more complex cases.  I will
take your word for it, though ;-)

>    foo ()
>    {
>      a ();
>      b ();
>    }
> 
> If for some reason the optimizer rearanged the code like this:
> 
>    foo ()
>    {
>      b ();
>      a ();
>    }

Are such rearrangements permitted?  I mean, are we talking about a
real-life situation here?

> Do you really want "break foo" to break on the line where a () is
> called?

It's hard to say, really.  There are arguments for both, but I
personally tend to think that stopping on the call to `a' is what I'd
want.

What do others think?

>                                        In other words, when I break on
> a function, I expect that by the time I reach that function breakpoint,
> none of the real code has been executed yet - I want to debug the
> function :-).

This could be impossible anyway, if the compiler moves some of the
body into the prologue, right?

> > Is it guaranteed that the line table is always sorted by PC?
> 
> Yep:
> 
> /* The order of entries in the linetable is significant.  They should
>    be sorted by increasing values of the pc field.

Well, granted, I've seen that comment.  But (a) are we sure all of our
comments are necessarily accurate to rely on them?, and (b) it
continues to say

                                                If there is more than
   one entry for a given pc, then I'm not sure what should happen (and
   I not sure whether we currently handle it the best way).

Not very reassuring...

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Joel Brobecker]

> I would think so.  What I show in my mail comes from "maint print
> symbols", so I have no reason to believe the lineinfo table does not
> reflect the COFF debug info.

You should be able to find another confirmation of this by looking
at the .s file.  I don't know what assembler directive is used on
DJGPP to emit lines, but it's usually straightforward.

> Maybe.  But the lines before that are just decorations, from the
> > Is it possible to patch this up in the coff line table reader?
> 
> What, by inventing extra entries in the table?  That could be
> dangerous, since we would be doing that without any clear idea of the
> code between 0x172c and 0x1748.

I don't think it matters what the code does, since otherwise you
would skip that code before inserting the breakpoint anyway.

> Even if we could, is that really better than the approach I suggested?

Not sure if this is better or not. The advantage of this approach is
that we protect other platforms from this form of debugging info -
one could argue that it's incomplete.

-- 
Joel

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Daniel Jacobowitz]

On Mon, May 11, 2009 at 11:49:10PM +0300, Eli Zaretskii wrote:
> Are such rearrangements permitted?  I mean, are we talking about a
> real-life situation here?

Definitely.  This happens all the time.

> > Do you really want "break foo" to break on the line where a () is
> > called?
> 
> It's hard to say, really.  There are arguments for both, but I
> personally tend to think that stopping on the call to `a' is what I'd
> want.
> 
> What do others think?

I agree with Pedro.  When the compiler moves something into the
prologue, generally, we try to stop on it - even if the prologue is
not yet finished.

> > /* The order of entries in the linetable is significant.  They should
> >    be sorted by increasing values of the pc field.
> 
> Well, granted, I've seen that comment.  But (a) are we sure all of our
> comments are necessarily accurate to rely on them?, and (b) it
> continues to say

This data structure relies on being sorted by PC.  You can see e.g. in
find_pc_sect_line.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Joel Brobecker]

> Are such rearrangements permitted?  I mean, are we talking about a
> real-life situation here?

I think they are permitted, especially if the statements are not
function calls. For instance, the compiler can certainly reverse
the following two statements:

   a = 1;
   b = 2;

I am not versed in the art of optimizing, but why not. I don't think
anything is forbidden as long as the program works as expected.

That being said, the example was just an extreme on meant to show
what it means to chose the line with the smaller number, and why
I prefer to select the first line in your case.

But, like you, I'm not too concerned about which line to select.
In practice, it's not going to happen much.  Even for you, I'm betting
that it's only happen when breaking on "main", because the compiler
should generate that stack re-alignment code only in the main, and
on functions that have special attributes meaning that it might be
called from some code whose stack is less aligned.

> >                                        In other words, when I break on
> > a function, I expect that by the time I reach that function breakpoint,
> > none of the real code has been executed yet - I want to debug the
> > function :-).
> 
> This could be impossible anyway, if the compiler moves some of the
> body into the prologue, right?

I can't remember what we do in that case. I'd have to look at
the code again.

> Well, granted, I've seen that comment.  But (a) are we sure all of our
> comments are necessarily accurate to rely on them?, and (b) it
> continues to say
> 
>                                                 If there is more than
>    one entry for a given pc, then I'm not sure what should happen (and
>    I not sure whether we currently handle it the best way).
> 
> Not very reassuring...

At least for the part that lines are sorted by ascending PC order,
I'm pretty sure. If we break that assumption, we should fix it,
because I think we rely on it in several places.

-- 
Joel

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

> Date: Mon, 11 May 2009 23:18:55 +0200
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Daniel Jacobowitz <drow@false.org>, gdb-patches@sourceware.org
> 
> > I would think so.  What I show in my mail comes from "maint print
> > symbols", so I have no reason to believe the lineinfo table does not
> > reflect the COFF debug info.
> 
> You should be able to find another confirmation of this by looking
> at the .s file.  I don't know what assembler directive is used on
> DJGPP to emit lines, but it's usually straightforward.

DJGPP uses gas, so the directives are the same as on GNU/Linux.

Do we still need more confirmations that the COFF info indeed lacks
line information for the opening brace, given the objdump output I
posted?

> > > Is it possible to patch this up in the coff line table reader?
> > 
> > What, by inventing extra entries in the table?  That could be
> > dangerous, since we would be doing that without any clear idea of the
> > code between 0x172c and 0x1748.
> 
> I don't think it matters what the code does, since otherwise you
> would skip that code before inserting the breakpoint anyway.
> 
> > Even if we could, is that really better than the approach I suggested?
> 
> Not sure if this is better or not. The advantage of this approach is
> that we protect other platforms from this form of debugging info -
> one could argue that it's incomplete.

Maybe it is, but knowing it's incomplete and completing it correctly
are two different things.  For starters, what line number could we
possibly add to the additional entry?  The only safe possibility would
be the same line as in some existing lineinfo entry whose line number
is the smallest one.  But that would mean "info line" could now
produce potentially incorrect or misleading information, couldn't it?

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

> Date: Mon, 11 May 2009 17:20:06 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org
> 
> > > Do you really want "break foo" to break on the line where a () is
> > > called?
> > 
> > It's hard to say, really.  There are arguments for both, but I
> > personally tend to think that stopping on the call to `a' is what I'd
> > want.
> > 
> > What do others think?
> 
> I agree with Pedro.

You mean, with Joel.

Okay, I will go with that approach, then.  But do we agree that my
general idea is the way to go, as opposed to fixing the lineinfo table
when it is read from the COFF file?

> > > /* The order of entries in the linetable is significant.  They should
> > >    be sorted by increasing values of the pc field.
> > 
> > Well, granted, I've seen that comment.  But (a) are we sure all of our
> > comments are necessarily accurate to rely on them?, and (b) it
> > continues to say
> 
> This data structure relies on being sorted by PC.  You can see e.g. in
> find_pc_sect_line.

Right.

Do you also agree with Joel that looping on all symtabs is not
necessary, in general, and that using the single symtab given by
SYMBOL_SYMTAB(sym) is enough?

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

> Date: Mon, 11 May 2009 23:49:10 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org
> 
> > Date: Mon, 11 May 2009 21:27:09 +0200
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: gdb-patches@sourceware.org
> > 
> > I'm always reluctant to introduce code I don't understand, and usually
> > leave it out until I see a bug - that's why I was asking. Have you
> > tried without this part?
> 
> No, but I will.

OK, I tried it, and it does work, at least in the 2 test cases I could
try.

Here's an updated patch, which incorporates the comments in this
thread:

  . It uses a single symtab for the function's symbol
  . It stops looking as soon as it finds the first linetable entry
    whose PC is in the range of the function's PC values

OK to commit?

2009-05-09  Eli Zaretskii  <eliz@gnu.org>

	* symtab.c (skip_prologue_using_lineinfo): New function.
	(find_function_start_sal): Use it to get to the first line of
	function's body that has an entry in the lineinfo table.

--- symtab.c~0	2009-04-01 02:21:07.000000000 +0300
+++ symtab.c	2009-05-16 13:49:42.296906500 +0300
@@ -2599,6 +2599,46 @@
   return pc;
 }
 
+/* Given a function start address FUNC_ADDR and SYMTAB, find the first
+   address for that function that has an entry in SYMTAB's line info
+   table.  If such an entry cannot be found, return FUNC_ADDR
+   unaltered.  */
+CORE_ADDR
+skip_prologue_using_lineinfo (CORE_ADDR func_addr, struct symtab *symtab)
+{
+  CORE_ADDR func_start, func_end;
+  struct linetable *l;
+  int ind, i, len;
+  int best_lineno = 0;
+  CORE_ADDR best_pc = func_addr;
+
+  /* Get the range for the function's PC values, or give up if we
+     cannot, for some reason.  */
+  if (!find_pc_partial_function (func_addr, NULL, &func_start, &func_end))
+    return func_addr;
+
+  l = LINETABLE (symtab);
+  if (l == NULL)
+    return func_addr;
+
+  /* Linetable entries are ordered by PC values, see the commentary in
+     symtab.h where `struct linetable' is defined.  Thus, the first
+     entry whose PC is in the range [FUNC_START..FUNC_END] is the
+     address we are looking for.  */
+  for (i = 0; i < l->nitems; i++)
+    {
+      struct linetable_entry *item = &(l->item[i]);
+
+      /* Don't use line numbers of zero, they mark special entries in
+	 the table.  See the commentary on symtab.h before the
+	 definition of struct linetable.  */
+      if (item->line > 0 && func_start <= item->pc && item->pc <= func_end)
+	return item->pc;
+    }
+
+  return func_addr;
+}
+
 /* Given a function symbol SYM, find the symtab and line for the start
    of the function.
    If the argument FUNFIRSTLINE is nonzero, we want the first line
@@ -2649,6 +2689,15 @@
       sal = find_pc_sect_line (pc, SYMBOL_OBJ_SECTION (sym), 0);
     }
 
+  /* If we still don't have a valid source line, try to find the first
+     PC in the lineinfo table that belongs to the same function.  */
+  if (funfirstline && sal.symtab == NULL)
+    {
+      pc = skip_prologue_using_lineinfo (pc, SYMBOL_SYMTAB (sym));
+      /* Recalculate the line number.  */
+      sal = find_pc_sect_line (pc, SYMBOL_OBJ_SECTION (sym), 0);
+    }
+
   sal.pc = pc;
 
   return sal;

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Joel Brobecker]

> 2009-05-09  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* symtab.c (skip_prologue_using_lineinfo): New function.
> 	(find_function_start_sal): Use it to get to the first line of
> 	function's body that has an entry in the lineinfo table.

The change looks OK to me, overall. I do have a couple of little
comments, but do feel free to check it in once the comments are
addressed.

> +  /* Get the range for the function's PC values, or give up if we
> +     cannot, for some reason.  */
> +  if (!find_pc_partial_function (func_addr, NULL, &func_start, &func_end))
> +    return func_addr;
> +  l = LINETABLE (symtab);
> +  if (l == NULL)
> +    return func_addr;

Perhaps you want to check for this before doing the lookup above,
since you have access to the linetable directly.  Although, in
practice, we know it should never fail, since the func_addr comes
from a symbol.  So we should always find the associated partial
symbol.

> +  /* Linetable entries are ordered by PC values, see the commentary in
                                                            ^^^^^^ comment
> +     symtab.h where `struct linetable' is defined.  Thus, the first
> +     entry whose PC is in the range [FUNC_START..FUNC_END] is the
                                                           ^^^
You probably want to say [FUNC_START..FUNC_END[, as FUNC_END should
be excluded from the function range. And therefore:

> +	 the table.  See the commentary on symtab.h before the
                             ^^^^^^^^ comment
> +	 definition of struct linetable.  */
> +      if (item->line > 0 && func_start <= item->pc && item->pc <= func_end)
                                                                  ^^
I believe that the second comparison should be a strict compare:
                                                         item->pc < func_end
> +  /* If we still don't have a valid source line, try to find the first
> +     PC in the lineinfo table that belongs to the same function.  */

That's just a suggestion, but I wouldn't mind if we provided a little
extra explainations about the situation where we have encountered this.
That way, when we later come back to this and try to determine whether
this is still relelvant or not, we'll find a little more context.

Something saying that this can happen if the first line in lineinfo data
for our function starts after the end of the function prologue.  This
was observed with DJGPP in a function where the prologue was followed
by some code realigning the stack, before the "real" code started.
The line info for that function started at the beginning of the "real"
code. So, there was no line info available for the instruction after
the prologue.  Something like that.

Cheers,
-- 
Joel

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Eli Zaretskii]

> Date: Wed, 20 May 2009 14:16:31 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Daniel Jacobowitz <drow@false.org>, gdb-patches@sourceware.org
> 
> > 2009-05-09  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* symtab.c (skip_prologue_using_lineinfo): New function.
> > 	(find_function_start_sal): Use it to get to the first line of
> > 	function's body that has an entry in the lineinfo table.
> 
> The change looks OK to me, overall. I do have a couple of little
> comments, but do feel free to check it in once the comments are
> addressed.

Thanks.  The patch I actually committed is reproduced below.

> > +  /* Get the range for the function's PC values, or give up if we
> > +     cannot, for some reason.  */
> > +  if (!find_pc_partial_function (func_addr, NULL, &func_start, &func_end))
> > +    return func_addr;
> > +  l = LINETABLE (symtab);
> > +  if (l == NULL)
> > +    return func_addr;
> 
> Perhaps you want to check for this before doing the lookup above,
> since you have access to the linetable directly.  Although, in
> practice, we know it should never fail, since the func_addr comes
> from a symbol.  So we should always find the associated partial
> symbol.

I moved the test of l == NULL before the call to
find_pc_partial_function.

As for always finding the partial symbol: a single test instruction is
not expensive enough, IMO, to justify giving up defensive code, even
if it should always succeed.  So I left it in place.

> > +  /* Linetable entries are ordered by PC values, see the commentary in
>                                                             ^^^^^^ comment

Hmm? are you saying that "commentary" is incorrect English?  If so, I
beg to differ. ;-)

> > +     symtab.h where `struct linetable' is defined.  Thus, the first
> > +     entry whose PC is in the range [FUNC_START..FUNC_END] is the
>                                                            ^^^
> You probably want to say [FUNC_START..FUNC_END[, as FUNC_END should
> be excluded from the function range. And therefore:
> 
> > +	 the table.  See the commentary on symtab.h before the
>                              ^^^^^^^^ comment
> > +	 definition of struct linetable.  */
> > +      if (item->line > 0 && func_start <= item->pc && item->pc <= func_end)
>                                                                   ^^
> I believe that the second comparison should be a strict compare:

Right, fixed.

> That's just a suggestion, but I wouldn't mind if we provided a little
> extra explainations about the situation where we have encountered this.

Is the comment I added before the call to skip_prologue_using_lineinfo
good enough?


2009-05-23  Eli Zaretskii  <eliz@gnu.org>

	* symtab.c (skip_prologue_using_lineinfo): New function.
	(find_function_start_sal): Use it to get to the first line of
	function's body that has an entry in the lineinfo table.


--- symtab.c~0	2009-04-01 02:21:07.000000000 +0300
+++ symtab.c	2009-05-23 12:52:12.609375000 +0300
@@ -2599,6 +2599,47 @@ find_function_start_pc (struct gdbarch *
   return pc;
 }
 
+/* Given a function start address FUNC_ADDR and SYMTAB, find the first
+   address for that function that has an entry in SYMTAB's line info
+   table.  If such an entry cannot be found, return FUNC_ADDR
+   unaltered.  */
+CORE_ADDR
+skip_prologue_using_lineinfo (CORE_ADDR func_addr, struct symtab *symtab)
+{
+  CORE_ADDR func_start, func_end;
+  struct linetable *l;
+  int ind, i, len;
+  int best_lineno = 0;
+  CORE_ADDR best_pc = func_addr;
+
+  /* Give up if this symbol has no lineinfo table.  */
+  l = LINETABLE (symtab);
+  if (l == NULL)
+    return func_addr;
+
+  /* Get the range for the function's PC values, or give up if we
+     cannot, for some reason.  */
+  if (!find_pc_partial_function (func_addr, NULL, &func_start, &func_end))
+    return func_addr;
+
+  /* Linetable entries are ordered by PC values, see the commentary in
+     symtab.h where `struct linetable' is defined.  Thus, the first
+     entry whose PC is in the range [FUNC_START..FUNC_END[ is the
+     address we are looking for.  */
+  for (i = 0; i < l->nitems; i++)
+    {
+      struct linetable_entry *item = &(l->item[i]);
+
+      /* Don't use line numbers of zero, they mark special entries in
+	 the table.  See the commentary on symtab.h before the
+	 definition of struct linetable.  */
+      if (item->line > 0 && func_start <= item->pc && item->pc < func_end)
+	return item->pc;
+    }
+
+  return func_addr;
+}
+
 /* Given a function symbol SYM, find the symtab and line for the start
    of the function.
    If the argument FUNFIRSTLINE is nonzero, we want the first line
@@ -2649,6 +2690,21 @@ find_function_start_sal (struct symbol *
       sal = find_pc_sect_line (pc, SYMBOL_OBJ_SECTION (sym), 0);
     }
 
+  /* If we still don't have a valid source line, try to find the first
+     PC in the lineinfo table that belongs to the same function.  This
+     happens with COFF debug info, which does not seem to have an
+     entry in lineinfo table for the code after the prologue which has
+     no direct relation to source.  For example, this was found to be
+     the case with the DJGPP target using "gcc -gcoff" when the
+     compiler inserted code after the prologue to make sure the stack
+     is aligned.  */
+  if (funfirstline && sal.symtab == NULL)
+    {
+      pc = skip_prologue_using_lineinfo (pc, SYMBOL_SYMTAB (sym));
+      /* Recalculate the line number.  */
+      sal = find_pc_sect_line (pc, SYMBOL_OBJ_SECTION (sym), 0);
+    }
+
   sal.pc = pc;
 
   return sal;

Re: [RFA] Fix "break foo" when `foo's prologue ends before line table

[Joel Brobecker]

> > > +  /* Linetable entries are ordered by PC values, see the commentary in
> >                                                             ^^^^^^ comment
> 
> Hmm? are you saying that "commentary" is incorrect English?  If so, I
> beg to differ. ;-)

It seems to me that "commentary" would be used in a different context,
not to refer to some text describing what a piece of code does.  But
I'm not a native English speaker, so I might be wrong.

> Is the comment I added before the call to skip_prologue_using_lineinfo
> good enough?

It looks great to me :)

-- 
Joel