Bugzilla – Full Text Bug Listing
| Summary: | [vm][cache] Model errors should be propagated to viewer. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Anton Leherbauer <aleherb+eclipse> | ||||||
| Component: | cdt-debug-dsf | Assignee: | Anton Leherbauer <aleherb+eclipse> | ||||||
| Status: | RESOLVED DUPLICATE | QA Contact: | Pawel Piech <pawel.1.piech> | ||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | ken.ryall, marc.khouzam, pawel.1.piech | ||||||
| Version: | 7.0 | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Anton Leherbauer
I noticed that VariableVMNode overrides handleFailedUpdate to sometimes set the update status.
@Override
protected void handleFailedUpdate(IViewerUpdate update) {
if (update instanceof IExpressionUpdate) {
update.setStatus(new Status(IStatus.ERROR, DsfUIPlugin.PLUGIN_ID, IDsfStatusConstants.INVALID_STATE, "Update failed", null)); //$NON-NLS-1$
update.done();
} else {
super.handleFailedUpdate(update);
}
Could this be used as a general solution?
(In reply to comment #1) > Could this be used as a general solution? I thought about that, too. It would work, I guess. My only concern is just that the status from the RM should be used, because that's what the ViewerDataRequestMonitor does. On the other hand, the simplicity of this solution is very tempting. Created attachment 169099 [details]
Proposed fix
This is the fix based on Marc's suggestion. It seems to work nicely.
(In reply to comment #3) > Created an attachment (id=169099) [details] > Proposed fix > > This is the fix based on Marc's suggestion. It seems to work nicely. Anything standing in the way of committing this today? I'd like to get a new build with EDC debugging working again. (In reply to comment #3) > Created an attachment (id=169099) [details] > Proposed fix > > This is the fix based on Marc's suggestion. It seems to work nicely. I just noticed that with this fix, after termination, the launch processes are no longer displayed. After testing the patch many times, I'm disappointed to say I get inconsistent behavior. It is always a much better result than before, but it is inconsistent. So, when launching a DSF-GDB session with a 2 seconds sleep at the start and _not_ stopping on main, I've seen such things as: - the launch is not expanded automatically - the launch is expanded and the container show a thread - the launch is expanded and the container has no children - when hitting the first breakpoint, the container icon shows suspended - when hitting the first breakpoint, the container icon shows running We may be opening up a can of worms with this change. I still think this fix is good, but maybe we should wait for the next CDT release? About the patch: In the change to DefaultVMModelProxyStrategy.java, did you forget to revert the line crm.done() instead of super.handleCompleted(); Also, can we trust getData() in the case of failure? I was thinking that the code would be even safer by doing if (isSuccess()) { counts[nodeIndex] = getData(); } else { counts[nodeIndex] = 0; } crm.done(); just in case getData() has garbage that is not null. (In reply to comment #5) > We may be opening up a can of worms with this change. I still think this fix > is good, but maybe we should wait for the next CDT release? Do you see the inconsistent behavior also with the patch I attached to bug 312817? I see that the launch is empty after terminating, but I don't see the inconsistent behavior. This fix is probably too general as I suspected. Still, caching failed updates is a serious issue. For 7.0 it might be safer to apply fixes only case-by-case, though. > About the patch: > > In the change to DefaultVMModelProxyStrategy.java, did you forget to revert the > line > crm.done() > instead of > super.handleCompleted(); Yes, indeed. > Also, can we trust getData() in the case of failure? > > I was thinking that the code would be even safer by doing > > if (isSuccess()) { > counts[nodeIndex] = getData(); > } else { > counts[nodeIndex] = 0; > } > crm.done(); > > just in case getData() has garbage that is not null. I agree, that's better. Created attachment 169135 [details]
Conservative fix
This fix is less general and should be safe.
(In reply to comment #7) > Created an attachment (id=169135) [details] > Conservative fix > > This fix is less general and should be safe. Does this fix address the EDC stack frame selection problem for you? I've committed the changes to EDC to have the stack service return an error if there are no stack frames. Just finished some testing: the first patch seems to fix the problem with stack frame selection. With the second patch the selection is maintained occasionally but fails inconstantly. (In reply to comment #6) > Do you see the inconsistent behavior also with the patch I attached to bug > 312817? With just the last fix of bug 312817, I still see the inconsistent behavior maybe once out of five launches. > I see that the launch is empty after terminating, but I don't see the > inconsistent behavior. Funny. For me, in that case, the launch is not empty after terminating. In fact, the launch only ends up empty after terminate for the 'too general' solution of patch "proposed fix" (In reply to comment #7) > This fix is less general and should be safe. I still see the inconsistent behavior with this patch :-( It could be a DSF-GDB services issue that was just brought to light. I will agree with Pawel in Bug 312817 comment #22 and say that we should leave this fix for the next CDT release and back out bug 310992 for this release. Not a bug. This was marked as INVALID because of Bug 312817 comment #22 which states "I think I'm the minority view here, but I don't think there's anything wrong with caching results of an error if the error is due to INVALID_STATE. That's because once the state changes, the model should send an event, view cache should get cleared and then valid data should be fetched. What gets us in trouble is the optimization that we've put in for stepping. This optimization tries to delay the refreshing of everything but the top stack frame, and then refresh everything else that needs to be updated after a brief delay (unless another step operation is started before then). Unfortunately, if we don't flush information about the list of children of threads and containers, we are not always able to calculate the correct delta to expand the thread stack and select the top stack frame. IMO at this point in the release cycle, we should just back out bug 310992 and sort out how to fix the stepping optimization after 7.0 (in 7.1 perhaps)." It may be too soon to mark this invalid. The changes suggested here are valid in that propagating the errors to the viewer is the right thing to do, and my assumption that it would cause the viewer to display errors in an ugly form should also be tested... and if true corrected. Also, I while I think INVALID_STATE and INVALID_HANDLE errors should be correctly cached, other errors, such as REQUEST_FAILED perhaps should not. Found a previous thread on this issue. *** This bug has been marked as a duplicate of bug 195722 *** |