Page MenuHomePhabricator

[clang][deps] NFC: Preserve the original frontend action
ClosedPublic

Authored by jansvoboda11 on Jun 1 2021, 7:19 AM.

Details

Summary

This patch stops adjusting the frontend action when clang-scan-deps is configured to use the full output format.

In a future patch, the dependency scanner needs to check whether the original compiler invocation builds a PCH. That's impossible when -Eonly et al. override -emit-pch.

The -Eonly flag is not needed - the dependency scanner explicitly sets up its own frontend action anyways.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Jun 1 2021, 7:19 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2021, 7:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Is there (or could there be) a mode where clang-scan-deps prints out the command-lines it sends to the dependency scanning action (maybe instead of actually scanning), so this could be tested?

clang/tools/clang-scan-deps/ClangScanDeps.cpp
442–443

Nit: blank line before the comment might make it easier to read; also the second line seems to have an indent, is that intentional?

Fix whitespace around fixme

Is there (or could there be) a mode where clang-scan-deps prints out the command-lines it sends to the dependency scanning action (maybe instead of actually scanning), so this could be tested?

The command-line adjustments are an implementation detail and there's a bunch of tests checking the make output format that exercise the code. So I'm not sure there's much value in explicitly testing the transformations. There's currently no way to print the adjusted command-line.

My reason for the FIXME is that we could get rid of bunch of Windows-specific logic by adjusting CompilerInvocation instead.

clang/tools/clang-scan-deps/ClangScanDeps.cpp
442–443

The indent is intentional, my IDE doesn't pick up unindented lines part of the FIXME. I've removed it so it's more in line with the rest of the codebase.

jansvoboda11 retitled this revision from [clang][deps] NFC: Do not adjust the original action to [clang][deps] NFC: Preserve the original frontend action.Jun 2 2021, 4:46 AM
jansvoboda11 edited the summary of this revision. (Show Details)
dexonsmith accepted this revision.Jun 13 2021, 1:05 PM

Okay, LGTM. (Sorry for the delay, I've been out.)

My reason for the FIXME is that we could get rid of bunch of Windows-specific logic by adjusting CompilerInvocation instead.

Please add that motivation to the FIXME itself.

This revision is now accepted and ready to land.Jun 13 2021, 1:05 PM

Okay, LGTM. (Sorry for the delay, I've been out.)

My reason for the FIXME is that we could get rid of bunch of Windows-specific logic by adjusting CompilerInvocation instead.

Please add that motivation to the FIXME itself.

No problem, thanks for the review(s)!

bmahjour added a subscriber: bmahjour.EditedJun 25 2021, 4:05 PM

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

Clang :: ClangScanDeps/headerwithdirname.cpp
Clang :: ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

The reason seems to be related to the fact that -fno-integrated-as is on by default on that platform. I get the same failure on Linux if I change the "compilation database" file to add -fno-integrated-as to the "command" line:

> /build_llvm/bin/clang-scan-deps -compilation-database ./headerwithdirname.cpp.tmp.cdb -j 1
> Error while scanning dependencies for /build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/headerwithdirname_input.cpp:
error: unable to handle compilation, expected exactly one compiler job in ' "clang" "-cc1" "-triple" "powerpc64le-unknown-linux-gnu" "-S" ...;  "/usr/bin/as" "-a64" "-mppc64" "-mlittle-endian" "-mpower8" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/foodir" "-I" "Inputs" "-o" "headerwithdirname_input.o" "/tmp/headerwithdirname_input-2e0050.s"; '

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

I think @jansvoboda11 is out this week (IIRC, back next week). A possible interim workaround would be to add an XFAIL: aix (or whatever the lit feature is for AIX triples)... are you able to land something like that to unblock you? (Seems like the correct fix for -fno-integrated-as could be a bit more complicated...)

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

Clang :: ClangScanDeps/headerwithdirname.cpp
Clang :: ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

The reason seems to be related to the fact that -fno-integrated-as is on by default on that platform. I get the same failure on Linux if I change the "compilation database" file to add -fno-integrated-as to the "command" line:

> /build_llvm/bin/clang-scan-deps -compilation-database ./headerwithdirname.cpp.tmp.cdb -j 1
> Error while scanning dependencies for /build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/headerwithdirname_input.cpp:
error: unable to handle compilation, expected exactly one compiler job in ' "clang" "-cc1" "-triple" "powerpc64le-unknown-linux-gnu" "-S" ...;  "/usr/bin/as" "-a64" "-mppc64" "-mlittle-endian" "-mpower8" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/foodir" "-I" "Inputs" "-o" "headerwithdirname_input.o" "/tmp/headerwithdirname_input-2e0050.s"; '

Thanks for the report and the reproducer. I'll try to get a fix ready tomorrow.

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

Clang :: ClangScanDeps/headerwithdirname.cpp
Clang :: ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

The reason seems to be related to the fact that -fno-integrated-as is on by default on that platform. I get the same failure on Linux if I change the "compilation database" file to add -fno-integrated-as to the "command" line:

> /build_llvm/bin/clang-scan-deps -compilation-database ./headerwithdirname.cpp.tmp.cdb -j 1
> Error while scanning dependencies for /build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/headerwithdirname_input.cpp:
error: unable to handle compilation, expected exactly one compiler job in ' "clang" "-cc1" "-triple" "powerpc64le-unknown-linux-gnu" "-S" ...;  "/usr/bin/as" "-a64" "-mppc64" "-mlittle-endian" "-mpower8" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/foodir" "-I" "Inputs" "-o" "headerwithdirname_input.o" "/tmp/headerwithdirname_input-2e0050.s"; '

Thanks for the report and the reproducer. I'll try to get a fix ready tomorrow.

Fixed here: https://reviews.llvm.org/D105695