Bugzilla – Full Text Bug Listing
| Summary: | CDT debugger 'Display As Array...' does not show elements of large array | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Rob Marissen <robm777> |
| Component: | cdt-debug-dsf-gdb | Assignee: | Nobody - feel free to take it <nobody> |
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> |
| Severity: | major | ||
| Priority: | P3 | CC: | cdtdoug, marc.khouzam, nobody, pawel.1.piech |
| Version: | 8.1.0 | ||
| Target Milestone: | 8.1.0 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
The cast info from a casted expression is not passed to the the child partitions, so when the "real" children are created they are treated as if they are of the original type. I added a new private type 'ICastedIndexedPartition' to address this problem and pushed the fix to Gerrit - https://git.eclipse.org/r/6295. Marc, could you please review it. It would be good if we fix it for Juno. (In reply to comment #1) > The cast info from a casted expression is not passed to the the child > partitions, so when the "real" children are created they are treated as if they > are of the original type. > I added a new private type 'ICastedIndexedPartition' to address this problem > and pushed the fix to Gerrit - https://git.eclipse.org/r/6295. > Marc, could you please review it. It would be good if we fix it for Juno. I'm out of the office until Monday, so I won't be able to do a good review until then. I think you should go ahead with the commit to make it for Juno. Checked in. Thanks Mikhail for fixing this in time for Juno. I think this fix is fine for the release. I would like to suggest changing it after Juno though.
I spent quite some time yesterday sorting out in my mind the Casting logic and the Partition logic in MIExpressions (and got quite a headache :)). Now that I have a better picture, I believe that the root of the problem is a bug in the Casting logic. Before I dive into the details, let me just mention a limitation in the current fix. When using this code:
int longarray[1000];
int *longarraypointer;
longarraypointer=&longarray[0];
when displaying longarraypointer as an array of 1000, the elements below the partitions are shown as:
*((longarraypointer)+0)@100[0]
but when looking at it as an array of 10 (so no partitions) the elements are shows as
longarraypointer[0]
I believe we should be showing the elements under the partitions the same way as without partitions.
That being said, lets focus on what I think is the root of the problem, which hopefully will bring a much simpler fix for the maintenance branch.
From what I understand:
- each MIExpression is self-sufficient. This is also true for expressions created as subexpressions
- a casted expression is also self-sufficient
=> sub-expressions of a casted expression are just expressions; they don't know they have been casted. This implies that the cast need not be passed to sub-expressions; all the children need is the properly formed expression that describe them
- a partition is also self-sufficient but keeps track of its parent ExpressionInfo
=> a partition of a casted expression need not know it has been casted, but it does need to point to the casted parent, not the original parent, that way the sub-expressions can be generated properly from the casted parent.
So I would expect the original code to work, as long as the partition points to the casted expression. So why doesn't it? It turns out that the class CastedExpressionDMC creates its base MIExpressionDMC.ExpressionInfo from the non-casted expression, but then overrides getExpression() to return the casted expression. This breaks down when we use MIExpression.getExpressionInfo() which does not use the overridden getExpression().
So, in getTopLevelIndexedPartitions(), when calling createIndexedPartition() for a casted expression, the call to exprCtx.getExpressionInfo() returns the non-casted expression. To prove this, I created a new ExpressionInfo() calling getExpression() explicitly and things seem better. I pushed this first solution to gerrit for you to see what I mean, it is based on master before you made your change:
https://git.eclipse.org/r/6324 patchset 1.
I don't think this should be the final fix though. Let's try to fix the actual bug of CastedExpressionDMC. What I tried to do is that when we create a CastedExpressionDMC, we set the ExpressionInfo to the casted expression. That way, when calling getExpressionInfo() we properly get the casted expression information. This seems to keep the casting logic working. However, partitions of casted expressions where not properly parenthesized, so I also fixed this. I pushed that better fix to gerrit as patchset 2.
I realize all this must be really cryptic when not explained face-to-face, so I hope seeing it in Gerrit will make more sense.
What do you think?
(In reply to comment #4) > Thanks Mikhail for fixing this in time for Juno. I think this fix is fine for > the release. I would like to suggest changing it after Juno though. > > I spent quite some time yesterday sorting out in my mind the Casting logic and > the Partition logic in MIExpressions (and got quite a headache :)). Now that I > have a better picture, I believe that the root of the problem is a bug in the > Casting logic. Before I dive into the details, let me just mention a > limitation in the current fix. When using this code: > > int longarray[1000]; > int *longarraypointer; > longarraypointer=&longarray[0]; > > when displaying longarraypointer as an array of 1000, the elements below the > partitions are shown as: > *((longarraypointer)+0)@100[0] > > but when looking at it as an array of 10 (so no partitions) the elements are > shows as > longarraypointer[0] > > I believe we should be showing the elements under the partitions the same way > as without partitions. > > That being said, lets focus on what I think is the root of the problem, which > hopefully will bring a much simpler fix for the maintenance branch. > > From what I understand: > - each MIExpression is self-sufficient. This is also true for expressions > created as subexpressions > - a casted expression is also self-sufficient > => sub-expressions of a casted expression are just expressions; they don't know > they have been casted. This implies that the cast need not be passed to > sub-expressions; all the children need is the properly formed expression that > describe them > > - a partition is also self-sufficient but keeps track of its parent > ExpressionInfo > => a partition of a casted expression need not know it has been casted, but it > does need to point to the casted parent, not the original parent, that way the > sub-expressions can be generated properly from the casted parent. > > So I would expect the original code to work, as long as the partition points to > the casted expression. So why doesn't it? It turns out that the class > CastedExpressionDMC creates its base MIExpressionDMC.ExpressionInfo from the > non-casted expression, but then overrides getExpression() to return the casted > expression. This breaks down when we use MIExpression.getExpressionInfo() > which does not use the overridden getExpression(). > > So, in getTopLevelIndexedPartitions(), when calling createIndexedPartition() > for a casted expression, the call to exprCtx.getExpressionInfo() returns the > non-casted expression. To prove this, I created a new ExpressionInfo() calling > getExpression() explicitly and things seem better. I pushed this first > solution to gerrit for you to see what I mean, it is based on master before you > made your change: > https://git.eclipse.org/r/6324 patchset 1. > > I don't think this should be the final fix though. Let's try to fix the actual > bug of CastedExpressionDMC. What I tried to do is that when we create a > CastedExpressionDMC, we set the ExpressionInfo to the casted expression. That > way, when calling getExpressionInfo() we properly get the casted expression > information. This seems to keep the casting logic working. However, > partitions of casted expressions where not properly parenthesized, so I also > fixed this. I pushed that better fix to gerrit as patchset 2. > > I realize all this must be really cryptic when not explained face-to-face, so I > hope seeing it in Gerrit will make more sense. > > What do you think? It seems that your second patch works fine assuming that it doesn't affect the casting logic somewhere else. What's the problem with it? (In reply to comment #5) > It seems that your second patch works fine assuming that it doesn't affect the > casting logic somewhere else. What's the problem with it? No known problem :) It just took me a while to sort all this out in my head, so I was concerned my explanation might have been more confusing than it should. I'll update the patch to current master, investigate a bit more about the casting aspect, and wait until the 8_1 branch is created to commit. I'll also try to add tests. Looking at bug 306555 which introduced the casting feature, I now see why CastedExpressionDMC ended up overriding getExpression(). The original patch was creating the casted expression inside the constructor to CastedExpressionDMC and therefore could not pass it to super() right away. Since MIExpressionsDMC did not have a setExpression() method, the expression was not set to the casted expression, but getExpression() was overridden instead. By the time the code was committed, building the casted expression was moved out of the constructor, but I didn't think to change the way the expression was handled. Note also that the casting feature came before the pretty printing feature, which added the use of ExpressionInfo, and further complicated things. All this to say that I feel pretty safe making the change which I have now pushed to gerrit as patchset 3. (In reply to comment #7) > All this to say that I feel pretty safe making the change which I have now > pushed to gerrit as patchset 3. I agree. Thanks Committed to master. I'm setting the milestone to 8.1.0 since Mikhail's original fix is part of Juno. I've messed things up with the change in parentheses handling. Good thing that part is not in Juno :) For example, with int array[10]; which we cast to char[] instead of getting 40 entries of one character, we get 40 entries which are each an array of 4 elements. The parens should be: ((char[])(array))[0] but instead they currently are (char[])(shortarray)[0]. the outer parens are missing which makes the array index apply too early. I'm working on a patch. I pushed a fix to Gerrit for the parentheses problem. It is a two-line change. https://git.eclipse.org/r/6426 I've also posted the tests in a separate review to make things easier. I added 7 new tests, but the two last ones currently fail due to bug 382692. https://git.eclipse.org/r/6427 I committed the fix and the JUnit tests to master. I'm now going to finish bug 382692 which will make the 2 failing tests pass. Thanks Mikhail for the review. |
Build Identifier: 20120531-0157 The debugger function 'Display As Array' does not work correctly for arrays > 100 elements. Such array is displayed as chunks of 100 elements, trying to open a chuck only shows 100 empty lines, instead of the actual array content. Using GNU gdb 6.8-debian on Debian 5.0 Reproducible: Always Steps to Reproduce: 1. Run the following program in the debugger and set a breakpoint on 'return 0'. 2. In the variables window, right click on bar, select 'Display As Array...', and enter "length 1000" 3. Try to open array element e.g. 300. Code to reproduce: #include <stdio.h> int main(int argc, char *argv[]) { int foo[1000]; int *bar; bar=&foo[0]; return 0; }