Community
Participate
Working Groups
Build Identifier: 20110615-0604 this is the mi log: 863,876 60-data-evaluate-expression --thread 1 --frame 0 k 863,876 60^done,value="{10, 0 <repeats 9999 times>}" 863,876 (gdb) 868,736 61-var-create --thread 1 --frame 0 - * k[0] 868,736 62-var-create --thread 1 --frame 0 - * k[1] 868,736 63-var-create --thread 1 --frame 0 - * k[2] 868,736 61^done,name="var6",numchild="0",value="10",type="int",thread-id="1",has_more="0" 868,736 (gdb) 868,736 62^done,name="var7",numchild="0",value="0",type="int",thread-id="1",has_more="0" 868,736 (gdb) 868,736 63^done,name="var8",numchild="0",value="0",type="int",thread-id="1",has_more="0" 868,736 (gdb) 870,017 64-var-set-format var5 octal 870,017 64^done,format="octal",value="010460574" 870,017 (gdb) 870,017 65-var-set-format var5 natural 870,017 65^done,format="natural",value="0x22617c" 870,017 (gdb) 870,017 66-var-set-format var5 binary 870,033 66^done,format="binary",value="1000100110000101111100" 870,033 (gdb) 870,033 67-var-set-format var5 natural 870,048 67^done,format="natural",value="0x22617c" 870,048 (gdb) 870,048 68-var-set-format var5 decimal 870,048 68^done,format="decimal",value="2253180" 870,048 (gdb) 870,048 69-var-set-format var5 natural 870,064 69^done,format="natural",value="0x22617c" 870,064 (gdb) 870,064 70-var-set-format var5 hexadecimal 870,080 70^done,format="hexadecimal",value="0x22617c" 870,080 (gdb) 870,080 71-var-set-format var5 natural 870,080 71^done,format="natural",value="0x22617c" ........ .... Reproducible: Always Steps to Reproduce: 1.Define a variable in the source: int k[1000] = {10}, 2.start Debug 3.run to the line 4.Move the mouse to the top of this variable in the editor, this cause the interface to die
The UI freezes because of the SWT tree viewer. The freeze happens not only when hoovering over a large array, but also when expanding it in the Variables or Expressions views. DSF should implement an equivalent of the standard model's "indexed value" approach.
(In reply to comment #1) > The UI freezes because of the SWT tree viewer. The freeze happens not only when > hoovering over a large array, but also when expanding it in the Variables or > Expressions views. > DSF should implement an equivalent of the standard model's "indexed value" > approach. DSF should only fetch the visible parts of the array. Usually that is 5 or 6 elements at a time. Why is it freezing?
(In reply to comment #2) > DSF should only fetch the visible parts of the array. Usually that is 5 or 6 > elements at a time. Why is it freezing? It is freezing inside SWT, nothing to do with DSF. I guess the viewer creates all nodes internally and then requests the data for the visible ones.
(In reply to comment #3) > (In reply to comment #2) > > DSF should only fetch the visible parts of the array. Usually that is 5 or 6 > > elements at a time. Why is it freezing? > > It is freezing inside SWT, nothing to do with DSF. I guess the viewer creates > all nodes internally and then requests the data for the visible ones. I see. Yes, I do believe we create the entire list of children, but it is the content we don't fetch. (In reply to comment #1) > DSF should implement an equivalent of the standard model's "indexed value" > approach. I think you are referring to Bug 207582. I've longed wished for that feature, but no one (including me) took the time to do it.
(In reply to comment #4) > I think you are referring to Bug 207582. I've longed wished for that feature, > but no one (including me) took the time to do it. Yes, that's the feature we have in CDI and JDT.
Created attachment 209289 [details] Implementaion of array partitions.
Created attachment 209290 [details] Image of indexed partition.
Created attachment 209291 [details] Screenshot of the Variables view displaying a large array
Marc, could you please review this patch when you have time?
(In reply to comment #9) > Marc, could you please review this patch when you have time? Awesome! Thanks for this! I will look at the patch tomorrow
Very nice feature! Even better than with CDI. With the details pane working too! Here are some minor comments: 1- how come we need a new icon in debug.ui? Can we use the one from CDI (I don't know where it is located though) 2- do we need IExpressions4? It only contains another interface so could be added to IExpressions directly. I don't see any errors from the API tool when doing that. 3- I wonder if it should be in VariableVMNode that we define the display format for the IIndexedPartitionDMContext? Maybe the MIExpressions service would be a better place for it since it does it already for everything else. The disadvantage I can see is that it would not work out of the box for DSF, but would require all extenders of DSF to implement it in their service; the flip side is that it would allow to easily change what that output would like like, by overriding the service. Also, I don't think it would work out-of-the-box anyway, since we need other support for this feature in the MIExpressions service. 4- In MIExpressions, why remove 'private' from private MIExpressionDMC(String sessionId, ExpressionInfo info, IDMContext parent) { 5- Would it be nice to have a preference for the size of a partition? Default being 100 6- If possible, we should have some JUnit test for the new parts of MIExpressions. I have to admit I haven't been very good about doing that myself recently, but I thought I would at least suggest it. But that should not prevent this patch from going in. 7- I tried a couple of obscure scenarios and found two that caused problems. To make things easier I changed PARTITION_LENGTH to be 10 instead of 100. I'm seeing failures with the below nested structure when you keep opening things. struct com { int zz[12]; }; com c[13]; 8- I'm also seeing problems with the (very rare) case of a struct with more than the partition limit of children (again I set PARTITION_LENGTH = 10). I believe the code tries to group elements even if they are not part of an array. CDI does not do that: struct com { int zz[12]; int allo1; int allo2; int allo3; int allo4; int allo5; int allo6; int allo7; int allo8; int allo9; int allo10; int allo11; int allo12; com c[13];
(In reply to comment #11) > 8- I'm also seeing problems with the (very rare) case of a struct with more > than the partition limit of children (again I set PARTITION_LENGTH = 10). > I believe the code tries to group elements even if they are not part of an > array. CDI does not do that: > struct com { > int zz[12]; > int allo1; > int allo2; > int allo3; > int allo4; > int allo5; > int allo6; > int allo7; > int allo8; > int allo9; > int allo10; > int allo11; > int allo12; };// missing from previous comment > com c[13];
(In reply to comment #11) Thanks for reviewing it. > Very nice feature! Even better than with CDI. With the details pane working > too! > > Here are some minor comments: > > 1- how come we need a new icon in debug.ui? Can we use the one from CDI (I > don't know where it is located though) > The CDI icon is not located in CDT plug-ins. CDI doesn't deal with the UI, the support for indexed partitions is implemented in the platform's standard debug model. The icon is only used internally and there is no API to access it (the image id is defined in IInternalDebugUIConstants). > 2- do we need IExpressions4? It only contains another interface so could be > added to IExpressions directly. I don't see any errors from the API tool when > doing that. > Good, I'll move it to IExpressions. > 3- I wonder if it should be in VariableVMNode that we define the display format > for the IIndexedPartitionDMContext? Maybe the MIExpressions service would be a > better place for it since it does it already for everything else. The > disadvantage I can see is that it would not work out of the box for DSF, but > would require all extenders of DSF to implement it in their service; the flip > side is that it would allow to easily change what that output would like like, > by overriding the service. Also, I don't think it would work out-of-the-box > anyway, since we need other support for this feature in the MIExpressions > service. > As far as I understand the design VariableVMNode is the label provider for 'IExpressionDMContext' elements. It provides the images and label names which are the same as the corresponding expressions except for this case. It should work out of box if the service returns 'IIndexedPartitionDMContext' elements, especially after I move 'IIndexedPartitionDMContext' to 'IExpressions'. For those who want to change the presentation I can make 'getExpressionDisplayName()' method protected to allow overriding it. Basically, it's the same idea as for the standard model which takes care of the UI side of partitions and allows implementations to plug into it by implementing 'IIndexedValue' interface. > 4- In MIExpressions, why remove 'private' from > private MIExpressionDMC(String sessionId, ExpressionInfo info, IDMContext > parent) { > No need to do it. > 5- Would it be nice to have a preference for the size of a partition? Default > being 100 > I'm not sure about this. I don't think there is a preference for same feature in JDT. By making the partition's size customizable we accept that it will work for all sizes which is not true unless you want to add a range for acceptable values. > 6- If possible, we should have some JUnit test for the new parts of > MIExpressions. I have to admit I haven't been very good about doing that > myself recently, but I thought I would at least suggest it. But that should > not prevent this patch from going in. > > 7- I tried a couple of obscure scenarios and found two that caused problems. > To make things easier I changed PARTITION_LENGTH to be 10 instead of 100. > > I'm seeing failures with the below nested structure when you keep opening > things. > struct com { > int zz[12]; > }; > com c[13]; > Do you mean failures in the detail pane or somewhere else? > > 8- I'm also seeing problems with the (very rare) case of a struct with more > than the partition limit of children (again I set PARTITION_LENGTH = 10). > I believe the code tries to group elements even if they are not part of an > array. CDI does not do that: > struct com { > int zz[12]; > int allo1; > int allo2; > int allo3; > int allo4; > int allo5; > int allo6; > int allo7; > int allo8; > int allo9; > int allo10; > int allo11; > int allo12; > com c[13]; My bad. I remember discussing this case with Alain and he convinced me that we should do the same for structures. Apparently it didn't work out. Makes sense for structures with 10000 children or more but it is very unrealistic case. It is possible though if the code is generated. I'll fix these but please let me know what do you think about the preference and display name issues Thanks
(In reply to comment #13) > The CDI icon is not located in CDT plug-ins. CDI doesn't deal with the UI, the > support for indexed partitions is implemented in the platform's standard debug > model. The icon is only used internally and there is no API to access it (the > image id is defined in IInternalDebugUIConstants). I get it. I really don't know much about the standard debug model I'm affraid. > As far as I understand the design VariableVMNode is the label provider for > 'IExpressionDMContext' elements. It provides the images and label names which > are the same as the corresponding expressions except for this case. > It should work out of box if the service returns 'IIndexedPartitionDMContext' > elements, especially after I move 'IIndexedPartitionDMContext' to > 'IExpressions'. > For those who want to change the presentation I can make > 'getExpressionDisplayName()' method protected to allow overriding it. > > Basically, it's the same idea as for the standard model which takes care of the > UI side of partitions and allows implementations to plug into it by > implementing 'IIndexedValue' interface. Ok, sounds good. > > 5- Would it be nice to have a preference for the size of a partition? Default > > being 100 > > > > I'm not sure about this. I don't think there is a preference for same feature > in JDT. By making the partition's size customizable we accept that it will work > for all sizes which is not true unless you want to add a range for acceptable > values. I find that partitions of 100 are pretty big, and I thought that people might prefer to have smaller partitions. I would suggest a range of 5 to 1000. An technical argument in favor of smaller partition is that with DSF we only fetch the content of a variable when it is visible. If the user wants to see element 90, she would need to scroll down over all 90 elements. With a smaller partition, that could be avoided. But in truth, I don't imagine any user choosing the smaller partition for that reason. I think it comes down to user-friendliness, and if the platform feels 100 is a good number, who am I to disagree? So, I'm ok either way for a preference or no preference. > > 7- I tried a couple of obscure scenarios and found two that caused problems. > > To make things easier I changed PARTITION_LENGTH to be 10 instead of 100. > > > > I'm seeing failures with the below nested structure when you keep opening > > things. > > struct com { > > int zz[12]; > > }; > > com c[13]; > > > > Do you mean failures in the detail pane or somewhere else? Actual failure in the variables view as you keep expanding levels. I can send you a screenshot and logs if you can't easily reproduce it. > > 8- I'm also seeing problems with the (very rare) case of a struct with more > > than the partition limit of children (again I set PARTITION_LENGTH = 10). > > I believe the code tries to group elements even if they are not part of an > > array. CDI does not do that: > > struct com { > > int zz[12]; > > int allo1; > > int allo2; > > int allo3; > > int allo4; > > int allo5; > > int allo6; > > int allo7; > > int allo8; > > int allo9; > > int allo10; > > int allo11; > > int allo12; > > com c[13]; > > My bad. I remember discussing this case with Alain and he convinced me that we > should do the same for structures. Apparently it didn't work out. > Makes sense for structures with 10000 children or more but it is very > unrealistic case. It is possible though if the code is generated. I guess it could prove useful, but I think it may not be simple as you will deal with different types. Up to you if you want to try to make it work :) > I'll fix these but please let me know what do you think about the preference > and display name issues Ok to leave as in the patch now. Thanks
(In reply to comment #14) > > > 7- I tried a couple of obscure scenarios and found two that caused problems. > > > To make things easier I changed PARTITION_LENGTH to be 10 instead of 100. > > > > > > I'm seeing failures with the below nested structure when you keep opening > > > things. > > > struct com { > > > int zz[12]; > > > }; > > > com c[13]; > > > > > > > Do you mean failures in the detail pane or somewhere else? > > Actual failure in the variables view as you keep expanding levels. I can send > you a screenshot and logs if you can't easily reproduce it. > Please, post the logs. I only get errors in the detail pane because the resulting expressions for intermediate levels are not correct.
Created attachment 209483 [details] Some logs (In reply to comment #15) > Please, post the logs. I only get errors in the detail pane because the > resulting expressions for intermediate levels are not correct. I have to leave now, but here are some logs I got with the partition at 100 and the arrays bigger. You can see errors at the bottom Let me know if those help and if they don't I'll reproduce properly and gather the info.
Created attachment 209487 [details] Updated patch. This patch: 1. moves 'IIndexedPartitionDMContext' interface to 'IExpressions' 2. makes 'VariableVMNode.getExpressionDisplayName()' method protected to allow overriding it 3. restores 'private' in the constructor of 'MIExpressionDMC' 4. fixes the usage of complex expression names 5. restricts the partitioning to array types
Marc, I have modified the patch. Can please try it to sees if it fixes the failures. I didn't add the preference we discussed, let's wait and see if there is a request for it. Re JUnit tests, I'll think about it later.
(In reply to comment #18) > Marc, I have modified the patch. Can please try it to sees if it fixes the > failures. Yes, the new patch fixes both problems I was seeing. Thanks. By total fluke though, I noticed two other bugs. The first is that types get re-used for partitions that have the same index and size. To reproduce, I have two arrays of different types; open the first array to create the partitions, and then open the second array, and you will see that the type of the partitions [0..9] is the type of the first array. I figured it is because MIExpressions is getting confused about different IndexedPartitionDMC. Looking at IndexedPartitionDMC.equals() I noticed it was not comparing fParentInfo, so I think it finds any [0..9] to be equal even if they are of different types. I added the line ((IndexedPartitionDMC) other).getParentInfo().equals(getParentInfo()) && and it fixed the problem. The other problem I haven't looked into but here is what happens. With PARTITION_LENGTH = 10; and code like int d[110][120]; I see: d int[110][120] [0..99] int[100][120] <=== Missing [100..119] > I didn't add the preference we discussed, let's wait and see if there is a > request for it. > Re JUnit tests, I'll think about it later. Ok with me.
I also noticed two small things in VariableVMNode.fillExpressionDataProperties(). The line update.setProperty(PROP_NAME, displayName); should probably go outside the if statement, or else the property won't be set in case the if statement is not entered. Also, the original code would use data.getName() to determine the name of the expression. The new code instead uses dmc.getExpression(). It so happens to be the same thing because of how MIExpressions sets the expressionData, but I don't think it is right to assume the two will always be the same. We should probably continue to use data.getName().
Created attachment 209858 [details] Updated patch.
Created attachment 209859 [details] Updated patch.
(In reply to comment #20) > I also noticed two small things in > VariableVMNode.fillExpressionDataProperties(). > > The line > update.setProperty(PROP_NAME, displayName); > should probably go outside the if statement, or else the property won't be set > in case the if statement is not entered. > > Also, the original code would use data.getName() to determine the name of the > expression. The new code instead uses dmc.getExpression(). It so happens to > be the same thing because of how MIExpressions sets the expressionData, but I > don't think it is right to assume the two will always be the same. We should > probably continue to use data.getName(). Marc, I have fixed all these issues. Sorry that I didn't test my patch enough - was traveling most of the last and previous weeks.
(In reply to comment #23) > Marc, I have fixed all these issues. Thanks. I'll have look. > Sorry that I didn't test my patch enough - > was traveling most of the last and previous weeks. I think it is simply that when reviewing a patch, one tries different things than the designer does. That's why it is always nice to have another pair of eyes :)
(In reply to comment #19) Thanks. I'm still seeing this bug with the new patch though: > The first is that types get re-used for partitions that have the same index and > size. To reproduce, I have two arrays of different types; open the first array > to create the partitions, and then open the second array, and you will see that > the type of the partitions [0..9] is the type of the first array. > > I figured it is because MIExpressions is getting confused about different > IndexedPartitionDMC. Looking at IndexedPartitionDMC.equals() I noticed it was > not comparing fParentInfo, so I think it finds any [0..9] to be equal even if > they are of different types. I added the line > ((IndexedPartitionDMC) other).getParentInfo().equals(getParentInfo()) && > and it fixed the problem. Also, I tried drag-and-dropping a partition from the variables view to the expressions view (I'm not sure if you were aware of this feature, we don't talk about it much). I had an array: int ab[15]; and I copied the [0..9] partition. The copy went well and the expression of the partition showed up in the expressions view properly as *((ab)+0)@10. But when I expanded that expression in the expressions view I got: 668,991 [MI] 154-var-create --thread 1 --frame 0 - * *((ab)+0)@10[0] 668,993 [MI] 154^error,msg="cannot subscript something of type `int'" as well as an NPE: java.lang.NullPointerException at org.eclipse.cdt.dsf.debug.ui.viewmodel.variable.VariableVMNode.getExpressionDisplayName(VariableVMNode.java:1385) at org.eclipse.cdt.dsf.debug.ui.viewmodel.variable.VariableVMNode$18.handleCompleted(VariableVMNode.java:791) at org.eclipse.cdt.dsf.concurrent.RequestMonitor$2.run(RequestMonitor.java:303) at org.eclipse.cdt.dsf.concurrent.ImmediateExecutor.execute(ImmediateExecutor.java:63) at org.eclipse.cdt.dsf.concurrent.RequestMonitor.done(RequestMonitor.java:300) at org.eclipse.cdt.dsf.concurrent.RequestMonitor.handleError(RequestMonitor.java:447) at org.eclipse.cdt.dsf.concurrent.RequestMonitor.handleErrorOrWarning(RequestMonitor.java:429) at org.eclipse.cdt.dsf.concurrent.RequestMonitor.handleFailure(RequestMonitor.java:415) at org.eclipse.cdt.dsf.concurrent.RequestMonitor.handleCompleted(RequestMonitor.java:378) at org.eclipse.cdt.dsf.concurrent.RequestMonitor$2.run(RequestMonitor.java:303) at org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor$TracingWrapperRunnable.run(DefaultDsfExecutor.java:374) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303) at java.util.concurrent.FutureTask.run(FutureTask.java:138) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:98) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:206) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:662) Sorry, this is turning out to be a more complex feature that would have seemed.
(In reply to comment #25) > The copy went well and the expression of the partition showed up in the > expressions view properly as > *((ab)+0)@10. I tried writing this expression manually in the expressions view on the master branch. I still see: 657,032 [MI] 132^error,msg="cannot subscript something of type `int'" but there is no NPE, and the view is filled with an error message (which does not happen with the NPE, where the view just adds a bunch of empty lines). So, I think that protecting against the NPE would be enough, as the other problem was already there. I opened Bug 369341 about the subscripting problem.
Created attachment 209927 [details] Updated patch. Sorry, I missed the first issue. I had tried adding a partition to the Expression view earlier but never expanded it I guess. I also tried "View Memory" action on a partition, it seems to be working fine.
(In reply to comment #27) > Created attachment 209927 [details] > Updated patch. > > Sorry, I missed the first issue. > I had tried adding a partition to the Expression view earlier but never > expanded it I guess. > I also tried "View Memory" action on a partition, it seems to be working fine. Ok, everything I reported is now working as expected. I don't want to be exasperating, but when I try running the JUnit tests, I get two new failures due to the changes to MIExpressions.getSubExpressions() When I run: org.eclipse.cdt.tests.dsf.gdb.tests.tests_7_3.MIExpressionsTest_7_3 I see a failure in: MIExpressionsTest.testChildren() and MIExpressionsTest.testConcurrentReadWriteChildren()
(In reply to comment #28) > I don't want to be exasperating, but when I try running the JUnit tests, I get > two new failures due to the changes to MIExpressions.getSubExpressions() > > When I run: > org.eclipse.cdt.tests.dsf.gdb.tests.tests_7_3.MIExpressionsTest_7_3 > I see a failure in: > MIExpressionsTest.testChildren() > and > MIExpressionsTest.testConcurrentReadWriteChildren() I fixed MIExpressions.getSubExpressions(), the tests mentioned above don't fail anymore. But MIExpressionsTest.testDeleteChildren() fails with the message: "Got child ((bar) f) instead of ((class bar) f)". It's an error in the test code - (bar)f should be used instead of (class bar)f. Do you want me to add this fix as a part of this patch? I am planning to add a new test to MIExpressionsTest to test the partitions. Should I package it with the main patch or submit it separately? Thanks
(In reply to comment #29) > (In reply to comment #28) > I fixed MIExpressions.getSubExpressions(), the tests mentioned above don't fail > anymore. Thanks! > But MIExpressionsTest.testDeleteChildren() fails with the message: > "Got child ((bar) f) instead of ((class bar) f)". It's an error in the test > code - (bar)f should be used instead of (class bar)f. Do you want me to add > this fix as a part of this patch? That is strange, which GDB version are you using? I added the extra 'class' keyword as part of Bug 320277. Do you see the test fail before your patch? > I am planning to add a new test to MIExpressionsTest to test the partitions. > Should I package it with the main patch or submit it separately? Up to you, but I think it is fine to commit your patch now. This nice feature should be added to the N&N: http://wiki.eclipse.org/CDT/User/NewIn90
(In reply to comment #30) > > But MIExpressionsTest.testDeleteChildren() fails with the message: > > "Got child ((bar) f) instead of ((class bar) f)". It's an error in the test > > code - (bar)f should be used instead of (class bar)f. Do you want me to add > > this fix as a part of this patch? > > That is strange, which GDB version are you using? > I added the extra 'class' keyword as part of Bug 320277. Do you see the test > fail before your patch? > Yes, it fails without my patch. > > I am planning to add a new test to MIExpressionsTest to test the partitions. > > Should I package it with the main patch or submit it separately? > > Up to you, but I think it is fine to commit your patch now. > I've already started writing a test case. I'll see if it takes more time then I can allocate I'll commit the patch without it. > This nice feature should be added to the N&N: > http://wiki.eclipse.org/CDT/User/NewIn90 Will do.
(In reply to comment #30) > That is strange, which GDB version are you using? Sorry, forgot to mention that I am using GDB 7.3.
(In reply to comment #31) > Yes, it fails without my patch. > > Sorry, forgot to mention that I am using GDB 7.3. Ok, I get it now. GDB 7.3.1 has a fix to add the 'class' keyword. That is why the test passes for me. If I run GDB 7.3.0, the test does fail. This is the output from 7.3.1 468,783 [MI] 22-var-info-path-expression var1.bar 468,784 [MI] 22^done,path_expr="((class bar) f)" I was taking the approach that for each GDB version, we should focus on its latest available maintenance build. So, if we talk about GDB 7.3, it means 7.3.1. And if 7.3.2 ever comes out, we move to 7.3.2. What do you think of that approach? I didn't think it was worth having JUnit tests for 7.3 and different ones for 7.3.1... > > This nice feature should be added to the N&N: > > http://wiki.eclipse.org/CDT/User/NewIn90 > > Will do. Thanks!
Created attachment 210087 [details] Updated patch + JUnit tests. Fixed the issues with the existing JUnit tests and added tests for the new feature. Marc, please try it and let me know if it is OK to commit.
(In reply to comment #34) > Created attachment 210087 [details] > Updated patch + JUnit tests. > > Fixed the issues with the existing JUnit tests and added tests for the new > feature. > Marc, please try it and let me know if it is OK to commit. Thanks for the including the JUnit tests! There is a missing 'return' at MIExpressions.java:1191, in the case of if (realNumChildren == 0) I noticed it because in the JUnit logs, I saw: java.lang.IllegalStateException: RequestMonitor: [Lorg.eclipse.cdt.dsf.debug.service.IExpressions$IExpressionDMContext;@99146a, done() method called more than once at org.eclipse.cdt.dsf.concurrent.RequestMonitor.done(RequestMonitor.java:285) at org.eclipse.cdt.dsf.mi.service.MIExpressions$12.handleSuccess(MIExpressions.java:1644) Also, for consistency, in MIExpressions.getSubExpressionCount(IExpressionDMC, int, int, DRM), can you change the line: if (realNum < getArrayPartitionLength()) { to if (realNum <= getArrayPartitionLength()) { It does not cause a bug, but it caused me to have to test the boundary case to make sure. Besides that, looks good to commit. I'm glad this will finally be part of DSF-GDB. Thanks!
Created attachment 210133 [details] Final patch.
Committed to the HEAD.
Updated N&N.
Created attachment 210145 [details] Tweak for IndexedPartitionDMC.toString() Hi Mikhail, If it is ok with you, I'll add a couple more JUnit tests to test double arrays (in a separate patch). As I'm writing them, I had to do some debugging and I noticed the output of an IndexedPartitionDMC: (gdb[0].proc[9527].threadGroup[i1],gdb[0].proc[9527].OSthread[1]) .thread[1].frame[0][0-100] notice two things: 1- the partition should be [0-99] instead of [0-100]. This is simply a missing -1 in IndexedPartitionDMC.toString() 2- the expression is not being shown. This is because IndexedPartitionDMC.toString() uses baseToString() which is for the parent expression. We could instead call super.toString() giving: (gdb[0].proc[10525].threadGroup[i1],gdb[0].proc[10525].OSthread[1]) .thread[1].frame[0] .expr[*((array_int)+0)@10000, *((array_int)+0)@10000, isDynamic=false][0-9999] or we could format it ourselves using baseToString() and getParentExpression() giving: (gdb[0].proc[10525].threadGroup[i1],gdb[0].proc[10525].OSthread[1]) .thread[1].frame[0] .expr[array_int][0-10000] I prefer the second one, myself. The attached patch implements those two points (and removes the calls to Integer.valueOf() which are not needed thanks to auto-boxing of java 5). Is it ok with you for me to commit it?
I would also like to modify the existing testArrays test. It currently takes 37 seconds for me to run because it checks all the children of array_foo[1200]. This creates 1200 variable objects and over 14000 MI commands. I realize that it is ok for JUnit tests to take a long time, but since I sometimes run them manually, it feels long (in fact, I thought it was an infinite loop). If it is ok with you, I'll modify it to check for a couple of children, instead of the full 1200 of them.
(In reply to comment #39) > Created attachment 210145 [details] > Tweak for IndexedPartitionDMC.toString() > > Hi Mikhail, > > If it is ok with you, I'll add a couple more JUnit tests to test double arrays > (in a separate patch). > > As I'm writing them, I had to do some debugging and I noticed the output of an > IndexedPartitionDMC: > > (gdb[0].proc[9527].threadGroup[i1],gdb[0].proc[9527].OSthread[1]) > .thread[1].frame[0][0-100] > > notice two things: > > 1- the partition should be [0-99] instead of [0-100]. > This is simply a missing -1 in IndexedPartitionDMC.toString() > > 2- the expression is not being shown. > This is because IndexedPartitionDMC.toString() uses baseToString() which is > for the parent expression. We could instead call super.toString() giving: > > (gdb[0].proc[10525].threadGroup[i1],gdb[0].proc[10525].OSthread[1]) > .thread[1].frame[0] > .expr[*((array_int)+0)@10000, *((array_int)+0)@10000, > isDynamic=false][0-9999] > > or we could format it ourselves using baseToString() and getParentExpression() > giving: > > (gdb[0].proc[10525].threadGroup[i1],gdb[0].proc[10525].OSthread[1]) > .thread[1].frame[0] > .expr[array_int][0-10000] > > I prefer the second one, myself. > > The attached patch implements those two points (and removes the calls to > Integer.valueOf() which are not needed thanks to auto-boxing of java 5). > > Is it ok with you for me to commit it? Sure, thanks for doing it. Can you please attach the JUnit patch as well when it's ready. I need it to backport to our code.
Created attachment 210150 [details] Two extra JUnit tests for double arrays Here is the patch for two unit tests for double arrays. It also limits the number of children we check for in testArrays() as I mentioned in comment 40 I will commit both to master now.
(In reply to comment #38) > Updated N&N. Thanks for that too :)
*** Bug 207582 has been marked as a duplicate of this bug. ***
(In reply to comment #42) > Created attachment 210150 [details] > Two extra JUnit tests for double arrays I realized there is another change part of this patch that I thought I should explain. I put back the original code for MIExpressionsTest.getChildren(IExpressionDMC, String[]) instead of having it call getChildren(parentDmc, -1, -1, expectedValues ) The reason is that for JUnit test, we want to exercise the different parts of code of our MIExpressions service. So I felt it was important to call both IExpressions.getSubExpressions(IExpressionDMC, DRM); and IExpressions.getSubExpressions(IExpressionDMC, int, int, DRM) even if it meant duplicate code in the test.
(In reply to comment #45) > (In reply to comment #42) > > Created attachment 210150 [details] > > Two extra JUnit tests for double arrays > > I realized there is another change part of this patch that I thought I should > explain. > > I put back the original code for > MIExpressionsTest.getChildren(IExpressionDMC, String[]) > instead of having it call > getChildren(parentDmc, -1, -1, expectedValues ) > > The reason is that for JUnit test, we want to exercise the different parts of > code of our MIExpressions service. So I felt it was important to call both > > IExpressions.getSubExpressions(IExpressionDMC, DRM); > and IExpressions.getSubExpressions(IExpressionDMC, int, int, DRM) > even if it meant duplicate code in the test. Agree, that is the right thing to do. Thanks for your patience when reviewing and testing my patches.
(In reply to comment #46) > Agree, that is the right thing to do. > > Thanks for your patience when reviewing and testing my patches. No problem at all.
Is there a way to disable this feature or can I increase the limit from 100 to 1000? It is utterly broken in the Juno release. It will only display the first element!! In addition, when I set the array length to exactly 100 it will show something like *((piSymbole)+0)@100 instead of piSymbole
(In reply to comment #48) > Is there a way to disable this feature or can I increase the limit from 100 to > 1000? The 100 number matches what is done by the general debug platform and the JDT, so it was not made configurable. > It is utterly broken in the Juno release. It will only display the first > element!! Can you let us know how to reproduce? > In addition, when I set the array length to exactly 100 it will show something > like > *((piSymbole)+0)@100 > instead of piSymbole This is because of Bug 381868 which was reported very late. Specifically, Bug 381868 comment 4 states: "when displaying longarraypointer as an array of 1000, the elements below the partitions are shown as: *((longarraypointer)+0)@100[0] " This has been fixed on master. Since you've noticed it quite quickly with Juno, I think we should try to fix it on the maintenance branch as well. I've opened Bug 384599 to track this.
(In reply to comment #49) Hi Marc, thanks for your quick response. > The 100 number matches what is done by the general debug platform and the JDT, so it was not made configurable. That's a pity. A was really happy with the fact that DSF did not partition the array display. See Bug 245650 (which is open since 2008!). > Can you let us know how to reproduce? int main( int argc, char **argv ) { int * array = new int[512]; for (int i = 0; i < 512; ++i) { array[i] = i; } delete [] array; //breakpoint here } 1) stop at breakpoint 2) in Variable View select array and choose Display as array; use length 512 3) open node array 4) open node [0..99] => only first element is shown 5) open node [100..199] => only first element is shown Here is the output from gdb traces (I cut everything before hitting the breakpoint): 178,061 34-data-evaluate-expression --thread 1 --frame 0 array 178,061 35-var-set-format var3 octal 178,061 34^done,value="0x602010" 178,061 (gdb) 178,061 35^done,format="octal",value="030020020" 178,062 (gdb) 178,062 36-var-set-format var3 natural 178,062 36^done,format="natural",value="0x602010" 178,062 (gdb) 178,062 37-var-set-format var3 binary 178,062 37^done,format="binary",value="11000000010000000010000" 178,063 (gdb) 178,063 38-var-set-format var3 natural 178,063 38^done,format="natural",value="0x602010" 178,063 (gdb) 178,063 39-var-set-format var3 decimal 178,063 39^done,format="decimal",value="6299664" 178,063 (gdb) 178,064 40-var-set-format var3 natural 178,064 40^done,format="natural",value="0x602010" 178,064 (gdb) 178,064 41-var-set-format var3 hexadecimal 178,064 41^done,format="hexadecimal",value="0x602010" 178,064 (gdb) 178,064 42-var-set-format var3 natural 178,064 42^done,format="natural",value="0x602010" 178,064 (gdb) 187,554 43-var-create --thread 1 --frame 0 - * *((array)+0)@512 187,555 43^done,name="var4",numchild="512",value="[512]",type="int [512]",thread-id="1",has_more="0"\ 187,555 (gdb) 187,555 44-var-create --thread 1 --frame 0 - * &(*((array)+0)@512) 187,556 44^done,name="var5",numchild="1",value="0x602010",type="int (*)[512]",thread-id="1",has_more\ ="0" 187,556 (gdb) 191,114 45-var-create --thread 1 --frame 0 - * *((array)+0)@100 191,115 45^done,name="var6",numchild="100",value="[100]",type="int [100]",thread-id="1",has_more="0"\ 191,116 (gdb) 191,116 46-var-create --thread 1 --frame 0 - * &(*((array)+0)@100) 191,117 46^done,name="var7",numchild="1",value="0x602010",type="int (*)[100]",thread-id="1",has_more\ ="0" 191,117 (gdb) 191,218 47-var-create --thread 1 --frame 0 - * *((array)+100)@100 191,218 48-var-create --thread 1 --frame 0 - * *((array)+200)@100 191,218 47^done,name="var8",numchild="100",value="[100]",type="int [100]",thread-id="1",has_more="0"\ 191,219 (gdb) 191,219 48^done,name="var9",numchild="100",value="[100]",type="int [100]",thread-id="1",has_more="0"\ 191,219 (gdb) 191,219 49-var-create --thread 1 --frame 0 - * &(*((array)+100)@100) 191,220 50-var-create --thread 1 --frame 0 - * &(*((array)+200)@100) 191,220 49^done,name="var10",numchild="1",value="0x6021a0",type="int (*)[100]",thread-id="1",has_mor\ e="0" 191,220 (gdb) 191,220 50^done,name="var11",numchild="1",value="0x602330",type="int (*)[100]",thread-id="1",has_mor\ e="0" 191,220 (gdb) 194,351 51-var-list-children var3 194,351 51^done,numchild="1",children=[child={name="var3.*array",exp="*array",numchild="0",type="int\ ",thread-id="1"}],has_more="0" 194,351 (gdb) 194,352 52-var-info-path-expression var3.*array 194,352 52^done,path_expr="*(array)" 194,352 (gdb) 194,353 53-var-evaluate-expression var3.*array 194,353 53^done,value="0" 194,354 (gdb)
(In reply to comment #50) > (In reply to comment #49) > Hi Marc, > thanks for your quick response. > > > The 100 number matches what is done by the general debug platform and the JDT, > so it was not made configurable. > That's a pity. A was really happy with the fact that DSF did not partition the > array display. See Bug 245650 (which is open since 2008!). I wasn't aware of this. I guess we could add a preference. Mikhail, what do you think about this case? > int main( int argc, char **argv ) > { > int * array = new int[512]; > for (int i = 0; i < 512; ++i) > { > array[i] = i; > } > > delete [] array; //breakpoint here > } > > 1) stop at breakpoint > 2) in Variable View select array and choose Display as array; use length 512 > 3) open node array > 4) open node [0..99] > => only first element is shown > 5) open node [100..199] > => only first element is shown I couldn't reproduce it with Juno... That being said, the code has changed for master. Are you able to try it with master? If things work ok with master, we can just migrate the fix to 8.1.1, but if you still see a problem, we will need to figure out how to reproduce it.
(In reply to comment #51) > I couldn't reproduce it with Juno... > That being said, the code has changed for master. Are you able to try it with > master? If things work ok with master, we can just migrate the fix to 8.1.1, > but if you still see a problem, we will need to figure out how to reproduce it. Well, I am using Juno RC3. Unfortunately, I cannot update to the final release because p2 chokes (cannot resolve some dependencies!!!). I will try to manually update (i.e. delete the existing installation and overwrite it with the final candidate) and then report my findings here
(In reply to comment #50) > > The 100 number matches what is done by the general debug platform and the JDT, > so it was not made configurable. > That's a pity. A was really happy with the fact that DSF did not partition the > array display. See Bug 245650 (which is open since 2008!). Without partitioning Eclipse hangs on large arrays. It is highly unlikely that the original issue will be fixed. That's the main reason why the partitioning was introduced. (In reply to comment #51) > (In reply to comment #50) > > (In reply to comment #49) > > Hi Marc, > > thanks for your quick response. > > > > > The 100 number matches what is done by the general debug platform and the JDT, > > so it was not made configurable. > > That's a pity. A was really happy with the fact that DSF did not partition the > > array display. See Bug 245650 (which is open since 2008!). > > I wasn't aware of this. I guess we could add a preference. > Mikhail, what do you think about this case? > I wouldn't do it. Adding a preference would allow users to increase the size of partitions. The main reason the partitioning was introduced is to keep the size small enough to avoid the hanging. > > int main( int argc, char **argv ) > > { > > int * array = new int[512]; > > for (int i = 0; i < 512; ++i) > > { > > array[i] = i; > > } > > > > delete [] array; //breakpoint here > > } > > > > 1) stop at breakpoint > > 2) in Variable View select array and choose Display as array; use length 512 > > 3) open node array > > 4) open node [0..99] > > => only first element is shown > > 5) open node [100..199] > > => only first element is shown > > I couldn't reproduce it with Juno... > That being said, the code has changed for master. Are you able to try it with > master? If things work ok with master, we can just migrate the fix to 8.1.1, > but if you still see a problem, we will need to figure out how to reproduce it. I believe we fixed this bug for Juno.
(In reply to comment #53) > > I wasn't aware of this. I guess we could add a preference. > > Mikhail, what do you think about this case? > > > > I wouldn't do it. Adding a preference would allow users to increase the size of > partitions. The main reason the partitioning was introduced is to keep the size > small enough to avoid the hanging. And here I was thinking this was mostly a user-friendly feature :) Since the real reason is to avoid hanging, I agree we should leave the partition to 100. Bug 245650 will have to be addressed on its own.
(In reply to comment #54) > And here I was thinking this was mostly a user-friendly feature :) > Since the real reason is to avoid hanging, I agree we should leave the > partition to 100. Bug 245650 will have to be addressed on its own. I would also prefer that Bug 245650 would address this problem. However, I am waiting now for more than 4 years.
(In reply to comment #52) > (In reply to comment #51) > > I couldn't reproduce it with Juno... > > That being said, the code has changed for master. Are you able to try it with > > master? If things work ok with master, we can just migrate the fix to 8.1.1, > > but if you still see a problem, we will need to figure out how to reproduce it. > Well, I am using Juno RC3. Unfortunately, I cannot update to the final release > because p2 chokes (cannot resolve some dependencies!!!). I will try to manually > update (i.e. delete the existing installation and overwrite it with the final > candidate) and then report my findings here OK, I reinstalled the Juno final candidate. And now everything works. Sorry for the false alarm.
(In reply to comment #56) > OK, I reinstalled the Juno final candidate. And now everything works. Sorry for > the false alarm. What about: > In addition, when I set the array length to exactly 100 it will show something > like > *((piSymbole)+0)@100 > instead of piSymbole Are you seeing this or not? Do we cancel Bug 384599?
(In reply to comment #57) > What about: > > In addition, when I set the array length to exactly 100 it will show something > > like > > *((piSymbole)+0)@100 > > instead of piSymbole > > Are you seeing this or not? Do we cancel Bug 384599? I am still seeing this.
