Community
Participate
Working Groups
I started hooking up the memory view for DSF and found that it has dependencies on IDebugTarget and IDebugElement interfaces scattered throughout. After some trial and error I eventually got a sample memory block to show up in the memory view. I'll attach the patch with the changes I made, but I'm pretty certain that there will be additional changes that we'll find with time.
Created attachment 70884 [details] Patch with initial changes to remove the IDebugTarget dependency.
Hi Pawel, Thanks for the patch. I have taken a brief look and tried to understand what problems the patch is tring to address. Here's a list of things you have found and some comments about the fix: 1) In Memory Block manager, we call #getDebugTarget and use it as the memory block retrieval for IMemoryBlock. This is intentional since the old IMemoryBlock interface does not provide a method to get to its memory block retreival. It is assumed that the debug target is its memory block retrieval. We cannot simply replace the call of #getMemoryBlock with #getAdapter(IMemoryBlockRetrieval.class) since existing clients may not provide an adapter for this. One solution is to provide a default adapter for IMemoryBlock to get to the IMemoryBlockRetrieval. 2) Assumption that the current context is a debug element. You have found it in AddMemoryRenderingAction, AddMemoryRenderingDialog, and MemoryViewUtils. The changes in AddMemoryRenderingAction look ok, since they are just old codes that should have been removed. In AddMemoryRenderingDialog and MemoryViewUtils, I am more concerned with the changes to replace the call #getAdapter(IMemoryBlockRetrieval.class) from #getDebugTarget(). Again, not all clients would have provided adapters to convert the current debug context to IMemoryBlockRetrieval. Therefore, I think we need to provide default adapters to convert IDebugTarget, IThread, IStackframe to IMemoryBlockRetrieval for the changes to work. 3) The view does not handle the case where IMemoryBlock#getDebugTarget returns null. You have added null checks in RenderingViewPane to address this. However, I am not 100% sure that this is the right thing to do. IMemoryBlock extends from IDebugElement which provides the #getDebugTarget method. No where in the javadoc specifies that the memory block can return null for its debug target. Therefore, I am not sure if it's correct to handle null returned by the memory block. I checked IExpression where it can return null for its target. It overrides the #getDebugTarget method and the javadoc speicifies that it can return null. In the DSF/flexible hiearchy world, do your elements implement IDebugElement at all? If they do, then do you return null for the debug target / launch? It will help if you can explain how memory blocks are organized in DSF? Targetting this post-3.3. Thanks... Samantha
Re: clients not providing adapters to IMemoryBlockRetrieval Instead of creating default adapters to IMemoryBlockRetrieval for the standard debug elements, a simpler approach is to call #getAdapter(..) first. If clients return null, fall back to the standard debug hierarchy. Will this work for DSF?
Thanks for the fast feedback. _IMemoryBlock.getAdapter(IMemoryBlockRetrieval)_ First of all, I should say that I implemented the basic IMemoryBlock interface (as opposed to IMemoryBlockExtension) because it's much simpler and I was only trying to create a proof of concept implementation. For all practical intents and purposes DSF will have to implement IMemoryBlockExtension. So I for point #1, I think it's fine to leave the requirement that the standard IMemoryBlock.getDebugTarget() be used to get IMemoryBlockRetrieval. BTW, I copied the IMemoryBlock.getAdapter() pattern from RenderingViewPange.getMemoryBlockRetrieval(). If IMemoryBlockExtension.getAdapter(IMemoryBlockRetrieval.class) is not always expected to work, that method should be changed. IMHO: When browsing through the code I found it difficult to figure out what is the expected methodology for accessing IMemoryBlockRetrieval from IMemoryBlock. It would help if it was clearly stated in the comments of the IMemoryBlock or IMemoryBlockExtension interface. Especially since IMemoryBlockExtension.getMemoryBlockRetrieval() already returns the retrieval (curiously it returns IMemoryBlockRetrieval and not IMemoryBlockRetrievalExtension). _IDebugElement_ Actually, so far we were able to get away with not implementing IDebugElement at all and my hope was that we could keep it this way on Principal. That said, we were planning to implement the standard model interfaces (IDebugElement, IDebugTarget, IThread, etc.) for backward compatibility and interfacing with 3-rd party tools. However, when doing the work that lead to this bug, I got concerned about the fact that the Memory view uses IDebugTarget.isTerminated() to determine whether a memory block is valid or not. So unless DSF implements this interface, the memory view may attempt to display memory block belonging to a terminated debug session. I also wonder if there are any other places where the standard debug model interfaces are accessed. Re Comment #3: That would certainly work just as well to get this feature un-blocked. But whatever you decide, I would suggest documenting it clearly somewhere.
Created attachment 71388 [details] For DSF: check for NPE and temporary fix for compilation error Hi, I added a couple of temporary fixes just to get us going (DSF/Memory Service): - Added a null pointer check in MemoryBlockContentAdapter - Changed setBackground() to setBackgrond() to quickly fix a compilation error. The real fix is to correct the spelling of the function in TextConsole but I didn't want to add another file in the patch for something that is probably already fixed somewhere else. Regards, /fc
Created attachment 72514 [details] Another NPE patch Hi, Another check for a null DebugTarget to avoid an NPE in DD/DSF. Supercedes the previous patch. Regards, /fc
I have tried this patch with our model which is not based on the standard hierarchy and it seems to work fine. Are there plans to apply it?
I will review the patch and look at applying it.
Darin, I have a question about null debug target. In the javadoc of IDebugElement, it's not specified that IDebugElement can return null for debug target. I am wondering if it's the right thing to do to check for null in the memory view code. If we do this in the memory view, shouldn't we do that in the entire platform and update the javadoc for IDebugElement as well? In IExpression, you overrode getDebugTarget to allow it to return null. I am wondering if that's what we should do for IMemoryBlock as well. Thanks... Sam
Created attachment 73162 [details] modified patch Slightly modified the patch to check for null for #getAdapter(IMemoryBlockRetrieval.class) Need testing
Created attachment 73163 [details] mylyn/context/zip
Re: IDebugElement I have found other places where the standard debug model interfaces are accessed. E.g. InstructionaPointerManager, BreakpoinsView where debug view selection is tracked, DebugUITools.getCurrentProcess. These references also have assumptions about standard debug model and do not expect the debug elements to return null debug target. In most of the cases, I think these references are done in adapters where clients have the ability to override. (e.g. in source display adapter.) In the case of the Memory View or Breakpoints View, this option is not available.
An IExpression is the only debug element that can return null for a debug target, since it can exist without a debug session and can be bound to a debug session dynamically (based on debug context).
There are still references to IDebugElement and IDebugTarget in RenderingViewPane::updateToolBarActionsEnablement and updateToolBarActionsEnablement::contextActivated. I'll attach a patch.
Created attachment 73346 [details] Removes references to IDebugElement and IDebugTarget in RenderingViewPane.
Here's the plan: * We should not do null check for IMemoryBlock.getDebugTarget or IDebugElement.getDebugTarget. A memory block cannot exist without a debug session, and must have a debug target associated with it. * I will review the memory view code and look for places where I check for IDebugTarget#isTerminated or #isDisconnected. Some of these calls are still valid. E.g. we do not want the content adapter to ask for memory if the memory block's debug target is already terminated. I will also look at teh code to see if there is any chances of eliminating some of the calls. * Re: how to get to the memory block retrieval: From a debug context, a context does not need to be a debug element. We will always call #getAdapter(IMemoryBlockRetrieval.class) to get the memory block retrieval. If this returns null, we will fall back to the debug target if possible. In this case, we will not do null check for its returned value. From a memory block, we will also always call #getAdapter(IMemoryBlockRetrieval.class) first and fall back to the debug target if the adapter call fails.
To clarify. Does this mean that IMemoryBlockExtension.getDebugTarget() also has to return a non-null value? Since IMemoryBlockExcension derives from IMemoryBlock. As far as calling IDebugTarget.isDisconnected()/isTerminated(). I think it would be perfectly acceptable to have content provider return an error with an appropriate error code (see bug 195907). If the debugger object that originated the memory block is indeed terminated, the memory view should receive an update which should force it to re-query the memory block, which will no longer be available. Or am I making some bad assumptions here? Thanks! Pawel
Re: IMemoryBlockExtension.getDebugTarget() returning non-null value Correct. If you implement IMemoryBlockExtension, its debug target must be a non-null value. I think a more general problem is that if you implement IDebugElement, and if you do not follow the standard debug model, there is still assumption in the code that the debug element must belong to a launch and a debug target. I am still having trouble picturing how this would work for DSF if you start implementing IStackFrame and then not implement its getDebugTarget or getThread methods. Re: isTerminated calls The isTerminated calls are used in several places, other than the content adapter. e.g. to determine if the CreateRendering should be created when all renderings are removed, action enablement. I have to review each of the cases to figure out what is the right thing to do. I am not sure if new debug exceptions will fix our problems in the memory view. By having a new debug exception, you are basically moving the #isTerminated check from the content adapter to the model. Although there is no spec saying that we cannot query for memory after a target is terminated, I am hestitant to remove the call because existing clients may not have expected this and we will break their code. Would it help if the isTerminated call is put in a protected method... and you can override by subclassing from the default memory block content adapter?
I'm a little confused at this point. I think if IMemoryBlockExtension.getDebugTarget() must return a valid IDebugTarget then we probably don't need to worry about references to isTerminated() or isDisconnected(), because the debug target will always have be available.... unless I'm missing something fundamental here. Currently DSF only implements the following interfaces from the standard debug model: ILaunch, IProcess, IBreakpoint, ISourceLookupDirector. ILaunch does not return a debug target as its child instead DSF provides a custom content provider for the launch which populates the view with custom model elements. As I mentioned before, we're planning to eventually create an implementation of the standard debug model for compatibility with some of the CDT views and actions, so it's not a show stopper if the memory view requires it as well. But I have to admit that I'm a little surprised at this decision because it seems that the platform has gone to great lengths to allow non-standard debug models to integrate with the common debugger views (even covering such details as perspective switching with ISuspendTrigger). So to see it stop short when it's come so far would be a little disappointing. :-)
I am a little confused with the use case too ... :) Would it be fair to say that the problem is that the memory view is only capable of showing renderings for IMemoryBlock, but not any arbitrary elements as inputs to the renderings? For debug view, variables view, registers view, supporting non-standard debug elements is done with flexible hierarchy and supporting any elements as input to the view. (except for debug view where the input is the launch manager, and you have control of what to display from ILaunch.) In the Expressions View, I think it has a similar problem with the memory view where it can only show IExpression. In the Memory View, it should allow clients to show memory blocks from any arbitrary elements that support IMemoryBlockRetrieval. However, in the rendering where the input must be an IMemoryBlock, the rendering cannot accept anything other than IMemoryBlock. And by doing that, the implementation must properly implement the IMemoryBlock interface, and return non-null value for getDebugTarget. I think we need to review references to isTerminated or isDisconnected in the framework, the rendering panes, in the actions, or in the view itself. We should not rely on having IDebugElement as debug context and do such checks. In the table rendering, IMemoryBlock is the only supported input to a the rendering, and the memory block must properly implement the interface and return non-null value for getDebugTarget. Does this make sense? or am I missing something?
I think it makes perfect sense :-) It would be nice if the memory view could show renderings that are not IMemoryBlock based. This could provide more flexibility and extendibility, but it could be a pretty big work item for the memory view, since all the actions related to the memory blocks would need to be made generic. Also, I'm not sure if anyone would actually try to create IMemoryBlock renderings since the IMemoryBlockExtension seems to be a pretty complete interface. And like I said before, in DSF we plan to implement IDebugTarget at some point anyway, so I don't want to create a lot of work for no good reason. It would be nice if there was some middle ground... Yes the expression view is also a problem for us because although it's a flexible-hierarchy view, there can only be one content provider registered for it. I filed bug 184233 to (hopefully) address that limitation.
Created attachment 74620 [details] Patch * Standardize how memory block retreival is obtained. We now use MemoryViewUtil to get memory block retrieval. * Handle cases where memory block retrieval can be null for any object. Need testing.
I tried the latest patch with DSF: created a memory block and a rendering... didn't see any problems yet :-) Thanks Pawel
Pawel, Thanks for getting back. I have checked in the latest patch.
Darin, please verify. Thanks...
Reopen to change target milestone.
fixed.
There is still a reference to IDebugTarget in RenderingViewPane::memoryBlockRenderingRemoved(), lines 564-567. Isn't it sufficient to check whether the memory block is removed or not without checking the target state?
Hi Mikhail, Thanks for the info. I marked this bug as fixed to indicate that I have committed the patch for Pawel and allowed him to get going with his non-standard debug model. However, I don't think all the work is complete, and I still need to do another round of review. I have created Bug 200033 to track that work item. Marking this bug as fixed. Please add additional information to Bug 200033. Thanks... Samantha
Verified.
