Bugzilla – Full Text Bug Listing
| Summary: | [registers] Vector register support | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Pawel Piech <pawel.1.piech> | ||||||||||||
| Component: | cdt-debug-dsf | Assignee: | Project Inbox <cdt-debug-dsf-inbox> | ||||||||||||
| Status: | NEW --- | QA Contact: | Jonah Graham <jonah> | ||||||||||||
| Severity: | enhancement | ||||||||||||||
| Priority: | P3 | CC: | cdtdoug, marc.khouzam, nobody, pchuong, Randy.Rohrbach, wlai | ||||||||||||
| Version: | 7.0 | Flags: | pawel.1.piech:
review?
(pchuong) |
||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Pawel Piech
Created attachment 199273 [details]
Screenshot of registers view with vector registers expanded as arrays.
Created attachment 199276 [details]
Screenshot of vector element bit count sub-menu.
Created attachment 199277 [details]
Screenshot of element number format sub-menu.
I'm almost finished with my initial implementation, which includes enhancements to the element number format support. However I've hit one major problem. The element number format support does not interact well with the cell editor and currently I experience a 5sec. deadlock every time I try to edit one of the register values. To fix this problem I'll need to extend the flexible hierarchy API in platform, which we can pick up in Juno M1 at the earliest. For now I'll commit my changes on a branch for review and further development. Implementations based on GDB can use "var-objects" to display the registers (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=317892). In this case the UI you are proposing to add would be useless. Are you planning to make it visible only for those implementations that provide corresponding services? (In reply to comment #5) > Implementations based on GDB can use "var-objects" to display the registers > (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=317892). In this case the UI > you are proposing to add would be useless. Are you planning to make it visible > only for those implementations that provide corresponding services? This is correct, it's an optional feature, based on an optional interface that a register service may implement. I don't expect that GDB would take advantage of it. Created attachment 199284 [details]
Patch with changes.
This is my initial implementation of the feature. It still needs some polish and commenting.
Patrick, I'd appreciate if you could take a look at some of the changes here. Along with this feature I'm providing a default implementation of IElementFormatProvider that you added in 7.0. Created attachment 199305 [details]
Updated patch (git fromat).
(In reply to comment #8) > Patrick, I'd appreciate if you could take a look at some of the changes here. > Along with this feature I'm providing a default implementation of > IElementFormatProvider that you added in 7.0. Pawel, I just got back from vacation. I'll try to review the patch as soon as I can. (In reply to comment #10) > Pawel, I just got back from vacation. I'll try to review the patch as soon as I > can. No worries, we're not even at M1 ;-) Patrick has asked me to review this patch. This is my initial feedback. I have no chance to run the code yet.
CDebugImages
- assume other people agree on this package relocaiton;
- assume they know what else are changed other than this relocation
org.eclipse.cdt.dsf.ui plugin.xml
- Two property testers org.eclipse.cdt.dsf.debug.internal.ui.viewmodel.actions.UpdateScopesPropertyTester for properties areUpdateScopesSupported, isUpdateScopeAvailable and isUpdateScopeActive are removed. I assume these are agreed by all other people.
WatchExpressionCellModifier
- queryElementFormat method adds a call to VMHandlerUtils.getViewerInput() but I don't see the method consumes the returned object. Is it a redundant call or an incomplete patch?
ElementNumberFormatChangedEvent
- Do you need this new event class? I created org.eclipse.cdt.dsf.debug.ui.viewmodel.update.ElementFormatEvent for events of element format changes.
ElementNumberFormatProvider
- we don't use it because we have our own for achieving different behaviors. I suggest other people to review it.
ElementNumberFormatsContribution
- line 121-124 below, line 122 'if-condition' should be availableFormats.size() insteadof FORMATS.size()
List<String> availableFormats = getAvailableFormats(provider, viewerInput, elementPaths);
if (FORMATS.size() == 0) {
return NO_ITEMS;
}
ExpressionVMProvider
- constructor should not create VectorProvider and ElementNumberFormatProvider objects. Derived classes may not want the same implementations.
Use protected methods to create objects and let derived classes to override those methods when needed.
- That also means the fFormatProvider and fVectorRegisterProvider should be declared with protected interface types rather than private concrete types.
- I expect fFormatProvider and fVectorRegisterProvider are optional and so can be null after this VM provider is fully constructed. Thus, any referecne to those new data memebers should check null pointer before using them.
- I don't see any places uses fFormatProvider other than construciton, dispose and getFormatProvider(). The getFormatProvider() is not called by any code.
- Our expression vm provider inherits this ExpressionVMProvider class but IVectorProvider does not work for us at all. Can this ExpressionVMProvider not to inherit the IVectorProvider interface at all?
There are places on the patch checks whether a vmprovider instanceof IVectorProvider and proceed the work. Since our vmproviders inherits the corresponding vmproviders, we cannot tell those code that our vmproviders are not a vector provider. Perhaps, we should use getAdapter approach by adding a method deep in the base interface IVMProvider or make it simply inherits IAdaptable.
RegisterVMProvider
- see exact same comments for ExpresionVMProvider.
VariableVMProvider
- constructor should not create ElementNumberFormatProvider object. Derived classes may not want the same implementation.
Use protected methods to create object and let derived classes to override those when needed.
- That also means the fFormatProvider should be declared with protected interface types rather than private concrete types.
- I expect fFormatProvider is optional and so can be null after this VM provider is fully constructed. Thus, any referecne to this new data memeber should check null pointer before using them.
- I don't see any places uses fFormatProvider other than construciton, dispose and getFormatProvider(). The getFormatProvider() is not called by any code.
I've extracted the number formatting logic from Pawel's patch and modified it a bit. I've also worked around the 5 second deadlock of comment 4 by not using the entire solution proposed here but a simpler one that does not require the UI thread. The result is part of the solution posted to bug 439624. I didn't take anything related to Vector register support, just the number formatting. |