[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

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

2

-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

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

499

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/DiagnosticCommonKinds.td

 def err_opt_not_valid_with_opt : Error<
   "option '%0' cannot be specified with '%1'">;
 def err_opt_not_valid_without_opt : Error<
   "option '%0' cannot be specified without '%1'">;
 def err_opt_not_valid_on_target : Error<
   "option '%0' cannot be specified on this target">;
 def err_invalid_feature_combination : Error<
   "invalid feature combination: %0">;
+def warn_invalid_feature_flags : Warning<
+  "feature flags '%0' should start with '+' or '-'">;
 
 // Source manager
 def err_cannot_open_file : Error<"cannot open file '%0': %1">, DefaultFatal;
 def err_file_modified : Error<
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
 def err_include_too_large : Error<

clang/lib/Basic/TargetInfo.cpp

void TargetInfo::adjust(DiagnosticsEngine &Diags, LangOptions &Opts) {
   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) {
+    if (F.empty())
+      continue;
     StringRef Name = F;
     // 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_invalid_feature_flags) << 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/Driver/invalid-target-feature.c

+// RUN: %clang -c -target aarch64 -Xclang -target-feature -Xclang '' -Xclang -target-feature -Xclang m %s 2>&1 \
+// RUN:    | FileCheck --check-prefix=XCLANG %s
+
+// XCLANG: warning: feature flags 'm' should start with '+' or '-'
+//