[Clang] Improve diagnostics about the invalid target feature.

[MTC]

Summary

Clang with debug builds will crash when run with empty target feature input. And the warning message is a little bit confusing. This patch adds an empty check and adds a new diagnostic to illustrate where goes wrong.

Before:

// clang will crash or emit '-' is not a recognized feature for this target (ignoring feature)
clang -cc1 -target riscv64-unknown-elf -target-feature '' ./test.c
// clang will emit '-' is not a recognized feature for this target (ignoring feature)
clang -cc1 -target riscv64-unknown-elf -target-feature m ./test.c

Now:

// clang will ignoring the empty input
clang -cc1 -target riscv64-unknown-elf -target-feature '' ./test.c
// clang will emit warning: feature flags 'm' should start with '+' or '-'
clang -cc1 -target riscv64-unknown-elf -target-feature m ./test.c

[tbaeder]

Some comments:

1. I think F should also be trim()ed (or was it strip()?) before the empty() check, otherwise you run into the exact same problem when someone supplies ' '.
2. The diagnostic could also mention what the + and - prefixes mean and what happened now that none has been supplied. E.g. "feature flag 'm' needs to start with either '+' to enable the feature or '-' to disable it. Ignoring it for now."
3. I think "should" is incorrect for the diagnostic and it should(heh) be either "must start" or "needs to start". Since this is a hard requirement.

[MTC]

Improve the diagnostic message and trim the spaces around the feature flags.

[MTC]

Appreciate your comments!

In D133563#3779599, @tbaeder wrote:

Some comments:

1. I think F should also be trim()ed (or was it strip()?) before the empty() check, otherwise you run into the exact same problem when someone supplies ' '.

Updated.

2. The diagnostic could also mention what the + and - prefixes mean and what happened now that none has been supplied. E.g. "feature flag 'm' needs to start with either '+' to enable the feature or '-' to disable it. Ignoring it for now."

Thanks for the tips, it looks better now.

Actually, it is hard for users to access the target feature directly and trigger this bug easily. I just found the problem when running clang with the frontend flags directly, i.e. through 'cc1' or 'Xclang'. Therefore, I am still hesitant about whether it is worth adding a new diagnostic kind.

[aaron.ballman]

Thank you for the fix! I think the new diagnostic is helpful because there is evidence that people will use these frontend target features somewhat commonly: https://sourcegraph.com/search?q=context:global+-Xclang+-target-feature+-file:.*test.*&patternType=standard but I do agree that it's a bit on the edge; we don't usually issue diagnostics for invalid frontend options. Because of that, I don't think we need a release note for this fix, this is more of a kindness than something we want to advertise to users.

Precommit CI found a valid failure:

******************** TEST 'Clang :: Misc/warning-flags.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   diagtool list-warnings > /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Misc/Output/warning-flags.c.tmp 2>&1
: 'RUN: at line 2';   /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --input-file=/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Misc/Output/warning-flags.c.tmp /var/lib/buildkite-agent/builds/llvm-project/clang/test/Misc/warning-flags.c
--
Exit Code: 1
 
Command Output (stderr):
--
/var/lib/buildkite-agent/builds/llvm-project/clang/test/Misc/warning-flags.c:21:8: error: CHECK: expected string not found in input
CHECK: Warnings without flags (66):
       ^
/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Misc/Output/warning-flags.c.tmp:1:1: note: scanning from here
Warnings with flags (1764):
^
 
Input file: /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Misc/Output/warning-flags.c.tmp
Check file: /var/lib/buildkite-agent/builds/llvm-project/clang/test/Misc/warning-flags.c
 
-dump-input=help explains the following input dump.
 
Input was:
<<<<<<
          1: Warnings with flags (1764):
check:21     X~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
          2:  backslash_newline_space [-Wbackslash-newline-escape]
check:21     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          3:  err_func_returning_qualified_void [-Wqualified-void-return-type]
check:21     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          4:  escaped_newline_block_comment_end [-Wcomment]
check:21     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          5:  ext_abstract_pack_declarator_parens [-Wanonymous-pack-parens]
check:21     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          6:  ext_adl_only_template_id [-Wc++20-extensions]
check:21     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          .
          .
          .
>>>>>>
 
--
 
********************

clang/include/clang/Basic/DiagnosticCommonKinds.td

330–331 ↗

(On Diff #459407)

You should also wrap this to roughly 80 columns and put it into a diagnostic group (I think InvalidCommandLineArgument is the correct group for this to use). Also, this diagnostic is specific to the frontend, so it should live in DiagnosticFrontendKinds.td instead of DiagnosticCommonKinds.td (we use the common file for diagnostics that are triggered across multiple components).

[MaskRay]

clang/test/Driver/invalid-target-feature.c

1 ↗

(On Diff #459407)

-target (legacy) => --target= for new tests.

---

That said, I feel that test/Driver is a somewhat awkward location as this doesn't really use driver features.
Perhaps test/Frontend and switch to %clang_cc1?

[MTC]

• Fix the clang/test/Misc/warning-flags.c failure pointed out by Aaron
• Move the test file to clang/Frontend as MaskRay suggested.

[MaskRay]

clang/include/clang/Basic/DiagnosticCommonKinds.td

330–331 ↗

(On Diff #459407)

Aaron's suggestion is that you place the warning in InvalidCommandLineArgument (-Winvalid-command-line-argument) so that you will not need to bump The list of warnings below should NEVER grow. It should gradually shrink to 0.

clang/lib/Basic/TargetInfo.cpp

498

trim is probably not the responsibility of cc1. cc1 can assume that the options are already in some checked forms.

[MTC]

@MaskRay Very much appreciate your reminder! I missed Aaron's comments.

• Put the flag into DiagnosticFrontendKinds.td and rename it to warn_fe_backend_invalid_feature_flag
• Remove the unneeded trim() call.

Btw: Given that I have put the flag into DiagnosticFrontendKinds.td, I have to include DiagnosticFrontend.h to enable it.

[aaron.ballman]

LGTM!

clang/include/clang/Basic/DiagnosticFrontendKinds.td

def warn_fe_override_module : Warning<
     "overriding the module target triple with %0">,
     InGroup<DiagGroup<"override-module">>;
 def warn_fe_backend_unsupported_fp_rounding : Warning<
     "overriding currently unsupported rounding mode on this target">,
     InGroup<UnsupportedFPOpt>;
 def warn_fe_backend_unsupported_fp_exceptions : Warning<
     "overriding currently unsupported use of floating point exceptions "
     "on this target">, InGroup<UnsupportedFPOpt>;
+def warn_fe_backend_invalid_feature_flag : Warning<
+    "feature flag '%0' must start with either '+' to enable the feature or '-'"
+    " to disable it; flag ignored">, InGroup<InvalidCommandLineArgument>;
 
 def err_incompatible_fp_eval_method_options : Error<
     "option 'ffp-eval-method' cannot be used with option "
     "%select{'fapprox-func'|'mreassociate'|'freciprocal'}0">;
 
 def remark_fe_backend_optimization_remark : Remark<"%0">, BackendInfo,
     InGroup<BackendOptimizationRemark>;
 def remark_fe_backend_optimization_remark_missed : Remark<"%0">, BackendInfo,

clang/lib/Basic/TargetInfo.cpp

 //===--- TargetInfo.cpp - Information about Target machine ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the TargetInfo and TargetInfoImpl interfaces.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetParser.h"
 #include <cstdlib>
 using namespace clang;
 
if (Opts.MaxBitIntWidth)
     MaxBitIntWidth = Opts.MaxBitIntWidth;
 }
 
 bool TargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeatureVec) const {
   for (const auto &F : FeatureVec) {
     StringRef Name = F;
+    if (Name.empty())
+      continue;
     // Apply the feature via the target.
-    bool Enabled = Name[0] == '+';
-    setFeatureEnabled(Features, Name.substr(1), Enabled);
+    if (Name[0] != '+' && Name[0] != '-')
+      Diags.Report(diag::warn_fe_backend_invalid_feature_flag) << Name;
+    else
+      setFeatureEnabled(Features, Name.substr(1), Name[0] == '+');
   }
   return true;
 }
 
 TargetInfo::CallingConvKind
 TargetInfo::getCallingConvKind(bool ClangABICompat4) const {
   if (getCXXABI() != TargetCXXABI::Microsoft &&
       (ClangABICompat4 || getTriple().isPS4()))

clang/test/Frontend/invalid-target-feature.c

+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature '' -target-feature m %s 2>&1 \
+// RUN:    | FileCheck -check-prefix=NO_PREFIX %s
+
+// NO_PREFIX: warning: feature flag 'm' must start with either '+' to enable the feature or '-' to disable it; flag ignored [-Winvalid-command-line-argument]
+
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature '' -target-feature ' n' %s 2>&1 \
+// RUN:    | FileCheck -check-prefix=NULL_PREFIX %s
+
+// NULL_PREFIX: warning: feature flag ' n' must start with either '+' to enable the feature or '-' to disable it; flag ignored [-Winvalid-command-line-argument]