Bug 333759 - "Variables" tool view sometimes not in sync with Frame Stack view
Summary: "Variables" tool view sometimes not in sync with Frame Stack view
Status: RESOLVED FIXED
Alias: None
Product: kdevelop
Classification: Applications
Component: CPP Debugger (show other bugs)
Version: 4.6.60
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: kdevelop-bugs-null
URL:
Keywords: junior-jobs
Depends on:
Blocks:
 
Reported: 2014-04-23 09:46 UTC by Kevin Funk
Modified: 2016-05-06 03:21 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Fix variable toolview not sync with framestack toolview (2.49 KB, patch)
2016-04-07 20:42 UTC, Peifeng Yu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Funk 2014-04-23 09:46:05 UTC
The Variables tool view sometimes shows local variables from a different stack frame than selected in the Frame Stack tool view.

Reproducible: Always

Steps to Reproduce:
1. Debug some application
2. Pause the debugger 
3. Hide the Variables tool view
3. Switch frame
4. Make the Variables tool view visible
Actual Results:  
Local variables of the previously selected frame are shown

Expected Results:  
Local variables of the currently selected frame should be shown
Comment 1 Peifeng Yu 2016-04-07 20:42:54 UTC
Created attachment 98282 [details]
Fix variable toolview not sync with framestack toolview
Comment 2 Peifeng Yu 2016-04-07 20:43:25 UTC
Hi, I'm looking into this bug right now. It seems the bug still exists in the latest version.

The exact steps to reproduce is as follow:
1. Debug some application
2. Pause the debugger
3. Collapse the variable list or hide the variables tool view
4. Switch to another frame, i.e. frame 1
5. Expand the variable list or show the variables tool view
6. *Important*: switch back to the same frame as in step 2

Actual Results:
Variables for frame 1 are shown

Expected Results:
Variables of the currently selected frame should be shown


The reason for this is that internally `IVariableController` keeps track of the current active frame/thread and only updates variables when active frame/thread changes. However, when variables list is hidden, the active frame/thread is not updated. Thus if the user changes frame in this case, the active frame/thread value remains what it was before the variables list is hidden. Then after making variables list visible and changing the frame again to the previous frame, `IVariableController` thinks there is not change at all and skips the update.


The attached patch fixes this issue by removing the whole tracking active frame/thread logic. This logic was introduced in 57b2d64f to make sure locals are updated only once. However, with the code evolving, other situations where update might be called with frame/thread unchanged are gone and the above is the only one left, which in fact also doesn't require such logic. Given this tracking logic makes no benefits but bugs only, I removed it in the patch which also fixes the bug.
Comment 3 Kevin Funk 2016-04-07 20:57:21 UTC
@Aetf: I'll have a look at the patch. Could you, in the meantime, post the patch to Phabricator, properly formatted including a commit message?

https://community.kde.org/Infrastructure/Phabricator#Posting_a_Patch
Comment 4 Kevin Funk 2016-04-07 21:13:46 UTC
I believe the patch can be simpler, without removing the "only update when active frame/thread changes" -- what about just clearing the active frame/thread in IVariableController::setAutoUpdate? Please have a look if this fixes the issue.
Comment 5 Peifeng Yu 2016-04-08 00:45:13 UTC
Posted on Phabricator now: https://phabricator.kde.org/D1351

Well, yes, it can be done by just make IVariableController::handleEvent correctly updates m_active{Frame,Thread} even if IVariableController::autoUpdate is UpdateNone. No need to touch IVariableController::setAutoUpdate though.

But I think there is duplication. Maybe my wording isn't clear, but the patch isn't removing the "only update when active frame/thread chagnes" functionality, as we only call IVariableController::update when handling IDebugSession::thread_or_frame_changed event. So no need to do the duplicate check (The update in IVariableController::setAutoUpdate handles different situations and is irrelevant here. However, I do wonder if it is needed all the time)

Besides, m_active{Frame,Thread} only mirror the same value in FrameStackModel::current{Frame,Thread} and it requires extra effort to keep them synced. So in my opinion we should remove m_active{Frame,Thread} to simplify the logic.

I understand removing them breaks binary compatibility. If that's what you are concerned, then I'm OK to change to the simpler workaround.
Comment 6 Kevin Funk 2016-04-08 06:53:29 UTC
@Niko: Can you remember why exactly you introduced this code in 57b2d64f?
Comment 7 Kevin Funk 2016-04-13 07:09:44 UTC
@Aetf: Please go for the simpler change, try if it works.

Note regarding "duplication": Yes, we might have a copy of thread/frame in IVariableController, but there's a reason for it: Updating the variables is expensive (lots of communication with GDB required), so we need to reduce the number of updates as much as possible. Thus we "cache" thread/frame here, not "duplicate".
Comment 8 Peifeng Yu 2016-04-13 18:33:09 UTC
(In reply to Kevin Funk from comment #7)
I am still not quite convinced (see below) but maybe the removal deserves it own patch. So, simpler fix submitted to Phabricator. 

For the "cache/duplicate" thing: I understand that we should avoid updating variables as much as possible due to communication costs. And that's what exactly the *thread_or_frame_changed* event handler for. Is there such a case that this handler is called without an actual change in active thread/frame? Given we now only check cached active thread/frame in this event handler, it looks duplicate to me.