icecc / icecream

Distributed compiler with a central scheduler to share build load
GNU General Public License v2.0
1.61k stars 252 forks source link

Don't remove -fdirectives-only when passed explicitly as argument #607

Closed RedBeard0531 closed 2 years ago

RedBeard0531 commented 2 years ago

This code (introduced by https://github.com/icecc/icecream/commit/968fca48dfbac0bad25e215960a4faf04376b423) unconditionally removes -fdirectives-only, even when it is passed on the command-line. This can lead to failures when icecc is compiling a blah.ii file that was produced with -fdirectives-only -E, because gcc doesn't know that it still needs to do the second half of preprocessing.

This scenario can pop up when using icecc with ccache and the CCACHE_NOCPP2=1 setting to tell cache to pass the preprocessed output to icecc. This is normally not valid, however if you explicitly pass -fdirectives-only (or -frewrite-includes for clang) it is. This can result in a significant performance improvement with cached distributed builds because preprocessing takes up a lot of the CPU cycles locally, and NOCPP2 reduces preprocessing by half (or more when combined with -fdirectives-only).

I suggest only removing -fdirectives-only if it was added by icecc, not if it was passed explicitly. Another option is to never remove it if the input file was already preprocessed because that is more clearly wrong, since the input requires that flag.

Repro:

test.cpp (should compile with warning)

#define FOO int
FOO foo() {} // no return to force warning

command-line: ICECC_PREFERRED_HOST=SOME_REMOTE_HOST ICECC_CARET_WORKAROUND=1 ICECC_DEBUG=debug CCACHE_LOGFILE=/dev/stderr CCACHE_PREFIX=icecc CCACHE_NOCPP2=1 ccache g++ -Wall -c test.cpp -fdirectives-only

selected output:

[2022-09-16T12:15:01.600485 23306] Command line: ccache g++ -Wall -c test.cpp -fdirectives-only
[2022-09-16T12:15:01.600690 23306] Executing /usr/bin/g++ -Wall -fdirectives-only -E test.cpp
[2022-09-16T12:15:01.606276 23306] Executing /usr/bin/icecc /usr/bin/g++ -Wall -fpreprocessed -fdirectives-only -c -o test.o /home/ubuntu/.ccache/tmp/test.stdout.ip-XXX.23306.NtFj8o.ii
ICECC[23310] 2022-09-16 12:15:01: invoked as: /usr/bin/icecc /usr/bin/g++ -Wall -fpreprocessed -fdirectives-only -c -o test.o /home/ubuntu/.ccache/tmp/test.stdout.ip-XXX.23306.NtFj8o.ii
ICECC[23310] 2022-09-16 12:15:01: command needs stdout/stderr workaround, recompiling locally
ICECC[23310] 2022-09-16 12:15:01: (set ICECC_CARET_WORKAROUND=0 to override)
ICECC[23310] 2022-09-16 12:15:01: local build forced by remote exception: Error 102 - command needs stdout/stderr workaround, recompiling locally
ICECC[23310] 2022-09-16 12:15:01: <building_local>
ICECC[23310] 2022-09-16 12:15:01: invoking: /usr/bin/g++ -Wall -fpreprocessed -c /home/ubuntu/.ccache/tmp/test.stdout.ip-XXX.23306.NtFj8o.ii -o test.o
ICECC[23310] 2022-09-16 12:15:01: </building_local: 7ms>
[2022-09-16T12:15:01.626417 23306] Compiler gave exit status 1
test.cpp:2:1: error: 'FOO' does not name a type
 FOO foo() {} // no return to force warning
 ^~~

Note that the local rebuild does not have -fdirectives-only so GCC does not know that it needs to do macro expansion of FOO to int, and that causes the compile to fail. In the real-world failure this results in many thousands of lines of error output. This repro avoids any #includes to minimize the output and make it obvious what is going wrong.

For comparison, this is the output of g++ -Wall -c test.cpp -fdirectives-only -E -o /tmp/test.ii ; g++ -Wall -c test.ii -fdirectives-only:

test.cpp: In function 'int foo()':
test.cpp:2:12: warning: no return statement in function returning non-void [-Wreturn-type]
 FOO foo() {} // no return to force warning
            ^
RedBeard0531 commented 2 years ago

Sorry an earlier version of the repro had a typo while I was trying to minimize it. Now updated to be correct.

deriamis commented 2 years ago

This is the same issue I raised last year in #550, which needed the repro steps and a suggested fix that you've helpfully provided here. I'm closing this issue as a duplicate of #550 so we have an accurate idea of when this issue was first raised. Please go subscribe to #550 for update notifications on this problem.