Community
Participate
Working Groups
The current implementation of the IRegisters service has the following issues: - members of aggregate types can't be viewed separately and their values can't be modified - types of the registers are not displayed correctly Using the GDB/MI variable objects mechnism would solve these problems and provide a better way to access registers in general.
Created attachment 172696 [details] Variable objects based registers service Extends MIRegisters and uses MIExpressions service to create and maintain variable objects for registers.
Marc, please take a look. There are some minor API changes, but nothing major.
I like this new functionality a lot! A great enhancement for Registers in DSF-GDB. Some comments on the patch: RegisterVMProvider ok GdbViewModelAdapter ok GdbRegisterVMProvider 4- sometimes 'session' is used, sometimes 'getSession()' is used. I think just getSession() is fine 5- copy/paste typo in 'subExpressioNode' GdbRegisterVMNode 6- In update( IHasChildrenUpdate[] updates ), the comment says that regGroups always have children, but I wonder if in this node we are dealing with regGroups or registers? 7- If update( IHasChildrenUpdate[] updates ) is valid (point #6), should we call if (!checkUpdate(update)) continue; as the first line of the loop, since that is what the base class does? 8- In updateElementsInSessionThread(), execDmc and expressionService are not used 9- When calling getRegisters() we now pass a regGroupDmc instead of createCompositeDMVMContext(update) I don't know exactly what this compositeDMVM is, but I think it creates a DMC that is multiple DMCs at once. Now, looking at MIRegisters.getRegisters() it seems to need to have access to a regGroupDMC, a ContainerDMC and a IMIExecDMC. I believe a ContainerDMC can be obtained as a parent of a regGroupDMC, but I'm not sure about a IMIExecDMC. Maybe that is why RegisterVMNode uses a compositeDMVM instead of a regGroupDMC? I believe the hierarchy is container | \ regGroup MIExec | / register 10- in updatePropertiesInSessionThread(), expService is not used 11- in updatePropertiesInSessionThread(), should we use handleFailedUpdate() instead of setting the status to ERROR? 12- What is @SuppressWarnings( "synthetic-access" )? When I removed it, I didn't see any warning 13- in updatePropertiesInSessionThread(), in the handleCompleted(), do we need to re-fecth the regDMC instead of using 14- in updatePropertiesInSessionThread(), in the handleCompleted(), I see we set PROP_NAME again, is that because the name in the getData() was not the one we want to use? Shouldn't we do that in the isSucess() case only? 15- I'm confused about the use of MessagesForRegisterVM.RegisterVMNode_Type_column__text_format. That string seems to expect 3 parameters, but in the new code we only give it one. The old code also confuses me as it gives it 5 parameters.... Is it me? GDBRegisters 16- I'm not a big fan of having GDBRegisterDMC extend MIExpressionDMC. If we write a new version of MIExpressionDMC for a new GDB version, we'll also have to update GDBRegisterDMC. Could we instead call IExpressions#createExpression() and maybe have the expression as a parent of the register? 17- We try, when possible, to avoid making context classes plublic. Can we make GDBRegisterDMC a protected class and always refer to it using IRegisterDMContext? If IRegisterDMContext does not have the methods you need, you can create an IGDBRegisterDMContext (alike IMIExecutionDMContext). This pays off when we write a new version of the service and change the implementation of GDBRegisterDMC 18- same as #17 for GDBRegisterData 19- In a service you don't need to call getSession().getExecutor() but you can call getExecutor() directly MIExpressions 1- Maybe we can avoid making MIExpressionDMC and ExpressionDMData public? 2- Tiny javadoc error for the new MIExpressionDMC constructor 3- The new code in createExpression() passes a IRegisterDMContext instead of IRegisterGroupDMContext
(In reply to comment #3) > I like this new functionality a lot! A great enhancement for Registers in > DSF-GDB. > > Some comments on the patch: > > RegisterVMProvider > ok > > GdbViewModelAdapter > ok > > GdbRegisterVMProvider > 4- sometimes 'session' is used, sometimes 'getSession()' is used. I think just > getSession() is fine > 5- copy/paste typo in 'subExpressioNode' > > GdbRegisterVMNode > 6- In update( IHasChildrenUpdate[] updates ), the comment says that regGroups > always have children, > but I wonder if in this node we are dealing with regGroups or registers? > 7- If update( IHasChildrenUpdate[] updates ) is valid (point #6), should we > call > if (!checkUpdate(update)) continue; > as the first line of the loop, since that is what the base class does? > 8- In updateElementsInSessionThread(), execDmc and expressionService are not > used > 9- When calling getRegisters() we now pass a regGroupDmc instead of > createCompositeDMVMContext(update) > I don't know exactly what this compositeDMVM is, but I think it creates a > DMC that is multiple > DMCs at once. Now, looking at MIRegisters.getRegisters() it seems to need > to have access to > a regGroupDMC, a ContainerDMC and a IMIExecDMC. I believe a ContainerDMC > can be obtained as > a parent of a regGroupDMC, but I'm not sure about a IMIExecDMC. Maybe that > is why > RegisterVMNode uses a compositeDMVM instead of a regGroupDMC? > > I believe the hierarchy is > container > | \ > regGroup MIExec > | / > register > > 10- in updatePropertiesInSessionThread(), expService is not used > 11- in updatePropertiesInSessionThread(), should we use handleFailedUpdate() > instead of setting the status to ERROR? > 12- What is @SuppressWarnings( "synthetic-access" )? When I removed it, I > didn't see any warning > 13- in updatePropertiesInSessionThread(), in the handleCompleted(), do we need > to re-fecth the regDMC instead of using > 14- in updatePropertiesInSessionThread(), in the handleCompleted(), I see we > set PROP_NAME again, is that because > the name in the getData() was not the one we want to use? Shouldn't we do > that in the isSucess() case only? > 15- I'm confused about the use of > MessagesForRegisterVM.RegisterVMNode_Type_column__text_format. That string > seems > to expect 3 parameters, but in the new code we only give it one. The old > code also confuses me as it gives it > 5 parameters.... Is it me? > > GDBRegisters > 16- I'm not a big fan of having GDBRegisterDMC extend MIExpressionDMC. If we > write a new version of MIExpressionDMC > for a new GDB version, we'll also have to update GDBRegisterDMC. Could we > instead call > IExpressions#createExpression() and maybe have the expression as a parent > of the register? > 17- We try, when possible, to avoid making context classes plublic. Can we > make GDBRegisterDMC a protected class > and always refer to it using IRegisterDMContext? If IRegisterDMContext > does not have the methods you need, > you can create an IGDBRegisterDMContext (alike IMIExecutionDMContext). > This pays off when we write > a new version of the service and change the implementation of > GDBRegisterDMC > 18- same as #17 for GDBRegisterData > 19- In a service you don't need to call getSession().getExecutor() but you can > call getExecutor() directly > > MIExpressions > > 1- Maybe we can avoid making MIExpressionDMC and ExpressionDMData public? > 2- Tiny javadoc error for the new MIExpressionDMC constructor > 3- The new code in createExpression() passes a IRegisterDMContext instead of > IRegisterGroupDMContext Marc, thank you very much for such a good review. I have to admit of being sloppy (again!). I need to digest all of it. But the main question for me is the following > 16- I'm not a big fan of having GDBRegisterDMC extend MIExpressionDMC. If we > write a new version of MIExpressionDMC > for a new GDB version, we'll also have to update GDBRegisterDMC. Could we > instead call > IExpressions#createExpression() and maybe have the expression as a parent > of the register? We have discussed this before and I have tried it. But to implement it we need a new intermediary node between the register node and the child expression nodes. That's the one of the reasons I decided to go back to my original design. At the same time the main idea of this patch is to treat registers as expressions, so all changes made to MIExpressions will be automatically applied to GdbRegisters.
(In reply to comment #4) > > 16- I'm not a big fan of having GDBRegisterDMC extend MIExpressionDMC. If we > > write a new version of MIExpressionDMC > > for a new GDB version, we'll also have to update GDBRegisterDMC. Could we > > instead call > > IExpressions#createExpression() and maybe have the expression as a parent > > of the register? > > We have discussed this before and I have tried it. But to implement it we need > a new intermediary node between the register node and the child expression > nodes. That's the one of the reasons I decided to go back to my original > design. > At the same time the main idea of this patch is to treat registers as > expressions, so all changes made to MIExpressions will be automatically applied > to GdbRegisters. Is there a need in the register clients to access the underlying expression context? My first instinct is that it should just be an internal implementation detail of the register context and not exposed as an API unless there's a use case for it.
(In reply to comment #5) > Is there a need in the register clients to access the underlying expression > context? My first instinct is that it should just be an internal > implementation detail of the register context and not exposed as an API unless > there's a use case for it. There is no need to access the underlying expression for clients. I don't think there is API extensions for it in this patch. The implementation extends MIExpressions implementation and the node classes use implementation details, exactly as you suggested.
(In reply to comment #4) > Marc, thank you very much for such a good review. I have to admit of being > sloppy (again!). I'm always worried about annoying people with my reviews, because I can get to be a little too nit-picky. Thank you for keeping an open-mind :-) > I need to digest all of it. Most of it is little things, nit-picking, as I said. > But the main question for me is the following Yeah, that's the big one :-O > > 16- I'm not a big fan of having GDBRegisterDMC extend MIExpressionDMC. If we > > write a new version of MIExpressionDMC > > for a new GDB version, we'll also have to update GDBRegisterDMC. Could we > > instead call > > IExpressions#createExpression() and maybe have the expression as a parent > > of the register? > > We have discussed this before and I have tried it. But to implement it we need > a new intermediary node between the register node and the child expression > nodes. That's the one of the reasons I decided to go back to my original > design. This is the *VMNode stuff? I'm not super familiar with that stuff, and I'm sure you have a point. This may not be a good idea because I haven't really thought it out, but since you have more experience, maybe you can have a quick opinion; could we have the Expressions service create the new GdbRegistersDMC? I mean, we seem to need a context that is both an expression and a register, so it could belong just as well in either service, no? Since GdbRegistersDMC simply implements IRegisterDMContext for its register part, it could be handled by the Expressions service. You would then use IExpressions#createExpression to actually create the register-expression DMC. I have to admit, this may be counter-intuitive at first, so maybe not such a good idea. What do you think? > At the same time the main idea of this patch is to treat registers as > expressions, so all changes made to MIExpressions will be automatically applied > to GdbRegisters. Yes, but if we write a new service GdbExpressions_7_2 and a new ExpressionDMC_7_2, that won't propagate automatically. For example, GDBProcesses (MIProcesses) and GDBProcesses_7_0 create two distinct MIExecutionDMC.
(In reply to comment #6) > (In reply to comment #5) > > Is there a need in the register clients to access the underlying expression > > context? My first instinct is that it should just be an internal > > implementation detail of the register context and not exposed as an API unless > > there's a use case for it. > > There is no need to access the underlying expression for clients. I don't think > there is API extensions for it in this patch. The implementation extends > MIExpressions implementation and the node classes use implementation details, > exactly as you suggested. I meant that GDBRegisterDMC could simply have the MIExpressionDMC as a memeber. Then there should be no issues from future extensions of MIExpressinDMC
(In reply to comment #8) > I meant that GDBRegisterDMC could simply have the MIExpressionDMC as a memeber. > Then there should be no issues from future extensions of MIExpressinDMC I was thinking that would be the way to go as well. But then it was not clear to be if the *VMNode code needed to deal with a DMContext that was both an register _and_ and expression.
(In reply to comment #9) > (In reply to comment #8) > > > I meant that GDBRegisterDMC could simply have the MIExpressionDMC as a memeber. > > Then there should be no issues from future extensions of MIExpressinDMC > > I was thinking that would be the way to go as well. But then it was not clear > to be if the *VMNode code needed to deal with a DMContext that was both an > register _and_ and expression. I haven't looked through the code patch, but AFAIK there should be no reason that a registers view VM should need to examine the register expressions.
(In reply to comment #7) > (In reply to comment #4) > > Marc, thank you very much for such a good review. I have to admit of being > > sloppy (again!). > > I'm always worried about annoying people with my reviews, because I can get to > be a little too nit-picky. Thank you for keeping an open-mind :-) > You're welcome :) > This is the *VMNode stuff? I'm not super familiar with that stuff, and I'm > sure you have a point. Yes, it is the *VMNode stuff. If RegisterDMContext is at the same time IExpressionDMContext, we only need to extend VariableVMNode for registers. Otherwise we need to write a VMNode for registers that uses Expressions service internally to display registers and an intermediary VMNode to connect it to the child variable nodes. This may result in code duplications or new abstract classes. > This may not be a good idea because I haven't really thought it out, but since > you have more experience, maybe you can have a quick opinion; could we have the > Expressions service create the new GdbRegistersDMC? I mean, we seem to need a > context that is both an expression and a register, so it could belong just as > well in either service, no? Since GdbRegistersDMC simply implements > IRegisterDMContext for its register part, it could be handled by the > Expressions service. You would then use IExpressions#createExpression to > actually create the register-expression DMC. I have to admit, this may be > counter-intuitive at first, so maybe not such a good idea. What do you think? > I am not very comfortable with this idea, but it's a question for Pawel. > > At the same time the main idea of this patch is to treat registers as > > expressions, so all changes made to MIExpressions will be automatically applied > > to GdbRegisters. > > Yes, but if we write a new service GdbExpressions_7_2 and a new > ExpressionDMC_7_2, that won't propagate automatically. For example, > GDBProcesses (MIProcesses) and GDBProcesses_7_0 create two distinct > MIExecutionDMC. Good point.
(In reply to comment #11) > Yes, it is the *VMNode stuff. If RegisterDMContext is at the same time > IExpressionDMContext, we only need to extend VariableVMNode for registers. > Otherwise we need to write a VMNode for registers that uses Expressions service > internally to display registers and an intermediary VMNode to connect it to the > child variable nodes. This may result in code duplications or new abstract > classes. Why can't use use the existing RegistersVMNode? Unless you're changing the API contract in IRegisters service it should work without any changes.
(In reply to comment #12) BTW, I don't think there'd be anything wrong with ditching the IRegisters service in the GDB implementation altogether if that's what you really want to do. GDB doesn't support register groups, bit fields, mnemonics, and all that other stuff, so it doesn't really take advantage of much of IRegisters service interface (or of Register VM implementation). You could just create a specialized version of VariablesVMNode for registers view as I gather you're doing already.
(In reply to comment #13) > (In reply to comment #12) > BTW, I don't think there'd be anything wrong with ditching the IRegisters > service in the GDB implementation altogether if that's what you really want to > do. GDB doesn't support register groups, bit fields, mnemonics, and all that > other stuff, so it doesn't really take advantage of much of IRegisters service > interface (or of Register VM implementation). You could just create a > specialized version of VariablesVMNode for registers view as I gather you're > doing already. Yes, that's exactly what this patch is about. I implemented a new register service that extends MIExpressions and a specialized version of VariableVMNode.
(In reply to comment #14) > Yes, that's exactly what this patch is about. I implemented a new register > service that extends MIExpressions and a specialized version of VariableVMNode. I see, it's starting to make more sense to me now. But in this case, why bother implementing IRegisters at all. Or if you really do need the IRegisters implementation, why not have MIExpressions (or a new GdbExpressions) implement it? Or keep the existing IRegisters implementation and just don't use it for the registers view.
(In reply to comment #15) > I see, it's starting to make more sense to me now. But in this case, why > bother implementing IRegisters at all. Or if you really do need the IRegisters > implementation, why not have MIExpressions (or a new GdbExpressions) implement > it? Or keep the existing IRegisters implementation and just don't use it for > the registers view. Good question, I guess I wasn't ready for such a radical move :) Also, I don't know how the bit field support works but if we get GDB supporting it we may try to use some parts of the register service and corresponding VM nodes.
(In reply to comment #16) > Good question, I guess I wasn't ready for such a radical move :) > Also, I don't know how the bit field support works but if we get GDB supporting > it we may try to use some parts of the register service and corresponding VM > nodes. Then it seems you have a choice: A) Stick with the IRegisters/Register*VMNode combination and extend it to sprinkle in expressions-related features. B) Use the expressions service and extend the VariableVMNode to represent groups, registers. The catch is that you may end up eventually duplicating features from A. IMO, mixing the two is going to be trouble ;-)
(In reply to comment #17) > Then it seems you have a choice: > A) Stick with the IRegisters/Register*VMNode combination and extend it to > sprinkle in expressions-related features. > B) Use the expressions service and extend the VariableVMNode to represent > groups, registers. The catch is that you may end up eventually duplicating > features from A. Yes, I would prefer to avoid duplicating the code. Not sure Marc would be happy with it. > IMO, mixing the two is going to be trouble ;-) It's already giving me a headache:)
Marc, I would like to modify the patch according Pawel's proposal to ditch IRegisters and use an extension of MIExpressions instead. It will have an option to get registers without getting the groups. What do you think about it?
(In reply to comment #19) > Marc, I would like to modify the patch according Pawel's proposal to ditch > IRegisters and use an extension of MIExpressions instead. It will have an > option to get registers without getting the groups. What do you think about it? Sounds good to me. I'm not sure until I see the patch, but it sounds like the code duplication is a problem that is worth having in this case.
*** Bug 317122 has been marked as a duplicate of this bug. ***
Mikhail, this proposal sounds really good and should be a good base for other Registers related functionality. Would you have a later version of your patch we can use to continue this effort ? It would be great to push your latest proposal to Gerrit so the contribution can evolve from there.
(In reply to comment #22) > Mikhail, this proposal sounds really good and should be a good base for > other Registers related functionality. > > Would you have a later version of your patch we can use to continue this > effort ? > It would be great to push your latest proposal to Gerrit so the > contribution can evolve from there. Alvaro, Sorry, I don't have a version of the patch that is compatible with master. Contributing it is on my todo list but I am too busy right now. I think October is the earliest I can work on it.
(In reply to Mikhail Khodjaiants from comment #23) > > Would you have a later version of your patch we can use to continue this > > effort ? > > It would be great to push your latest proposal to Gerrit so the > > contribution can evolve from there. > > Alvaro, > Sorry, I don't have a version of the patch that is compatible with master. > Contributing it is on my todo list but I am too busy right now. I think > October is the earliest I can work on it. Hi Mikhail, I believe you are using this feature in your product, no? In that case, did you ever update it to be closer to master than the patch attached to this bug? The changed proposed in this bug and the implementation of register groups in bug 235747 will collide, and it would be good to better understand the changes to avoid re-doing the work of the register groups. If you have anything newer version than the attached patch, can you push it to gerrit (or attach it to the bug and I'll push it to gerrit)? If you don't have a newer version, and the latest code is the one attached, it will be good to put it on gerrit anyway, but I can do that. Just let me know, if you don't mind. Thanks
(In reply to Marc Khouzam from comment #24) > (In reply to Mikhail Khodjaiants from comment #23) > > > Would you have a later version of your patch we can use to continue this > > > effort ? > > > It would be great to push your latest proposal to Gerrit so the > > > contribution can evolve from there. > > > > Alvaro, > > Sorry, I don't have a version of the patch that is compatible with master. > > Contributing it is on my todo list but I am too busy right now. I think > > October is the earliest I can work on it. > > Hi Mikhail, > I believe you are using this feature in your product, no? In that case, did > you ever update it to be closer to master than the patch attached to this > bug? > > The changed proposed in this bug and the implementation of register groups > in bug 235747 will collide, and it would be good to better understand the > changes to avoid re-doing the work of the register groups. > > If you have anything newer version than the attached patch, can you push it > to gerrit (or attach it to the bug and I'll push it to gerrit)? If you > don't have a newer version, and the latest code is the one attached, it will > be good to put it on gerrit anyway, but I can do that. Just let me know, if > you don't mind. > > Thanks Marc, we are planning to switch to CDT 8.2 for our next release and I will start working on it this week or next week. Updating the var-object patch is part of the task. If you are OK with this time frame, I'll submit the updated patch as soon as it is ready. There are some minor issues with the current patch though.
(In reply to Mikhail Khodjaiants from comment #25) > Marc, we are planning to switch to CDT 8.2 for our next release and I will > start working on it this week or next week. Updating the var-object patch is > part of the task. If you are OK with this time frame, I'll submit the > updated patch as soon as it is ready. There are some minor issues with the > current patch though. That sounds great. The idea was to get familiar with it a bit to see the impacts on the register groups. The other option is to go ahead with the register groups before the varObj change. This would probably mean that the varObj change would become more complicated as it would need to avoid breaking the register group feature, if that feature was already on master. What is your recommendation? I was thinking that the varObj going in first may be easier.
(In reply to Marc Khouzam from comment #26) > That sounds great. The idea was to get familiar with it a bit to see the > impacts on the register groups. > > The other option is to go ahead with the register groups before the varObj > change. This would probably mean that the varObj change would become more > complicated as it would need to avoid breaking the register group feature, > if that feature was already on master. > > What is your recommendation? I was thinking that the varObj going in first > may be easier. I may be wrong and haven't looked at the implementation of the register groups but in theory the new services for register groups should work for all implementations of the existing services. The var-object patch is just a different implementation of the existing services. Does the register group support require API changes in the existing services?
(In reply to Mikhail Khodjaiants from comment #27) > I may be wrong and haven't looked at the implementation of the register > groups but in theory the new services for register groups should work for > all implementations of the existing services. If it ends up being an independent service, I agree with you. I haven't started the review yet, and I wanted to keep your patch in mind to guide that review. > The var-object patch is just a > different implementation of the existing services. Does the register group > support require API changes in the existing services? Probably not since we can't break the API. But I thought the varObj patch might define a new interface for the registers, which the regGroups could add to. We can start with the register groups interface, and then merge the varObj one into it if appropriate. I'll start the review for register groups and see where it leads.
(In reply to Marc Khouzam from comment #28) > > The var-object patch is just a > > different implementation of the existing services. Does the register group > > support require API changes in the existing services? > > Probably not since we can't break the API. But I thought the varObj patch > might define a new interface for the registers, which the regGroups could > add to. We can start with the register groups interface, and then merge the > varObj one into it if appropriate. I'll check tomorrow if the var-object patch introduces a new API that may affect the registers groups but I doubt it.
The implementation of Register groups is based on top of the Registers service and introduces and extension API on IRegisters2 (group service extension api) I am not very familiar with the patch but from the comments it seems we will need some adjustments to make it work with Register groups. If the implementation is later updated as per comment 19, we will need further updates. However it will be great to start with an updated patch that works on master
Modified the patch to make it compatible with master and pushed it to Gerrit - https://git.eclipse.org/r/16167.
(In reply to Mikhail Khodjaiants from comment #31) > Modified the patch to make it compatible with master and pushed it to Gerrit > - https://git.eclipse.org/r/16167. There is problem with this patch: adding a register group to the Expressions view using "Watch" doesn't work.
(In reply to Mikhail Khodjaiants from comment #31) > Modified the patch to make it compatible with master and pushed it to Gerrit > - https://git.eclipse.org/r/16167. Thanks Mikhail!
I did some quick test and it is looking pretty good, it behaves consistent for All stop and non stop mode, I observed an exception cascading from a failed assert while expanding complex registers e.g. xmm0 I will continue looking deeper. Thanks for the updated patch Mikhail !
