Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 354194

Summary:

Custom scanner discovery selection is not honored if file has any custom properties

Product:

[Tools] CDT

Reporter:

John Cortell <john.cortell>

Component:

cdt-build

Assignee:

Project Inbox <cdt-core-inbox>

Status:

RESOLVED FIXED

QA Contact:

Andrew Gvozdev <angvoz.dev>

Severity:

normal

Priority:

CC:

cdtdoug

Version:

8.0

Flags:

angvoz.dev: review+

Target Milestone:

---

Hardware:

OS:

Windows XP

Whiteboard:

Attachments:

Description	Flags
Diff file referenced in reproducibility steps	none
Proposed fix	john.cortell: iplog-
Proposed fix	john.cortell: iplog-

Description John Cortell

2011-08-08 18:39:45 EDT

In a project that is using a custom scanner discovery profile, customizing any source file (property) results in the default SD profile being used for that file.

1. Add a custom scanner discovery profile that is exactly the same as
org.eclipse.cdt.managedbuilder.core.GCCManagedMakePerProjectProfileC but
which specifies -Dwhatever in its commandline. This involves some small
additions/modifications to various plugin.xmls. I've attached a diff that details those changes. The diff includes a println in CommandLauncher.java that will help us see what's going on.
2. Launch a runtime workbench
3. Create a Hello World ANSI C project and add two c files that each
have only 
   #ifdef whatever
   // yadayada
   #endif
Also add these three lines to main.c
4. Bring up the Discovery Options properties page and switch the discovery
profile to the "myprofile" one
5. Build the project. 
In the console (in the debugger Eclipse instance), you should see a single scanner discovery execution, courtesy of the println we added in step 1.

   Launching: gcc -E -P -v -Dwhatever -dD C:/<somepath>/specs.c

Open each of the source files and note that the #ifdef whatever/#endif's are not grayed. So far so good. Our custom SD profile defines "whatever" in its cmdline, thus the observed behavior is what we expect.
6. Bring up the properties for any file in the project. Make some innocuous change like enabling "Verbose" in C/C++ Buid > Settings > Tool Settings > Miscellaneous
7. Clear the Console in the debugging Eclipse instance, then back in the runtime workbench clean and build the project 
You will now see two invocations of scanner discovery

   Launching: gcc -E -P -v -Dwhatever -dD C:/<somepath>/specs.c
   Launching: gcc -E -P -v -dD C:/<somepath>/specs.c

Note that one lacks the -Dwhatever switch. This tells us that the additional 
SD invocation is based on the stock SD profile instead of our custom one.
This is unexpected since we haven't changed any settings to do with scanner discovery in step 6. If you bring up the file you customized in the editor, you'll now see that #ifdef whatever/#endif is now grayed, further revealing that the customized file is being indexed using the stock scanner discovery. Again, there's no reason that should be happening. Customize a third file and the problem grows. Now you'll see three invocations of scanner discovery, with two of them being based on the stock profile.

Comment 1 John Cortell

2011-08-08 18:42:29 EDT

I've debugged this a bit and it appears the problem is in CfgScannerConfigInfoFactory2.CfgInfo.createMap(), 
which for lack of finding an exact match for the customized file in 'configMap', 
effectively adds a new element to the map that uses the default scanner profile for the compiler tool. Set a breakpoint at the call to 

   info = container.createInfo(baseContext, id)

at line 195.

Comment 2 John Cortell

2011-08-08 18:43:37 EDT

Created attachment 201110 [details]
Diff file referenced in reproducibility steps

Comment 3 John Cortell

2011-08-10 18:56:41 EDT

Created attachment 201286 [details]
Proposed fix

I've come up with this fix, but keep in mind this is the first time I've ever looked at Scanner Discovery code. So, this really needs some close scrutiny from someone more knowledgeable in that code.

Unless I'm way off, there's two problems at hand. One is that the attempt to match the file to a discovery profile fails because a customized file doesn't inherently have a storageModule/scannerConfigBuildInfo element in the project description (.cproject), and the fallback position is to use a default profile. This fallback is obviously not right; we should be using the profile selected by the user for the particular configuration and language (i.e., tool + inputType). The second problem is that we create a temporary mapping, but effectively make it permanent by adding it to the project description (more specifically, by adding it to the IScannerConfigBuilderInfo2Set and leaving it there). Its permanency isn't so much the issue as is the fact that it becomes stagnant once the user selects a different profile. Only the folderInfo element (representing "/") in the .cproject is updated when the user changes the profile selection. So, prior to this fix if you customize a file, then change the Scanner profile selection, you can see in the .cproject that the custom file remains associated with the default scanner profile instead of the newly selected one. Furthermore, there's no way to remove that stale association other than by removing the element from the .cproject with a text file.

Comment 4 John Cortell

2011-08-10 18:59:28 EDT

Andew, please review

Comment 5 John Cortell

2011-08-10 19:07:14 EDT

Validation of the fix must include resetting the scanner profile selection after a file has been customized and the profile changed to the custom one. This will expose the second bug I referenced in comment # 3

Comment 6 Andrew Gvozdev

2011-08-12 15:54:35 EDT

(In reply to comment #3)
> Created attachment 201286 [details]
> Proposed fix
I cannot apply the patch. Commit 98fa6c5..30760cf referenced in the patch is missing in CDT repository. Do you know a way of applying patches like that?

Comment 7 John Cortell

2011-08-15 11:05:07 EDT

Created attachment 201500 [details]
Proposed fix

New patch file created with 'git diff origin/master..master'

Comment 8 Andrew Gvozdev

2011-08-16 16:13:53 EDT

(In reply to comment #3)
> Created attachment 201286 [details]
> Proposed fix
> I've come up with this fix, but keep in mind this is the first time I've ever
> looked at Scanner Discovery code. So, this really needs some close scrutiny from
> someone more knowledgeable in that code.
I have looked at his code on a few occasions but the details fade a while ago. I am afraid that is you who knows it best at the moment. What I can do just to take a look at the patch and see if it sets any alarm.

> Unless I'm way off, there's two problems at hand. One is that the attempt to
> match the file to a discovery profile fails because a customized file doesn't
> inherently have a storageModule/scannerConfigBuildInfo element in the project
> description (.cproject), and the fallback position is to use a default profile.
> This fallback is obviously not right; we should be using the profile selected by
> the user for the particular configuration and language (i.e., tool + inputType).
> The second problem is that we create a temporary mapping, but effectively make
> it permanent by adding it to the project description (more specifically, by
> adding it to the IScannerConfigBuilderInfo2Set and leaving it there). Its
> permanency isn't so much the issue as is the fact that it becomes stagnant once
> the user selects a different profile. Only the folderInfo element (representing
> "/") in the .cproject is updated when the user changes the profile selection.
> So, prior to this fix if you customize a file, then change the Scanner profile
> selection, you can see in the .cproject that the custom file remains associated
> with the default scanner profile instead of the newly selected one. Furthermore,
> there's no way to remove that stale association other than by removing the
> element from the .cproject with a text file.
That kind of make sense while you explain it. Just one remark - from your comments in the code, map entry is added to the container only to obtain the "info". Is it possible to create the "info" not adding it in there, that way it won't be necessary to remove it? However I have no advice how to do that, feel free to ignore that remark as rant. So flagging the review with "+".

Comment 9 John Cortell

2011-08-17 10:08:04 EDT

(In reply to comment #8)
> That kind of make sense while you explain it. Just one remark - from your
> comments in the code, map entry is added to the container only to obtain the
> "info". Is it possible to create the "info" not adding it in there, that way it
> won't be necessary to remove it? However I have no advice how to do that, feel
> free to ignore that remark as rant. So flagging the review with "+".

The only way I see is to add a method to IScannerConfigBuilderInfo2Set to create the info but not 'record' it.  In theory, this would be a little cleaner than asking it to create it and then asking it to forget it, but changing the interface to slightly optimize this single use case doesn't seem worth it.

Thanks for the review.

Comment 10 John Cortell

2011-08-17 10:14:47 EDT

Fix committed/pushed.

Comment 11 CDT Genie

2011-08-17 10:23:05 EDT

*** cdt git genie on behalf of John Cortell ***

    Bug 354194 - Custom scanner discovery selection is not honored if file
    has any custom properties

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=e51889143728a4d3b80cee368a6784a182584024

Comment 12 CDT Genie

2011-09-19 16:23:03 EDT

*** cdt git genie on behalf of John Cortell ***

    Bug 354194 - My previous fix accidentally prevented the creation of the config info map entry for the project root folder, thus scanner discovery preferences were no longer being persisted. This adjustment adds an exeption for that rcinfo.

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=dabef579a2a28361bbb6278a5c71a2afc25b1d0e