Bug 336888

Comment 1 Marc Khouzam 2011-02-10 21:28:28 EST

Created attachment 188733 [details]
Potential fix

This fix should tell GDB to set a method breakpoint when we have a method name.

However, looking at CDI's BreakpointManager#createMIBreakInsert() I saw the text copied below, which shows that using the line number is preferred over the method name.  In fact, GDB sets the breakpoint at a line number even if we create it with a method name.

I'm thinking we leave things alone and mark this bug as invalid.

  // GDB does not seem to accept function arguments when
  // we use file name:
  // (gdb) break file.c:Test(int)
  // Will fail, altought it can accept this
  // (gdb) break file.c:main
  // so fall back to the line number or
  // just the name of the function if lineno is invalid.
  int paren = function.indexOf('(');
  if (paren != -1) {
      if (no <= 0) {
          String func = function.substring(0, paren);
          line.append(func);
      } else {
          line.append(no);
      }
  } else {
      line.append(function);
  }

Comment 2 Marc Khouzam 2011-02-10 21:29:02 EST

Mikhail, does it make sense to you to leave method breakpoints to use the line number?

Comment 3 Marc Khouzam 2011-02-10 21:32:35 EST

Created attachment 188734 [details]
Comment to explain why we are using the line number

If Mikhail thinks it is ok to mark this bug as invalid, I will commit this patch which adds a comment so that we don't go through this again.

Comment 4 Nobody - feel free to take it 2011-02-11 14:52:39 EST

(In reply to comment #2)
> Mikhail, does it make sense to you to leave method breakpoints to use the line
> number?

Marc, in the CDI code line breakpoint is preferred only if the function name contains parentheses, otherwise it sets a breakpoint on a method. 
We also need to check if the latest GDB handles this problem better.

Replacing method breakpoints by line breakpoints internally is misleading. When source code is modified the line breakpoint can move to a completely different location.

Comment 5 Marc Khouzam 2011-02-11 15:00:44 EST

(In reply to comment #4)
> (In reply to comment #2)
> > Mikhail, does it make sense to you to leave method breakpoints to use the line
> > number?
> 
> Marc, in the CDI code line breakpoint is preferred only if the function name
> contains parentheses, otherwise it sets a breakpoint on a method. 
> We also need to check if the latest GDB handles this problem better.

I did try and although it is better, it is still inconsistent about the use or parentheses.  We should still remove them before setting the bp.

> Replacing method breakpoints by line breakpoints internally is misleading. When
> source code is modified the line breakpoint can move to a completely different
> location.

That is a very good argument.  So we should definitely set the method breakpoint.  What do you think about my first patch then "Potential fix".  That should do the trick.

One question though, if the source code changes, won't the marker of the method bp have a line number that is no longer valid?  Is that a problem?

Comment 6 Patrick Chuong 2011-02-11 15:06:51 EST

Marc, we have the same problem to tackle, the marker needs to be shifted. 

Did you also consider linked resource for setting breakpoint? The marker needs to associate with the linked resource, but the breakpoint needs to know about the physical file. Does gdb need this behavior too?

Comment 7 Marc Khouzam 2011-02-11 15:26:59 EST

(In reply to comment #6)
> Marc, we have the same problem to tackle, the marker needs to be shifted. 

JDT is not perfect about that either I think.
I wonder how important it really is to handle code changes for breakpoints?

> Did you also consider linked resource for setting breakpoint? The marker needs
> to associate with the linked resource, but the breakpoint needs to know about
> the physical file. Does gdb need this behavior too?

I've never used Eclipse Linked Resources (unless you are talking about symbolic links in Linux?)

Comment 8 Nobody - feel free to take it 2011-02-11 15:33:39 EST

(In reply to comment #5)
> That is a very good argument.  So we should definitely set the method
> breakpoint.  What do you think about my first patch then "Potential fix".  That
> should do the trick.
> 

I've tried your fix and it seems to work.

> One question though, if the source code changes, won't the marker of the method
> bp have a line number that is no longer valid?  Is that a problem?

When editing a file with the C editor everything seems to be fine: the marker moves with the line. The hover info is wrong because it is not updated.
The problem is when the file is modified outside of Eclipse but I think nothing we can do about it.

Another thing I have noticed is that a method breakpoint doesn't appear in the disassembly.

Comment 9 Patrick Chuong 2011-02-11 15:52:32 EST

(In reply to comment #7)
> I've never used Eclipse Linked Resources (unless you are talking about 
> symbolic links in Linux?)

no, not symbolic links. You can add a new file to a project by linking it to a physical file. i.e a file in another project.

Comment 10 CDT Genie 2011-02-18 06:23:04 EST

*** cdt cvs genie on behalf of mkhouzam ***
Bug 336888: Method breakpoints are not set as actual method breakpoints

[*] MIBreakpoints.java 1.17 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpoints.java?root=Tools_Project&r1=1.16&r2=1.17

Comment 11 Marc Khouzam 2011-02-18 06:38:28 EST

(In reply to comment #8)
> (In reply to comment #5)
> > That is a very good argument.  So we should definitely set the method
> > breakpoint.  What do you think about my first patch then "Potential fix".  That
> > should do the trick.
> > 
> 
> I've tried your fix and it seems to work.

I've committed it to HEAD.
Mikhail, can you mark as reviewed (formality)

> Another thing I have noticed is that a method breakpoint doesn't appear in the
> disassembly.

I've opened Bug 337554 and Bug 337555

Comment 12 Marc Khouzam 2011-02-18 06:40:00 EST

(In reply to comment #6)
> Marc, we have the same problem to tackle, the marker needs to be shifted. 
> 
> Did you also consider linked resource for setting breakpoint? The marker needs
> to associate with the linked resource, but the breakpoint needs to know about
> the physical file. Does gdb need this behavior too?

I don't fully grasp the issue.  Can you give me an example so I can reproduce the problem?

Also, can you open a new bug if you feel we should?

I'll mark the current bug as fixed.