Bug 109506 - "Open Declaration" (hardly ever) works for large standard make project
Summary: "Open Declaration" (hardly ever) works for large standard make project
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-core (show other bugs)
Version: 3.0   Edit
Hardware: PC Linux-GTK
: P3 normal (vote)
Target Milestone: 4.0   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-09-14 10:53 EDT by Norbert Plött CLA
Modified: 2007-05-18 01:27 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Norbert Plött CLA 2005-09-14 10:53:18 EDT
In a project I am working on code navigation is very unreliable. I have drilled 
into the behavior of "Open Declaration", which functions only for rare, non 
reproducable examples. Here are some more details of my setup:

Host OS: Linux
CDT Version: 3.0 Release
Project Type: Standard Make C++
The project contains some 4.500 files - the eCos OS plus an application.
The eCos sources are linked into the project with a linked folder.

Here is my testcase:
from file src/sock_u.c, function definition for sock_doselect() navigate to the 
definition in ../cfg/sock_usr.h with "open declaration". Instead of opening the 
header file the search returns upon itself, i.e. the function name gets 
selected in the sock_u.c file.

In order to learn more about the working of "open declaration" I created 
another mini managed make C project which contains little more than a main() 
function and header. "Open declaration" works perfectly for this little test 
program.

I drilled into the sources to find out where the difference is and arranged for 
some trace output during the search process. The crucial difference seems to be 
the order in which the IASTName for either the declaration or the definition is 
registered in the CScope for the translation unit:

===Trace from real-life eCos project===
Adding to CScope sock_doselect hash 19166803 
in /home/pn3484/BDIEvaluierung/PNIO/sock/src/sock_usr.h
OpenDeclarationsAction selected name sock_doselect hash 13922000 
in /home/pn3484/BDIEvaluierung/PNIO/sock/src/sock_u.c
CVisitor visiting declarator sock_doselect hash 19166803 
in /home/pn3484/BDIEvaluierung/PNIO/sock/src/sock_usr.h
CVisitor visiting declarator sock_doselect hash 13922000 
in /home/pn3484/BDIEvaluierung/PNIO/sock/src/sock_u.c
Name added to visitor
======

===Trace from mini testing project===
OpenDeclarationsAction selected name main hash 14307591 
in /home/pn3484/workspace3.1/demo/main.c
Adding to CScope main hash 14307591 in /home/pn3484/workspace3.1/demo/main.c
CVisitor visiting declarator main hash 9804480 
in /home/pn3484/workspace3.1/demo/main.h
Name added to visitor
CVisitor visiting declarator main hash 14307591 
in /home/pn3484/workspace3.1/demo/main.c
Name added to visitor
======

In the real-life project the IASTName of the declaration is registered in the 
CScope. Later, when the searching visitor examines the declaration and wants 
resolve it's binding then CScope.getBinding( IASTName name, boolean resolve ) 
does not resolve the binding since it is called with resolve=false.

In the testing project the other IASTName (of the definition) is registered and 
the binding is resolved in spite of resolve=false because the IASTNames of 
definition and declaration are different objects and will be resolved anyway.

Now I only (at most) half understand what is going on and why the CScope works 
the way it does. I do not think it will be practical to supply the big project 
due to it's sheer size. I am very willing to supply more details and help 
further if someone who understands the workings of the AST framework will 
comment on this.
Comment 1 Norbert Plött CLA 2005-09-16 05:32:02 EDT
[Doug Schaefer]
> The standard first question I ask when someone reports things like this is:
> Are your paths set properly in the project properties. Without knowing where
> to find include files, parsing is tremendously difficult.

[Norbert Ploett]
Yeah well, (blush) actually there are only the auto-discovered paths set. Also 
I have not defined any source folders (so that the project directory is the 
default source folder) 
Auto-discovered paths are some  /usr/... and the ecos kernel's header files. 
That's pretty good.

And also ...

The path structure for my example is as follows:

<project dir>
 +- sock
	+- cfg
	|	+- sock_inc.h
	+- src
		+- sock_usr.h
		+- sock_u.c

sock_u.c locally includes sock_inc.h:
#include "../cfg/sock_inc.h"


sock_inc.h locally includes sock_usr.h:
#include "../src/sock_usr.h"

So this is actually included around two corners, but does not require any 
include path settings for the compiler.
Also during my debugging sessions I have seen that the file is found and 
parsed. The problem seems to be in resolving the binding correctly later when 
the IASTName is searched for. Can this also happen due to missing path settings?

Comment 2 Norbert Plött CLA 2005-09-30 03:56:10 EDT
Just for completeness I set up

<project dir>/sock/cfg and
<project dir>/sock/src

as C/C++ include paths with identical behaviour i.e. "Open Declaration" fails 
(whereas "Open Definiton" brings me happily from the header to the source file).

As stated above I did not expect anything else because of my analysis 
concerning the CScope.getBinding() but I tried setting the paths just to make 
sure :-)
Comment 3 Andrew Niefer CLA 2005-09-30 10:46:10 EDT
Note that there is only one binding regardless of which IASTName you start at,
both the declaration and the definition return the same binding.

Without actually debugging anything, and just guessing, I'm going to say that I
don't think the problem is CScope.getBinding().  Instead I would suggest
considering OpenDeclarationsAction and DOMSearchUtil.getNames().  The
OpenDeclarationsAction via the DOMSearchUtil first resolves the binding, then
finds all the declarations for the binding, and then opens the first declaration
in that list.  Since definitions are also declarations, I expect what is
happening is in that list of declarations, the definition happens to come before
the declaration you want.

I suggest that at the least, instead of just using domNames[0],
OpenDeclarationsAction.run() should consider domNames[1] if domNames[0] is a
definition.  Or somehow consider the whole list and pick the "best" one.
Comment 4 Norbert Plött CLA 2005-10-04 09:15:37 EDT
> I suggest that at the least, instead of just using domNames[0],
> OpenDeclarationsAction.run() should consider domNames[1] if domNames[0] is a
> definition.  Or somehow consider the whole list and pick the "best" one.

In my case the DOMSearchUtil.getNamesFromDOM() returns an array of length 1 (!).
The contained IASTName is the definition. 
Can I find out any more for you?
Comment 5 Norbert Plött CLA 2005-10-05 03:33:28 EDT
I have now taken a deeper look into the working of the DOMSearchUtil and found 
the following:

1. The OpenDeclarationsAction.run() method triggers the search via the 
DOMSearchUtil.

DOMSearchUtil calls the translation unit to get the matching declarations

CASTTranslationUnit.getDeclarations() initiates the visitor pattern 
which navigates via the CASTSimpleDeclaration to the CASTFunctionDeclarator 
which represents the place in the header where the focus should be switched to.

Now CVisitor.CollectDeclarationsAction.visit(IASTDeclarator declarator) is 
called 
and should eventually add the visited CASTFunctionDeclarator to the 
CollectDeclarationsAction:

...
				} else if ( parent instanceof 
IASTSimpleDeclaration ) {
					// prototype parameter with no 
identifier isn't a declaration of the K&R C parameter 
//					if ( binding instanceof CKnRParameter 
&& declarator.getName().toCharArray().length == 0 )
//						return PROCESS_CONTINUE;
					
/* this test should succeed */	if( (declarator.getName() != null && 
declarator.getName().resolveBinding() == binding) ) {
/* addName() should be called */		addName(declarator.getName());
					}
				} 

However resolution of declarator.getName().resolveBinding() leads to a new 
object being created, 
consequently the second condition 

declarator.getName().resolveBinding() == binding 

fails and therefore this IASTName is not added to the CollectDeclarationsAction 
and therefore is not found.


Why is that so?

2. CASTName.resolveBinding() finds that the CASTName's binding field is null 
and calls CVisitor.createBinding(IASTName)
which calls CVisitor.createBinding(IASTDeclarator declarator, IASTName name)
which calls CVisitor.createBinding( IASTDeclarator declarator )

This method tries to resolve the binding from the scope:

            binding = ( scope != null ) ? scope.getBinding( name, false ) : 
null;

but scope.getBinding() returns null.

The subsequent if ... else construct ends up in the default else branch:

				} else {
					binding = new CVariable( name );
				}

i.e. a new object is created for the binding.

Subsequently an attempt is made to add the name to the scope

		if( scope != null && binding != null )
            try {
                scope.addName( name );
            } catch ( DOMException e ) {
            }

but the scope does not add this name because it is identical 
to the name which was already entered in the scope 
and consequently has the same index.

Now why does scope.getBinding( name, false) return null?


3. CScope.getBinding() looks up a binding from it's binding CharArrayObjectMap 
array.

179        Object o = bindings[type].get( name.toCharArray() );

In my debug session o is the same CASTName object which was also passed as 
parameter.
It is now noted that o does not implement IBinding and therefore  the next test 
is

187        if( (resolve || ((IASTName)o).getBinding() != null) && ( o != 
name ) )
188            return ((IASTName)o).resolveBinding();

This test fails because resolve is passed as false and o and name are in fact 
identical.
I do not understand this part of the code and the functioning of CScope in 
general. 
An expert (the author?) must look at this.

Comment 6 Andrew Niefer CLA 2005-11-02 23:09:09 EST
I have not been able to reproduce your problem.

However, I wonder why after scope.getBinding() returns null, you end up with a
new CVariable instead of a new function.

If the bug is with the CScope.getBinding, you could try the following condition:
if( (resolve && o != name) || ((IASTName)o).getBinding() != null )
instead of
if( (resolve || ((IASTName)o).getBinding() != null) && (o != name) )
Comment 7 Markus Schorn CLA 2007-05-15 11:11:50 EDT
Norbert, is this still a problem with 4.0?
Comment 8 Norbert Plött CLA 2007-05-16 07:07:01 EDT
Markus, thanks for looking into this. I do not have the original example handy anymore, but I looked at the behaviour of my standard qhull example and I am pretty impressed. I did not encounter a single case where "open declaration" would not have worked. So I guess it is fixed.

The material I collected is probably outdated anyway with the new indexer :-)
Comment 9 Doug Schaefer CLA 2007-05-16 11:30:37 EDT
(In reply to comment #8)
> Markus, thanks for looking into this. I do not have the original example handy
> anymore, but I looked at the behaviour of my standard qhull example and I am
> pretty impressed. I did not encounter a single case where "open declaration"
> would not have worked. So I guess it is fixed.
> The material I collected is probably outdated anyway with the new indexer :-)

Very cool. If I remember correctly qhull has some pretty intense source.
Comment 10 Norbert Plött CLA 2007-05-18 01:27:31 EDT
Setting the target milestone for the records :-)