Bugzilla – Full Text Bug Listing
| Summary: | Memory Browser updates UI when target is in Run and Halt states | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Alain Lee <a-lee> | ||||
| Component: | cdt-memory | Assignee: | cdt-debug-inbox <cdt-debug-inbox> | ||||
| Status: | CLOSED DUPLICATE | QA Contact: | Ted Williams <ted> | ||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | cdtdoug, john.cortell, marc.khouzam, Randy.Rohrbach | ||||
| Version: | 8.0 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 7 | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Alain Lee
Created attachment 204774 [details] patch This is in additiona to the fix for Bug 356346 Lee, it's not clear to me what you're claiming is not working well. If GUI items need to be updated when transitioning between the running and suspended states, and you're scenario involves making these transitions very often, it sounds like flickering is something to be expected. (In reply to comment #2) > Lee, it's not clear to me what you're claiming is not working well. If GUI > items need to be updated when transitioning between the running and suspended > states, and you're scenario involves making these transitions very often, it > sounds like flickering is something to be expected. John, Randy fixed most of the flickering in Bug 356346. However, I still cannot enter an expression in the memory Browser expression area if a continuous breakpoint is runing. When the target is switched from halt to run or vice versa, Memory Browser tries to set the text in the Go To Address box. Since the continuous breakpoint happens so often, the cursor in the Go To Address ia always reset to the beginning of the text field. Therefore, I was not able to enter a text. If the debug context event shows that the context is the same as the current context, it is not necessary to update the tab and the Go To Address box. (In reply to comment #3) > John, > Randy fixed most of the flickering in Bug 356346. However, I still cannot enter > an expression in the memory Browser expression area if a continuous breakpoint > is runing. When the target is switched from halt to run or vice versa, Memory > Browser tries to set the text in the Go To Address box. Since the continuous > breakpoint happens so often, the cursor in the Go To Address ia always reset to > the beginning of the text field. Therefore, I was not able to enter a text. If > the debug context event shows that the context is the same as the current > context, it is not necessary to update the tab and the Go To Address box. Like other debug views, when the target is running and target access target is unavailable, the memory browser should not show stale data. If it's doing so today, that's a bug IMO. I'm against adding anything that further entrenches that bad behavior. But I have issues with this patch beyond that. We shouldn't store a debug context in a view without a provision to clear that reference when the context goes away. Otherwise, the view will keep the object alive. A debug context may itself end up keeping quite a few things around that should all technically be cleaned up when a session ends. (In reply to comment #4) > Like other debug views, when the target is running and target access target is > unavailable, the memory browser should not show stale data. If it's doing so > today, that's a bug IMO. I'm against adding anything that further entrenches > that bad behavior. But I have issues with this patch beyond that. We shouldn't > store a debug context in a view without a provision to clear that reference > when the context goes away. Otherwise, the view will keep the object alive. A > debug context may itself end up keeping quite a few things around that should > all technically be cleaned up when a session ends. Our product allows users to enter an expression and show the data even though the target is running. Our debugger will halt the target, read the data from the target and resume the target. This is going to be an issue to our customers. (In reply to comment #5) > Our product allows users to enter an expression and show the data even though > the target is running. Our debugger will halt the target, read the data from > the target and resume the target. This is going to be an issue to our > customers. Interesting. OK, can you instead change the solution to not set the control's text if the control currently has the desired string? Would that not solve your problem? (In reply to comment #6) > Interesting. OK, can you instead change the solution to not set the control's > text if the control currently has the desired string? Would that not solve your > problem? This won't work in all cases. If I am typing in the Go To Address field and a debug change event happens, the desired string is obviously not the same as the one I am tying and the string I have typed will get overwritten. Probably, I should set the desired string only when I need to reload the expression list. I can work with Randy when he is back to work this week or next week. (In reply to comment #7) > This won't work in all cases. If I am typing in the Go To Address field and a > debug change event happens, the desired string is obviously not the same as the > one I am tying and the string I have typed will get overwritten. Probably, I > should set the desired string only when I need to reload the expression list. I > can work with Randy when he is back to work this week or next week. The moment you start trying to predictably control what's going to happen when the user and the debugger are both trying to set the UI at the same time...that sounds like a handful of trouble. Sorry. I wouldn't touch this with a ten foot pole. Curious. What do you do with all the other debug views whose content changes on transition from run to halt. Are they all providing the behavior you want and you're just having problem getting the memory browser to play nicely? (In reply to comment #8) > The moment you start trying to predictably control what's going to happen when > the user and the debugger are both trying to set the UI at the same time...that > sounds like a handful of trouble. Sorry. I wouldn't touch this with a ten foot > pole. > Curious. What do you do with all the other debug views whose content changes on > transition from run to halt. Are they all providing the behavior you want and > you're just having problem getting the memory browser to play nicely? So far, Memory Browser is the only one that is giving us problem. When the debug context is not changed, it is ok to update the state of the UI controls. I don't think it is necessary to update the UI controls' content. I think this is why Randy's patch does not reload the expression list if the context is not changed. For the same reason, I think we don't need to reload the expression from the previous PerformGo action to the text field as well. Right now, our issue is that users are unable to enter an expression. (In reply to comment #9) > So far, Memory Browser is the only one that is giving us problem. When the > debug context is not changed, it is ok to update the state of the UI controls. > I don't think it is necessary to update the UI controls' content. I think this > is why Randy's patch does not reload the expression list if the context is not > changed. For the same reason, I think we don't need to reload the expression > from the previous PerformGo action to the text field as well. Right now, our > issue is that users are unable to enter an expression. Really? Then somewhere I'm not following you. When the user Resumes the target (not 'Step', mind you), the Debug, Variables, Register, and Expressions view update their contents. How are you not getting flickering there? But in general, what you're trying to do is limit how the memory browser reacts to target state changes with the sole purpose of improving the usability of your specialized feature. Once you start introducing additional state and caching that is otherwise not needed (except to, again, help your feature), that makes me nervous. Such things aren't real improvements to the masses and they complicate the design, which in turn increases the likelihood of bugs. (In reply to comment #10) > Really? Then somewhere I'm not following you. When the user Resumes the target > (not 'Step', mind you), the Debug, Variables, Register, and Expressions view > update their contents. How are you not getting flickering there? > But in general, what you're trying to do is limit how the memory browser reacts > to target state changes with the sole purpose of improving the usability of > your specialized feature. Once you start introducing additional state and > caching that is otherwise not needed (except to, again, help your feature), > that makes me nervous. Such things aren't real improvements to the masses and > they complicate the design, which in turn increases the likelihood of bugs. For expression or register views, the data is updated or constantly refreshed. So, does the Memory Rendering. This is expected. However, the issue we are seeing is the Go To Address text field that is being reloaded constantly. This is the similar issue Randy was trying to adress in in Bug 356346. (In reply to comment #11) > For expression or register views, the data is updated or constantly refreshed. > So, does the Memory Rendering. This is expected. However, the issue we are > seeing is the Go To Address text field that is being reloaded constantly. This > is the similar issue Randy was trying to adress in in Bug 356346. I'm not crazy about the change in 356346. It makes an assumption that the saved expressions for a given context won't change from one suspended event to another. Now, in practice, that is a safe assumption to make. But why are we even making the assumption? It's not because there's a performance need for the optimization. It's purely to make a custom behavior in TI's product work better. And this particular TI feature builds on top of what I consider bad behavior to begin with. It relies on the memory browser not clearing its GUI when the target resumes, contrary to how other debug views work. It uses the stale GUI in the memory browser to give the user a means to temporarily suspend the target and show the memory at a new expression (or at the same expression, I suppose, if you just hit enter in the Go To bar). Now, while it's bad enough that the memory browser allows itself to go stale after a resume, at least we can give it credit for trying to *fully* refresh itself when the target next suspends. But what your proposed fixes do is hack away at that updating logic to try to "optimize it". Again, why optimize? Because the view fully updating itself gets in the way of the TI feature. So this cutting and snipping of the update logic seems ill motivated to me. Generally speaking, optimizations complicate an implementation--they often require additional state, as they do in this case. And the more state you have, the more can go wrong. The change in 356346 is isolated enough and harmless that I'm not going to make a big fuss about it. But it turns out it didn't really fix the TI issue. So now you're proposing further hacking of the update logic. I'm sorry, but vendors can change CDT as they see fit--their copy. Changes that are made to the eclipse.org copy should benefit the community as a whole, and not include specialized tweaks that serve no purpose other than to appease a custom feature in a vendor's product. Please know that I'm not trying to pick on TI. As you know, I've put in significant hours in the distant and recent past addressing issues you've brought up. I just really think these changes you're proposing don't belong in the community code. (In reply to comment #12) > I'm sorry, but vendors can change CDT as they see fit--their copy. Changes that > are made to the eclipse.org copy should benefit the community as a whole, and > not include specialized tweaks that serve no purpose other than to appease a > custom feature in a vendor's product. I haven't followed the details of this bug so what follows is not a comment on the technical validity or not of John's statement to this case but on the spirit of it. Tweaks made to CDT for features not part of CDT tend to complicate the public code base without improving it. Furthermore, those tweaks quickly become impossible to maintain, first because they are un-testable with stock CDT, and second because their reason for being is quickly forgotten and therefore the tweak is not kept up-to-date. I think the way around that is to actually contribute the feature for which the tweak is needed, if it makes sense for CDT. (In reply to comment #13) > I haven't followed the details of this bug so what follows is not a comment on > the technical validity or not of John's statement to this case but on the > spirit of it. > Tweaks made to CDT for features not part of CDT tend to complicate the public > code base without improving it. Furthermore, those tweaks quickly become > impossible to maintain, first because they are un-testable with stock CDT, and > second because their reason for being is quickly forgotten and therefore the > tweak is not kept up-to-date. > I think the way around that is to actually contribute the feature for which the > tweak is needed, if it makes sense for CDT. I have talked to the developers at TI. We can fix it in our code. *** This bug has been marked as a duplicate of bug 356346 *** Mark I understand your comments about features which do not help the common cause. I do not think this is the case here. If need be I will add the breakpoint functionality which this is representing to the PDA. That is what is is for. We have several features in debug CDT, which are only shown in PDA because the simpler GDB engine simply does not supply them ( register bit fields for example ). Randyu (In reply to comment #16) Randy, see latest comments in bug 356346. I've proposed changes that are improvements to the browser and that should address the problems you are having with your breakpoints (this bugzilla and duplicate 356346). |