[Clang][Driver] Warn about -c/-S with -fsyntax-only
#98607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Dmitriy Chestnykh (chestnykh) ChangesEmit warning that Full diff: https://github.com/llvm/llvm-project/pull/98607.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 4a674f67b8e1b..4440725ccd923 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -794,10 +794,6 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
Args.hasArg(options::OPT_coverage))
FProfileDir = Args.getLastArg(options::OPT_fprofile_dir);
- // TODO: Don't claim -c/-S to warn about -fsyntax-only -c/-S, -E -c/-S,
- // like we warn about -fsyntax-only -E.
- (void)(Args.hasArg(options::OPT_c) || Args.hasArg(options::OPT_S));
-
// Put the .gcno and .gcda files (if needed) next to the primary output file,
// or fall back to a file in the current directory for `clang -c --coverage
// d/a.c` in the absence of -o.
diff --git a/clang/test/Driver/warn-fsyntax-only-c-S.c b/clang/test/Driver/warn-fsyntax-only-c-S.c
new file mode 100644
index 0000000000000..bb1cd5fd1183c
--- /dev/null
+++ b/clang/test/Driver/warn-fsyntax-only-c-S.c
@@ -0,0 +1,9 @@
+// RUN: %clang -fsyntax-only -S %s 2>&1 | FileCheck %s --check-prefix=CHECK-ASM
+// RUN: %clang -fsyntax-only -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-OBJ
+// RUN: %clang -fsyntax-only -S -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOTH
+
+// CHECK-ASM: warning: argument unused during compilation: '-S' [-Wunused-command-line-argument]
+// CHECK-OBJ: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument]
+
+// CHECK-BOTH: warning: argument unused during compilation: '-S' [-Wunused-command-line-argument]
+// CHECK-NEXT: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument]
|
|
CC: @shafik @ChuanqiXu9 |
Emit warning that `-S` and/or `-c` arguments are not used if `-fsyntax-only` is also passed to clang `addPGOAndCoverageFlags` is not the right place to produce this warning Now `-fsyntax-only -c/-S` combination handles like `-fsyntax-only -E` in `BuildJobs()` driver function
MaskRay
reviewed
Jul 15, 2024
| @@ -794,10 +794,6 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C, | |||
| Args.hasArg(options::OPT_coverage)) | |||
| FProfileDir = Args.getLastArg(options::OPT_fprofile_dir); | |||
|
|
|||
| // TODO: Don't claim -c/-S to warn about -fsyntax-only -c/-S, -E -c/-S, | |||
There was a problem hiding this comment.
Thanks for catching this. I left this in a07b135 to be cleaned up later but I forgot to remove this TODO.
MaskRay
reviewed
Jul 15, 2024
| @@ -0,0 +1,9 @@ | |||
| // RUN: %clang -fsyntax-only -S %s 2>&1 | FileCheck %s --check-prefix=CHECK-ASM | |||
There was a problem hiding this comment.
Perhaps just warn-fsyntax-only.c
| // CHECK-OBJ: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument] | ||
|
|
||
| // CHECK-BOTH: warning: argument unused during compilation: '-S' [-Wunused-command-line-argument] | ||
| // CHECK-NEXT: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument] |
There was a problem hiding this comment.
CHECK-NEXT is not used by any FileCheck command
There was a problem hiding this comment.
I've added -fsyntax-only -E case. -c -S i think is like duplication of -c and -S test cases
- Rename to warn-fsyntax-only.c - Add `-fsyntax-only -E` testcase - Remove `-c -S` case because we really don't need it
MaskRay
approved these changes
Jul 22, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/2918 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/2069 Here is the relevant piece of the build log for the reference: |
|
@chestnykh Can you take a look at the bot failures and revert if you need time to investigate so we can get the bots back to green? |
dyung
added a commit
that referenced
this pull request
Jul 23, 2024
…)" This reverts commit 1efcc53.
dyung
added a commit
that referenced
this pull request
Jul 23, 2024
…0052) Reverts #98607 The test added was failing on some build bots: - https://lab.llvm.org/buildbot/#/builders/144/builds/2918 - https://lab.llvm.org/buildbot/#/builders/46/builds/2069
sgundapa
pushed a commit
to sgundapa/upstream_effort
that referenced
this pull request
Jul 23, 2024
Emit warning that `-S` and/or `-c` arguments are not used if `-fsyntax-only` is also passed to clang `addPGOAndCoverageFlags` is not the right place to produce this warning Now `-fsyntax-only -c/-S` combination handles like `-fsyntax-only -E` in `BuildJobs()` driver function
sgundapa
pushed a commit
to sgundapa/upstream_effort
that referenced
this pull request
Jul 23, 2024
…m#100052) Reverts llvm#98607 The test added was failing on some build bots: - https://lab.llvm.org/buildbot/#/builders/144/builds/2918 - https://lab.llvm.org/buildbot/#/builders/46/builds/2069
|
FWIW, in addition to That sounds like a reasonable thing to do to me, so it's probably fine, but it'd be nice to have that aspect acknowledged as one intended behaviour change as well. |
mstorsjo
added a commit
to mstorsjo/gas-preprocessor
that referenced
this pull request
Jul 25, 2024
A command like "cc -c -E" is tautological; the -c is ignored, when we explicitly specify that we want to preprocess only. Since llvm/llvm-project@6461e53 and llvm/llvm-project#98607, Clang now warns about the unused "-c" argument in this case. We already did omit the "-c" argument when preprocessing (with cl.exe) for armasm, but do this for other cases as well.
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: Emit warning that `-S` and/or `-c` arguments are not used if `-fsyntax-only` is also passed to clang `addPGOAndCoverageFlags` is not the right place to produce this warning Now `-fsyntax-only -c/-S` combination handles like `-fsyntax-only -E` in `BuildJobs()` driver function Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251113
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
…0052) Reverts #98607 The test added was failing on some build bots: - https://lab.llvm.org/buildbot/#/builders/144/builds/2918 - https://lab.llvm.org/buildbot/#/builders/46/builds/2069
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: Remove the TODO I left in commit a07b135. We will now warn about `-c/-S` with `-fsyntax-only`. This relands #98607 with a specific target triple. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250579
Thanks for pointing this out. Yes, the warning for |
mstorsjo
added a commit
to FFmpeg/gas-preprocessor
that referenced
this pull request
Jul 29, 2024
A command like "cc -c -E" is tautological; the -c is ignored, when we explicitly specify that we want to preprocess only. Since llvm/llvm-project@6461e53 and llvm/llvm-project#98607, Clang now warns about the unused "-c" argument in this case. We already did omit the "-c" argument when preprocessing (with cl.exe) for armasm, but do this for other cases as well.
MaskRay
added a commit
that referenced
this pull request
Jul 30, 2024
compile_commands.json entries often have -c. When adding -fsyntax-only, we should remove -c to prevent the following warning: ``` % clang -c -fsyntax-only a.c clang: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument] ``` Previously, -c and -S were inappropriately claimed in `addPGOAndCoverageFlags` (see the workaround added by commit a07b135). Now the workaround have been removed (#98607). (clangDriver reports a -Wunused-command-line-argument diagnostic for each unclaimed option.) Fix #100909 Pull Request: #101103
Harini0924
pushed a commit
to Harini0924/llvm-project
that referenced
this pull request
Aug 1, 2024
Remove the TODO I left in commit a07b135. We will now warn about `-c/-S` with `-fsyntax-only`. This relands llvm#98607 with a specific target triple.
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this pull request
Aug 7, 2024
compile_commands.json entries often have -c. When adding -fsyntax-only, we should remove -c to prevent the following warning: ``` % clang -c -fsyntax-only a.c clang: warning: argument unused during compilation: '-c' [-Wunused-command-line-argument] ``` Previously, -c and -S were inappropriately claimed in `addPGOAndCoverageFlags` (see the workaround added by commit a07b135). Now the workaround have been removed (llvm#98607). (clangDriver reports a -Wunused-command-line-argument diagnostic for each unclaimed option.) Fix llvm#100909 Pull Request: llvm#101103
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Emit warning that
-Sand/or-carguments are not used if-fsyntax-onlyis also passed to clangaddPGOAndCoverageFlagsis not the right placeto produce this warning
Now
-fsyntax-only -c/-Scombination handles like-fsyntax-only -EinBuildJobs()driver function