Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 308055 - Register values are not updated when switching between frames
Summary: Register values are not updated when switching between frames
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-cdi (show other bugs)
Version: 6.0.2   Edit
Hardware: PC Windows 7
: P3 major (vote)
Target Milestone: 7.0   Edit
Assignee: Nobody - feel free to take it CLA
QA Contact: John Cortell CLA
URL:
Whiteboard:
Keywords:
: 256917 (view as bug list)
Depends on:
Blocks: 308052
  Show dependency tree
 
Reported: 2010-04-04 19:54 EDT by Nobody - feel free to take it CLA
Modified: 2010-07-27 05:15 EDT (History)
5 users (show)

See Also:
nobody: iplog-
nobody: review+


Attachments
Fix. (46.10 KB, patch)
2010-04-04 20:20 EDT, Nobody - feel free to take it CLA
nobody: iplog-
Details | Diff
Updated patch. (46.13 KB, patch)
2010-04-05 13:55 EDT, Nobody - feel free to take it CLA
nobody: iplog-
Details | Diff
Patch updated according John's suggestions. (45.78 KB, patch)
2010-04-05 19:36 EDT, Nobody - feel free to take it CLA
nobody: iplog-
nobody: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nobody - feel free to take it CLA 2010-04-04 19:54:35 EDT
This is a regression from earlier versions. Register values should be evaluated against the stack frame selected in the Debug view. Switching to another frame should force the re-evaluation of the entire view in the new context. 
This is a result of https://bugs.eclipse.org/bugs/show_bug.cgi?id=176627.
Comment 1 Nobody - feel free to take it CLA 2010-04-04 20:20:01 EDT
Created attachment 163787 [details]
Fix.

This patch replaces the default implementation provided by the platform. It contains the set of adapters required to integrate a CDI-backed debugger into the Registers view.
Comment 2 Nobody - feel free to take it CLA 2010-04-04 20:32:44 EDT
The proposed patch also fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=308052.
Comment 3 Nobody - feel free to take it CLA 2010-04-04 22:11:57 EDT
John, could you please review this patch. It's quite big and I'm not very comfortable submitting it without review. Please don't use GDB 7.0, there is a problem with it and you won't see registers at all.
Comment 4 John Cortell CLA 2010-04-05 11:19:50 EDT
Mikhail, with this patch, the Registers view is empty in a CDI session. Did you perhaps forget to include something in the patch file?
Comment 5 Nobody - feel free to take it CLA 2010-04-05 11:29:21 EDT
(In reply to comment #4)
> Mikhail, with this patch, the Registers view is empty in a CDI session. Did you
> perhaps forget to include something in the patch file?

Is it working without my patch? Which version of gdb you are using? There is a problem with 7.0.
Comment 6 John Cortell CLA 2010-04-05 11:36:20 EDT
Jeesh. I overlooked (forgot, really) your comment # 3. I'll try again :-)
Comment 7 Nobody - feel free to take it CLA 2010-04-05 11:52:38 EDT
(In reply to comment #6)
> Jeesh. I overlooked (forgot, really) your comment # 3. I'll try again :-)

I was told that the problem was fixed in the GDB HEAD.
Comment 8 John Cortell CLA 2010-04-05 12:11:47 EDT
Hm. Well, I tried with MinGW 6.8 gdb, and now I see content in the Registers view, but no values. I backed out your changes and the values show up. I reapplied your patch and, again, no values for the registers.
Comment 9 Nobody - feel free to take it CLA 2010-04-05 12:14:19 EDT
(In reply to comment #8)
> Hm. Well, I tried with MinGW 6.8 gdb, and now I see content in the Registers
> view, but no values. I backed out your changes and the values show up. I
> reapplied your patch and, again, no values for the registers.

Let me try my patch again, maybe something is missing.
Comment 10 Nobody - feel free to take it CLA 2010-04-05 13:24:05 EDT
(In reply to comment #8)
> Hm. Well, I tried with MinGW 6.8 gdb, and now I see content in the Registers
> view, but no values. I backed out your changes and the values show up. I
> reapplied your patch and, again, no values for the registers.

I can see the problem now. If the view is open before the session started everything works fine. Otherwise, the values appear only after stepping or resuming. Will fix.
Comment 11 Marc Khouzam CLA 2010-04-05 13:37:29 EDT
I didn't look at the cause of the problem but is it limited to CDI, and not affecting DSF?
Comment 12 Nobody - feel free to take it CLA 2010-04-05 13:46:59 EDT
(In reply to comment #11)
> I didn't look at the cause of the problem but is it limited to CDI, and not
> affecting DSF?

DSF doesn't support this feature. I was thinking of submitting a request for it.
Comment 13 Nobody - feel free to take it CLA 2010-04-05 13:55:59 EDT
Created attachment 163821 [details]
Updated patch.
Comment 14 Nobody - feel free to take it CLA 2010-04-05 13:58:05 EDT
John, please try the updated patch.
Comment 15 Nobody - feel free to take it CLA 2010-04-05 14:00:59 EDT
Correct version.
Comment 16 John Cortell CLA 2010-04-05 14:02:16 EDT
(In reply to comment #12)
> (In reply to comment #11)
> > I didn't look at the cause of the problem but is it limited to CDI, and not
> > affecting DSF?
> 
> DSF doesn't support this feature. I was thinking of submitting a request for
> it.

I could swear someone at Nokia opened one a while back.
Comment 17 John Cortell CLA 2010-04-05 16:33:47 EDT
Mikhail, here's my feedback. All minor stuff...

- in CRegisterManager.setCurrentFrame, the call to getRegisterGroups() would be better situated within the if scope. Within that loop, I'd improve readability by making use of Java 5; that would even do away with the need for the 'groups' local variable.
- shouldn't CRegisterManagerProxies query the context service rather than assume the Debug view is providing the context?
- I think the name RegisterViewerInputProvider is misleading, especially looking at it from the plugin.xml perspective. A CDT debug element type can only have one such provider, and it is not viewer specific. Now, it so happens that right now we only need the input translation for the Registers view, but if next month we need it for another view, RegisterViewerInputProvider would need to handle that view too, and clearly that would be confusing. I think a better name would be CViewerInputProvider
- The code in CDebugElementAdapterFactory that returns the RegisterViewerInputProvider could probably be consolidated a bit by having a single check that does three instanceof checks, following the pattern elsewhere in that method.
- Seems to me the method CRegisterManagerModelProxy.refreshRoot is extraneous; the logic could just be located in update()
- I think CRegisterManagerProxy.handleDebugEvents(DebugEvent[]) is subject to a race condition NPE. It should call getModelProxy() and store it in a local variable and use that local thereafter.
- I think the comment in CRegisterManagerProxyMementoProvider.getElementName(Object, IPresentationContext) is probably misleading. That code will maintain a single expansion state for all of CDT.
- I'd remove the commented out constants in RegistersViewColumnPresentation.
Comment 18 Nobody - feel free to take it CLA 2010-04-05 16:40:11 EDT
(In reply to comment #17)
Thanks John, I owe you a drink, "White Russian" I guess. Will change the patch according your recomendations.
Comment 19 Nobody - feel free to take it CLA 2010-04-05 19:36:28 EDT
Created attachment 163850 [details]
Patch updated according John's suggestions.
Comment 20 Nobody - feel free to take it CLA 2010-04-05 19:39:26 EDT
Applied to HEAD.
Comment 21 John Cortell CLA 2010-04-06 12:44:53 EDT
Mikhail, I just noticed that the Breakpoints view fails to show breakpoints in a CDI session as a result of this patch. If the Debug view selection is CDT debug element, then the breakpoints view goes empty.
Comment 22 Nobody - feel free to take it CLA 2010-04-06 12:47:36 EDT
(In reply to comment #21)
> Mikhail, I just noticed that the Breakpoints view fails to show breakpoints in
> a CDI session as a result of this patch. If the Debug view selection is CDT
> debug element, then the breakpoints view goes empty.

Hmm, interesting... I suspect the selection of adaptors is affected. I'll check it, thanks
Comment 23 Nobody - feel free to take it CLA 2010-04-06 14:15:32 EDT
Fixed. Replaced CViewerInputProvider by two input providers: for stack frames and for others (default). Each extends the corresponding platform input provider.
Comment: it's getting more and more complicated to support the Standard Debug Model based implementations. Many of the platform classes we use are internal, the consequences of which are not clear. Minor changes in the platform classes can break major features in CDT.
Comment 24 John Cortell CLA 2010-04-16 11:53:08 EDT
Mikhail, I'm seeing an empty registers view for CDI.
Comment 25 Nobody - feel free to take it CLA 2010-04-16 11:54:52 EDT
(In reply to comment #24)
> Mikhail, I'm seeing an empty registers view for CDI.

It's not GDB 7.0, is it?
Comment 26 John Cortell CLA 2010-04-16 11:58:04 EDT
Ugh. I don't know why I have a mental block with that issue... :-( This is a serious problem, though. We're not going to ship Helios with it, are we? How difficult can it be to address? After all, DSF-GDB is able to get the register info from gdb 7.0.
Comment 27 Nobody - feel free to take it CLA 2010-04-16 12:11:25 EDT
(In reply to comment #26)
> Ugh. I don't know why I have a mental block with that issue... :-( This is a
> serious problem, though. We're not going to ship Helios with it, are we? How
> difficult can it be to address? After all, DSF-GDB is able to get the register
> info from gdb 7.0.
I was told that it had been fixed in the HEAD of GDB. 
As far as I know DSF doesn't have an internal object model, it delegates all requests to the backend.That's why it it is not affected by this bug.
We request the list of registers only once, at the begining. Theoretically, it's possible to try to get registers on each suspend. Not sure though that doing it to avoid this particular bug is a good idea.
Comment 28 Marc Khouzam CLA 2010-04-16 14:40:43 EDT
(In reply to comment #27)
> (In reply to comment #26)
> > Ugh. I don't know why I have a mental block with that issue... :-( This is a
> > serious problem, though. We're not going to ship Helios with it, are we? How
> > difficult can it be to address? After all, DSF-GDB is able to get the register
> > info from gdb 7.0.
> I was told that it had been fixed in the HEAD of GDB. 
> As far as I know DSF doesn't have an internal object model, it delegates all
> requests to the backend.That's why it it is not affected by this bug.
> We request the list of registers only once, at the begining. Theoretically,
> it's possible to try to get registers on each suspend. Not sure though that
> doing it to avoid this particular bug is a good idea.

How about only asking again if the first list of register was empty?
Comment 29 Nobody - feel free to take it CLA 2010-04-16 14:44:15 EDT
(In reply to comment #28)
> How about only asking again if the first list of register was empty?

I thought about that, just didn't want to spend time on it. OK, I'll reopen https://bugs.eclipse.org/bugs/show_bug.cgi?id=309212.
Comment 30 Nobody - feel free to take it CLA 2010-04-16 14:58:54 EDT
(In reply to comment #28)
> How about only asking again if the first list of register was empty?
Marc, I hope you're not trying to keep me busy :-)
Comment 31 Marc Khouzam CLA 2010-04-17 20:26:41 EDT
(In reply to comment #30)
> (In reply to comment #28)
> > How about only asking again if the first list of register was empty?
> Marc, I hope you're not trying to keep me busy :-)

Not at all.  In fact, I already thought of some bugs patches I'll need you review, so the least busy you are the better :-)
Comment 32 Teodor Madan CLA 2010-07-27 05:15:49 EDT
*** Bug 256917 has been marked as a duplicate of this bug. ***