[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

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

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

497

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 flag '%0' needs to start with either '+' to enable the feature or '-' to disable it; ignoring it for now">;
 
 // 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

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;
+    Name = Name.trim();
+    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_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/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' needs to start with either '+' to enable the feature or '-' to disable it; ignoring it for now
+
+// 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' needs to start with either '+' to enable the feature or '-' to disable it; ignoring it for now

clang/test/Misc/warning-flags.c

(2) It prevents us adding new warnings to Clang that have no -W flag.  All
     new warnings should have -W flags.
 
     If you add a new warning without a flag, this test will fail.  To fix
     this test, simply add a warning group to that warning.
 
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (66):
+CHECK: Warnings without flags (67):
 
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex
 CHECK-NEXT:   ext_template_arg_extra_parens
 CHECK-NEXT:   ext_typecheck_cond_incompatible_operands

 CHECK-NEXT:   warn_fe_backend_unsupported
 CHECK-NEXT:   warn_fe_cc_log_diagnostics_failure
 CHECK-NEXT:   warn_fe_cc_print_header_failure
 CHECK-NEXT:   warn_fe_macro_contains_embedded_newline
 CHECK-NEXT:   warn_ignoring_ftabstop_value
 CHECK-NEXT:   warn_implements_nscopying
 CHECK-NEXT:   warn_incompatible_qualified_id
 CHECK-NEXT:   warn_invalid_asm_cast_lvalue
+CHECK-NEXT:   warn_invalid_feature_flags
 CHECK-NEXT:   warn_maynot_respond
 CHECK-NEXT:   warn_method_param_redefinition
 CHECK-NEXT:   warn_missing_case_for_condition
 CHECK-NEXT:   warn_missing_dependent_template_keyword
 CHECK-NEXT:   warn_missing_whitespace_after_macro_name
 CHECK-NEXT:   warn_mt_message
 CHECK-NEXT:   warn_no_constructor_for_refconst
 CHECK-NEXT:   warn_not_compound_assign