Comment 1 David Pochet 2010-10-08 10:04:38 EDT

Found a trivial fix in org.eclipse.cdt.internal.core.settings.model.CConfigurationDescriptionCache.getBuildVariablesContributor()

	@Override
	public ICdtVariablesContributor getBuildVariablesContributor() {
		// Assume no contributor when data is not loaded yet
	    if ( fData == null )
	    {
	        return null;
	    }
	    return fData.getBuildVariablesContributor();
	}

We need to deliver a product based on this patch in late October, so we'll provide a repackaged cdt.core for this one.
Do you think it would be possible to integrate this fix in 7.0.2?

PS: would be great to commit it ASAP, so I can grab a CDT nightly build for my October delivery ;)

Comment 2 Andrew Gvozdev 2010-10-08 10:20:49 EDT

Can you attach the fix as eclipse patch? Also, it would help if you could provide a sample of build definition extension in order to reproduce the issue.

Comment 3 James Blackburn 2010-10-08 10:54:43 EDT

Hmm. We need to be very careful about just quietly handling this.  

AFAIK the cache objects represent the read-only configuration, so if fData is null here I worry that it'll never become non-null.  
Alternatively, how are clients meant to determine whether getMacros returns an invalid value because the configuration isn't correctly loaded, or because the value doesn't exist?

Comment 4 David Pochet 2010-10-11 05:42:14 EDT

(In reply to comment #3)
> Hmm. We need to be very careful about just quietly handling this.
> 
> AFAIK the cache objects represent the read-only configuration, so if fData is
> null here I worry that it'll never become non-null.
> Alternatively, how are clients meant to determine whether getMacros returns an
> invalid value because the configuration isn't correctly loaded, or because the
> value doesn't exist?
If you take a look at the (long) stack trace, you'll see that fData is null since it is currently under initialization
(at org.eclipse.cdt.internal.core.settings.model.CConfigurationDescriptionCache.loadData(CConfigurationDescriptionCache.java:95))
And during this init, CDT is looking at the enabled tools
And in this case a tool enablement is conditioned on a build variable which is contributed by a build macro provider
The purpose of the fix is just to consider that during initialization, since we can't access to the Build Variables Contributor, we just consider we don't have one...

Comment 5 David Pochet 2010-10-11 05:55:41 EDT

Created attachment 180586 [details]
Patch for org.eclipse.cdt.core project

(In reply to comment #2)
> Can you attach the fix as eclipse patch? Also, it would help if you could
> provide a sample of build definition extension in order to reproduce the issue.
Patch created and attached. I work on a small sample plugin highlighting the issue.

Comment 6 David Pochet 2010-10-11 08:11:05 EDT

Created attachment 180594 [details]
Sample plugin helping to reproduce the issue

Comment 7 David Pochet 2010-10-11 08:17:43 EDT

(In reply to comment #2)
> Can you attach the fix as eclipse patch? Also, it would help if you could
> provide a sample of build definition extension in order to reproduce the issue.
Please use attached plug-in to reproduce the issue.
This plug-in provides a new project type/toolchain/configuration base on MinGW one to build an exe file.
The toolchain provides two compilers, enabled regarding the value of MySampleMacro build variable, contributed through org.foo.bar.bug327001.BuildMacroSupplier.
You can verify the correct enablement of the tool by providing a generic variable (through the Preferences > Run & Debug > String Substitution) named "ToolVersion":
* set it to "V2" to enable "tool2"
* set it to anything else to enable "tool1"
Until there, everything is OK.

Now, just close and reopen the project, and you'll get the NPE mentioned above.
If you give a try with the proposed fix, the project behaves correctly again when closed then reopened.

Comment 8 David Pochet 2010-10-13 03:03:41 EDT

(In reply to comment #2)
Hi Andrew,
Please can you tell me if there is any chance to commit this patch before the end of next week?
(just a matter of organization for us, to see if we need to publish a patched CDT by ourselves or not)
Thanks.

Comment 9 Andrew Gvozdev 2010-10-13 11:30:24 EDT

I'll take a look. I won't promise anything yet as the core build model could be swampy when considering all implications. I'll need James' second opinion in any case.

Comment 10 Andrew Gvozdev 2010-10-14 10:47:16 EDT

Created attachment 180885 [details]
alternative patch

(In reply to comment #5)
> Created an attachment (id=180586)
> Patch for org.eclipse.cdt.core project

I don't think checking for null is the correct fix here. Some questions are popping up:
- Why a build variable is being evaluated in the context of configuration description being loaded? Either there is no need to evaluate or if there is the need wrong configuration description is used.
- I see cases with CConfigurationDescriptionCache.copyBuildData(CBuildData, boolean) down in stack trace and getBuildVariablesContributor() is called when fInitializing is "true". Does it mean we are trying to copy from non-inited configuration or fInitializing is not reset properly, or, again, wrong description is used? Not sure, it smells fishy.

On the other hand, I think that an exception evaluating one enablement expression should not abort the whole operation. I suggest to catch the exception, log it, consider the expression false, and proceed with evaluating next expression. This does not fix the root cause but would mitigate the consequences IMO.

James, what do you think?

Comment 11 David Pochet 2010-10-18 02:48:14 EDT

Hi Andrew,
I just hope someone would find a definitive fix without logging anything... Because in our case, such a patch means an error entry in the workspace log for each project, each time the workbench is started...

Comment 12 James Blackburn 2010-10-18 06:47:25 EDT

(In reply to comment #10)
> I don't think checking for null is the correct fix here. Some questions are
> popping up:
> ...
> James, what do you think?

I'm having difficulty with this as well.  It seems that incorrectly evaluating the variable results in an invalid toolchain:

FolderInfo.filterTools(ITool[], IManagedProject) line: 316	
FolderInfo.getFilteredTools() line: 325	
Configuration.getFilteredTools() line: 1053	
ManagedBuildManager.performValueHandlerEvent(IConfiguration, int, boolean) line: 3298	

where filterTools is returning a vector with just an assembler and linker.

As soon as the data cache is available, the compiler tool is available. Isn't this discrepancy a real bug that will be masked by simply returning null?

Comment 13 Andrew Gvozdev 2010-10-18 10:07:22 EDT

If it is just messages in the log and you guys think we are better off letting exceptions loose while evaluating expressions - that is fine with me. It is a real bug of course regardless and if somebody develops a real fix it would be better of course.

Comment 14 David Pochet 2010-10-19 05:17:55 EDT

So, is there any chance someone to commit one of the proposed patchs, even if the bug is not closed and a definitive fix has to be found?

Comment 15 David Pochet 2010-10-29 04:50:34 EDT

Just an update concerning the initially proposed patch. We've integrated it for three weeks now, and we didn't found any regression.
We took the option to provide our own CDT build for our November product delivery, but we strongly hope to have an "official" fix for next CDT 7.0.2 release (which would make us avoid rebuilding our own each time...)
Thanks!

Comment 16 David Pochet 2010-11-19 03:25:23 EST

Just a thought I sent to cdt-dev mailing list, and I copy there, for history logging:

My interpretation would be:
We are in a case where fdata member is under initialization (cf CConfigurationDescriptionCache.loadData(CConfigurationDescriptionCache.java:95) in the stack trace)
For any reason, we need to check if current instance provides a Build Variable Contributor (cf CConfigurationDescriptionCache.getBuildVariablesContributor(CConfigurationDescriptionCache.java:450))
As fdata is not yet initialized at this time, the patch proposal is to temporarily consider that there is no Build Variable Contributor (until fdata will be non-null)
Since build variables seems not to be cached anywhere, the next time the variable is evaluated, fdata member is non-null and normal behavior is resumed.