Bug 166660 - [breakpoints] Debugger stops at breakpoint which is disabled
Summary: [breakpoints] Debugger stops at breakpoint which is disabled
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug (show other bugs)
Version: 3.1.1   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 4.0.3   Edit
Assignee: Doug Schaefer CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2006-12-04 12:02 EST by Elena Laskavaia CLA
Modified: 2008-06-22 02:22 EDT (History)
1 user (show)

See Also:


Attachments
Patch for 3.1 branch (32.16 KB, patch)
2006-12-04 15:21 EST, Elena Laskavaia CLA
bjorn.freeman-benson: iplog+
Details | Diff
Project to reproduce (324.94 KB, application/zip)
2008-01-02 17:44 EST, Elena Laskavaia CLA
no flags Details
Another patch for 4.0 (3.04 KB, patch)
2008-01-16 15:55 EST, Elena Laskavaia CLA
bjorn.freeman-benson: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elena Laskavaia CLA 2006-12-04 12:02:35 EST
Visible simptoms:
- debugger stop at breakpoint which is disabled
- debugger do not stop at breakpoint which is enabled
- debugger stops at breakpoint which is deleted
- no exceptions or errors are visible in console or log

Conditions to reproduce:
- Remote debugging
- Target machine slower than host
- Perform operation (disable, enable or delete) on several breakpoints (6 should be enough)
- Process under debugging should be running (or blocked in kernel call), but not suspended by debugger

Symptoms visible using mi debug options:
- After pressing disable on 6 breakpoints selection - only 3 breakpoints are actually get disabled, usually every second one
- Time that passes from first log output to last for single user operation (disabling 6 breakpoint) is 15 seconds (!!!)
Comment 1 Elena Laskavaia CLA 2006-12-04 15:06:17 EST
Investigation results:
Several problems in the code contribute to this behavior:
- All exception thrown by mi interface are IGNORED
- If operation is not performed (exception is thrown) 
breakpoints are left in inconsistent state,
disabled in ui, but enabled in debugger itself
- When mass breakpoints operation is performed (disable/enable)
it creates a job per each breakpoint, which is not really effective on the host,
but the worse part that it creates a massive attack on the target machine
sending suspend/resume signals for EACH breakpoint. 
- So if more than one breakpoint is disable if would send suspend, than resume, than suspend immediately. 
Target receive second suspend probably while processing precessing
resume, while it still thinks process is not running, so it will just ignore it.
So than debugger would wait 5 second for target to be suspended. As a result it would disable 3 breakpoints and
wait for 5 sec for 3 others which would miss suspend signal = 15 sec in total.
Comment 2 Elena Laskavaia CLA 2006-12-04 15:21:55 EST
Created attachment 54994 [details]
Patch for 3.1 branch

Unfortunately patch is not simple. Currently CDI interface
does not have a method that allow massive update for breakpoints. There
is deleteBreakpoints methods though.
So I added void updateBreakpoints(ICDIBreakpoint[] breakpoints, Boolean[] enable, ICDICondition condition[]) throws CDIException; 
method in ICDIBreakpointManagement and all implementors and change code
to call this method on breakpoint update event.
Also I have to add logging methods in MIPlugin and add logging instead of
ignoring MI exceptions.
And firing breakpoint update event if fails, which would
restore breakpoint state prior to update (except for delete, it needs to be fixed later too)

With the provided patch debugger does not problem disabling/enabling and deleting
as many breakpoints as you want and it happend in less than a second.
If problem happend it would log it in the eclipse log and restore breakpoint state prior to update event.
Comment 3 Nobody - feel free to take it CLA 2006-12-05 06:38:03 EST
Does your patch work when you enable/disable/delete breakpoints from the gdb console line?
Did you test it on different platforms? Or when there are several debug sessions running?
One of the reasons this is code so complicated is that we have to support the Eclipse breakpoint framework and the gdb console. This is the source of potential race conditions and deadlocks. All breakpoint operations are event-based and the breakpoint UI in Eclipse is not good enough for remote/slow targets.
Comment 4 Nobody - feel free to take it CLA 2006-12-05 06:55:31 EST
Another problem is logging. The log is intended to store the errors that happen in Eclipse/CDT. We still log some messages, but our intention is to get rid of it.
If a breakpoint operation fails it is definitely not an error in Eclipse. The UI should be able to indicate it to the user. Popping up error messages is not a good option, we have already tried it. Unfortunately at the moment there is no simple solutionfor it in Eclipse.
Comment 5 Elena Laskavaia CLA 2006-12-05 09:56:32 EST
I did not test it on another platforms, but I did it exactly as it was
working before, except that suspent event sent only ones than all
breakpoints events, than resume. I did not change methods that disable/enabled
breakpoint individualy, is this how console gdb works? How can I test it?
Do you see any problems how it might
affect other cases? Are there junit tests I can run to check that?

About logging - I don't want window to pop-up either. It just stores it
in the log, so you can see it if you open error log or errors view.
Comment 6 Elena Laskavaia CLA 2006-12-05 10:20:09 EST
Just tried disabling breakpoint from console. Works.
disable breakpoints 1-6   - works too
I tested logging before I fixed that - it did not pop-window - just
printed the problem in the error log.
Comment 7 Nobody - feel free to take it CLA 2006-12-05 10:37:27 EST
I can't say by looking at the code whether it will work or not. I spent many days catching the possible conditions and I need a time to see if the patch doesn't break anything.
Regarding logging. As I said the log is intented to keep the errors in Eclipse, not the inability of the target to do something. We used to log almost everything and we were getting bug reports for every log entry.
Comment 8 Elena Laskavaia CLA 2006-12-05 11:57:31 EST
We don't need to log everthing. But we must process errors.
If user disable breakpoint and target failed to process it
we must display either error dialog or at minimum log entry.
I don't see how we can just ignore the fact that user operation has failed
and get away with it. I spent several days to reproduce that problem 
becuase there was no indication of a failure.
Comment 9 Nobody - feel free to take it CLA 2006-12-05 12:23:41 EST
Again, it is not a purpose of log to provide information to the end users. We've been there before and that's not the right way to go. 
By the way, as far as I know the Error Log is not a part of the IDE QNX is shipping.
If you don't like how the breakpoints are presented in Eclipse come up with a better solution. 
Comment 10 Doug Schaefer CLA 2006-12-05 12:40:50 EST
Yes, the error log is for reporting software bugs. If there are things the user needs to deal with this should be shown in dialogs. Although I haven't seen a good consistent way accross Eclipse to do that.
Comment 11 Elena Laskavaia CLA 2006-12-05 12:45:34 EST
I agree with that, then it have to be a dialog
"Unable to disable some of the breakpoints" or something like
that (like in status line?). 
Comment 12 Nobody - feel free to take it CLA 2006-12-05 13:13:47 EST
Many users find the popup dialogs very annoying. That's why we decided to avoid it and use the labels in the views in most cases. Unfortunately it doesn't work for breakpoints.

Comment 13 Doug Schaefer CLA 2007-01-30 13:16:13 EST
I am going to look at this for 3.1.2. We have a major customer complaining about this and need an answer.
Comment 14 Doug Schaefer CLA 2007-02-05 16:05:02 EST
Taking a deeper look I see a number of problems with the synchronization with the MI Target suspend command. The problem is really visible when doing multiple suspend/resume command sequences as is done when enabling or disabling multiple breakpoints while the target is running. This really needs proper asynchronous command handling to queue up the commands until the target is properly suspended.

This is likely too much to do in the next couple of days and have proper testing. We'll likely need to push this out until 4.0. In the meantime Elena mentions that there are parts of this patch that will at least update the breakpoint statuses to properly reflect what is happening. I'll take a look at those parts for 3.1.2.
Comment 15 Nobody - feel free to take it CLA 2007-02-06 05:11:40 EST
Doug, is it possible to submit your changes to the 3.1.x branch but not in 3.1.2. We would like to have a more or less reliable product without serious last minute changes.
As I already mentioned the patch doesn't fix the problem, it only changes the timing, that makes it work in one particular situation on one particular computer. I am not opposing the changes Elena is proposing, but I am familiar with the problem and I know there is no simple solution for it.
When you are looking at the problem, please, keep in mind that breakpoints can be set/removed/changed from the gdb console as well as from the breakpoint view. There is also a situation when multiple debug sessions use the same source code and for example, the disable breakpoint operation can fail on one target but succeed on the other.
Comment 16 Doug Schaefer CLA 2007-02-06 07:14:59 EST
Whatever change I do will be for 3.1.2. I can easily reproduce the problem on cygwin and I'm sure on all MI based clients. This is a poor experience for everyone and my intention is to make it better for everyone. I am not planning on applying the patch as is but only those parts that improve the accuracy of breakpoint status.

In my opinion, basing algorithms on timing is highly unreliable as is and not what I'd expect in a reliable product.
Comment 17 Nobody - feel free to take it CLA 2007-02-06 07:43:07 EST
(In reply to comment #16)
I have seen this problem debugging Java code with JDT. It doesn't happen very often, but the CDT targets usually are slower.
Comment 18 Doug Schaefer CLA 2007-02-07 14:30:28 EST
After wittling away all the contraversial bits that I thought required significant testing, I wasn't left with much and it still didn't really solve the root of the problem.

So I'll push the whole issue off to 4.0 for a proper solution.
Comment 19 Doug Schaefer CLA 2007-06-04 10:52:01 EDT
I haven't been able to reproduce this in CDT 4. This could possibly be fixed. Can you try and reproduce it? I am going to move it off the CDT 4 list since we may need our own debugger integration running on top before we can fix.
Comment 20 Doug Schaefer CLA 2007-09-19 10:20:05 EDT
I'll look closer at debug issues post 4.0.1/CDT summit.
Comment 21 Doug Schaefer CLA 2007-11-13 15:46:47 EST
Pushing this out to 4.0.3. We may end up not fixing this at all and instead wait for DSF to save us...
Comment 22 Elena Laskavaia CLA 2008-01-02 17:44:45 EST
Created attachment 86035 [details]
Project to reproduce

I port my project to mingw. It is reproducible.
To reproduce:
- Import this project
- To compile it you need pthreads-win32 libraries and includes for mingw and fix library path for the project
- Try to create 5 breakpoints and enable them one by one quickly (or mass enable/disable)
- UI will pretend it is all good, but it did not actually enable them because of internal errors, which you cannot see either, only when you enable eclipse debug mi output and see no response from the debugger.
Comment 23 Elena Laskavaia CLA 2008-01-16 15:55:27 EST
Created attachment 87098 [details]
Another patch for 4.0

Another attempt to send a patch for this. This patch would not solve problem with target synchronization, but it hopefully fix problem with synchronization between UI and debugger state.

1) sent breakpoint change event in both cases: success or failure to enable breakpoint to make sure ui breakpoints reflect the actual state of CDI/MI breakpoints
2) make sure resumeInferior re-enable events processing in any case, in particular to prevent situation when suspend disable events and then throw an exception via attempt to suspend
3) change an order or re-enabling events and resume in case when resume can cause breakpoint be hit which can be missed otherwise
Comment 24 Doug Schaefer CLA 2008-01-25 16:01:49 EST
I've applied this patch. Please test to make sure nothing's broken especially with MinGW and maybe Linux gdb as well. Thanks!

Applied to HEAD and cdt_4_0.