Community
Participate
Working Groups
It would be nice to display values returned from function calls in the Variables view. Visual Studio displays values returned from function calls in its Autos view. When stepping over foo(bar()); two lines appear in the Autos view: bar returned <value1> foo returned <value2> The same thing happens when stepping out of a function.
There was a patch proposed in Bug #302320 (for CDI version). It would be cool to have this feature.
(In reply to comment #1) > There was a patch proposed in Bug #302320 (for CDI version). It would be > cool to have this feature. Looking at the patch in bug 302320, I saw the *stopped event after an -exec-finish contained the return value of the method. For example: stopped,reason="function-finished",frame={addr="0x0000000000401270",func="main",args= [{name="argc",value="1"},{name="argv",value="0x7fffffffde68"}],file="../src/DSFTestApp.cpp",fullname="DSFTestApp.cpp",line="262"}, gdb-result-var="$4",return-value="11", thread-id="1",stopped-threads=["1"],core="0" We could do that relatively easily for DSF. I was also thinking that we could go beyond the step-finish and, if possible, support the following: - step-finish and showing the resulting value automatically - stepping over a line with a method call and showing the resulting value automatically - stepping over a line with multiple method calls and showing all resulting values I haven't dived into it, but I'm thinking we could force a step-into and a step-finish to obtain the return value of the call, for each step over. I think performance would be ok, but we'll have to see. One tweak would be that when we are fast stepping, we don't provide the return value, and avoid this extra step.
(In reply to comment #2) It would be nice to display the return value in the Variables view similar to how VS does it.
When a step-finish is done, GDB stores the return value of the method in a convenience var such as $1. Currently, when trying to look at $1 we get an error because we automatically try to fetch its address, which is not valid. This is tracked by Bug 378729.
I've decided to divide this effort into logical updates. The first step is to take advantage of the fact that after a step-finish operation, GDB stores the return value in a convenience variable (e.g., $1). I've update the MIStack service to keep track of such return values and provide them to the variables view. I've pushed an patch to gerrit for that part: https://git.eclipse.org/r/15377. If you step-return out of a method, you should now see the return value at the top of the variables view (the label must be updated though, see below). Note that the patch is based on the fix to Bug 378729. What remains: 1- provide proper label. So, instead of showing $1 which does not mean anything to the user, we must show "<method> returned" or something of the sort. 2- deal with the "View in memory" action when right-clicking on these new return values in the variable view, which currently causes an error (because convenience variables don't have an address). 3- show return values of methods for any step-over operation instead of only when step-returning. I have a prototype of this, which simulates a step-over operation by doing repetitive step-in/step-return operations. It works well but I'm nervous about replacing GDB's actual step-over operation. I'll post the prototype anyway, and we can see what to do about it.
(In reply to comment #5) > What remains: > 1- provide proper label. So, instead of showing $1 which does not mean > anything to the user, we must show "<method> returned" or something of the > sort. I've posted patchset 2 at https://git.eclipse.org/r/15377. That version updates the label properly. For simplicity, instead of "foo() returned" I just put "foo()" but I'm open to suggestions. The label logic was a bit tricky, due to implementation details. The requirement is that an expression needs to support an 'alias'. So, $1 would have alias "foo()". The patch supports this specifically for the return value use case by having the Expression service take care of the alias. It does that by keeping track of where a thread is stopped, and then, when it gets an MIFunctionFinishedEvent, it creates the alias based on the name of that previous method. My first instinct was that the stack service should be the one taking care of determining the method name that returned. However, the stack service would then need to pass that information to the expression service. That would mean new APIs: - IStack.IVariableDMData2 to provide a new getAlias() method - a new IExpressions4 to provide createExpression(ctx, expression, alias) to pass that alias to the expression service and the expression service would still have to delete the aliases when appropriate, so it would still need to manage them a bit. The alternative would be to add yet another API for the stack service to delete them from the expression service. To avoid all those APIs, I put the logic directly in the MIExpressions service class, but I'm conflicted about it.
I ran into a race condition where I received an MIStoppedEvent before the _preceding_ IResumed event. This can happen because an IResumed event is sent a little later once IRunControl has finished processing MIRunningEvent, while an MIStopped event is sent as soon as it is received from GDB. The impact what that an alias could be cleared before it was actually used. I pushed patchset 3 to only work with IResumed and ISuspended event, which is the proper way to do things.
I've rebased and pushed patchset 4. Mikhail, can you have a look when you have a moment? I think the only thing missing for this first part (assuming the implementation decision is ok), is to look into disabling the "show in memory" action from the variables view for those return values. I'll look at that when I come back from vacation.
Marc, Did you leave the "Watch" menu enabled intentionally? It adds $1 to the Expressions view but it doesn't seem to be very useful.
(In reply to Mikhail Khodjaiants from comment #9) > Marc, > > Did you leave the "Watch" menu enabled intentionally? It adds $1 to the > Expressions view but it doesn't seem to be very useful. I think I missed that. I'll have a look.
(In reply to Mikhail Khodjaiants from comment #9) > Did you leave the "Watch" menu enabled intentionally? It adds $1 to the > Expressions view but it doesn't seem to be very useful. $1 in this case is where GDB stored the returned value, and this expression remains valid throughout the rest of the debug session. However I agree that it will normally mean nothing to the user. Furthermore, that value will never change again, so there is not much point is showing it in the expressions view. I'll try to disable the "watch" on those entries.
(In reply to Marc Khouzam from comment #11) > I'll try to disable the "watch" on those entries. This is not trivial. Here is what I believe needs to be done to achieve this: 1-the MIStack service must create a special type of variable for $1, say IConvenienceVariableDMContext (new API #1) 2- step 1 probably should not be in MIStack but in a GdbStack specialized class (new API #2) 3- GdbVariableVMNode must then check for IConvenienceVariableDMContext and choose to create a special expression for such a context. This will require a new IExpressions.createConvenienceExpression() (new API #3) 4- but since we can't augment IExpressions, we'll need a new IExpressions4 or an IGdbExpressions (new API #4) 4- this IGdbExpressions.createConvenienceExpression() will need to return a new IConvenienceExpressionDMC (new API #5) 5- GdbVariableVMNode will then map IConvenienceExpressionDMC to a new GdbConvenienceVariableExpressionVMC (new API #6) 6- And finally, now that GdbVariableVMNode will know it is dealing with a GdbConvenienceVariableExpressionVMC, it will be able to turn off the "Watch" action by using a new GdbVariableExpressionFactory that can properly implement IWatchExpressionFactoryAdapter2 (new API #7) So, a pretty convoluted path requiring at least 7 new APIs, one new service extension and one new interface. On the one hand, if this is what is needed to get this done, we just have to do it. On the other, how annoying is having the "Watch" action enabled? Mikhail, what do you think? Is it worth the code changes? I'm not trying to sway you either way, I just rather discuss this before coding the changes than after, just in case :) (although I'm close to done anyway).
(In reply to Marc Khouzam from comment #12) > (In reply to Marc Khouzam from comment #11) > > > I'll try to disable the "watch" on those entries. > > This is not trivial. Here is what I believe needs to be done to achieve > this: > > 1-the MIStack service must create a special type of variable for $1, say > IConvenienceVariableDMContext (new API #1) > 2- step 1 probably should not be in MIStack but in a GdbStack specialized > class (new API #2) > 3- GdbVariableVMNode must then check for IConvenienceVariableDMContext and > choose to create a special expression for such a context. This will require > a new IExpressions.createConvenienceExpression() (new API #3) > 4- but since we can't augment IExpressions, we'll need a new IExpressions4 > or an IGdbExpressions (new API #4) > 4- this IGdbExpressions.createConvenienceExpression() will need to return a > new IConvenienceExpressionDMC (new API #5) > 5- GdbVariableVMNode will then map IConvenienceExpressionDMC to a new > GdbConvenienceVariableExpressionVMC (new API #6) > 6- And finally, now that GdbVariableVMNode will know it is dealing with a > GdbConvenienceVariableExpressionVMC, it will be able to turn off the "Watch" > action by using a new GdbVariableExpressionFactory that can properly > implement IWatchExpressionFactoryAdapter2 (new API #7) > > So, a pretty convoluted path requiring at least 7 new APIs, one new service > extension and one new interface. > > On the one hand, if this is what is needed to get this done, we just have to > do it. On the other, how annoying is having the "Watch" action enabled? > > Mikhail, what do you think? Is it worth the code changes? I'm not trying > to sway you either way, I just rather discuss this before coding the changes > than after, just in case :) (although I'm close to done anyway). I think it is better to leave the patch as it is, at least for now. If something else comes up we'll take another look at it.
(In reply to Mikhail Khodjaiants from comment #13) > I think it is better to leave the patch as it is, at least for now. If > something else comes up we'll take another look at it. Thanks. While in traffic though, I had the idea that maybe I could avoid all this API mess by simply checking if the expression starts with a '$' at the time we need to know if "watch" should be disabled. I'll try that out tomorrow to see if it is possible.
(In reply to Marc Khouzam from comment #14) > While in traffic though, I had the idea that maybe I could avoid all this > API mess by simply checking if the expression starts with a '$' at the time > we need to know if "watch" should be disabled. I'll try that out tomorrow > to see if it is possible. I thought about it too. Wouldn't it affect watching a register? What about the expression groups? Maybe we should check whether the expression is '$<number>'.
(In reply to Mikhail Khodjaiants from comment #15) > (In reply to Marc Khouzam from comment #14) > > While in traffic though, I had the idea that maybe I could avoid all this > > API mess by simply checking if the expression starts with a '$' at the time > > we need to know if "watch" should be disabled. I'll try that out tomorrow > > to see if it is possible. > > I thought about it too. Wouldn't it affect watching a register? It's good you mentioned it, I kept thinking of convenience variables and trace variables and forgot about registers. In the Registers view, a register is of type RegisterVMC instead of VariableExpressionVMC so it won't be affect. Also, a reg in the reg view does not start with a $ anyway. And there won't be registers in the variables view. To my surprise, the "watch" action is enabled in the expressions view, so registers in there would be affected. I guess the "watch" action can be useful in the Expressions view as it can be applied to children elements. > What about the expression groups? Yes, since "watch" works in the Expressions view, that would be a problem if the first element of the group is a register. > Maybe we should check whether the expression is '$<number>'. I agree. That would be safe for all above cases. Thanks! I have posted an updated version on gerrit.
Hi Marc. I tried the current patch, very promising! It looks like this only works with non-stop enabled, is this intentional? gdb 7.5.91.20130417-cvs-ubuntu
I've been working on and off on this feature. I've finally pushed the new version to gerrit. Here what has improved: 1- all-stop was broken (thanks Marc-Andre/Mikhail for noticing!). Fixed in next patch. 2- "View memory" was still enabled in the context menu although it does not make sense for a return value. This is fixed by the patch posted to bug 418710, which is a dependency to this fix. 3- I enabled the MIExpressions JUnit tests to also run with non-stop mode (they only ran in all-stop before) 4- I added two JUnit tests for return values, one to verify a simple return value, and one to verify a complex return value 5- Casting a return value to may cause errors in the detail pane. This is tracked by bug 422701. It happens when casting a child of a return value, which is a rare enough case that I don't think we should block this fix because of it. 6- When a return value was a pointer, expanding that pointer in the view would make the $1 string visible E.g., foo() returned int * 0xabcdef *$1 int 12345 Same issue when casting a return value to an array: foo() returned int[1] 0xabcdef $1[0] int 12345 I wasn't sure what would be best to show instead, so I went with the most obvious solution: foo() returned int * 0xabcdef *(foo() returned) int 12345 Same issue when casting a return value to an array: foo() returned int[1] 0xabcdef (foo() returned)[0] int 12345 7- Enhanced expressions are not really impacted by return values. Currently, the only way to show a return value using enhanced expressions is by using '=*' or '*' which will show that return value, along with all other variables. What I thought would be nice is that the user could create an enhanced expression such as "=*returned" to always show the return value of the selected thread, if there is one. I think this is an improvement that should be handled in a separate bug it we feel it is useful.
Marc-Andre noticed that the methods GdbVariableVMNode.isConvenienceVariable() and GdbVariableVMNode.isRegisterExpression() can give false positives for variables containing a $ sign (but not starting with one). Variables like 'ca$h' or 'a$12a". Those variables will not have the "View In Memory" button and sometimes the "Watch" button. Let's discuss if that is ok or not. If we allow those buttons all the time, so as to avoid false positives what will happen? Using "View In Memory" for a return value will pop-up an error dialogue saying "Can't view memory on ($1)", while "Watch" will add the expression $1 to the Expression view. The $1 expression is not a big deal although kind of cryptic. The error dialogue annoys me a little more, I find it less friendly. If we look at likeliness of the user being impacted, we need to compare how often there will be variables with a $ defined in the program vs how often the user will see a return value in CDT. I personally think return-values will be more common, especially if we starts supporting them for step-over operations. So the question comes down to: what is worse, an error dialog when trying to see the memory of a return value (more common case), or a missing "view memory/watch" option for a variable containing a $ (rarer case). We could figure out a way to solve both problems in the code, but from what I had previously investigated, I believe this will be larger effort that the benefit it will bring. I'm leaning towards accepting the limitation for variables containing a $. What do people think?
(In reply to Marc Khouzam from comment #19) > So the question comes down to: what is worse, an error dialog when trying to > see the memory of a return value (more common case), or a missing "view > memory/watch" option for a variable containing a $ (rarer case). > > We could figure out a way to solve both problems in the code, but from what > I had previously investigated, I believe this will be larger effort that the > benefit it will bring. > > I'm leaning towards accepting the limitation for variables containing a $. > > What do people think? I don't know how common it is to have $ in a variable but I've never seen is personally. So I'm assuming this is very rare therefore I agree with you.
I've opened bug 428140 to add support to show return values after a step over operation. I'm changing this bug title to reflect the fact that we now have support to show return values after a step-return operation. I committed this feature from Gerrit just now. Thanks Marc-Andre and Mikhail for the reviews.
