Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 360280 - [breakpoints] Reposition breakpoints when planted on invalid line
Summary: [breakpoints] Reposition breakpoints when planted on invalid line
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug (show other bugs)
Version: 8.0   Edit
Hardware: PC Linux
: P3 enhancement (vote)
Target Milestone: 8.1.0   Edit
Assignee: Pawel Piech CLA
QA Contact: Ken Ryall CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-07 16:24 EDT by Pawel Piech CLA
Modified: 2014-01-29 22:57 EST (History)
10 users (show)

See Also:
nobody: review+
pchuong: review+


Attachments
Proposed patch (49.04 KB, patch)
2012-02-01 01:07 EST, Scott Tepavich CLA
cdtdoug: iplog+
Details | Diff
Patch to PDA to support repositioning breakpoints. (23.85 KB, patch)
2012-02-07 17:12 EST, Pawel Piech CLA
cdtdoug: iplog-
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pawel Piech CLA 2011-10-07 16:24:37 EDT
If user sets a breakpoint on a line where non an invalid source line, there should be a way for the debugger to reposition the breakpoint once it is installed on target.  

Workflow details:
1) User is able to create breakpoints at any location in the source.  This should remain unchanged to avoid having the tool restrict the user from doing something non-obvious.

2) Once breakpoint is created the debugger integration sends a request to debug engine to plant a breakpoint at a given location.

3) Debug engine comes back with the line number where the breakpoint was actually planted, this may be different than the requested line.

4) The debugger integration, updates the breakpoint with the planted breakpoint location.

5) Breakpoint is moved in editor to the new location.  In breakpoints view, the breakpoint label shows the original line followed by the actual planted line.  The icon overlay is a small arrow instead of the checkmark (although this stuff is too small for most people to discern anyway).

6) When debug session is terminated.  The breakpoint is moved back to the original location selected by user.

Implementation details:
Instead of tracking breakpoint location using the single IMarker.LINE_NUMBER property, the line number ends up being tracked by three properties:
ATTR_REQUESTED_LINE, ATTR_PLANTED_LINE_LIST, and IMarker.LINE_NUMBER.  The list of planted line locations allows for resolving conflicts between multiple debug targets.  Multiple targets moving a breakpoint to different locations is a rare and not important in terms of workflow, but it's important technically as it could lead to dangerous feedback loops.
Comment 1 Kirk Beitz CLA 2011-11-04 06:37:41 EDT
adding Ling to the CC: list, because for Nokia, he implemented something like this in the last few months for EDC , and might have comments/pointers to code that could be re-factored into CDT instead.
Comment 2 Pawel Piech CLA 2011-11-04 16:33:10 EDT
Thanks, Ling please do share any experience you've had implementing this feature.  

I've recently hooked up CDT breakpoints to our proprietary debugger back end, and got the repositioning working a bit too easily it seemed.  So this may be a quick feature.  The problem I have, however, is that I rely on our debugger engine to provide the correct line #, but GDB does not seem to give me that information.  So I may have to use PDA as the reference implementation, which is not ideal.
Comment 3 Ling Wang CLA 2011-11-07 15:05:18 EST
(In reply to comment #2)
> Thanks, Ling please do share any experience you've had implementing this
> feature.  
> 
> I've recently hooked up CDT breakpoints to our proprietary debugger back end,
> and got the repositioning working a bit too easily it seemed.  So this may be a
> quick feature.  The problem I have, however, is that I rely on our debugger
> engine to provide the correct line #, but GDB does not seem to give me that
> information.  So I may have to use PDA as the reference implementation, which
> is not ideal.

I implemented it for EDC (see bug 323457 for detail). It relies on the symbol reader (the Dwarf parser) in EDC to provide the correct line #. The API defined and used is (see bug 337493 for detail):
org.eclipse.cdt.debug.edc.internal.services.dsf.Modules.findClosestLineWithCode()

And the code that moves the breakpoint is in
org.eclipse.cdt.debug.edc.internal.services.dsf.BreakpointAttributeTranslator.resolveBreakpoint()

So my solution is similar to your but not really applicable to GDB.
Comment 4 Ling Wang CLA 2011-11-07 15:53:29 EST
(In reply to comment #0)
> Workflow details:
> 1) User is able to create breakpoints at any location in the source.  This
> should remain unchanged to avoid having the tool restrict the user from doing
> something non-obvious.
> 
> 2) Once breakpoint is created the debugger integration sends a request to debug
> engine to plant a breakpoint at a given location.
> 
> 3) Debug engine comes back with the line number where the breakpoint was
> actually planted, this may be different than the requested line.

With EDC that has no debugger backend, the symbol reader tells debugger which line in the neighborhood of the original line can have a breakpoint set on it.

 
> 
> 4) The debugger integration, updates the breakpoint with the planted breakpoint
> location.
> 
> 5) Breakpoint is moved in editor to the new location.  In breakpoints view, the
> breakpoint label shows the original line followed by the actual planted line. 
> The icon overlay is a small arrow instead of the checkmark (although this stuff
> is too small for most people to discern anyway).
> 
> 6) When debug session is terminated.  The breakpoint is moved back to the
> original location selected by user.

The above #5 and #6 are nice to have, but I don't see real value to users.

> 
> Implementation details:
> Instead of tracking breakpoint location using the single IMarker.LINE_NUMBER
> property, the line number ends up being tracked by three properties:
> ATTR_REQUESTED_LINE, ATTR_PLANTED_LINE_LIST, and IMarker.LINE_NUMBER.  The list
> of planted line locations allows for resolving conflicts between multiple debug
> targets.  Multiple targets moving a breakpoint to different locations is a rare
> and not important in terms of workflow, but it's important technically as it
> could lead to dangerous feedback loops.

That is a hairy case we thought about but didn't bother to support in EDC as it is really rare to run into the "dangerous feedback loop". To run into it, you have to debug the same code on target A and B simultaneously, and the compilers on A and B have to be different and especially different when generating line info for the same source line(s). 
Other than that, if the user just debugs on target A and B one after another (which is kind of common in practice), the outcome may not be best (breakpoint may be moved to a line that's not the best candidate) but not unacceptable either.
Comment 5 Scott Tepavich CLA 2012-02-01 01:07:22 EST
Created attachment 210352 [details]
Proposed patch

Attached is a proposed patch to allow breakpoint relocation.
Comment 6 Pawel Piech CLA 2012-02-01 11:13:05 EST
(In reply to comment #5)
> Created attachment 210352 [details]
> Proposed patch
> 
> Attached is a proposed patch to allow breakpoint relocation.

Hi Scott,
I've worked with you to polish this patch, but for everyone else's benefit, could you list the details of what the patch implements?
Comment 7 Scott Tepavich CLA 2012-02-01 20:40:16 EST
Hey Pawel –

The patch enables the underlying framework/implementation to manage relocating breakpoints (when the backend debugger is capable).  It does so without breaking backward compatibility.  The patch basically consists of the following:
    1.)	New ICLineBreakpoint2  and ICBreakpoint 2 interfaces (which extend the existing).
These were created to preserve backward compatibility, while allowing the definition of new attributes:
    REQUESTED_LINE – track the user specified line.
    REQUESTED_SOURCE_HANDLE – track the user specified source
    REQUESTED_CHAR_START/REQUESTED_CHAR_END – track user specified column.
And methods to manage these attributes.
    2.)	These attributes then allow the existing breakpoint classes, to update usable messages to the user, when their breakpoint has been relocated to another line.
    3.)	A small update to allow the user to edit a relocated breakpoint to another location.

Thank you very much for the time you spent on the review and feedback.
Comment 8 Pawel Piech CLA 2012-02-07 17:02:01 EST
Hi Mikhail (or anyone else with a vested interested), could you review the API extensions for the ICBreakpoint and ICLineBreakpoint interfaces?  

The patch itself only enables debugger integrations to reposition the breakpoint safely.  It does not change the gdb debugger integration since the gdb debugger does not return the valid source line upon planting the breakpoint.  I did update the PDA debugger integration to validate and test the feature and I'll attach that patch as well.
Comment 9 Pawel Piech CLA 2012-02-07 17:12:08 EST
Created attachment 210697 [details]
Patch to PDA to support repositioning breakpoints.

In order to validate this API feature I updated the PDA example to support CDT breakpoints and update the install count upon breakpoint install.  To do so, I had to migrate PDA to the BreakpointMediator2 implementaiton.  I also had to make some changes to the mediator 2 to make it work.  

To set the CDT breakpoint, user has to open the PDA source file using the C editor, then double-click.  To force a breakpoint to reposition to another line, the PDABreakpointAttributeTranslator.updateBreakpointStatus() needs to be rigged a little (uncomment the commented out block).  

I'll plan to commit these changes to the PDA example along with the main patch.
Comment 10 Pawel Piech CLA 2012-02-07 17:13:46 EST
(In reply to comment #9)
> ...
> breakpoints and update the install count upon breakpoint install.  To do so, I
> had to migrate PDA to the BreakpointMediator2 implementaiton.  I also had to
> make some changes to the mediator 2 to make it work.  

Patrick, I believe you are using the BreakpointMediator2, can you take a look at the changes I made to make sure it doesn't break anything for you?
Comment 11 Nobody - feel free to take it CLA 2012-02-07 18:25:05 EST
(In reply to comment #8)
> Hi Mikhail (or anyone else with a vested interested), could you review the API
> extensions for the ICBreakpoint and ICLineBreakpoint interfaces?  
>

I'll look at it.
 
> The patch itself only enables debugger integrations to reposition the
> breakpoint safely.  It does not change the gdb debugger integration since the
> gdb debugger does not return the valid source line upon planting the
> breakpoint.

Do you mean gdb doesn't reposition a breakpoint set at an invalid line, right?
Comment 12 Pawel Piech CLA 2012-02-07 23:40:16 EST
(In reply to comment #11)
> Do you mean gdb doesn't reposition a breakpoint set at an invalid line, right?
Right... I think :-)  It certainly plants the breakpoint on a valid address but it does not echo back what line number that address corresponds to.
Comment 13 Marc Khouzam CLA 2012-02-08 09:26:39 EST
(In reply to comment #12)
> (In reply to comment #11)
> > Do you mean gdb doesn't reposition a breakpoint set at an invalid line, right?
> Right... I think :-)  It certainly plants the breakpoint on a valid address but
> it does not echo back what line number that address corresponds to.

I had brought this up a while ago to the GDB community.  I am not aware of any improvements, although it would be worth trying with GDB 7.4:
http://sourceware.org/ml/gdb/2009-04/msg00142.html
Comment 14 Nobody - feel free to take it CLA 2012-02-08 12:23:48 EST
The API looks good to me. I didn't tried the PDA patch though.
Comment 15 Patrick Chuong CLA 2012-02-08 13:11:07 EST
Pawel, the patch looks good. I don't use BreakpointMediator for TI breakpoint, however, I do use dsf breakpoint VM* implementations.
Comment 16 Nobody - feel free to take it CLA 2012-02-08 13:25:16 EST
Trying to fix the review flag.
Comment 17 Pawel Piech CLA 2012-02-08 14:52:59 EST
Cool.  I committed the changes.  Thanks for the review.
Comment 18 CDT Genie CLA 2012-02-29 13:24:34 EST
*** cdt git genie on behalf of Scott Tepavich ***

    Bug 360280 - [breakpoints] Reposition breakpoints when planted on invalid line

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=b9255e88a54c84de5af5774cecb4bdad0373e5fa
*** cdt git genie on behalf of Scott Tepavich ***

    Bug 360280 - [breakpoints] Reposition breakpoints when planted on invalid line

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=b9255e88a54c84de5af5774cecb4bdad0373e5fa
Comment 19 CDT Genie CLA 2012-02-29 13:24:36 EST
*** cdt git genie on behalf of Pawel Piech ***

    Bug 360280 - [breakpoints] Reposition breakpoints when planted on invalid line (PDA Example - Added updating of C Breakpoint status to set installed flag and update line number)

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=1dbaf3c1baad7a1fd713b8b2c211f13d442eda7a
Comment 20 CDT Genie CLA 2012-02-29 13:24:39 EST
*** cdt git genie on behalf of Pawel Piech ***

    Bug 360280 - [breakpoints] Reposition breakpoints when planted on invalid line (PDA Example - Added updating of C Breakpoint status to set installed flag and update line number)

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=1dbaf3c1baad7a1fd713b8b2c211f13d442eda7a