Bugzilla – Full Text Bug Listing
Created attachment 176645 [details] modify output of var-info-path-expression I have changed the bug title to reflect better the problem. It is not possible to show the value of a base class member because the expression returned by gdb via var-info-path-expression is wrong. var-info-path-expression returns (*(testbase*) this) but for data-evaluate-expression we need (*(struct testbase*) this) This was fixed for CDI in bug #304010 My patch will also cover bug #308678 which describes a similar problem with base classes in a namespace. Marc, could you have a look at my patch? I would like to see this included in the fall release. Hopefully, it is okay that I added your name to this bug. (In reply to comment #2) > Marc, > > could you have a look at my patch? I would like to see this included in the > fall release. > Hopefully, it is okay that I added your name to this bug. As you know by now, a little reminder like this is a good way to get things moving :-) I had a look at the patch and I was bothered by the technique of using baseClassName.equals(child.getType()) to know if we are dealing with a base class. I wondered if the name of a variable could be the same as its type. So I added the line testbase testbase; as a member of the test class. Things compiled. Then this member triggered the code of the patch by mistake and I got Failed to execute MI command: -data-evaluate-expression "(struct testbase)var2.public" We'd need some other way to know we are dealing with such a case Also, two minor comments: 1- I don't think you need to call setData() with the new childFullExpression. 2- The new code should probably be in an else case of if (childFullExpression.length() == 0) { I was also thinking that it may be safer to use the result of var-info-path-expression and insert the 'struct' in there instead of creating a 'hard-coded' new full expression ourselves; I haven't quite figured out when this case happens, so maybe that would be unnecessary though. (In reply to comment #3) > As you know by now, a little reminder like this is a good way to get things > moving :-) I am learning :) > I had a look at the patch and I was bothered by the technique of using > baseClassName.equals(child.getType()) to know if we are dealing with a base > class. Yeah, that looks weird. To be honest, I got this idea from the CDI code in org.eclipse.cdt.debug.mi.core.cdi.model.Variable > I wondered if the name of a variable could be the same as its type. > > So I added the line > testbase testbase; > as a member of the test class. Things compiled. > Then this member triggered the code of the patch by mistake and I got > > Failed to execute MI command: > -data-evaluate-expression "(struct testbase)var2.public" > > We'd need some other way to know we are dealing with such a case Patch to follow. > > Also, two minor comments: > 1- I don't think you need to call setData() with the new childFullExpression. This is necessary when you have a second level derived class. e.g. class testbase { float fBase; }; class test1 : public testbase { float f1; }; class test2 : public test1 { public: test2() { printf("%d", aiVal[0]); } }; > 2- The new code should probably be in an else case of > if (childFullExpression.length() == 0) { > > I was also thinking that it may be safer to use the result of > var-info-path-expression and insert the 'struct' in there instead of creating a > 'hard-coded' new full expression ourselves; Created attachment 178451 [details] modify output of var-info-path-expression beware of variables that are named after their type I modified the patch so variables that name is identical to their type (see comment #3) are excluded from the modification. Created attachment 178470 [details]
Test for GDB's behavior
GDB's bahavior is a little more complicated that it seems.
The attached file is meant to be run directly in GDB command-line and gives a test that allows to cause the problem or not.
It seems that have a constructor or not affects the syntax error. So, it seems that it is not -var-info-path-expression that has the problem but GDB's variable printing. Understanding GDB's behavior should help us figure out how to work around it the best way.
I'm going to follow-up with the gdb mailing list after doing a little more debugging.
(In reply to comment #6) > Created an attachment (id=178470) [details] > Test for GDB's behavior > > GDB's bahavior is a little more complicated that it seems. > The attached file is meant to be run directly in GDB command-line and gives a > test that allows to cause the problem or not. > > It seems that have a constructor or not affects the syntax error. So, it seems > that it is not -var-info-path-expression that has the problem but GDB's > variable printing. Understanding GDB's behavior should help us figure out how > to work around it the best way. > > I'm going to follow-up with the gdb mailing list after doing a little more > debugging. -var-info-path-expression does work as expected from http://sourceware.org/gdb/current/onlinedocs/gdb/GDB_002fMI-Variable-Objects.html "For example, suppose C is a C++ class, derived from class Base, and that the Base class has a member called m_size. Assume a variable c is has the type of C and a variable object C was created for variable c. Then, we'll get this output: (gdb) -var-info-path-expression C.Base.public.m_size ^done,path_expr=((Base)c).m_size) " I assume the existence of the constructor in your example influences the vtable. But you may be right that gdb has a printing problem. But should we wait for gdb to fix this or have a workaround in Eclipse? (In reply to comment #7) > I assume the existence of the constructor in your example influences the > vtable. But you may be right that gdb has a printing problem. But should we > wait for gdb to fix this or have a workaround in Eclipse? We'll need a workaround ourselves. But if we understand what GDB is doing, we can make sure our workaround is complete. Also, I'd like to get this fixed in GDB. And finally, we'll need to make sure our workaround does not break things, if/when GDB is fixed. (In reply to comment #8) > (In reply to comment #7) > > > I assume the existence of the constructor in your example influences the > > vtable. But you may be right that gdb has a printing problem. But should we > > wait for gdb to fix this or have a workaround in Eclipse? > > We'll need a workaround ourselves. But if we understand what GDB is doing, we > can make sure our workaround is complete. Also, I'd like to get this fixed in > GDB. And finally, we'll need to make sure our workaround does not break > things, if/when GDB is fixed. Well, that is what I basically meant in my last comment. Created attachment 178509 [details]
JUnit test for this bug
This JUnit test reproduces the bug so we can make sure it is properly fixed.
I committed the test to HEAD.
*** cdt cvs genie on behalf of mkhouzam *** Bug 320277: JUnit test to make sure we properly view variables of base class in derived class [*] ExpressionTestApp.cc 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/data/launch/src/ExpressionTestApp.cc?root=Tools_Project&r1=1.2&r2=1.3 [*] MIExpressionsTest.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/MIExpressionsTest.java?root=Tools_Project&r1=1.11&r2=1.12 There is already a GDB bug that seems to be the same as what we see here: http://sourceware.org/bugzilla/show_bug.cgi?id=11912 The explanation is that the GDB parser treats the base class name in the expression as its constructor's name instead. In the example below, in the expression: ((*(testbase*) this)).aiVal)[0] testbase is mistaken to be the constructor by GDB. The suggested workaround was to add the keyword 'class' (same as 'struct'), as we are proposing to do in DSF-GDB. (In reply to comment #5) > Created an attachment (id=178451) [details] > modify output of var-info-path-expression beware of variables that are named > after their type > > I modified the patch so variables that name is identical to their type (see > comment #3) are excluded from the modification. I still feel un-easy about this solution. The trick to exclude variables that have the same name as their type does not feel very safe (fullExp != gdbName) relies on our handling of private/public/protected and on their existence. What about C programs that don't use those? I wasn't able to find a counter example, but it seems risky. I guess we could check for C++ like CDI does. Also, nested children logic (if (parent != null)) does not quite work. I added 'int *a;' in testbase and I'm getting errors. This is because the parent of an expression can be the public/private/protected element, so we would need to go another parent up. Also for the nested children logic, I'm not convinced that if we have a child, we can always access it using parent.child. Couldn't there be other cases like ->? Created attachment 178566 [details]
More JUnit tests
Here are more JUnit tests for this bug, which go into more details. They will help get our solution working.
Committed to HEAD.
*** cdt cvs genie on behalf of mkhouzam *** Bug 320277: More JUnit test to make sure we properly view variables of base class in derived class [*] ExpressionTestApp.cc 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/data/launch/src/ExpressionTestApp.cc?root=Tools_Project&r1=1.3&r2=1.4 [*] MIExpressionsTest.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/MIExpressionsTest.java?root=Tools_Project&r1=1.12&r2=1.13 (In reply to comment #13) > I still feel un-easy about this solution. > The trick to exclude variables that have the same name as their type does not > feel very safe (fullExp != gdbName) relies on our handling of > private/public/protected and on their existence. It is a little bit voodoo. But as long as you don't provide a counter case I would say it is safe to use :) > What about C programs that don't use those? I wasn't able to find a counter > example, but it seems risky. I guess we could check for C++ like CDI does. I as thinking about this, too. But I didn't find any example that could cause problems and I don't know how to check for C++. > Also, nested children logic (if (parent != null)) does not quite work. > I added 'int *a;' in testbase and I'm getting errors. This is because the > parent of an expression can be the public/private/protected element, so we > would need to go another parent up. Hmm, I don't get any errors when adding "int *a" to testbase. However, I am testing the example from comment #0 manually and not your unit test. Both unit tests fail with a time out (the rest of the MIExpressionTests do work). java.lang.Exception: Timed out waiting for ServiceEvent: org.eclipse.cdt.dsf.mi.service.command.events.MIStoppedEvent at org.eclipse.cdt.tests.dsf.gdb.framework.ServiceEventWaitor.waitForEvent(ServiceEventWaitor.java:87) at org.eclipse.cdt.tests.dsf.gdb.framework.SyncUtil.resumeUntilStopped(SyncUtil.java:317) at org.eclipse.cdt.tests.dsf.gdb.framework.SyncUtil.resumeUntilStopped(SyncUtil.java:326) at org.eclipse.cdt.tests.dsf.gdb.framework.SyncUtil.runToLocation(SyncUtil.java:381) at org.eclipse.cdt.tests.dsf.gdb.framework.SyncUtil.runToLocation(SyncUtil.java:373) at org.eclipse.cdt.tests.dsf.gdb.tests.MIExpressionsTest.testBaseChildrenBug(MIExpressionsTest.java:419) > Also for the nested children logic, I'm not convinced that if we have a child, > we can always access it using parent.child. Couldn't there be other cases like > ->? I have to think about this. We could check parent.isPointer() and then use -> ? (In reply to comment #16) > (In reply to comment #13) > > > I still feel un-easy about this solution. > > The trick to exclude variables that have the same name as their type does not > > feel very safe (fullExp != gdbName) relies on our handling of > > private/public/protected and on their existence. > It is a little bit voodoo. But as long as you don't provide a counter case I > would say it is safe to use :) I might have to accept that one as I don't see another option at this time. > > What about C programs that don't use those? I wasn't able to find a counter > > example, but it seems risky. I guess we could check for C++ like CDI does. > I as thinking about this, too. But I didn't find any example that could cause > problems and I don't know how to check for C++. To check for C++ we need to start using -var-info-expression. But I would be ok with delaying this until we find a example that shows we need it. > > Also, nested children logic (if (parent != null)) does not quite work. > > I added 'int *a;' in testbase and I'm getting errors. This is because the > > parent of an expression can be the public/private/protected element, so we > > would need to go another parent up. > Hmm, I don't get any errors when adding "int *a" to testbase. However, I am > testing the example from comment #0 manually and not your unit test. I was able to see this manually by opening down to *a and the selecting it. > Both unit > tests fail with a time out (the rest of the MIExpressionTests do work). Did you update the entire test.dsf.gdb plugin? You also need the changes from ExpressionTestApp.cc. > > Also for the nested children logic, I'm not convinced that if we have a child, > > we can always access it using parent.child. Couldn't there be other cases like > > ->? > I have to think about this. We could check parent.isPointer() and then use -> ? I'd have to think about it too... (In reply to comment #4) > > Also, two minor comments: > > 1- I don't think you need to call setData() with the new childFullExpression. > This is necessary when you have a second level derived class. > e.g. > class testbase { > float fBase; > }; > > class test1 : public testbase { > float f1; > }; > > class test2 : public test1 { > public: > test2() > { > printf("%d", aiVal[0]); > } > }; > I agree that you need to set the full expression of the child to be the new one we created, but the setData() call is something else, which will make getData() change. But since we don't use getData() anymore, there is no need, as far as I can see. (In reply to comment #17) > > > Also, nested children logic (if (parent != null)) does not quite work. > > > I added 'int *a;' in testbase and I'm getting errors. This is because the > > > parent of an expression can be the public/private/protected element, so we > > > would need to go another parent up. > > Hmm, I don't get any errors when adding "int *a" to testbase. However, I am > > testing the example from comment #0 manually and not your unit test. > > I was able to see this manually by opening down to *a and the selecting it. OK, I was able now to partly reproduce this. I used the code from the unit test: class Base { public: int nested; int* pNested; }; class BaseTest: public Base { public: BaseTest() {} void test() { nested = 8; return; } Base Base; // make sure we don't get confused by the same name }; int main(void) { BaseTest b; b.test(); return 0; } I had to open in Variables View b.Base (the second one!) and select pNested and then dig further. But in my case it only fails because pNested=0x0 ! (Cannot access memory 0x0). If I add the line b.Base.pNested = (int*)malloc(1); I can inspect the variable then w/o any problems. Or did you see another error? > > > Both unit > > tests fail with a time out (the rest of the MIExpressionTests do work). > > Did you update the entire test.dsf.gdb plugin? You also need the changes from > ExpressionTestApp.cc. I have all updates. But both tests fail here in this line: MIStoppedEvent stoppedEvent = SyncUtil.runToLocation("BaseTest::test"); Is it possible to get the gdb traces in the Junit test? I am using gdb 7.1 (x86_64). Created attachment 192600 [details]
modify output of var-info-path-expression beware of variables that are named after their type
I updated the patch to the current HEAD version and fixed a bug. The Junit test does now succeed.
> I updated the patch to the current HEAD version and fixed a bug. The Junit test > does now succeed. Marc, what do you think about it? > I had a look at the patch and I was bothered by the technique of using > baseClassName.equals(child.getType()) to know if we are dealing with a base > class. I have noticed that QtCreator uses a similar approach "isBaseClass = field.name == stripClassTag(str(field.type))" see http://qt.gitorious.org/qt-creator/qt-creator/blobs/master/share/qtcreator/gdbmacros/dumper.py (In reply to comment #21) > > I updated the patch to the current HEAD version and fixed a bug. The Junit test > > does now succeed. > Marc, what do you think about it? Sorry for the delay. I've started looking at it but go pulled away again. But I'm hopeful for this week. Created attachment 194406 [details] Minor updates to Axel's last patch I cleaned-up the patch a little by renaming variables and adding more comments. I also fixed a JUnit that was failing with the changes. However, I still don't feel this patch is safe. The problem is in the fact that once we start creating our own expression to add 'struct', we have to keep looking for it and having a special case for all children. I'm talking about the case: } else if ( parent != null ) { This else clause will be hit for every single child expression which does not have the same name as type, which is pretty much every child. In that majority of cases, we start looking for the inserted 'struct' in different potential places, and then building our own expressions instead of using the output of var-info-path-expression. I think the risk is too big for the benefit. CDI was able to fix it easily because it builds the expression itself, and stores it. In our case, because we rely on GDB's var-info-path-expression, we rely on GDB to give us the proper expression for the parent and for any of the children. To fix this more safely, I think we would need a new member in our MIVariableObject class to remember if we can trust var-info-path-expression for this hierarchy of varObjects. Once we detect that a varObj is having the problem and that we insert the 'struct', we should mark it in a way, so that when we deal with any of its children, or children of children, etc, we no longer use var-info-path-expression. If we do that, once GDB is fixed, we should no longer ignore var-info-path-expression, which means we'll have to version MIVariableManager. I'm not convinced this problem is worth all the effort. I think we should fix GDB instead and live with the bug for older versions. I posted a fix at http://sourceware.org/ml/gdb-patches/2010-09/msg00204.html, but I haven't pushed further. With a little effort, I could probably get it in for 7.2.1 and for 7.3. What do you think? (In reply to comment #23) > To fix this more safely, I think we would need a new member in our > MIVariableObject class to remember if we can trust var-info-path-expression for > this hierarchy of varObjects. Once we detect that a varObj is having the > problem and that we insert the 'struct', we should mark it in a way, so that > when we deal with any of its children, or children of children, etc, we no > longer use var-info-path-expression. > > If we do that, once GDB is fixed, we should no longer ignore > var-info-path-expression, which means we'll have to version MIVariableManager. > I'm not convinced this problem is worth all the effort. > > I think we should fix GDB instead and live with the bug for older versions. I > posted a fix at http://sourceware.org/ml/gdb-patches/2010-09/msg00204.html, but > I haven't pushed further. With a little effort, I could probably get it in for > 7.2.1 and for 7.3. > > What do you think? I see the problem with the special case for the children. Of course, a gdb patch would be preferable. However, not everyone can update its gdb (I am using gdb 7.2 at the moment, so updating to 7.2.1 should be no problem for me. But others might be using some older version , e.g. RHEL6 ships gdb 7.0). Do you think the changes with respect to MIVariableObject might be doable for the Indigo release in June? (In reply to comment #24) > I see the problem with the special case for the children. Of course, a gdb > patch would be preferable. However, not everyone can update its gdb (I am using > gdb 7.2 at the moment, so updating to 7.2.1 should be no problem for me. But > others might be using some older version , e.g. RHEL6 ships gdb 7.0). > Do you think the changes with respect to MIVariableObject might be doable for > the Indigo release in June? If we limit the change to children of the buggy-case, I'd be ok with submitting it in the next two weeks. Is that enough time for you? (In reply to comment #25) > If we limit the change to children of the buggy-case, I'd be ok with submitting > it in the next two weeks. Is that enough time for you? OK. Hopefully, I will have a new patch within this week. Created attachment 194636 [details] Update to Marc's patch. Remember if we used the workaround to create are own var-info-expression >To fix this more safely, I think we would need a new member in our >MIVariableObject class to remember if we can trust var-info-path-expression for >this hierarchy of varObjects. Once we detect that a varObj is having the problem >and that we insert the 'struct', we should mark it in a way, so that< when we >deal with any of its children, or children of children, etc, we no< longer use >var-info-path-expression. I added a new boolean to MIVariableObject to remember if we used the 'struct' workaround. That way we will only check the children of this varObject. >If we do that, once GDB is fixed, we should no longer ignore >var-info-path-expression, which means we'll have to version MIVariableManager. How would you do this? (In reply to comment #27) > >If we do that, once GDB is fixed, we should no longer ignore > >var-info-path-expression, which means we'll have to version MIVariableManager. > How would you do this? I'm not sure what is easier, but we have a couple of options: 1- wait for GDB to be fixed to know which version has the fix and version MIVariableManager (seems complicated, maintenance wise) 2-before inserting the 'struct' keyword, we can verify if var-info-path-expression already has the 'struct' or 'class' keyword. This is a hack to me. GDB could fix the problem in another way and we would not know. 3- the very first time (and only then) we are about to insert the 'struct' keyword, we first send a command to GDB using -data-evaluate-expression with the result of -var-info-path-expression. If the command fails, we know we have to apply our fix; else we know we can trust GDB. I think #3 is easier to code but is more obscure. It would need some clear comments. It has the advantage that once GDB fixes the problem, we would already be ready for it. I suggest trying that approach and seeing how it looks. (In reply to comment #28) > I'm not sure what is easier, but we have a couple of options: > 1- wait for GDB to be fixed to know which version has the fix and version > MIVariableManager (seems complicated, maintenance wise) > 2-before inserting the 'struct' keyword, we can verify if > var-info-path-expression already has the 'struct' or 'class' keyword. This is > a hack to me. GDB could fix the problem in another way and we would not know. > 3- the very first time (and only then) we are about to insert the 'struct' > keyword, we first send a command to GDB using -data-evaluate-expression with > the result of -var-info-path-expression. If the command fails, we know we have > to apply our fix; else we know we can trust GDB. > > I think #3 is easier to code but is more obscure. It would need some clear > comments. It has the advantage that once GDB fixes the problem, we would > already be ready for it. I suggest trying that approach and seeing how it > looks. I would also favor option #3. Interestingly, this is the way QtCreator handles this gdb bug. Could you give me some hints how to implement it? > I would also favor option #3. Interestingly, this is the way QtCreator handles
> this gdb bug.
> Could you give me some hints how to implement it?
Try this:
create an expressionDMC using the result from -var-info-path-expression
then
fCommandControl.queueCommand(
fCommandFactory.createMIDataEvaluateExpression(expressionDMC),
new DataRequestMonitor<MIDataEvaluateExpressionInfo>(getExecutor(), rm) {
@Override
protected void handleError() {
// GDB bug
}
});
(In reply to comment #30) > > I would also favor option #3. Interestingly, this is the way QtCreator handles > > this gdb bug. > > Could you give me some hints how to implement it? > > Try this: > create an expressionDMC using the result from -var-info-path-expression Dumb question: How do I create an expressionDMC? (In reply to comment #31) > (In reply to comment #30) > > > I would also favor option #3. Interestingly, this is the way QtCreator handles > > > this gdb bug. > > > Could you give me some hints how to implement it? > > > > Try this: > > create an expressionDMC using the result from -var-info-path-expression > Dumb question: How do I create an expressionDMC? I knew I should have mentioned that, but I was rushed, sorry. IExpressionDMContext exprDmc= expressions.createExpression(parentDmc, exprStr); The parentDmc can be a frame, a thread or a memory dmc, as shown in MIExpressions.createExpression(IDMContext, ExpressionInfo) (In reply to comment #32) > > Dumb question: How do I create an expressionDMC? > > I knew I should have mentioned that, but I was rushed, sorry. > > IExpressionDMContext exprDmc= expressions.createExpression(parentDmc, exprStr); > > The parentDmc can be a frame, a thread or a memory dmc, as shown in > MIExpressions.createExpression(IDMContext, ExpressionInfo) I still don't get it. I tried the following: IExpressionDMContext expressionDMC = null; IFrameDMContext frameDmc = DMContexts.getAncestorOfType(getRootToUpdate().getControlDMContext(), IFrameDMContext.class); if (frameDmc != null) { expressionDMC= new MIExpressionDMC( getSession().getId(), new ExpressionInfo( childFullExpression, childFullExpression), frameDmc); } However, frameDmc is always null. I think this DMC thing is beyond my Eclipse/Java knowledge. (In reply to comment #33) > IExpressionDMContext expressionDMC = null; > IFrameDMContext frameDmc = > DMContexts.getAncestorOfType(getRootToUpdate().getControlDMContext(), > IFrameDMContext.class); The controlDMContext never has a frame as a parent, so it makes sense it is null. We need to choose a thread for which to create the expression. However, that particular variable object is only valid for a particular thread, and I'm not sure we store that anywhere. I'll try to have a look this afternoon to see if we can do anything. (In reply to comment #34) > We need to choose a thread for which to create the expression. However, that > particular variable object is only valid for a particular thread, and I'm not > sure we store that anywhere. I'll try to have a look this afternoon to see if > we can do anything. Anything I can do on this? (In reply to comment #35) > (In reply to comment #34) > > We need to choose a thread for which to create the expression. However, that > > particular variable object is only valid for a particular thread, and I'm not > > sure we store that anywhere. I'll try to have a look this afternoon to see if > > we can do anything. > Anything I can do on this? Sorry, I'll work on it this morning. (In reply to comment #27) > Created attachment 194636 [details] > Update to Marc's patch. Remember if we used the workaround to create are own > var-info-expression > > >To fix this more safely, I think we would need a new member in our >MIVariableObject class to remember if we can trust var-info-path-expression for >this hierarchy of varObjects. Once we detect that a varObj is having the problem >and that we insert the 'struct', we should mark it in a way, so that< when we >deal with any of its children, or children of children, etc, we no< longer use >var-info-path-expression. > I added a new boolean to MIVariableObject to remember if we used the 'struct' > workaround. That way we will only check the children of this varObject. Great. Can you name it with a lower-case letter first? Also, remove the @since tag since it is not public. I'm guessing you set the hasCastToBaseClassWorkaround flag at the top because we need to include the fakeChild case in there too, right? Can you put a comment to say that? My first reaction was to push down that code to the new else clause below, which I think would have been wrong. Can you explain to me this part: if (getParent().getExpression().contains("(" + CAST_PREFIX)){ //$NON-NLS-1$ // children of base class used in derived class childFullExpression = "(" + getParent().getExpression() + ")." + child.getExp(); //$NON-NLS-1$ //$NON-NLS-2$ } else if (parentExpression.contains("(" + CAST_PREFIX)){ //$NON-NLS-1$ // dereferenced pointer of base class in derived class childFullExpression = "*" + parentExpression; //$NON-NLS-1$ } Do we need to check for the presence of CAST_PREFIX now that we have checked for hasCastToBaseClassWorkaround? Can you give and example for the if case and one for the else case? I don't understand what we are doing here. Thanks > >If we do that, once GDB is fixed, we should no longer ignore > >var-info-path-expression, which means we'll have to version MIVariableManager. > How would you do this? I'll commit the above patch, once you make the changes, and we can deal with this part separately. Created attachment 195322 [details]
Fix to JUnit tests
Fix for JUnit tests to match the new output
(In reply to comment #37) > Great. Can you name it with a lower-case letter first? Also, remove the @since > tag since it is not public. No problem. > I'm guessing you set the hasCastToBaseClassWorkaround flag at the top because > we need to include the fakeChild case in there too, right? Can you put a > comment to say that? My first reaction was to push down that code to the new > else clause below, which I think would have been wrong. The check for the must indeed stay on top in case of fake children like var1.public. Otherwise the flag would not be inherited to all children. > Can you explain to me this part: > > if (getParent().getExpression().contains("(" + CAST_PREFIX)){ //$NON-NLS-1$ > // children of base class used in derived class > childFullExpression = "(" + getParent().getExpression() + ")." + > child.getExp(); //$NON-NLS-1$ //$NON-NLS-2$ > } else if (parentExpression.contains("(" + CAST_PREFIX)){ //$NON-NLS-1$ // > dereferenced pointer of base class in derived class > childFullExpression = "*" + parentExpression; //$NON-NLS-1$ > } > > Do we need to check for the presence of CAST_PREFIX now that we have checked > for > hasCastToBaseClassWorkaround? > > Can you give and example for the if case and one for the else case? I don't > understand what we are doing here. It is a bit complicated. You should have a look at the Junit C++ code for better understanding. But I will try to explain it. - We must check for the CAST_PREFIX because the parent can be a fake variable, e.g. var1.public - The parent will be "(Base*)this" when we try to inspect a base class variable in the derived class (if clause). In this case the parent expression will have the CAST_PREFIX and we must construct the correct child expression. - When we inspect a variable of the derived class the parent will be "this" (w/o the cast to the base class!). However, the parent still has the hasCastToBaseClassWorkaround flag. But in this case we should not modify the child expression. - I changed the else clause to make it more understandable } else if (isPointer()){ This test is necessary when the base class has a pointer variable which must be dereferenced ((struct Base*)(this)).pNested Created attachment 195429 [details]
Remember if we used the workaround to create are own var-info-expression
Update for last patch (see my last comment for details).
Looking at
if (child.getExp().equals(child.getType()) && parentExpression != getGdbName()) {
[snip]
hasCastToBaseClassWorkaround = true;
}
it seems we are setting the new flag for the parent instead of the child. I believe this will cause the workaround to be triggered for all grandchildren of this expression, although it may be only one of its children that needs it, so a subset of its grandchildren. I think I need an example to explain.
Say you have:
parent (hasWorkaround==false)
currentVarObj (hasWorkaround==false)
child1 <---- this one needs the workaround
grandchild1
child2
grandchild2
when we notice child1 needs the workaround, the code will bring us to the following state:
parent (hasWorkaround==false)
currentVarObj (hasWorkaround==true) <-- changed instead of child1
child1
grandchild1
child2
grandchild2
That means that when we examine grandchild2 and do
if (getParent() != null && getParent().hasCastToBaseClassWorkaround) {
hasCastToBaseClassWorkaround = true;
}
getparent() will look at currentVarObj and will think that grandchild2 needs the workaround.
But maybe I'm mis-reading things... I haven't confirmed by testing.
It also hit me that the clean way to do this change is to improve and use buildChildExpression() instead of calling var-info-path-expression. Currently, we will call var-info-path-expression, and then ignore its result completely. I'll modify the patch in those lines and post the result to see what you think.
I got pulled to a customer issue. But we can try this for the next build. (In reply to comment #41) > Looking at > if (child.getExp().equals(child.getType()) && parentExpression != > getGdbName()) { > [snip] > hasCastToBaseClassWorkaround = true; > } > > it seems we are setting the new flag for the parent instead of the child. I > believe this will cause the workaround to be triggered for all grandchildren of > this expression, although it may be only one of its children that needs it, so > a subset of its grandchildren. I think I need an example to explain. >.... I thought about your objection and did some tests. parent "this" child "Base" (here we need the CAST_PREFIX to get (struct Base*)(this) grandchild "var1.Base.public" => check still needed child2 "childOfBaseTest" => check no longer needed So your suggestion would indeed prevent some useless checks. I did not find a way to flag the child directly (it is of type MIVar) so I added a second boolean (not really nice). if (getParent() != null) { if (getParent().childWillNeedCastToBaseClassWorkaround){ hasCastToBaseClassWorkaround = true; childWillNeedCastToBaseClassWorkaround = false; } else if (getParent().hasCastToBaseClassWorkaround){ hasCastToBaseClassWorkaround = true; } Created attachment 195660 [details]
Remember if we used the workaround to create are own var-info-expression
Here is the patch according to my last comment.
Let's continue and finish this bug. However, I'm worried about the risk about putting it for Indigo since we are so late. I prefer to plan it for Indigo's first maintenance release (CDT 8.0.1) (In reply to comment #45) > Let's continue and finish this bug. However, I'm worried about the risk about > putting it for Indigo since we are so late. > > I prefer to plan it for Indigo's first maintenance release (CDT 8.0.1) As a user who his plagued with this (gdb) bug I would prefer the Indigo release :-) But it is up to you. You know the DSF code much better and have the commit rights (and the responsibility). (In reply to comment #46) > (In reply to comment #45) > > Let's continue and finish this bug. However, I'm worried about the risk about > > putting it for Indigo since we are so late. > > > > I prefer to plan it for Indigo's first maintenance release (CDT 8.0.1) > As a user who his plagued with this (gdb) bug I would prefer the Indigo release > :-) > But it is up to you. You know the DSF code much better and have the commit > rights (and the responsibility). I understand, but MIVariableManager is pretty complex and I would prefer to have such a fix soak a little, just in case. Thanks for understanding. (In reply to comment #45) > Let's continue and finish this bug. However, I'm worried about the risk about > putting it for Indigo since we are so late. What about committing the patch right after the Indigo release? So the Indigo release would be safe and I could use a nightly build from the 8.0 branch. If I would encounter any problems we would have enough time until the maintenance release. (In reply to comment #48) > (In reply to comment #45) > > Let's continue and finish this bug. However, I'm worried about the risk about > > putting it for Indigo since we are so late. > What about committing the patch right after the Indigo release? So the Indigo > release would be safe and I could use a nightly build from the 8.0 branch. If I > would encounter any problems we would have enough time until the maintenance > release. Absolutely. As soon as the branch is open. (In reply to comment #49) > > What about committing the patch right after the Indigo release? So the Indigo > > release would be safe and I could use a nightly build from the 8.0 branch. If I > > would encounter any problems we would have enough time until the maintenance > > release. > > Absolutely. As soon as the branch is open. Marc, can you commit the patch? (In reply to comment #50) > (In reply to comment #49) > > > What about committing the patch right after the Indigo release? So the Indigo > > > release would be safe and I could use a nightly build from the 8.0 branch. If I > > > would encounter any problems we would have enough time until the maintenance > > > release. > > > > Absolutely. As soon as the branch is open. > Marc, can you commit the patch? I'll have a look again this week. Created attachment 200851 [details]
Fix based on "Remember if we used the workaround to create are own var-info-expression"
I modified the proposed patch into something that I felt was a bit cleaner:
1- I moved the logic that played with the childExpression to before we call -var-info-path-expression. That way, we don't need to make that call to GDB if we won't be using it.
2- I used a local class ChildFullExpressionInfo, to allow us to know if the workaround was used for the child (instead of a new global childWillNeedCastToBaseClassWorkaround)
3- to know if a variable has the same name as its type, instead of doing "parentExpression != getGdbName()", which uses an internal implementation detail (that we use the GdbName as an expression, I used "!isAccessQualifier(getExpressionInfo().getRelExpr())"
4- I had to enhance buildChildExpression() because it was being used in more cases, now that we have to build our own expression for every descendant of a derived class.
5- I added buildDerivedChildExpression() to do the special casting.
The MIExpresions JUnit tests pass for GDB 6.6 to 7.3.
And what pleased me even more is that with GDB 6.6, more tests pass than before because we can handle derived classes ourselves (6.6 does not have the command -var-info-path-expression).
Axel, can you try this patch, look it over too and tell me what you think?
I'm still not happy with this fix. We are affecting too many expressions for a small set that has the problem. With the latest patch, every expression that is a derived class or a descent of it, will require us to build the full expression ourselves instead of using -var-info-path-expression. Descendants of derived expressions can take any format allowed by C/C++, so we are effectively bypassing GDB for all derived classes. On the other hand, from what I can see, the problem seems to only happens for derived classes that: 1- the parent or child class has a constructor AND 2- we are currently within the child class when looking at the content (going through 'this'). If we look at the content of the derived class when not in it (not using 'this'), the problem does not happen. The problem is that, in MIVariableManager, we don't cover all possibilities when creating the full expression ourselves. Our MIVariableManager.buildExpression method is very rudimentary compared to how GDB does it in -var-info-path-expression; in fact, GDB has one method for C code (171 lines) and a different one for C++ code (108 lines). I doubt that our 23 lines of buildExpression() is covering everything. So, we have three options: 1- fix GDB (we just missed the release of 7.3, so it may take some months). I'll follow-up on http://sourceware.org/ml/gdb-patches/2010-09/msg00204.html 2- find a way to limit the impact to only the broken cases and their children (at least only when going through 'this') 3- try to code an exhaustive buildExpression() method, like GDB does. (In reply to comment #52) > I modified the proposed patch into something that I felt was a bit cleaner: > Axel, can you try this patch, look it over too and tell me what you think? Sorry, for my late answer, However, I am still struggling with EGit (e.g. Quick diff shows no change after applying your patch). I had a look at your patch and have no objections. (In reply to comment #53) > I'm still not happy with this fix. > We are affecting too many expressions for a small set that has the problem. > > With the latest patch, every expression that is a derived class or a descent of > it, will require us to build the full expression ourselves instead of using > -var-info-path-expression. Descendants of derived expressions can take any > format allowed by C/C++, so we are effectively bypassing GDB for all derived > classes. > > On the other hand, from what I can see, the problem seems to only happens for > derived classes that: > 1- the parent or child class has a constructor > AND > 2- we are currently within the child class when looking at the content (going > through 'this'). If we look at the content of the derived class when not in it > (not using 'this'), the problem does not happen. > > The problem is that, in MIVariableManager, we don't cover all possibilities > when creating the full expression ourselves. Our > MIVariableManager.buildExpression method is very rudimentary compared to how > GDB does it in -var-info-path-expression; in fact, GDB has one method for C > code (171 lines) and a different one for C++ code (108 lines). I doubt that > our 23 lines of buildExpression() is covering everything. > > So, we have three options: > > 1- fix GDB (we just missed the release of 7.3, so it may take some months). > I'll follow-up on http://sourceware.org/ml/gdb-patches/2010-09/msg00204.html > > 2- find a way to limit the impact to only the broken cases and their children > (at least only when going through 'this') > > 3- try to code an exhaustive buildExpression() method, like GDB does. Did not have we the same discussion some months ago? :) My answer from comment #24 still holds. I would prefer to fix this bug in Eclipse and do not want to wait for a gdb bugfix. I don't know how long this will take (there was no clear response to your submitted patch and the GDB bugzilla entry is very old) and updating GDB is not always an option. However, an exhaustive rebuild of the GDB code to build the expression is beyond our scope. Thus, I recommend option #2. Let's see if I find something to limit the scope. I will let you know. (In reply to comment #55) > Did not have we the same discussion some months ago? :) > My answer from comment #24 still holds. I would prefer to fix this bug in > Eclipse and do not want to wait for a gdb bugfix. I don't know how long this > will take (there was no clear response to your submitted patch and the GDB > bugzilla entry is very old) and updating GDB is not always an option. I agree that upgrading GDB is not good for everyone (and since we missed 7.3 it may be a while). But it is still worth fixing in GDB so I've posted a follow-up to my GDB fix: http://sourceware.org/ml/gdb-patches/2011-08/msg00108.html > an exhaustive rebuild of the GDB code to build the expression is beyond our > scope. Thus, I recommend option #2. Let's see if I find something to limit the > scope. I will let you know. Thanks, that would be my preferred solution as well. Created attachment 200983 [details]
Updated latest fix to easily turn off
This patch is identical to the previous one, except that it add a protected method MIVariableManager.needFixForGDBBug320277(). This will allow to easily override this special behavior for GDBs that don't have the bug.
But we still need to reduce the impact of this fix to only failed cases.
Axel, FYI, I'll be on vacation for the next two weeks.
*** cdt git genie on behalf of Marc Khouzam ***
Bug 320277: Cannot view variables of base class in derived class
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f0f38554230a5ef1c6e19ee1d10e664fb2585a4c
(In reply to comment #57) > But we still need to reduce the impact of this fix to only failed cases. Still on vacation but I had a thought. The best way to do this fix safely is to only trigger the workaround once GDB fails. I haven't look into if it is feasible or not, but here is the idea. Use the current algorithm of MIVariableManager (no fix), but look for a GDB failure. Once GDB fails, trigger the workaround. It may be that the failure happens for a child of the expression we have to change, but I believe we can go back up the hierarchy and fix the problem at the parent, and mark that hierarchy as needing the fix. (In reply to comment #59) > Still on vacation but I had a thought. The best way to do this fix safely is > to only trigger the workaround once GDB fails. Well, you had this idea before (see comment #28 option 3). And again I do not know how to get the parentDMC (see my answer #33). Any advice? (In reply to comment #60) > Well, you had this idea before (see comment #28 option 3). And again I do not > know how to get the parentDMC (see my answer #33). Any advice? OK. I finally managed to get this working as described in comment #28 . As soon as it is working properly I will submit a new patch. (In reply to comment #61) > (In reply to comment #60) > > Well, you had this idea before (see comment #28 option 3). And again I do not > > know how to get the parentDMC (see my answer #33). Any advice? > OK. I finally managed to get this working as described in comment #28 . As soon > as it is working properly I will submit a new patch. From comment #28: > 3- the very first time (and only then) we are about to insert the 'struct' > keyword, we first send a command to GDB using -data-evaluate-expression with > the result of -var-info-path-expression. If the command fails, we know we have > to apply our fix; else we know we can trust GDB. Now that I understand that it is not very easy to figure out in which case we should add the 'struct' keyword and when we don't need to, I think we should always go to GDB and see if we get an error. Once we get an error, we then know all children will need the workaround, but any varObj not part of such a hierarchy should still go through GDB first. My original thought in comment 28 was to have a single on/off flag for the entire MIVariableManager, but that is too broad. Created attachment 202188 [details]
To check if GDB actually has this bug we call -data-evaluate-expression with the return value of -var-info-path-expression
If a derived class is cast to its base class we check if GDB actually has this bug by calling -data-evaluate-expression with the return value of -var-info-path-expression. If -data-evaluate-expression fails we know GDB is buggy and we will build the expression ourselves. This check is done only once. For the children we check the value of hasCastToBaseClassWorkaround.
(In reply to comment #56) > I agree that upgrading GDB is not good for everyone (and since we missed 7.3 it > may be a while). > But it is still worth fixing in GDB so I've posted a follow-up to my GDB fix: > http://sourceware.org/ml/gdb-patches/2011-08/msg00108.html I committed the fix for GDB this morning: http://sourceware.org/ml/gdb-patches/2011-08/msg00489.html The other good news is that a GDB 7.3.1 should be released very soon and will contain that fix. From August 19: http://sourceware.org/ml/gdb-patches/2011-08/msg00372.html "So, the plan, now, is to make a 7.3.1 release ASAP, particularly since we now have the fix for the NetBSD build failure...." (In reply to comment #64) > I committed the fix for GDB this morning: > http://sourceware.org/ml/gdb-patches/2011-08/msg00489.html > > The other good news is that a GDB 7.3.1 should be released very soon and will > contain that fix. From August 19: > http://sourceware.org/ml/gdb-patches/2011-08/msg00372.html > > "So, the plan, now, is to make a 7.3.1 release ASAP, particularly > since we now have the fix for the NetBSD build failure...." That's cool. But I am still using gdb 7.2. So I need the Eclipse workaround :) Did you have a look at my latest patch? (In reply to comment #65) > That's cool. But I am still using gdb 7.2. So I need the Eclipse workaround :) > Did you have a look at my latest patch? I had a look and it seems pretty good. Thanks for posting it. I'll properly try it out review it sometime this week. (In reply to comment #66) > I had a look and it seems pretty good. Thanks for posting it. > I'll properly try it out review it sometime this week. Any chance to get this into the SR1 update release? (In reply to comment #63) > Created attachment 202188 [details] > To check if GDB actually has this bug we call -data-evaluate-expression with > the return value of -var-info-path-expression > > If a derived class is cast to its base class we check if GDB actually has this > bug by calling -data-evaluate-expression with the return value of > -var-info-path-expression. If -data-evaluate-expression fails we know GDB is > buggy and we will build the expression ourselves. This check is done only once. > For the children we check the value of hasCastToBaseClassWorkaround. I finally reviewed the patch in detail. I like the solution. I have cleaned it up a bit and will post a new version that I am planning on committing once I get to the office. Here are some comments on the original patch. I made the corresponding changes already, in the patch I will post. 1- We don't have a clear rule to know for which expressions GDB will fail, which is why we actually try -data-evaluate-expression and see if the expression fails or not. This is not only to know if GDB has the bug, but to know if the current expression is affected. So, since we don't know for sure when an expression should fail or now, we cannot know if needFixForGDBBug320277() should be true or false. Therefore, I removed setNeedFixForGDBBug320277() and the variable needFixForGDBBug320277; instead we'll just have the protected method needFixForGDBBug320277() which can be overridden for GDB versions that we know don't need the fix. 2- cleaned up some missing done() calls and one extra one. 3- Although I liked the check to fStatus.getCode() == 10004 and the error message, we still need to do something for any other error. So, I made it more general and if we have any error, we build the expression ourselves. There is one more thing I want to try, which is to merge buildDerivedChildExpression() and buildChildExpression(), to see if it can be used for GDB < 6.7 which didn't have the -var-info-path-expression command. But I'll do that as a separate patch. Thanks Axel! Once I post the patch, I hope you can confirm it works as you would expect. BTW, GDB 7.3.1, which has this bug fixed, has been released Created attachment 202899 [details]
Updated Axel's fix that uses -data-evaluate-expression (master)
Here is the patch for master. I'll have to modify slightly for 8_0 because I cannot add APIs.
Axel, how does it look?
I'll commit to master now.
Oh wait, Axel, you need to confirm in this bug that you wrote this code by yourself (my changes are ok), and that you have to right to contribute to eclipse. Created attachment 202902 [details]
Updated Axel's fix that uses -data-evaluate-expression (8_0)
This is the cdt_8_0 fix which updates the plugin version but also makes the needFixForGDBBug320277() method private, because we cannot add a new API.
Once I get Axel's confirmation he can contribute his patch to eclipse, I'll commit to 8_0
(In reply to comment #71) > Oh wait, Axel, you need to confirm in this bug that you wrote this code by > yourself (my changes are ok), and that you have to right to contribute to > eclipse. I wrote all the code by myself and I have the right to contribute it (it is private work). (In reply to comment #73) > (In reply to comment #71) > > Oh wait, Axel, you need to confirm in this bug that you wrote this code by > > yourself (my changes are ok), and that you have to right to contribute to > > eclipse. > I wrote all the code by myself and I have the right to contribute it (it is > private work). I committed the patches to master and cdt_8_0. Thanks Axel for not giving up :-) (In reply to comment #70) > Here is the patch for master. I'll have to modify slightly for 8_0 because I > cannot add APIs. > > Axel, how does it look? I just had a short look at the patch and it looks good (I guess I will have no time before the weekend to test the patch). >3- Although I liked the check to fStatus.getCode() == 10004 and the error >message, we still need to do something for any other error. So, I made it more >general and if we have any error, we build the expression ourselves. I added this check so we are sure that GDB has this specific bug. So we need to test GDB only once per session and not for every possible base class. That's why I introduced the needFixForGDBBug320277 variable. (In reply to comment #74) > I committed the patches to master and cdt_8_0. > > Thanks Axel for not giving up :-) Thanks for your support. BTW, you can also close bug #308678 . This patch should resolve that problem, too. (In reply to comment #75) > >3- Although I liked the check to fStatus.getCode() == 10004 and the error > >message, we still need to do something for any other error. So, I made it more > >general and if we have any error, we build the expression ourselves. > I added this check so we are sure that GDB has this specific bug. So we need to > test GDB only once per session and not for every possible base class. That's > why I introduced the needFixForGDBBug320277 variable. The problem is that if the call to -data-evaluate-expression works for one expression, it does not mean that GDB does not have the bug. For example, when we are not dealing with 'this' the problem does not occur. So, I don't think it was right to disable the fix if things worked once. Makes sense? (In reply to comment #77) > The problem is that if the call to -data-evaluate-expression works for one > expression, it does not mean that GDB does not have the bug. For example, when > we are not dealing with 'this' the problem does not occur. So, I don't think > it was right to disable the fix if things worked once. > Makes sense? OK, I didn't think about this case. *** cdt git genie on behalf of Axel Mueller ***
Bug 320277: Cannot view variables of base class in derived class
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=e44884f7ea63d5edcda8cd165de5ac23b741e226
*** cdt git genie on behalf of Axel Mueller ***
Bug 320277: Cannot view variables of base class in derived class
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=84fa9f1b4912b6a7a9adf2cdf226ea54e430f81e
|
Build Identifier: Version: Helios Release (3.6.0) Build id: 20100527-0614, CDT 7.0 Build the following code and put a breakpoint at the printf command. Then try to inspect this->testbase.aiVal in the variables view. You will get this: (((*(testbase*) this)).aiVal)[0] Error: Unable to create variable object Reproducible: Always Steps to Reproduce: #include <cstdio> class testbase { public: testbase(){ aiVal[0] = 1; }; protected: int aiVal[5]; }; class test : public testbase { public: test() { printf("%d", aiVal[0]); } }; int main(void) { test t; return 0; } BTW, when you debug the same code with the CDI debug framework you can inspect the variables flawlessly. This bug might be in relation to #308678