RFC: The Constructor Breakpoint Issue

RFC: The Constructor Breakpoint Issue

[David Lecomber]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

Specifically, this one:
gdb/1091: Constructor breakpoints ignored
gdb/1193: g++ 3.3 creates multiple constructors: gdb 5.3 can't set
breakpoints

I see this happening in other situations.  For example, Intel's compiler
seems to produce two versions of each subroutine -- perhaps each is
optimized for a different architecture.  This means that, say, 'break
test.f:65' will usually put the breakpoint in the wrong one (c.f.
Murphy's Law).

I have implemented a fix, which works, presently I'm not too proud of
it, so I'm going to check it through before proposing the patch
formally..

However, does anyone have any problem with the approach.  

In summary, I change the linetable_entry structure to have a pointer,
'next', which will be the next linetable_entry in the table which has
the same source code and line.  

find_line_pc then actually modifies the symtab_and_line parameter to get
the linetable_entry (rather than just pulling the PC from it).

In the breakpoints code, if a symtab_and_line has several entries in the
chain of entries, it sets a new breakpoint for each one.  

This does mean that break test.f:65 will announce each breakpoint
separately.  Also, the first bpoint set is known by the line/source, the
others can be distinguished by their address.

It cures the breakpoint in the constructor issue, which is probably the
quickest way for a GNU compiler based check of the fix.

d.



[-- Attachment #2: bp.patch --]
[-- Type: text/x-patch, Size: 12181 bytes --]

Index: gdb/symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.140
diff -c -p -r1.140 symtab.c
*** gdb/symtab.c	2 Oct 2004 09:55:15 -0000	1.140
--- gdb/symtab.c	14 Oct 2004 00:22:29 -0000
*************** init_sal (struct symtab_and_line *sal)
*** 700,705 ****
--- 700,706 ----
    sal->line = 0;
    sal->pc = 0;
    sal->end = 0;
+   sal->entry = 0;
  }
  \f
  
*************** done:
*** 2307,2326 ****
     The source file is specified with a struct symtab.  */
  
  int
! find_line_pc (struct symtab *symtab, int line, CORE_ADDR *pc)
  {
    struct linetable *l;
    int ind;
! 
!   *pc = 0;
    if (symtab == 0)
      return 0;
  
!   symtab = find_line_symtab (symtab, line, &ind, NULL);
    if (symtab != NULL)
      {
        l = LINETABLE (symtab);
!       *pc = l->item[ind].pc;
        return 1;
      }
    else
--- 2308,2329 ----
     The source file is specified with a struct symtab.  */
  
  int
! find_line_pc (struct symtab_and_line *sal)
  {
    struct linetable *l;
    int ind;
!   struct symtab *symtab;
!   symtab = sal->symtab;
!   sal->pc = 0;
    if (symtab == 0)
      return 0;
  
!   symtab = find_line_symtab (sal->symtab, sal->line, &ind, NULL);
    if (symtab != NULL)
      {
        l = LINETABLE (symtab);
!       sal->pc = l->item[ind].pc;
!       sal->entry = &(l->item[ind]);
        return 1;
      }
    else
*************** find_line_pc_range (struct symtab_and_li
*** 2341,2348 ****
    struct symtab_and_line found_sal;
  
    startaddr = sal.pc;
!   if (startaddr == 0 && !find_line_pc (sal.symtab, sal.line, &startaddr))
!     return 0;
  
    /* This whole function is based on address.  For example, if line 10 has
       two parts, one from 0x100 to 0x200 and one from 0x300 to 0x400, then
--- 2344,2352 ----
    struct symtab_and_line found_sal;
  
    startaddr = sal.pc;
!   if (startaddr == 0 && !find_line_pc (&sal))
!     return 0;  
!   startaddr = sal.pc;
  
    /* This whole function is based on address.  For example, if line 10 has
       two parts, one from 0x100 to 0x200 and one from 0x300 to 0x400, then
*************** find_line_common (struct linetable *l, i
*** 2391,2396 ****
--- 2395,2402 ----
    if (l == 0)
      return -1;
  
+   *exact_match = 0;
+ 
    len = l->nitems;
    for (i = 0; i < len; i++)
      {
*************** find_line_common (struct linetable *l, i
*** 2398,2418 ****
  
        if (item->line == lineno)
  	{
! 	  /* Return the first (lowest address) entry which matches.  */
! 	  *exact_match = 1;
! 	  return i;
  	}
  
!       if (item->line > lineno && (best == 0 || item->line < best))
  	{
! 	  best = item->line;
! 	  best_index = i;
  	}
      }
- 
-   /* If we got here, we didn't get an exact match.  */
- 
-   *exact_match = 0;
    return best_index;
  }
  
--- 2404,2451 ----
  
        if (item->line == lineno)
  	{
! 	  if (*exact_match) 
! 	    {
! 	      /* create a chain of matching lines: for inlined and
! 		 default arg constructors often line number has several
! 		 PCs.
! 	       */
! 	      l->item[i].next = l->item[best_index].next;
! 	      l->item[best_index].next = &(l->item[i]);
! 	    }
! 	  else 
! 	    {
! 	      /* Return the first (lowest address) entry which matches.  */
! 	      *exact_match = 1;
! 	      best_index = i;
! 	      best = lineno;
! 	      if (l->item[i].next) 
! 		{
! 		  /* have already found this line and other matches */
! 		  return i;
! 		}
! 	    }
  	}
  
!       if (!(*exact_match) 
! 	  && item->line > lineno && (best == 0 || item->line <= best))
  	{
! 	  if (best == item->line) 
! 	    {
! 	      if (!l->item[best_index].next) 
! 		{
! 		  /* two matching 'best' lines */
! 		  l->item[i].next = l->item[best_index].next;
! 		  l->item[best_index].next = &(l->item[i]);
! 		}
! 	    }
! 	  else 
! 	    {
! 	      best = item->line;
! 	      best_index = i;
! 	    }
  	}
      }
    return best_index;
  }
  
Index: gdb/symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.92
diff -c -p -r1.92 symtab.h
*** gdb/symtab.h	10 Jun 2004 20:05:44 -0000	1.92
--- gdb/symtab.h	14 Oct 2004 00:22:29 -0000
*************** struct linetable_entry
*** 708,713 ****
--- 708,714 ----
  {
    int line;
    CORE_ADDR pc;
+   struct linetable_entry *next;
  };
  
  /* The order of entries in the linetable is significant.  They should
*************** struct symtab_and_line
*** 1198,1203 ****
--- 1199,1206 ----
  
    CORE_ADDR pc;
    CORE_ADDR end;
+ 
+   struct linetable_entry *entry;
  };
  
  extern void init_sal (struct symtab_and_line *sal);
*************** extern struct symtab_and_line find_pc_se
*** 1256,1262 ****
  
  /* Given a symtab and line number, return the pc there.  */
  
! extern int find_line_pc (struct symtab *, int, CORE_ADDR *);
  
  extern int find_line_pc_range (struct symtab_and_line, CORE_ADDR *,
  			       CORE_ADDR *);
--- 1259,1265 ----
  
  /* Given a symtab and line number, return the pc there.  */
  
! extern int find_line_pc (struct symtab_and_line *);
  
  extern int find_line_pc_range (struct symtab_and_line, CORE_ADDR *,
  			       CORE_ADDR *);
Index: gdb/buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.40
diff -c -p -r1.40 buildsym.c
*** gdb/buildsym.c	11 Sep 2004 10:24:45 -0000	1.40
--- gdb/buildsym.c	14 Oct 2004 00:22:32 -0000
*************** record_line (struct subfile *subfile, in
*** 733,738 ****
--- 733,739 ----
    e = subfile->line_vector->item + subfile->line_vector->nitems++;
    e->line = line;
    e->pc = ADDR_BITS_REMOVE(pc);
+   e->next = 0;
  }
  
  /* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */
Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.183
diff -c -p -r1.183 breakpoint.c
*** gdb/breakpoint.c	8 Oct 2004 17:30:46 -0000	1.183
--- gdb/breakpoint.c	14 Oct 2004 00:22:34 -0000
*************** struct breakpoint *
*** 4061,4073 ****
  set_raw_breakpoint (struct symtab_and_line sal, enum bptype bptype)
  {
    struct breakpoint *b, *b1;
! 
    b = (struct breakpoint *) xmalloc (sizeof (struct breakpoint));
    memset (b, 0, sizeof (*b));
    b->loc = allocate_bp_location (b, bptype);
!   b->loc->requested_address = sal.pc;
    b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
!                                                bptype);
    if (sal.symtab == NULL)
      b->source_file = NULL;
    else
--- 4061,4075 ----
  set_raw_breakpoint (struct symtab_and_line sal, enum bptype bptype)
  {
    struct breakpoint *b, *b1;
!   CORE_ADDR pc;
!   pc = sal.pc;
!     
    b = (struct breakpoint *) xmalloc (sizeof (struct breakpoint));
    memset (b, 0, sizeof (*b));
    b->loc = allocate_bp_location (b, bptype);
!   b->loc->requested_address = pc;
    b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
! 					       bptype);
    if (sal.symtab == NULL)
      b->source_file = NULL;
    else
*************** set_raw_breakpoint (struct symtab_and_li
*** 4105,4111 ****
  	b1 = b1->next;
        b1->next = b;
      }
! 
    check_duplicates (b);
    breakpoints_changed ();
  
--- 4107,4113 ----
  	b1 = b1->next;
        b1->next = b;
      }
!   
    check_duplicates (b);
    breakpoints_changed ();
  
*************** create_breakpoints (struct symtabs_and_l
*** 4927,4978 ****
      int i;
      for (i = 0; i < sals.nelts; i++)
        {
  	struct breakpoint *b;
  	struct symtab_and_line sal = sals.sals[i];
! 
  	if (from_tty)
  	  describe_other_breakpoints (sal.pc, sal.section);
  	
- 	b = set_raw_breakpoint (sal, type);
- 	set_breakpoint_count (breakpoint_count + 1);
- 	b->number = breakpoint_count;
- 	b->cond = cond[i];
- 	b->thread = thread;
- 	if (addr_string[i])
- 	  b->addr_string = addr_string[i];
- 	else
- 	  /* addr_string has to be used or breakpoint_re_set will delete
- 	     me.  */
- 	  b->addr_string = xstrprintf ("*0x%s", paddr (b->loc->address));
- 	b->cond_string = cond_string[i];
- 	b->ignore_count = ignore_count;
- 	b->enable_state = bp_enabled;
- 	b->disposition = disposition;
- 	/* If resolving a pending breakpoint, a check must be made to see if
- 	   the user has specified a new condition or commands for the 
- 	   breakpoint.  A new condition will override any condition that was 
- 	   initially specified with the initial breakpoint command.  */
- 	if (pending_bp)
- 	  {
- 	    char *arg;
- 	    if (pending_bp->cond_string)
- 	      {
- 		arg = pending_bp->cond_string;
- 		b->cond_string = savestring (arg, strlen (arg));
- 		b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
- 		if (*arg)
- 		  error ("Junk at end of pending breakpoint condition expression");
- 	      }
- 	    /* If there are commands associated with the breakpoint, they should 
- 	       be copied too.  */
- 	    if (pending_bp->commands)
- 	      b->commands = copy_command_lines (pending_bp->commands);
- 	    
- 	    /* We have to copy over the ignore_count and thread as well.  */
- 	    b->ignore_count = pending_bp->ignore_count;
- 	    b->thread = pending_bp->thread;
- 	  }
- 	mention (b);
        }
    }    
  }
--- 4929,4995 ----
      int i;
      for (i = 0; i < sals.nelts; i++)
        {
+ 	char *addr_str;
+ 	struct linetable_entry *le;
  	struct breakpoint *b;
  	struct symtab_and_line sal = sals.sals[i];
! 	le = sals.sals[i].entry;
  	if (from_tty)
  	  describe_other_breakpoints (sal.pc, sal.section);
+ 
+ 	addr_str = addr_string[i];
+ 	do  {
+ 	  if (le) 
+ 	    sal.pc = le->pc;
+ 	  
+ 	  b = set_raw_breakpoint (sal, type);
+ 
+ 	  set_breakpoint_count (breakpoint_count + 1);
+ 	  b->number = breakpoint_count;
+ 	  b->cond = cond[i];
+ 	  b->thread = thread;
+ 	  if (addr_str)
+ 	    {
+ 	      b->addr_string = addr_str;
+ 	      addr_str = NULL;
+ 	    }
+ 	  else
+ 	    /* addr_string has to be used or breakpoint_re_set will delete
+ 	       me.  */
+ 	    b->addr_string = xstrprintf ("*0x%s", paddr (b->loc->address));
+ 	  b->cond_string = cond_string[i];
+ 	  b->ignore_count = ignore_count;
+ 	  b->enable_state = bp_enabled;
+ 	  b->disposition = disposition;
+ 	  /* If resolving a pending breakpoint, a check must be made to see if
+ 	     the user has specified a new condition or commands for the 
+ 	     breakpoint.  A new condition will override any condition that was 
+ 	     initially specified with the initial breakpoint command.  */
+ 	  if (pending_bp)
+ 	    {
+ 	      char *arg;
+ 	      if (pending_bp->cond_string)
+ 		{
+ 		  arg = pending_bp->cond_string;
+ 		  b->cond_string = savestring (arg, strlen (arg));
+ 		  b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+ 		  if (*arg)
+ 		    error ("Junk at end of pending breakpoint condition expression");
+ 		}
+ 	      /* If there are commands associated with the breakpoint, they should 
+ 		 be copied too.  */
+ 	      if (pending_bp->commands)
+ 		b->commands = copy_command_lines (pending_bp->commands);
+ 	      
+ 	      /* We have to copy over the ignore_count and thread as well.  */
+ 	      b->ignore_count = pending_bp->ignore_count;
+ 	      b->thread = pending_bp->thread;
+ 	    }
+ 	  mention (b);
+ 	  if (le) le = le->next;
+ 	} while (le);
+ 	
  	
        }
    }    
  }
*************** break_at_finish_command_1 (char *arg, in
*** 5617,5630 ****
  void
  resolve_sal_pc (struct symtab_and_line *sal)
  {
-   CORE_ADDR pc;
- 
    if (sal->pc == 0 && sal->symtab != NULL)
      {
!       if (!find_line_pc (sal->symtab, sal->line, &pc))
  	error ("No line %d in file \"%s\".",
  	       sal->line, sal->symtab->filename);
!       sal->pc = pc;
      }
  
    if (sal->section == 0 && sal->symtab != NULL)
--- 5634,5645 ----
  void
  resolve_sal_pc (struct symtab_and_line *sal)
  {
    if (sal->pc == 0 && sal->symtab != NULL)
      {
!       if (!find_line_pc (sal))
  	error ("No line %d in file \"%s\".",
  	       sal->line, sal->symtab->filename);
! 
      }
  
    if (sal->section == 0 && sal->symtab != NULL)

Re: RFC: The Constructor Breakpoint Issue

[David Lecomber]

[-- Attachment #1: Type: text/plain, Size: 2553 bytes --]

Morning all,

I have reformatted this so it actually compiles - I hope...

Unfortunately this patch busts a lot of test cases (about 30..).    I
have discovered that it is being over-zealous in applying breakpoints
and need an expert to suggest solutions.

The first broken example is gdb.base/display.c - there we have a for
loop.  So, first line of the for loop (display.c:14) is at PC xxxxxx,
and also at xxxxxx + some no. which presumably is where the increment
part of the for loop occurs.  Do we want to get both these addresses for
breakpoints, or not..  because it happens..

I see the intel compiler sets the default is_stmt field to zero, and
never does anything else with it, and the GNU compilers set it to 1. 
However, dwarf2read ignores this info anyway.

I cannot see anything in the dwarf2 line info that enables us to
distinguish between the two cases - those where the lines are not really
duplicate, and those that are.  I think everyone would find fixing this
issue important..  

Opinions??

d.



On Thu, 2004-10-14 at 01:37, David Lecomber wrote:
> Specifically, this one:
> gdb/1091: Constructor breakpoints ignored
> gdb/1193: g++ 3.3 creates multiple constructors: gdb 5.3 can't set
> breakpoints
> 
> I see this happening in other situations.  For example, Intel's compiler
> seems to produce two versions of each subroutine -- perhaps each is
> optimized for a different architecture.  This means that, say, 'break
> test.f:65' will usually put the breakpoint in the wrong one (c.f.
> Murphy's Law).
> 
> I have implemented a fix, which works, presently I'm not too proud of
> it, so I'm going to check it through before proposing the patch
> formally..
> 
> However, does anyone have any problem with the approach.  
> 
> In summary, I change the linetable_entry structure to have a pointer,
> 'next', which will be the next linetable_entry in the table which has
> the same source code and line.  
> 
> find_line_pc then actually modifies the symtab_and_line parameter to get
> the linetable_entry (rather than just pulling the PC from it).
> 
> In the breakpoints code, if a symtab_and_line has several entries in the
> chain of entries, it sets a new breakpoint for each one.  
> 
> This does mean that break test.f:65 will announce each breakpoint
> separately.  Also, the first bpoint set is known by the line/source, the
> others can be distinguished by their address.
> 
> It cures the breakpoint in the constructor issue, which is probably the
> quickest way for a GNU compiler based check of the fix.
> 
> d.
> 
> 

[-- Attachment #2: bpchanges.patch --]
[-- Type: text/x-patch, Size: 17403 bytes --]

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.183
diff -c -p -r1.183 breakpoint.c
*** gdb/breakpoint.c	8 Oct 2004 17:30:46 -0000	1.183
--- gdb/breakpoint.c	15 Oct 2004 13:24:26 -0000
*************** struct breakpoint *
*** 4061,4073 ****
  set_raw_breakpoint (struct symtab_and_line sal, enum bptype bptype)
  {
    struct breakpoint *b, *b1;
! 
    b = (struct breakpoint *) xmalloc (sizeof (struct breakpoint));
    memset (b, 0, sizeof (*b));
    b->loc = allocate_bp_location (b, bptype);
!   b->loc->requested_address = sal.pc;
    b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
!                                                bptype);
    if (sal.symtab == NULL)
      b->source_file = NULL;
    else
--- 4061,4075 ----
  set_raw_breakpoint (struct symtab_and_line sal, enum bptype bptype)
  {
    struct breakpoint *b, *b1;
!   CORE_ADDR pc;
!   pc = sal.pc;
!     
    b = (struct breakpoint *) xmalloc (sizeof (struct breakpoint));
    memset (b, 0, sizeof (*b));
    b->loc = allocate_bp_location (b, bptype);
!   b->loc->requested_address = pc;
    b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
! 					       bptype);
    if (sal.symtab == NULL)
      b->source_file = NULL;
    else
*************** set_raw_breakpoint (struct symtab_and_li
*** 4105,4111 ****
  	b1 = b1->next;
        b1->next = b;
      }
! 
    check_duplicates (b);
    breakpoints_changed ();
  
--- 4107,4113 ----
  	b1 = b1->next;
        b1->next = b;
      }
!   
    check_duplicates (b);
    breakpoints_changed ();
  
*************** create_breakpoints (struct symtabs_and_l
*** 4927,4978 ****
      int i;
      for (i = 0; i < sals.nelts; i++)
        {
  	struct breakpoint *b;
  	struct symtab_and_line sal = sals.sals[i];
! 
  	if (from_tty)
  	  describe_other_breakpoints (sal.pc, sal.section);
  	
- 	b = set_raw_breakpoint (sal, type);
- 	set_breakpoint_count (breakpoint_count + 1);
- 	b->number = breakpoint_count;
- 	b->cond = cond[i];
- 	b->thread = thread;
- 	if (addr_string[i])
- 	  b->addr_string = addr_string[i];
- 	else
- 	  /* addr_string has to be used or breakpoint_re_set will delete
- 	     me.  */
- 	  b->addr_string = xstrprintf ("*0x%s", paddr (b->loc->address));
- 	b->cond_string = cond_string[i];
- 	b->ignore_count = ignore_count;
- 	b->enable_state = bp_enabled;
- 	b->disposition = disposition;
- 	/* If resolving a pending breakpoint, a check must be made to see if
- 	   the user has specified a new condition or commands for the 
- 	   breakpoint.  A new condition will override any condition that was 
- 	   initially specified with the initial breakpoint command.  */
- 	if (pending_bp)
- 	  {
- 	    char *arg;
- 	    if (pending_bp->cond_string)
- 	      {
- 		arg = pending_bp->cond_string;
- 		b->cond_string = savestring (arg, strlen (arg));
- 		b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
- 		if (*arg)
- 		  error ("Junk at end of pending breakpoint condition expression");
- 	      }
- 	    /* If there are commands associated with the breakpoint, they should 
- 	       be copied too.  */
- 	    if (pending_bp->commands)
- 	      b->commands = copy_command_lines (pending_bp->commands);
- 	    
- 	    /* We have to copy over the ignore_count and thread as well.  */
- 	    b->ignore_count = pending_bp->ignore_count;
- 	    b->thread = pending_bp->thread;
- 	  }
- 	mention (b);
        }
    }    
  }
--- 4929,4995 ----
      int i;
      for (i = 0; i < sals.nelts; i++)
        {
+ 	char *addr_str;
+ 	struct linetable_entry *le;
  	struct breakpoint *b;
  	struct symtab_and_line sal = sals.sals[i];
! 	le = sals.sals[i].entry;
  	if (from_tty)
  	  describe_other_breakpoints (sal.pc, sal.section);
+ 
+ 	addr_str = addr_string[i];
+ 	do  {
+ 	  if (le) 
+ 	    sal.pc = le->pc;
+ 	  
+ 	  b = set_raw_breakpoint (sal, type);
+ 
+ 	  set_breakpoint_count (breakpoint_count + 1);
+ 	  b->number = breakpoint_count;
+ 	  b->cond = cond[i];
+ 	  b->thread = thread;
+ 	  if (addr_str)
+ 	    {
+ 	      b->addr_string = addr_str;
+ 	      addr_str = NULL;
+ 	    }
+ 	  else
+ 	    /* addr_string has to be used or breakpoint_re_set will delete
+ 	       me.  */
+ 	    b->addr_string = xstrprintf ("*0x%s", paddr (b->loc->address));
+ 	  b->cond_string = cond_string[i];
+ 	  b->ignore_count = ignore_count;
+ 	  b->enable_state = bp_enabled;
+ 	  b->disposition = disposition;
+ 	  /* If resolving a pending breakpoint, a check must be made to see if
+ 	     the user has specified a new condition or commands for the 
+ 	     breakpoint.  A new condition will override any condition that was 
+ 	     initially specified with the initial breakpoint command.  */
+ 	  if (pending_bp)
+ 	    {
+ 	      char *arg;
+ 	      if (pending_bp->cond_string)
+ 		{
+ 		  arg = pending_bp->cond_string;
+ 		  b->cond_string = savestring (arg, strlen (arg));
+ 		  b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+ 		  if (*arg)
+ 		    error ("Junk at end of pending breakpoint condition expression");
+ 		}
+ 	      /* If there are commands associated with the breakpoint, they should 
+ 		 be copied too.  */
+ 	      if (pending_bp->commands)
+ 		b->commands = copy_command_lines (pending_bp->commands);
+ 	      
+ 	      /* We have to copy over the ignore_count and thread as well.  */
+ 	      b->ignore_count = pending_bp->ignore_count;
+ 	      b->thread = pending_bp->thread;
+ 	    }
+ 	  mention (b);
+ 	  if (le) le = le->next;
+ 	} while (le);
+ 	
  	
        }
    }    
  }
*************** break_at_finish_command_1 (char *arg, in
*** 5617,5630 ****
  void
  resolve_sal_pc (struct symtab_and_line *sal)
  {
-   CORE_ADDR pc;
- 
    if (sal->pc == 0 && sal->symtab != NULL)
      {
!       if (!find_line_pc (sal->symtab, sal->line, &pc))
  	error ("No line %d in file \"%s\".",
  	       sal->line, sal->symtab->filename);
!       sal->pc = pc;
      }
  
    if (sal->section == 0 && sal->symtab != NULL)
--- 5634,5645 ----
  void
  resolve_sal_pc (struct symtab_and_line *sal)
  {
    if (sal->pc == 0 && sal->symtab != NULL)
      {
!       if (!find_line_pc (sal))
  	error ("No line %d in file \"%s\".",
  	       sal->line, sal->symtab->filename);
! 
      }
  
    if (sal->section == 0 && sal->symtab != NULL)
Index: gdb/buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.40
diff -c -p -r1.40 buildsym.c
*** gdb/buildsym.c	11 Sep 2004 10:24:45 -0000	1.40
--- gdb/buildsym.c	15 Oct 2004 13:24:26 -0000
*************** record_line (struct subfile *subfile, in
*** 733,738 ****
--- 733,739 ----
    e = subfile->line_vector->item + subfile->line_vector->nitems++;
    e->line = line;
    e->pc = ADDR_BITS_REMOVE(pc);
+   e->next = 0;
  }
  
  /* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */
Index: gdb/symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.140
diff -c -p -r1.140 symtab.c
*** gdb/symtab.c	2 Oct 2004 09:55:15 -0000	1.140
--- gdb/symtab.c	15 Oct 2004 13:24:28 -0000
*************** init_sal (struct symtab_and_line *sal)
*** 700,705 ****
--- 700,706 ----
    sal->line = 0;
    sal->pc = 0;
    sal->end = 0;
+   sal->entry = 0;
  }
  \f
  
*************** done:
*** 2307,2326 ****
     The source file is specified with a struct symtab.  */
  
  int
! find_line_pc (struct symtab *symtab, int line, CORE_ADDR *pc)
  {
    struct linetable *l;
    int ind;
! 
!   *pc = 0;
    if (symtab == 0)
      return 0;
  
!   symtab = find_line_symtab (symtab, line, &ind, NULL);
    if (symtab != NULL)
      {
        l = LINETABLE (symtab);
!       *pc = l->item[ind].pc;
        return 1;
      }
    else
--- 2308,2329 ----
     The source file is specified with a struct symtab.  */
  
  int
! find_line_pc (struct symtab_and_line *sal)
  {
    struct linetable *l;
    int ind;
!   struct symtab *symtab;
!   symtab = sal->symtab;
!   sal->pc = 0;
    if (symtab == 0)
      return 0;
  
!   symtab = find_line_symtab (sal->symtab, sal->line, &ind, NULL);
    if (symtab != NULL)
      {
        l = LINETABLE (symtab);
!       sal->pc = l->item[ind].pc;
!       sal->entry = &(l->item[ind]);
        return 1;
      }
    else
*************** find_line_pc_range (struct symtab_and_li
*** 2341,2348 ****
    struct symtab_and_line found_sal;
  
    startaddr = sal.pc;
!   if (startaddr == 0 && !find_line_pc (sal.symtab, sal.line, &startaddr))
!     return 0;
  
    /* This whole function is based on address.  For example, if line 10 has
       two parts, one from 0x100 to 0x200 and one from 0x300 to 0x400, then
--- 2344,2352 ----
    struct symtab_and_line found_sal;
  
    startaddr = sal.pc;
!   if (startaddr == 0 && !find_line_pc (&sal))
!     return 0;  
!   startaddr = sal.pc;
  
    /* This whole function is based on address.  For example, if line 10 has
       two parts, one from 0x100 to 0x200 and one from 0x300 to 0x400, then
*************** find_line_common (struct linetable *l, i
*** 2391,2396 ****
--- 2395,2402 ----
    if (l == 0)
      return -1;
  
+   *exact_match = 0;
+ 
    len = l->nitems;
    for (i = 0; i < len; i++)
      {
*************** find_line_common (struct linetable *l, i
*** 2398,2418 ****
  
        if (item->line == lineno)
  	{
! 	  /* Return the first (lowest address) entry which matches.  */
! 	  *exact_match = 1;
! 	  return i;
  	}
  
!       if (item->line > lineno && (best == 0 || item->line < best))
  	{
! 	  best = item->line;
! 	  best_index = i;
  	}
      }
- 
-   /* If we got here, we didn't get an exact match.  */
- 
-   *exact_match = 0;
    return best_index;
  }
  
--- 2404,2451 ----
  
        if (item->line == lineno)
  	{
! 	  if (*exact_match) 
! 	    {
! 	      /* create a chain of matching lines: for inlined and
! 		 default arg constructors often line number has several
! 		 PCs.
! 	       */
! 	      l->item[i].next = l->item[best_index].next;
! 	      l->item[best_index].next = &(l->item[i]);
! 	    }
! 	  else 
! 	    {
! 	      /* Return the first (lowest address) entry which matches.  */
! 	      *exact_match = 1;
! 	      best_index = i;
! 	      best = lineno;
! 	      if (l->item[i].next) 
! 		{
! 		  /* have already found this line and other matches */
! 		  return i;
! 		}
! 	    }
  	}
  
!       if (!(*exact_match) 
! 	  && item->line > lineno && (best == 0 || item->line <= best))
  	{
! 	  if (best == item->line) 
! 	    {
! 	      if (!l->item[best_index].next) 
! 		{
! 		  /* two matching 'best' lines */
! 		  l->item[i].next = l->item[best_index].next;
! 		  l->item[best_index].next = &(l->item[i]);
! 		}
! 	    }
! 	  else 
! 	    {
! 	      best = item->line;
! 	      best_index = i;
! 	    }
  	}
      }
    return best_index;
  }
  
Index: gdb/symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.92
diff -c -p -r1.92 symtab.h
*** gdb/symtab.h	10 Jun 2004 20:05:44 -0000	1.92
--- gdb/symtab.h	15 Oct 2004 13:24:28 -0000
*************** struct linetable_entry
*** 708,713 ****
--- 708,714 ----
  {
    int line;
    CORE_ADDR pc;
+   struct linetable_entry *next;
  };
  
  /* The order of entries in the linetable is significant.  They should
*************** struct symtab_and_line
*** 1198,1203 ****
--- 1199,1206 ----
  
    CORE_ADDR pc;
    CORE_ADDR end;
+ 
+   struct linetable_entry *entry;
  };
  
  extern void init_sal (struct symtab_and_line *sal);
*************** extern struct symtab_and_line find_pc_se
*** 1256,1262 ****
  
  /* Given a symtab and line number, return the pc there.  */
  
! extern int find_line_pc (struct symtab *, int, CORE_ADDR *);
  
  extern int find_line_pc_range (struct symtab_and_line, CORE_ADDR *,
  			       CORE_ADDR *);
--- 1259,1265 ----
  
  /* Given a symtab and line number, return the pc there.  */
  
! extern int find_line_pc (struct symtab_and_line *);
  
  extern int find_line_pc_range (struct symtab_and_line, CORE_ADDR *,
  			       CORE_ADDR *);
Index: gdb/mi/mi-cmd-disas.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-disas.c,v
retrieving revision 1.20
diff -c -p -r1.20 mi-cmd-disas.c
*** gdb/mi/mi-cmd-disas.c	30 Sep 2002 15:57:26 -0000	1.20
--- gdb/mi/mi-cmd-disas.c	15 Oct 2004 13:24:29 -0000
*************** mi_cmd_disassemble (char *command, char 
*** 145,155 ****
  
    if (line_seen && file_seen)
      {
        s = lookup_symtab (file_string);
        if (s == NULL)
  	error ("mi_cmd_disassemble: Invalid filename.");
!       if (!find_line_pc (s, line_num, &start))
  	error ("mi_cmd_disassemble: Invalid line number");
        if (find_pc_partial_function (start, NULL, &low, &high) == 0)
  	error ("mi_cmd_disassemble: No function contains specified address");
      }
--- 145,162 ----
  
    if (line_seen && file_seen)
      {
+       struct symtab_and_line sal;
+       init_sal(&sal);
        s = lookup_symtab (file_string);
        if (s == NULL)
  	error ("mi_cmd_disassemble: Invalid filename.");
! 
!       sal.symtab = s;
!       sal.line = line_num;
! 
!       if (!find_line_pc (&sal))
  	error ("mi_cmd_disassemble: Invalid line number");
+       start = sal.pc;
        if (find_pc_partial_function (start, NULL, &low, &high) == 0)
  	error ("mi_cmd_disassemble: No function contains specified address");
      }
Index: gdb/tui/tui-layout.c
===================================================================
RCS file: /cvs/src/src/gdb/tui/tui-layout.c,v
retrieving revision 1.18
diff -c -p -r1.18 tui-layout.c
*** gdb/tui/tui-layout.c	13 Mar 2004 14:14:01 -0000	1.18
--- gdb/tui/tui-layout.c	15 Oct 2004 13:24:30 -0000
*************** tui_set_layout_for_display_command (cons
*** 509,514 ****
--- 509,515 ----
  static CORE_ADDR
  extract_display_start_addr (void)
  {
+   struct symtab_and_line s;
    enum tui_layout_type cur_layout = tui_current_layout ();
    CORE_ADDR addr;
    CORE_ADDR pc;
*************** extract_display_start_addr (void)
*** 518,527 ****
      {
      case SRC_COMMAND:
      case SRC_DATA_COMMAND:
!       find_line_pc (cursal.symtab,
! 		    TUI_SRC_WIN->detail.source_info.start_line_or_addr.line_no,
! 		    &pc);
!       addr = pc;
        break;
      case DISASSEM_COMMAND:
      case SRC_DISASSEM_COMMAND:
--- 519,530 ----
      {
      case SRC_COMMAND:
      case SRC_DATA_COMMAND:
! 
!       init_sal(&s);
!       s.symtab = cursal.symtab;
!       s.line = TUI_SRC_WIN->detail.source_info.start_line_or_addr.line_no;
!       find_line_pc (&s);
!       addr = s.pc;
        break;
      case DISASSEM_COMMAND:
      case SRC_DISASSEM_COMMAND:
Index: gdb/tui/tui-win.c
===================================================================
RCS file: /cvs/src/src/gdb/tui/tui-win.c,v
retrieving revision 1.20
diff -c -p -r1.20 tui-win.c
*** gdb/tui/tui-win.c	26 Jul 2004 14:53:06 -0000	1.20
--- gdb/tui/tui-win.c	15 Oct 2004 13:24:30 -0000
*************** make_visible_with_new_height (struct tui
*** 1313,1319 ****
  	    line.line_no = cursal.line;
  	  else
  	    {
! 	      find_line_pc (s, cursal.line, &line.addr);
  	    }
  	  tui_update_source_window (win_info, s, line, TRUE);
  	}
--- 1313,1325 ----
  	    line.line_no = cursal.line;
  	  else
  	    {
! 
! 	      struct symtab_and_line sal;
! 	      init_sal(&sal);
! 	      sal.symtab = s;
! 	      sal.line = cursal.line;
! 	      find_line_pc (&sal);
! 	      line.addr = sal.pc;
  	    }
  	  tui_update_source_window (win_info, s, line, TRUE);
  	}
Index: gdb/tui/tui-winsource.c
===================================================================
RCS file: /cvs/src/src/gdb/tui/tui-winsource.c,v
retrieving revision 1.15
diff -c -p -r1.15 tui-winsource.c
*** gdb/tui/tui-winsource.c	16 Feb 2004 21:05:09 -0000	1.15
--- gdb/tui/tui-winsource.c	15 Oct 2004 13:24:30 -0000
*************** void
*** 172,184 ****
  tui_update_source_windows_with_line (struct symtab *s, int line)
  {
    CORE_ADDR pc;
    union tui_line_or_address l;
!   
    switch (tui_current_layout ())
      {
      case DISASSEM_COMMAND:
      case DISASSEM_DATA_COMMAND:
!       find_line_pc (s, line, &pc);
        tui_update_source_windows_with_addr (pc);
        break;
      default:
--- 172,191 ----
  tui_update_source_windows_with_line (struct symtab *s, int line)
  {
    CORE_ADDR pc;
+ 
+   struct symtab_and_line sal;
+ 
    union tui_line_or_address l;
! 
!   init_sal(&sal);
    switch (tui_current_layout ())
      {
      case DISASSEM_COMMAND:
      case DISASSEM_DATA_COMMAND:
!       sal.symtab = s;
!       sal.line = line;
!       find_line_pc (&sal);
!       pc = sal.pc;
        tui_update_source_windows_with_addr (pc);
        break;
      default:
*************** tui_update_source_windows_with_line (str
*** 186,192 ****
        tui_show_symtab_source (s, l, FALSE);
        if (tui_current_layout () == SRC_DISASSEM_COMMAND)
  	{
! 	  find_line_pc (s, line, &pc);
  	  tui_show_disassem (pc);
  	}
        break;
--- 193,202 ----
        tui_show_symtab_source (s, l, FALSE);
        if (tui_current_layout () == SRC_DISASSEM_COMMAND)
  	{
! 	  sal.symtab = s;
! 	  sal.line = line;
! 	  find_line_pc (&sal);
! 	  pc = sal.pc;
  	  tui_show_disassem (pc);
  	}
        break;

Re: RFC: The Constructor Breakpoint Issue

[Daniel Jacobowitz]

On Fri, Oct 15, 2004 at 02:41:39PM +0100, David Lecomber wrote:
> Morning all,
> 
> I have reformatted this so it actually compiles - I hope...
> 
> Unfortunately this patch busts a lot of test cases (about 30..).    I
> have discovered that it is being over-zealous in applying breakpoints
> and need an expert to suggest solutions.
> 
> The first broken example is gdb.base/display.c - there we have a for
> loop.  So, first line of the for loop (display.c:14) is at PC xxxxxx,
> and also at xxxxxx + some no. which presumably is where the increment
> part of the for loop occurs.  Do we want to get both these addresses for
> breakpoints, or not..  because it happens..
> 
> I see the intel compiler sets the default is_stmt field to zero, and
> never does anything else with it, and the GNU compilers set it to 1. 
> However, dwarf2read ignores this info anyway.

Yes; I've been thinking on and off about fixing this for the GNU
compilers.

> I cannot see anything in the dwarf2 line info that enables us to
> distinguish between the two cases - those where the lines are not really
> duplicate, and those that are.  I think everyone would find fixing this
> issue important..  
> 
> Opinions??

You can't tell from the line table.  You can tell if you have both a
line table and a .debug_info section; in the case that is interesting
to you, the two PCs will be in different DW_TAG_subprogram trees.
Given the order in which we parse things I'm not sure if you'll be able
to check that easily.

I don't much like the idea of your hack.  However, since no one has
had the time for a thorough fix _still_, if you can get it to work, it
may be a good idea.

I haven't looked at your implementation yet, though.

-- 
Daniel Jacobowitz

Re: RFC: The Constructor Breakpoint Issue

[David Lecomber]

Hi Dan
> You can't tell from the line table.  You can tell if you have both a
> line table and a .debug_info section; in the case that is interesting
> to you, the two PCs will be in different DW_TAG_subprogram trees.
> Given the order in which we parse things I'm not sure if you'll be able
> to check that easily.

How about this idea.. Getting into the dwarf2 area looks like quite a
major set of mods, which ultimately will be supported only by the
dwarves, which is not good.  However, inspection of the result of
find_pc_function() suggests it is different on the occasions when it
needs to be, such as in C++ constructors, and the same when we want it
to be - such as the start and end of for loops.

Cheers
d.


d.

Re: RFC: The Constructor Breakpoint Issue

[Jim Blandy]

David Lecomber <david@streamline-computing.com> writes:
> > You can't tell from the line table.  You can tell if you have both a
> > line table and a .debug_info section; in the case that is interesting
> > to you, the two PCs will be in different DW_TAG_subprogram trees.
> > Given the order in which we parse things I'm not sure if you'll be able
> > to check that easily.
> 
> How about this idea.. Getting into the dwarf2 area looks like quite a
> major set of mods, which ultimately will be supported only by the
> dwarves, which is not good.  However, inspection of the result of
> find_pc_function() suggests it is different on the occasions when it
> needs to be, such as in C++ constructors, and the same when we want it
> to be - such as the start and end of for loops.

What you're suggesting here, if I understand, is to take all the
occurrences of a given source line in the line table (and there may be
many, due not just to things like 'for' loops, but also due to
instruction scheduling and other sorts of optimizations), look to see
which function each falls in, and report the first occurrence from
each function.

Sounds good to me (although note that I don't approve symtab changes
any more, so this is just advice).  Some thoughts:

The line number lookup code actually looks for "best" matches, not
just exact matches; see symtab.c:find_line_common.  Preserving that
behavior will require a bit of care.

The downside that comes to mind is that this approach requires GDB to
always traverse the full linetable, instead of stopping as soon as it
finds an exact match, as it does now.

It'll be a substantial change to the line lookup machinery, since
those functions are not designed, at present, to return multiple
values.  But it's not that hairy in the aerial view.

There are some opportunities for optimization.  A symtab's linetable
and its blockvector's blocks are both sorted in order of increasing
addresses.  So one could traverse the blockvector and linetable in
parallel (skipping blocks that don't represent functions), and avoid
doing a full symbol lookup by pc for each plausible line.

Re: RFC: The Constructor Breakpoint Issue

[Daniel Jacobowitz]

On Wed, Oct 20, 2004 at 11:00:25AM -0500, Jim Blandy wrote:
> The line number lookup code actually looks for "best" matches, not
> just exact matches; see symtab.c:find_line_common.  Preserving that
> behavior will require a bit of care.
> 
> The downside that comes to mind is that this approach requires GDB to
> always traverse the full linetable, instead of stopping as soon as it
> finds an exact match, as it does now.

I think the data structures involved will have to change to do this, or
it will just be too painful.

-- 
Daniel Jacobowitz

Re: RFC: The Constructor Breakpoint Issue

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:

> On Wed, Oct 20, 2004 at 11:00:25AM -0500, Jim Blandy wrote:
> > The line number lookup code actually looks for "best" matches, not
> > just exact matches; see symtab.c:find_line_common.  Preserving that
> > behavior will require a bit of care.
> > 
> > The downside that comes to mind is that this approach requires GDB to
> > always traverse the full linetable, instead of stopping as soon as it
> > finds an exact match, as it does now.
> 
> I think the data structures involved will have to change to do this, or
> it will just be too painful.

Will it be such a big change?  It seems reasonable to assume that line
number queries are evenly distributed across the line tables.  So on
average, every query traverses half the line table now.  This change
would double the number of entries we examine for a single query.  So
you've doubled your search time, and doubled your working set for the
query.

Unlike queries by address, I can't think offhand of a situation where
queries by line occur that are not in response to user input.  This
doesn't seem like a show-stopper to me.


If the data structures do need to change, let me throw an idea out
there:

Our current structures are ordered by address, not by source location.
But adding a new structure indexed by source location could be a big
memory hit.

One way to reduce the size of that structure is to record the
information inexactly.  That is, instead of a table mapping line
numbers to lists of address ranges, we could have a table mapping
*ranges* of line numbers to lists of address ranges.  Or, ranges of
line numbers to lists of blocks, since blocks have start and end
addresses.  Then we could then use this information to narrow down the
range of PC values we need to search; since the line table is already
sorted by PC (I don't know why we don't use a binary search there),
that would cut down the amount of linear searching needed to something
acceptable.