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

Bug 313370

Summary: GCC Per File Discovery never picks up built-ins
Product: [Tools] CDT Reporter: Doug Schaefer <cdtdoug>
Component: cdt-buildAssignee: Doug Schaefer <cdtdoug>
Status: RESOLVED FIXED QA Contact: Andrew Gvozdev <angvoz.dev>
Severity: normal    
Priority: P3 CC: jamesblackburn+eclipse, recoskie
Version: 7.0   
Target Milestone: 8.0   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Scheduled patch. cdtdoug: iplog-

Description Doug Schaefer CLA 2010-05-18 10:52:12 EDT 
I'm trying to use gcc per file scanner discovery for Android projects. I see that the build output parser picks up the commands. However the CommonBuilder updates the scanner config before the built-ins are calculated by the ScannerConfigBuilder leaving the commands with an empty scanner info. When the SC builder runs, there are no new commands to figure out.

As a result, per file scanner discovery never reports built-ins, or so it seems.

Also, the scanner config builder only runs on full builds so if you add files, it never runs anyway.
Comment 1 Chris Recoskie CLA 2010-05-18 10:55:56 EDT 
Yeah, I had to do a lot of work to get that working right for the XLC collector.  Basically I had to hack in tracking the project-wide built-ins to the per file XLC collector (which started off as just a clone of the GNU one).
Comment 2 Andrew Gvozdev CLA 2010-05-18 11:01:54 EDT 
I think it was by design. If you use per file scanner discovery you parse -I -D options from compiler command and by definition those are not builtins. Not saying that design is good but could you elaborate how built-in compiler options could be per file?
Comment 3 Doug Schaefer CLA 2010-05-18 11:14:07 EDT 
The built-ins can be different per file, e.g., if you use different -m arguments. But, yeah, generally they are the same. But then you only need to calculate them per CCommandDSC for which there should only be a few, where few is close to one.

And either way, per file discovery doesn't pick them up at the project level either. I get nothing.

What I am seeing is the CommonBuilder kicking off the ScannerConfigBuilder to updateScannerConfiguration which does PerFileSICollector.applyFileDeltas(). So when the real ScannerConfigBuilder comes along (and only on full builds for some reason) to run the SCDMakefileGenerator to calculate the built-ins, it never sees the new commands.

The right answer, I guess, would be for the applyFileDeltas step to calculate the built-ins for the new commands. Or to run the SCDMakefileGenerator before the applyFileDeltas.
Comment 4 Doug Schaefer CLA 2010-05-18 16:50:00 EDT 
Created attachment 169017 [details]
Scheduled patch.

I'm about to commit the following fix. This undoes moving the call to the ScannerConfigBuilder to the CommonBuilder and turns the scanner config builder back on for incremental builds. This allows the built-ins to get collected for new commands.

I need to understand the affects of that. What problem were we trying to solve by running the sc builder less. We need to make sure any changes done to scanner discovery work for all profiles, or fix the profiles to work better.
Comment 5 Andrew Gvozdev CLA 2010-05-18 17:05:57 EDT 
-1 for turning ScannerConfigBuilder on INCREMENTAL_BUILD by default. It is pretty expensive to run the external command every build. In most cases it is enough to run it just on FULL_BUILD event (which is run after CLEAN_BUILD or as a first build). If you really need it you can tick it on in Properties->Builders->"Scanner Configuration Builder"->Edit->"During manual builds". See bug 306708.
Comment 6 James Blackburn CLA 2010-05-18 17:55:17 EDT 
(In reply to comment #5)
> -1 for turning ScannerConfigBuilder on INCREMENTAL_BUILD by default. It is
> pretty expensive to run the external command every build. In most cases it is

non managed builds, including the scanner discovery part, should now run with a null scheduling rule.  The result is that even if it takes some time, the workspace isn't locked so the performance impact should be comparatively negligible compared with 6.0.
Comment 7 Doug Schaefer CLA 2010-05-18 21:56:14 EDT 
(In reply to comment #5)
> -1 for turning ScannerConfigBuilder on INCREMENTAL_BUILD by default. It is
> pretty expensive to run the external command every build. In most cases it is
> enough to run it just on FULL_BUILD event (which is run after CLEAN_BUILD or as
> a first build). If you really need it you can tick it on in
> Properties->Builders->"Scanner Configuration Builder"->Edit->"During manual
> builds". See bug 306708.

-1 for turning it off in the first place and breaking gcc per file scanner discovery :). It only works if it is on. Testing of the previous change should have shown that.

The scanner config builder should only do things if there are new built-ins to calculate, which is rare. This is the case with gcc per file. If performance is an issue, then we should be looking at fixing that.
Comment 8 Andrew Gvozdev CLA 2010-05-18 23:59:28 EDT 
(In reply to comment #7)
> (In reply to comment #5)
> -1 for turning it off in the first place and breaking gcc per file scanner
> discovery :). It only works if it is on. Testing of the previous change should
> have shown that.
Hmm, per-file was working for CommonBuilder, I tested it after James pointed it out. Maybe I missed a case.

> The scanner config builder should only do things if there are new built-ins to
> calculate, which is rare. This is the case with gcc per file. If performance is
> an issue, then we should be looking at fixing that.
bug 306708 is about per project discovery. After you backed the change it will run "g++ -E -P -v -dD specs.cpp" needlessly every build. Dunno about null scheduling rule, it still pops up progress dialog for each language for a few seconds. Besides, as James mentioned in bug 306708 c#12, even per-file discovery won't work correctly as ScannerConfigBuilder is not configurations aware.

We need to find a solution that accommodates both needs, per file and per project, I don't think they should be in odds with each other.
Comment 9 Doug Schaefer CLA 2010-05-19 10:38:58 EDT 
(In reply to comment #8)
> Hmm, per-file was working for CommonBuilder, I tested it after James pointed it
> out. Maybe I missed a case.

Yup, create an empty project, then add a file. When you add the file, the spec parser never runs.

> bug 306708 is about per project discovery. After you backed the change it will
> run "g++ -E -P -v -dD specs.cpp" needlessly every build. Dunno about null
> scheduling rule, it still pops up progress dialog for each language for a few
> seconds. Besides, as James mentioned in bug 306708 c#12, even per-file
> discovery won't work correctly as ScannerConfigBuilder is not configurations
> aware.

Well, if that process produces the same result all the time, we should be stopping it. There is code in the per file discovery that prevents it from running if there are no changes. The per project needs to do the same.
 
> We need to find a solution that accommodates both needs, per file and per
> project, I don't think they should be in odds with each other.

Right, the builder was put there for a reason and it is expected that it runs on all builds. We just need to make sure it isn't doing work that it doesn't need to do. Just turning it off isn't the right solution.
Comment 10 Chris Recoskie CLA 2010-05-19 10:49:22 EDT 
(In reply to comment #9)
> > After you backed the change it will
> > run "g++ -E -P -v -dD specs.cpp" needlessly every build. Dunno about null
> > scheduling rule, it still pops up progress dialog for each language for a few
> > seconds. Besides, as James mentioned in bug 306708 c#12, even per-file
> > discovery won't work correctly as ScannerConfigBuilder is not configurations
> > aware.
> Well, if that process produces the same result all the time, we should be
> stopping it. There is code in the per file discovery that prevents it from
> running if there are no changes. The per project needs to do the same.

The problem though is that the compiler you are using might change from build to build, which might mean different info will be discovered.  The problem is that it's hard to know if it's different until after you've done it.

Scanner discovery build commands can be the values of macros or environment variables, and as well all know, environment variables can change at any time, without the IDE really knowing about it unless it actively polls all the environment variables before every build and looks for changes in ones that appear in build commands.  This can be done but is a bit ugly as you might have nested macros and so you have to re-evaluate the build command multiple times.
Comment 11 Doug Schaefer CLA 2010-05-19 14:37:19 EDT 
(In reply to comment #10)
> The problem though is that the compiler you are using might change from build
> to build, which might mean different info will be discovered.  The problem is
> that it's hard to know if it's different until after you've done it.

The build output parsing should be able to pick out that there's a new compiler and signal to the scanner config builder that it needs to run.
 
> Scanner discovery build commands can be the values of macros or environment
> variables, and as well all know, environment variables can change at any time,
> without the IDE really knowing about it unless it actively polls all the
> environment variables before every build and looks for changes in ones that
> appear in build commands.  This can be done but is a bit ugly as you might have
> nested macros and so you have to re-evaluate the build command multiple times.

If your compiler is affected by these things without affecting the build output, then, yes, you would need to run all the time, or live with the inconsistencies until a rebuild is done, assuming we can change the behavior in FULL versus INCREMENTAL.
Comment 12 Andrew Gvozdev CLA 2010-05-19 22:07:47 EDT 
(In reply to comment #9)
> Right, the builder was put there for a reason and it is expected that it runs on
> all builds. We just need to make sure it isn't doing work that it doesn't need
> to do. Just turning it off isn't the right solution.
Alright, I reopen bug 306708 then. Just turning it back on doesn't work that great either.

(In reply to comment #10)
> The problem though is that the compiler you are using might change from build to
> build, which might mean different info will be discovered.  The problem is that
> it's hard to know if it's different until after you've done it.
If you change compiler you really want to run FULL_BUILD that's for sure or you are asking for trouble.
Comment 13 Doug Schaefer CLA 2010-05-19 22:19:43 EDT 
(In reply to comment #12)
> Alright, I reopen bug 306708 then. Just turning it back on doesn't work that
> great either.

Agreed. Thanks!

> (In reply to comment #10)
> > The problem though is that the compiler you are using might change from build to
> > build, which might mean different info will be discovered.  The problem is that
> > it's hard to know if it's different until after you've done it.
> If you change compiler you really want to run FULL_BUILD that's for sure or you
> are asking for trouble.
Comment 15 Andrew Gvozdev CLA 2012-05-30 09:35:13 EDT 
I assume this was fixed with the commit from comment 14. Closing the bug.