[Clang][Driver] New parameter allow-unrecognized-arguments #162201
Conversation
This parameter is used to suppress the ``Unknown argument '...'`` error that clang will emit whenever it encounters an unknown argument. This is probably an error to make sure the user fixes it's mistake by either removing the argument or renaming it, but there are some cases where it's not possible to fix the issue. For instance, CMake now injects gcc-specific arguments in the clang-tidy command that breaks static-analysis (https://gitlab.kitware.com/cmake/cmake/-/issues/26283) This will also allow users to run clang-tidy / clangd on a gcc-based project without the need to maintain two separate build commands to run llvm-based tools. By enabling this parameter, the user is able to downgrade the error to a warning (unknown-argument) that he can further silence using the ``-Qunused-arguments`` flag if needed. Fixes: llvm#108455
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Félix-Antoine Constantin (felix642) ChangesThis is another attempt at fixing #108455. Please see the initial discussion in the following PR: #111453 This parameter is used to suppress the This is probably an error to make sure the user fixes it's mistake by either removing the argument or renaming it, but there are some cases where it's not possible to fix the issue. This will also allow users to run clang-tidy / clangd on a gcc-based project without the need to maintain two separate build commands to run llvm-based tools. By enabling this parameter, the user is able to downgrade the error to a warning (unknown-argument) that he can further silence using the Fixes: #108455 Full diff: https://github.com/llvm/llvm-project/pull/162201.diff 4 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 2ef609831637e..de0d32f68253a 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1011,6 +1011,9 @@ def : Flag<["-"], "Xcompiler">, IgnoredGCCCompat;
def Z_Flag : Flag<["-"], "Z">, Group<Link_Group>;
def all__load : Flag<["-"], "all_load">;
def allowable__client : Separate<["-"], "allowable_client">;
+def allow_unrecognized_arguments : Flag<["--"], "allow-unrecognized-arguments">,
+ Visibility<[ClangOption, CLOption]>,
+ HelpText<"Ignore unrecognized command-line arguments instead of reporting an error.">;
def ansi : Flag<["-", "--"], "ansi">, Group<CompileOnly_Group>;
def arch__errors__fatal : Flag<["-"], "arch_errors_fatal">;
def arch : Separate<["-"], "arch">, Flags<[NoXarchOption,TargetSpecific]>;
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 85a1335785542..4f2ac9ba8271c 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -303,29 +303,31 @@ InputArgList Driver::ParseArgStrings(ArrayRef<const char *> ArgStrings,
}
}
- for (const Arg *A : Args.filtered(options::OPT_UNKNOWN)) {
- unsigned DiagID;
- auto ArgString = A->getAsString(Args);
- std::string Nearest;
- if (getOpts().findNearest(ArgString, Nearest, VisibilityMask) > 1) {
- if (!IsCLMode() &&
- getOpts().findExact(ArgString, Nearest,
- llvm::opt::Visibility(options::CC1Option))) {
- DiagID = diag::err_drv_unknown_argument_with_suggestion;
- Diags.Report(DiagID) << ArgString << "-Xclang " + Nearest;
+ if (!Args.hasArg(options::OPT_allow_unrecognized_arguments)) {
+ for (const Arg *A : Args.filtered(options::OPT_UNKNOWN)) {
+ unsigned DiagID;
+ auto ArgString = A->getAsString(Args);
+ std::string Nearest;
+ if (getOpts().findNearest(ArgString, Nearest, VisibilityMask) > 1) {
+ if (!IsCLMode() &&
+ getOpts().findExact(ArgString, Nearest,
+ llvm::opt::Visibility(options::CC1Option))) {
+ DiagID = diag::err_drv_unknown_argument_with_suggestion;
+ Diags.Report(DiagID) << ArgString << "-Xclang " + Nearest;
+ } else {
+ DiagID = IsCLMode() ? diag::warn_drv_unknown_argument_clang_cl
+ : diag::err_drv_unknown_argument;
+ Diags.Report(DiagID) << ArgString;
+ }
} else {
- DiagID = IsCLMode() ? diag::warn_drv_unknown_argument_clang_cl
- : diag::err_drv_unknown_argument;
- Diags.Report(DiagID) << ArgString;
+ DiagID = IsCLMode()
+ ? diag::warn_drv_unknown_argument_clang_cl_with_suggestion
+ : diag::err_drv_unknown_argument_with_suggestion;
+ Diags.Report(DiagID) << ArgString << Nearest;
}
- } else {
- DiagID = IsCLMode()
- ? diag::warn_drv_unknown_argument_clang_cl_with_suggestion
- : diag::err_drv_unknown_argument_with_suggestion;
- Diags.Report(DiagID) << ArgString << Nearest;
+ ContainsError |= Diags.getDiagnosticLevel(DiagID, SourceLocation()) >
+ DiagnosticsEngine::Warning;
}
- ContainsError |= Diags.getDiagnosticLevel(DiagID, SourceLocation()) >
- DiagnosticsEngine::Warning;
}
for (const Arg *A : Args.filtered(options::OPT_o)) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 422375240bab6..0ba29db471617 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -4991,15 +4991,17 @@ bool CompilerInvocation::CreateFromArgsImpl(
Diags.Report(diag::err_drv_missing_argument)
<< Args.getArgString(MissingArgIndex) << MissingArgCount;
- // Issue errors on unknown arguments.
- for (const auto *A : Args.filtered(OPT_UNKNOWN)) {
- auto ArgString = A->getAsString(Args);
- std::string Nearest;
- if (Opts.findNearest(ArgString, Nearest, VisibilityMask) > 1)
- Diags.Report(diag::err_drv_unknown_argument) << ArgString;
- else
- Diags.Report(diag::err_drv_unknown_argument_with_suggestion)
- << ArgString << Nearest;
+ if (!Args.hasArg(options::OPT_allow_unrecognized_arguments)) {
+ // Issue errors on unknown arguments.
+ for (const auto *A : Args.filtered(OPT_UNKNOWN)) {
+ auto ArgString = A->getAsString(Args);
+ std::string Nearest;
+ if (Opts.findNearest(ArgString, Nearest, VisibilityMask) > 1)
+ Diags.Report(diag::err_drv_unknown_argument) << ArgString;
+ else
+ Diags.Report(diag::err_drv_unknown_argument_with_suggestion)
+ << ArgString << Nearest;
+ }
}
ParseFileSystemArgs(Res.getFileSystemOpts(), Args, Diags);
diff --git a/clang/test/Driver/unsupported-option.c b/clang/test/Driver/unsupported-option.c
index 7234e52571582..465f1879b31c8 100644
--- a/clang/test/Driver/unsupported-option.c
+++ b/clang/test/Driver/unsupported-option.c
@@ -32,3 +32,7 @@
// RUN: not %clang -c -Qunused-arguments --target=aarch64-- -mfpu=crypto-neon-fp-armv8 %s 2>&1 \
// RUN: | FileCheck %s --check-prefix=QUNUSED_ARGUMENTS
// QUNUSED_ARGUMENTS: error: unsupported option '-mfpu=' for target 'aarch64--'
+
+// RUN: %clang %s -invalid --allow-unrecognized-arguments -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=UNKNOWN_ARGUMENT
+// UNKNOWN_ARGUMENT: warning: argument unused during compilation: '-invalid'
|
|
@nicovank @carlosgalvezp @AaronBallman @HerrCai0907 @5chmidti @MaskRay |
Thanks for investigating fixes here!
Personally, I'm not a big fan of having a command line option to control diagnostics of command line options. I think command line options are generally processed in a left-to-right order where the last flag "wins", which is kind of awkward with this design. e.g., I think this probably should remain an error. Passing unknown inputs to the compiler and expecting to get a known output is a pretty big ask IMO. That said, maybe others have a different opinion.
IMO, that's a CMake bug; adding an option we have to support forever to work around that behavior is not really ideal. |
|
Thank you for the feedback @AaronBallman. Just to clarify:
Undoubtedly, this was only an example on the reasoning behind this change. But I do think it is annoying from a user's perspective to be unable to use clang-tidy / clangd in those situations. They have to either wait for a fix from (in that case cmake) or us to properly fix the issue. This is not the first time this kind of issue happened and this is definitely not going to be the last one. The main use case I see for this option would be to give the user the ability to run clang-tidy, clangd (or any clang-based tool as a matter of fact) on a gcc project and to get some decents results without having to maintain two separate build commands or to write some convoluted script to transform the gcc-based command line into something that clang will not complain about. If we give the user the ability to run clang-based tools without any changes on they're project would, in my opinion, greatly simplify the integration of those tools in existing projects. It follows the logic of "Make it work and then make it right" and the "Unrecognized argument" is probably one of the only thing keeping them from doing that.
That being said, I'm wondering if it would be better to instead modify the This would limit the scope of this option and would give us the ability to enable it only for the use-cases that we think are needed. |
|
You can use the If we decide to add this functionality, we could consider implementing --start-ignore-unknown-arguments / --end-ignore-unknown-arguments flags. This would be similar to the existing --start-no-unused-arguments / --end-no-unused-arguments flags, which are used to suppress warnings. @mstorsjo Alternative: |
|
Referring to command-line flags which clearly specify the standards documents they're referring to in the flag itself as "gcc-specific arguments" seems disingenuous and unprofessional. The reference to Kitware's proposal is the first result from google. The kitware issue clearly describes how to turn these off in the cmake file:
As I understand it, modules are a pretty fundamental change to the way C++ dependencies work. The
I also see that I also see that my locally installed copy of The linked issue regarding cmake describes these three arguments as problematic: The proposed methodology to directly ignore these inputs seems tantamount to a rejection of the proposals for interoperable module support across build systems.
Given that modules are a standard API, and that clang already supports precisely the functionality needed, it's unclear why this is being described as "gcc-based". Is there context that's not being stated here?
Given that my laptop's clang++ version supports very similar command-line arguments already, I'm curious why clang-tidy wouldn't be able to make use of that support from clang. Is there some deeper issue that stops clang-tidy from being able to support C++20 modules, or from using the code in clang that processes module dependencies? Much like the make jobserver protocol that ninja and cargo now implement, gcc describes a very explicit effort of at least 5 years to get external buy-in for the exact functionality that is being described as "gcc-based" here (https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Module-Mapper.html):
Removing arguments which define dependency relationships from the command line, based upon internal filtering logic that's defined in the specific version of clang-tidy's compiled C++ code and not in the build system, strikes me as a decision which could incur later user requests for an additional flag to cancel out the first flag filtering functionality, if the user ever figures out their dependency issue was because of this filtering logic in the first place. In particular, many build systems such as bazel will cache process executions based upon the command-line arguments, especially if those arguments specify input files for the compiler to read. It would seem more appropriate to make such incompatibilities visible to the build system by writing diagnostics about unsupported flags to a separate output file, instead of subtly changing the semantics of command-line parsing in a data-dependent manner. |
|
FWIW,
This sounds like it could be a solution!
I don't think this would work, because it would need to be added to the GCC compile flags, and then those flags would be unknown to GCC? Regarding modules, I think it's out of scope for this ticket and deserves an RFC/ticket of its own. One example use case I've personally come across is as follows:
The problem I see with simply removing flags is that the code may not be analysed correctly, which may lead to other errors, which people will then ask us to patch, and so on. |
I wasn't aware this was an option. This could definitely be an option to solve the linked issue!
Wouldn't this be the same issue no matter what option we add? GCC will not recognize the flag and complain. Unless the user adds them conditionally based on the compiler they are using.
We're not removing those flags, we are simply downgrading the error to a warning. So there's still some feedback from clang that those flags we're not used.
What I mean by "gcc-specific arrguments" is arguments that are recognized by GCC, but not clang for various reasons. The linked issue mentioned flags related to modules, but this PR is not related to modules specifically. There's a handful of flags that are not compatible with clang. @carlosgalvezp gave another example with some
I agree with that. But the aforementioned flags would not prevent clang from compiling the code if they were ignored, they are simply used as optimization in the final binary. So the fact that clang-tidy refuses to analyze this code is more of a burden on the user knowing that this won't impact the final analysis. As previously mentioned, I do understand why this is a hard error. Clang doesn't know if it needs those flags or not so it's better to let the user fix it than to simply ignore them.
I'm sorry if I misunderstand this part. I don't think I've mentioned adding some internal filtering logic to clang. If we decide to go forward and add an option to ignore some flags this has to be explicitly enabled by the user since this could cause some other issues if this was implicitly enabled. |
The flags would be ignored/removed and not passed to clang-tidy (because it can't understand them). This means clang-tidy would analyze code based on different semantics compared to what GCC is seeing. Depending on which checks are enabled, this may lead to false positives/negatives, which the user would then report to us.
This is not true in the general case, it depends on the flag. I would be inclined to have something like the original PR, i.e. add a Additionally, document that if people use these features, they do so at their own risk and may lead to further errors or false positives/negatives. Once again, a pre-requisite for a correct clang-tidy analysis is that clang can compile the code without errors. |
I think you're right. There's no reason to complexify this. I'll reopen the original PR with some minor tweaks and we'll keep
I think that's fair. I'll add a mention in the documentation that ignoring some flags can incur some false positive / negative so the users are at least aware of that. |
6 participants
This is another attempt at fixing #108455. Please see the initial discussion in the following PR: #111453
This parameter is used to suppress the
Unknown argument '...'error that clang will emit whenever it encounters an unknown argument.This is probably an error to make sure the user fixes it's mistake by either removing the argument or renaming it, but there are some cases where it's not possible to fix the issue.
For instance, CMake now injects gcc-specific arguments in the clang-tidy command that breaks static-analysis
(https://gitlab.kitware.com/cmake/cmake/-/issues/26283)
This will also allow users to run clang-tidy / clangd on a gcc-based project without the need to maintain two separate build commands to run llvm-based tools.
By enabling this parameter, the user is able to downgrade the error to a warning (unknown-argument) that he can further silence using the
-Qunused-argumentsflag if needed.Fixes: #108455