Bugzilla – Full Text Bug Listing
| Summary: | You cannot enter an expression in the memory Browser expression area if a continuous breakpoint is runing | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Randy Rohrbach <Randy.Rohrbach> | ||||||
| Component: | cdt-memory | Assignee: | Randy Rohrbach <Randy.Rohrbach> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Ted Williams <ted> | ||||||
| Severity: | major | ||||||||
| Priority: | P2 | CC: | a-lee, cdtdoug, john.cortell, Randy.Rohrbach | ||||||
| Version: | 8.0 | Flags: | Randy.Rohrbach:
review?
(pawel.1.piech) |
||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Randy Rohrbach
*** cdt git genie on behalf of Randy Rohrbach ***
Bug 356346 - Do not reload expressions if the debug context is the same
as before.
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=b80903c9e09952184bebdeb275ab646416deac48
I have checked fixes in to the 8.0 maintenance branch and the 9.0 master branch. Randy *** cdt git genie on behalf of Randy Rohrbach ***
Bug 356346 - Do not reload expressions if the debug context is the same
as before.
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=a87ea0e3ab32a7ca56886dab7a6ec84500c89795
Note that the reproducibility steps do not in fact show a problem. A breakpoint with an ignore count does not repeatedly toggle the target state in Eclipse/CDT. The backend (gdb, whatever) might under the covers suspend the target, but that state doesn't surface to the user. You can, however, reproduce the described behavior by setting a breakpoint with a Resume action of, say, one second. That said, I disagree that the described behavior is a bug. The view has the right to fully update itself when a target suspends. The real problem here, IMO, is that the browser allows the user to enter an expression while the target is running. See my comments in bug 352805. If we neglect to disable the "Go To" field in the browser while the target is running, and the user tries to enter an expression during that time, then he's rolling the dice. If the target suspends while he's in the middle of entering the new expression...Sorry, Charlie. The view has the right to full update all its GUI when the target suspends. Again, what we really should be doing is preventing the user from trying to enter an expression in that scenario, not go the other way and try to make sure he's not interrupted by a suspend event. *** Bug 352805 has been marked as a duplicate of this bug. *** I've backed out the change since it does not in fact solve the issue. Furthermore, I contend there is no real issue here and that the proposed changes should not be pursued in the community code. See comment in duplicate bug 352805 for details. Closing this as invalid. *** cdt git genie on behalf of John Cortell ***
Bug 356346 - You cannot enter an expression in the memory Browser expression area if a continuous breakpoint is runing (backed out change; see bugzilla for explanation)
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=9445f4c6a4e165c65790585e0e79dcf2a86a49f7
Folks
John, I spent some time with this last night and I agree that the description
I supplied to reproduce the issue is incorrect. When I tested this with our
commercial product I was using breakpoints which have a continue ( after a
specific time option ). So with our debug engine this causes run/stop events
to occur on a regular basis.
In the current product the expression lists are maintained on a per debug
context basis. So when you switch between debuggees, you change lists. The
old code would redraw the expression lists without discerning whether it was
the same debuggee or not. This was causing editing which was occuring at the
time to be affected, by always returning to the beginning of the edit buffer.
So my mistake in creating the description here. I tested the fix with
WRS product and honestly thought the description I put in was workable.
I have retested with the changes in and yes, I am still seeing the issue.
Alain's changes are correct where he wants to put them and will resolve the
issue.
I read through your comments about comparing and saving the current debug
context for comparison and there is nothing wrong with doing so. Almost all
of the update policy evaluators keep track of old and new debuggees to try
and make optimal decisions about updating.
I do agree that any target data should naturally update on suspend and resume. However, that is not what is being managed here. The management is
of an expression list which has been entered by the users for multiple
debuggees. The lists are independent of target data and thus can and should
be managed differently.
Perhaps there are other solutions, but this is a legitimate problem which
should handled by the community. Although it may be difficult to reproduce
with the Open Source Reference model, it is real with several commercial
products. I tested Alain's fix in my environment and it does resolve the
issue. Not sure why I was fooled before, but I was.
John,
Please work with Alain on this and get the fix back in. It is real
and important. I am not as familiar with GIT and right now not of
real sound mind and body. I tore my knee up badly a week ago and I
had surgery 3 days ago to repair it ( completely tore the QUAD tendon
which means I cannot walk ). So I am have a knee which feels like it
is the size of a basketball and the painkillers are making me pretty
tired and fuzzy. I have six weeks of flat on my back and then therapy
for months to learn to walk again.
Thanks
Randy
(In reply to comment #8) Randy, I'm sorry to hear about your knee. It sounds pretty bad. I hope your recovery goes faster than you're predicting. As for this issue, I am unconvinced that there is a problem. The difficulty here is that the Go To control has a double role. It acts as both an input field and a label. When a suspend event occurs, it is the view's responsibility to update its content. That means not only showing the latest values in memory, but making sure that the view as a whole is coherent. The value of the Go To field, because it acts as a label, is part of what the view needs to make sure is "up to date". If the user has changed the text in the field, and a suspend event occurs, it is technically not sufficient for the view to just update the memory values. It needs to make sure the Go To field correctly identifies the memory that was just updated. What Alain's changes do is prevent the view from guaranteeing a complete coherent picture when a suspend event occurs. Why does he want that change? Because the view fully updating itself gets in the way of a custom TI feature which allows the user to enter a new expression while the target is running (to cause a brief suspension of the target to fetch and show the requested memory). I'm against hacking the view's update logic to support a particular vendor's custom feature that uses the view in a way that was it was not designed to be used. Let me try to put this argument to rest with a specific use case where the proposed changes to optimize/short circuit the updateTab() could degrade behavior. Lets consider a debug session with a regular breakpoint set. The user has the target suspended and the memory browser showing memory at 0x1000. He resumes the target. While the target is running, he starts typing a new expression into the Go To field ("gSometh") but has a change of heart and instead decides he's going to read some code to figure out what to do next. A few moments later, a breakpoint hits. The memory browser updates the values it is showing for the memory at 0x1000 but now, due to the proposed "fix", leaves the browser showing the expression as "gSometh". So you end up with a view that is basically lying to the user. It says: here is the contents of memory at expression gSometh. With the current behavior, this couldn't happen, since the view will fully update itself and ensure coherency of its GUI. John Thanks for the in-depth explanation. I guess we are just going to have to agree to disagree. Based on your description below you believe the expressions input area is representing the address of data being display. That is what the Label tabs for each created rendered do. For me the expressions entry area is just that, a free form entry that can be in any state totally independent of the state of the information being displayed. So lets use this example. I go to the browser and create a valid render. Then I start to edit a new expression, but not completely finishing it ( it is a very long complex expression to get right ). My target hits a breakpoint which updates the renderer I have. I go in to debug mode stepping many/many ( I have alot of patience ). Once I reach a certain point, I go back to finishing my edit. It is no longer there. Why should state changes effect data entry for a field like that. We just seem to have different philosophies about this field. Both Alain's customers and mine ran in to this. Perhaps there is some other better solution which would accommodate both of our views. Randy (In reply to comment #10) Randy, the label in the tab is the evaluated address of the expression. The browser uses the Go To field to display the *expression* that was used to create the tab. Create two renderings: one for "&main" and one for "&printf" and toggle back and forth between the two tabs. Do you still claim the browser isn't using the Go To field as a label? :-) The scenario you describe is indeed problematic, and it speaks to the more general dilemma I mentioned, which is that the browser uses the Go To field as both input and as a label. So, I'm starting to see a solution/compromise here. 1. Let's get the expression to show in the tab, along-side the resolved address, as is done in the Memory view. 2. Let's get the browser to stop shoving the expression in the Go To field when it updates, period. Not just if the new context = old context. What do you think? Oh, and regarding (1), we would show the expression only if the string differs from the resolved address. I.e., no point in having the tab say "0x1000 - 0x1000 <Traditional>" (In reply to comment #11) > 2. Let's get the browser to stop shoving the expression in the Go To field when > it updates, period. Not just if the new context = old context. > What do you think? Can you elaborate (2) above? I need to understand how this will affect any of the our existing features. (In reply to comment #13) > Can you elaborate (2) above? I need to understand how this will affect any of > the our existing features. The proposal there is that the browser will never shove anything into the Go To field (other then the history items). It will strictly be an input field. More specifically, it will never call clearAll() on the Combo field. It will always leave the first element alone and update the remaining ones to contain the relevant history expressions. (In reply to comment #12) > Oh, and regarding (1), we would show the expression only if the string differs > from the resolved address. I.e., no point in having the tab say > "0x1000 - 0x1000 <Traditional>" Memory Browser currently shows the viewportAddress (i.e. the address of the top left cell) in the tab. Is it going to be changed? For example, if the viewportAddress is 0x1000, the tab shows: "0x1000 <Traditional>" If I scroll down or page down to 0x2000, the tab shows "0x2000 <Traditional>" With the proposed change, which address will the tab show: "viewportAddress - expression <Traditional>", or "evaluated address - expression <Traditional>", or (In reply to comment #15) > Memory Browser currently shows the viewportAddress (i.e. the address of the top > left cell) in the tab. Is it going to be changed? > > For example, if the viewportAddress is 0x1000, the tab shows: > "0x1000 <Traditional>" > > If I scroll down or page down to 0x2000, the tab shows > "0x2000 <Traditional>" > > With the proposed change, which address will the tab show: > "viewportAddress - expression <Traditional>", or > "evaluated address - expression <Traditional>", or Interesting. I just assumed it was the static evaluated address. So what I propose is that we show "viewportAddress - expression(XXX) <Traditional>" where XXX is the offset of the viewport address from the expression's evaluated address. E.g., "0x2000 - gSomeVar(+0x1000) <Traditional>" Two additional notes: (a) The "()" would be omitted if the offset is zero (b) The expression would be omitted when a numeric value is entered as the expression (In reply to comment #16) > Interesting. I just assumed it was the static evaluated address. So what I > propose is that we show > "viewportAddress - expression(XXX) <Traditional>" > where XXX is the offset of the viewport address from the expression's evaluated > address. E.g., Is there any reason to show the offset rather than the evaluated address? (In reply to comment #17) > Is there any reason to show the offset rather than the evaluated address? Are you suggesting the following? 0x3000 - gSomeDir(0x1000) Personally, I find 0x3000 - gSomeDir(+0x2000) a lot more useful. It quickly tells me how far off (and in which direction) I've scrolled/paged from the expression. The other way requires doing some math in your head. The advantage may be more obvious with larger and less neater numbers. (In reply to comment #18) > Are you suggesting the following? > 0x3000 - gSomeDir(0x1000) > Personally, I find > 0x3000 - gSomeDir(+0x2000) > a lot more useful. It quickly tells me how far off (and in which direction) > I've scrolled/paged from the expression. The other way requires doing some math > in your head. The advantage may be more obvious with larger and less neater > numbers. Just curious. Both are fine with me. I will work on these changes. Folks I have just caught up with the discussion and this seems to be a reasonable compromise for everyone involved. Thanks for working on the compromise. I know I have been pretty out of touch the past few days. My situation is improving on a weekly basis. Randy Created attachment 206037 [details]
patch
(In reply to comment #22) > Created attachment 206037 [details] > patch I'll review this first thing next week. reopening; taking new approach (In reply to comment #22) > Created attachment 206037 [details] > patch Alain, I've been unsuccessful in applying the patch. I've tried both via eGit and via the git cmdline ('git apply <file>'). How did you create the patch? (In reply to comment #25) > (In reply to comment #22) > > Created attachment 206037 [details] [details] > > patch > Alain, I've been unsuccessful in applying the patch. I've tried both via eGit > and via the git cmdline ('git apply <file>'). How did you create the patch? I am looking into it. (In reply to comment #25) > (In reply to comment #22) > > Created attachment 206037 [details] [details] > > patch > Alain, I've been unsuccessful in applying the patch. I've tried both via eGit > and via the git cmdline ('git apply <file>'). How did you create the patch? John, I was able to apply the patch. I created the patch using eGit from Head. From the context menu Team->Show in history, I clicked Create Patch. When I applied the patch, 1. I selected the memoryBrowser plugin from the Target Resource dialog. 2. Click next. 3. In the Review Patch dialog, I selected '3' in Ignore leading path segments. (In reply to comment #27) > 3. In the Review Patch dialog, I selected '3' in Ignore leading path segments. Agh! Forgot about the 'leading path segments' thing. Thanks. Created attachment 206803 [details]
revised patch
I made some adjustments/fixes. The only behavioral changes are
1. the viewport displacement now shows the correct direction (was reversed in original patch)
2. show '+' when displacement is positive, instead of implying it.
3. Original patch neglected to clear history entries when Go To field is empty. I believe this would have appended one context's history to another's.
4. Improper comparison of expression to viewport. Creating a tab with a hex address would initially show "0x1000 - 0x1000 <Traditional>" instead of "0x1000 <Traditional>"
Non-behavioral changes: comments and style adjustments
Alain, please review these adjustments and validate them in your environment.
(In reply to comment #29) > Created attachment 206803 [details] > revised patch > I made some adjustments/fixes. The only behavioral changes are > 1. the viewport displacement now shows the correct direction (was reversed in > original patch) > 2. show '+' when displacement is positive, instead of implying it. > 3. Original patch neglected to clear history entries when Go To field is empty. > I believe this would have appended one context's history to another's. > 4. Improper comparison of expression to viewport. Creating a tab with a hex > address would initially show "0x1000 - 0x1000 <Traditional>" instead of "0x1000 > <Traditional>" > Non-behavioral changes: comments and style adjustments > Alain, please review these adjustments and validate them in your environment. I tested the revised patch. The changes look good to me. Pushed to master branch *** cdt git genie on behalf of John Cortell ***
Bug 356346 - You cannot enter an expression in the memory Browser expression area if a continuous breakpoint is runing
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=551220367b4fbf90df8a8fe3b3d0187342cfc672
|