Community
Participate
Working Groups
Build Identifier: M20100909-0800 Put the following code into main and try to debug. Watch variables view. It will have 2 variables (value) and if select one it starts flashing. So if variable is composite (class/struct) you couldn't go inside and check what's inside. { size_t value = 10; } size_t value = 20; P.S. I know it's not a good style to do this but it's a bug anyway :). Reproducible: Always Steps to Reproduce: 1. Compile 2. Debug 3. Select value variable in Variables view.
Wow. I didn't realize it was this bad :-O When two variables of the same name are in scope, although one is shadowing the other, GDB returns both variables to us. We then create two _identical_ contexts for the variables view. The flickering happens because if we select one element, since there is a second one that is identical, the view seems to not know which one to choose. We need to decide how to deal with the situation of having multiple variables of the same name. 1- Do we show both at the same time (like now) assuming we can get the proper values? 2- Or do we only show one, the one that it is scope? When both variables with the same name are in scope, I don't know of a way to ask GDB to get the value of the shadowed variable. Therefore, at this time, I only see that we can implement solution 2. Even with solution 2, another problem will occur in the if we stop at line 1 of the following program: printf(""); //line 1 { int myvar = 10;// line 3 } int myvar = 20; at line 1, gdb will only report the variable that is in scope and we will create a variable object to track it. When we step to line 3, gdb will report both variables since they are both in scope. The problem is that we will re-use the variable object that we created which still tracks the now-shadowed myvar. So we won't show the value of the current myvar. We somehow need to know that we need to create a different variable object for the new myvar. The only way I can think of right now of knowing there is a new myvar, is to notice that GDB reports two variables with that name, when it used to report only one. Not sure how we could use this in the code though... I have to think more about this.
Ideally gdb would provide a way to distinguish between both. But I didn't find something usable. Have you raised this issue to the gdb mailing list? Gdb's command line print selects the innermost, so they should have some means to figure that out. Some brain storming: MIStack - I would still tend towards MIStack returning only one of the duplicates (see pawels comments in bug 328573). In a debugger, I'm normally interested in the one that is active at the current line of code. - MIStack could determine the address of the local variable sending -data-evaluate-expression for &(localVarName). The address becomes an attribute of MIVariableDMC. Clients could use the address to see that the innermost instance has changed, though I don't know if that helps anybody. MIExpressions: - the first step in updating a root variable could be to determine that its address has changed. Again, this could happen via -data-evalulate-expression &varNam. If the address changed, we throw away all children and create it the variable anew (would need thourough testing with pretty printers). My basic assumption here is that -data-evaluate-expression would refer to the innermost variable. - instead of binding local variables to stack frames, could they be floating?
(In reply to comment #2) > Ideally gdb would provide a way to distinguish between both. But I didn't find > something usable. > > Have you raised this issue to the gdb mailing list? Gdb's command line print > selects the innermost, so they should have some means to figure that out. I haven't yet, but I will. > - the first step in updating a root variable could be to determine that its > address has changed. Again, this could happen via -data-evalulate-expression > &varNam. If the address changed, we throw away all children and create it the > variable anew (would need thourough testing with pretty printers). My basic > assumption here is that -data-evaluate-expression would refer to the innermost > variable. That is a good idea. Using the address of the expression sounds like a good way to figure out if a varObj should be discarded. My suggestion was to use -stack-list-locals and if an expression was there more than once, we would discard the varObj; but that is not as elegant and will require more hacking. I like your solution better. > - instead of binding local variables to stack frames, could they be floating? I've been thinking about that since yesterday. Floating varObj (-var-create * @ var) would easily allow us to fix this particular issue, since the varObj would point to the inner most variable (although we would have to make sure that works properly with GDB). Another advantage would be that we would not need to figure out which frame an expression belonged to, to find its varObj; instead, there would be a single varObj per expression. The main technical drawback to this solution is that the same varObj would change whenever we would point to another stack frame. So, unlike now where we do a -var-update when the program suspends, if we use floating varObj, we would need to do a var-update every time the user chose a new stack frame. You can imagine the case where the user would go back in forth in the debug view between two stack frames, and we would constantly need to go to the backend to as GDB to make the varObj point to the current stack frame. To optimize this, we would need a new caching mechanism where MIVariableManager would cache information about a varObj for each frame, to avoid going to the backend. This is still not perfect because we could only cache was has been requested; at the next stack frame selection, the user may ask for more data, which is not cached, and we'd have to send a new -var-update. Of course, using floating varObj would also require major re-design and test. Besides fixing this bug, I don't see a another advantage of using floating varObj, so I'm currently of the opinion we should not go that route.
(In reply to comment #3) > Besides fixing this bug, I don't see a another advantage of using floating > varObj, so I'm currently of the opinion we should not go that route. Non of the two aproaches is very simple, but another argument against floating varobjs: this solution would impact things that already work. While checking for the changed address triggers new code to be executed only for the problem case. And how much worse could it become?
(In reply to comment #4) > (In reply to comment #3) > > Besides fixing this bug, I don't see a another advantage of using floating > > varObj, so I'm currently of the opinion we should not go that route. > Non of the two aproaches is very simple, but another argument against floating > varobjs: this solution would impact things that already work. While checking > for the changed address triggers new code to be executed only for the problem > case. And how much worse could it become? Yes, let's not completely change our current design :-) So, where do we stand? I believe we can do the following. As a first step, we only show a single expression if there are duplicates, and we make sure that we show the correct content. So, we do the &(var) trick to see if we should scrap an existing varObj. I'm hoping this will be almost free since we already do fetch &(var). As an optional second step, if we feel it is a good feature, we could show all duplicate variable names, but make sure each one show the right content. Since we cannot have different varObj for the same expression in the same frame, only the innermost var will be backed by a varObj; for the other variables, we will only know their value and type using "-stack-list-locals 2". We would then have to properly 'reject' any other user request such as modifying that unaccessible variable, or looking at the details. I'm not sure this is worth the effort, and I know I won't do it myself :-) So, for step 1, Jens, do you want to take a stab at it?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Besides fixing this bug, I don't see a another advantage of using floating > > > varObj, so I'm currently of the opinion we should not go that route. > > Non of the two aproaches is very simple, but another argument against floating > > varobjs: this solution would impact things that already work. While checking > > for the changed address triggers new code to be executed only for the problem > > case. And how much worse could it become? > Yes, let's not completely change our current design :-) > So, where do we stand? I believe we can do the following. As a first step, we > only show a single expression if there are duplicates, and we make sure that we > show the correct content. So, we do the &(var) trick to see if we should scrap > an existing varObj. I'm hoping this will be almost free since we already do > fetch &(var). > As an optional second step, if we feel it is a good feature, we could show all > duplicate variable names, but make sure each one show the right content. Since > we cannot have different varObj for the same expression in the same frame, only > the innermost var will be backed by a varObj; for the other variables, we will > only know their value and type using "-stack-list-locals 2". We would then > have to properly 'reject' any other user request s uch as modifying that > unaccessible variable, or looking at the details. I'm not sure this is worth > the effort, and I know I won't do it myself :-) > So, for step 1, Jens, do you want to take a stab at it? From what I understand, this would mean there still is a fix needed to eliminate the flickering, and make sure the local variable appears only once. This sounds like the fix attached to bug328573 should do the job in a proper manner. This would be the easy part of step 1:-) If you have something else, this is fine as well. Now, the remaining part is to fix MIVariableManager & Co to show the correct value. I can have a look. However I want to fix two critical issues with pretty printing support first. And I will fix any other critical issues first that might show up with the pretty printing support in the meantime. So it will take a while until I would be ready (I only have my sparetime). We would have a deal if you fix the break point marker update for pending breakpoints, latest gdb version, of course...
(In reply to comment #6) > From what I understand, this would mean there still is a fix needed to > eliminate the flickering, and make sure the local variable appears only once. > This sounds like the fix attached to bug328573 should do the job in a proper > manner. This would be the easy part of step 1:-) If you have something else, > this is fine as well. I attached a slight modification of your solution to bug 328573. We can start by committing this if you agree with my change. > Now, the remaining part is to fix MIVariableManager & Co to show the correct > value. I can have a look. However I want to fix two critical issues with pretty > printing support first. And I will fix any other critical issues first that > might show up with the pretty printing support in the meantime. So it will take > a while until I would be ready (I only have my sparetime). We would have a deal > if you fix the break point marker update for pending breakpoints, latest gdb > version, of course... Noted :-) I'll see what my 'spare time' allows me to do.
(In reply to comment #7) > I attached a slight modification of your solution to bug 328573. We can start > by committing this if you agree with my change. I've committed this fix, so the flickering is gone. Now we have to fix the fact that we may not be showing the right content.
Created attachment 183119 [details] test case + fix When starting to look at the pretty printing issues, it hit me that MIVariableManager actually has everything we would need. All that is to be added is another criterion when a variable goes out-of-scope. So, here we go. The MIExpressionsTests as well as the PrettyPrinterTests still pass.
(In reply to comment #9) > Created an attachment (id=183119) [details] > test case + fix > > When starting to look at the pretty printing issues, it hit me that > MIVariableManager actually has everything we would need. All that is to be > added is another criterion when a variable goes out-of-scope. > > So, here we go. The MIExpressionsTests as well as the PrettyPrinterTests still > pass. Thanks Jens! I'm looking at it now. Let me ask, in MIRootVariableObject#update(), even before this patch, why were we using a CountingRequestMonitor? Also, shouldn't we update the updatesPending, event if the object is now out of scope? We may need a but to clean that up, I missed it in my review.
(In reply to comment #10) > Let me ask, in MIRootVariableObject#update(), even before this patch, why were > we using a CountingRequestMonitor? Also, shouldn't we update the > updatesPending, event if the object is now out of scope? We may need a bug to > clean that up, I missed it in my review. It took me a while but I figured out that the CountingRm was used as a 'trick' to avoid duplicating code. And that the updatesPending were handled properly. But I still wend ahead and cleaned it up because it was hard to understand. I did it as part of Bug 302121.
I will adapt it to the new version of MIVariableManager. There is also one thing that I don't understand with when and address expression exists, and when not. I will provide a new patch as soon as I have understood that.
Created attachment 183209 [details] new version Hope I did my homework regarding the RequestMonitor usage:-) In addition, I realized that the address expression must be evaluated in the stack frame matching the MIRootVariable object. I provided a solution that I think looks correct, just it isn't. I use frameDMC = DMContexts.getAncestorOfType(exprCtx, IFrameDMContext.class); to determine a frame context that a later use to call fCommandFactory.createMIDataEvaluateExpression(frameDMC, addressExpression) But it turns out that the applied --frame option is wrong. This makes the MIExpressionsTest#testLocalVariables fail because it runs into an error. Can provide me with a hint what is wrong with it?
Created attachment 183267 [details] revised version I think I have it know. Ready for feedback.
Created attachment 183394 [details] updated to HEAD This patch is updated to contain the fix Marc recently did for bug 330289 such that it is compatible with HEAD again.
