[Clang] remove text extension from diag::err_drv_invalid_value_with_suggestion

[nickdesaulniers]

Summary

This hinders translations, as per:
https://clang.llvm.org/docs/InternalsManual.html#the-format-string

[MaskRay]

@rtrieu Do we have a way appending arbitrary messages to a diagnostic template?

[rtrieu]

In D101387#2720500, @MaskRay wrote:

@rtrieu Do we have a way appending arbitrary messages to a diagnostic template?

Not an arbitrary number. The diagnostic format string is indexed, so it diagnostic string needs to know ahead of time how many arguments that will be passed to it.

The format types can be nested so you can do:

"Some types: %select{|%1|%1 %2|%1 %2 %3|%1 %2 %3 and more}0"

Then you can pass {0,1,2,3} first, then follow with (0,1,2,3) strings. And passing 4 first with 3 strings gets the "and more" attached to the end. Otherwise, the string concatenation will need to be done before passing to the diagnostic.

For this case, I think to capture the variations used, we could use:

"invalid value '%1' in '%0', %select{|for %3,}2 $plural{0:value must be %5|[1,4]:valid argument for '%0' are:%select{|%5|%5 %6|%5 %6 %7|%5 %6 %7 %8}4}4"

%0 - flag name
%1 - invalid value
%2 - if true, add extra "for %3"
%3 - string for extra "for", needed but ignored when %2 is false
%4 - count for valid arguments. If 0, arbitrary string after "value must be"
%5-%8 - valid arguments

This supports up to 4 types and an arbitrary string. Unifying the flang and clang diagnostics could simplify the string a bit more.

[rsmith]

Note that, per https://clang.llvm.org/docs/InternalsManual.html#the-format-string,

Diagnostics should never take random English strings as arguments: you shouldn’t use “you have a problem with %0” and pass in things like “your argument” or “your return value” as arguments.

... so this diagnostic needs to be fundamentally restructured: either it should be split into multiple diagnostics for the various individual messages, or a %select should be added with the appropriate message text, or similar.

[nickdesaulniers]

• rework diags

[nickdesaulniers]

• fix flang diag placeholders

[nickdesaulniers]

While I appreciate the flexibility afforded by %select; I find the suggestions unreadable.

[xbolva00]

clang/include/clang/Basic/DiagnosticDriverKinds.td

282

invalid value '%1' in '%0', value must be '%2' or greater

?

Current wording sounds strange imho.

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticDriverKinds.td

282

I don't care what color we paint the bikeshed, but please suggest a color if you don't like the one I've chosen.

[xbolva00]

clang/include/clang/Basic/DiagnosticDriverKinds.td

282

?

I suggested.

[rsmith]

clang/lib/Driver/ToolChains/Clang.cpp

3115

I think there should be no leading space here; there's already a space in the format string. (By default FileCheck ignores differences in amount of whitespace which I imagine is why the test is passing. I think -strict-whitespace would catch this.)

3141

Likewise here.

[nickdesaulniers]

• remove extra whitespace, use xbolva00's sugguestion text

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticDriverKinds.td

282

I'm sorry; I don't know how I misread what you said.

[xbolva00]

clang/include/clang/Basic/DiagnosticDriverKinds.td

282

No problem, thanks :)

[nickdesaulniers]

Ready for rereview.

clang/include/clang/Basic/DiagnosticDriverKinds.td

282

Does the updated patch look good?

[nickdesaulniers]

@FarisRehman when I run llvm-lit -vv flang/test/Driver/fixed-line-length.f90 I see:

UNSUPPORTED: Flang :: Driver/fixed-line-length.f90 (1 of 1)

This is with -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt;flang" used, and a flang binary produced. Does REQUIRES: new-flang-driver imply I should be doing something else?

[xbolva00]

clang/include/clang/Basic/DiagnosticDriverKinds.td

282

Yeah!

[awarzynski]

In D101387#2730043, @nickdesaulniers wrote:

@FarisRehman when I run llvm-lit -vv flang/test/Driver/fixed-line-length.f90 I see:

UNSUPPORTED: Flang :: Driver/fixed-line-length.f90 (1 of 1)

This is with -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt;flang" used, and a flang binary produced. Does REQUIRES: new-flang-driver imply I should be doing something else?

Currently flang is just a bash script that's a wrapper for the old driver. The new driver is only enabled conditionally - you have to set FLANG_BUILD_NEW_DRIVER to build it. The binary is called flang-new (hopefully soon it will be renamed as flang).

The changes in Flang LGTM, thanks for working on this!

[MaskRay]

clang/include/clang/Basic/DiagnosticDriverKinds.td

220

Note %0 is duplicated in valid values to '%0' - elsewhere we use , expected ...

[nickdesaulniers]

• trim diag as per @MaskRay

[nickdesaulniers]

In D101387#2731436, @awarzynski wrote:

In D101387#2730043, @nickdesaulniers wrote:

@FarisRehman when I run llvm-lit -vv flang/test/Driver/fixed-line-length.f90 I see:

UNSUPPORTED: Flang :: Driver/fixed-line-length.f90 (1 of 1)

This is with -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt;flang" used, and a flang binary produced. Does REQUIRES: new-flang-driver imply I should be doing something else?

Currently flang is just a bash script that's a wrapper for the old driver. The new driver is only enabled conditionally - you have to set FLANG_BUILD_NEW_DRIVER to build it. The binary is called flang-new (hopefully soon it will be renamed as flang).

The changes in Flang LGTM, thanks for working on this!

$ llvm-lit -vv flang/test/Driver/fixed-line-length.f90
PASS: Flang :: Driver/fixed-line-length.f90 (1 of 1)

Nice, thanks for the instructions and info!

[xbolva00]

clang/test/Driver/stack-protector-guard.c

38

Not very happy with suggestion, maybe worse than before. It sounds to me that now this suggests

"-mstack-protector-guard-reg=fs gs" which is a bad suggestion...

Better (?):
error: invalid value {{.*}} in 'mstack-protector-guard-reg=', expected 'XX or 'YY'

as both use sites suggest two values, this wording could be good enough.

[nickdesaulniers]

clang/test/Driver/stack-protector-guard.c

38

In the follow up commit, D100919, this flag will have 3 values.

[nickdesaulniers]

clang/test/Driver/stack-protector-guard.c

38

Perhaps:

error: invalid value {{.*}} in 'mstack-protector-guard-reg=', expected one of: fs gs

[MaskRay]

One last nit. Perhaps leave it open for two days in case other reviewers have further comments.

clang/include/clang/Basic/DiagnosticDriverKinds.td

279

The two can reuse err_drv_invalid_value_with_suggestion

[xbolva00]

clang/test/Driver/stack-protector-guard.c

38

or use select,
"expected %select('XX'| 'XX or 'YY')" for generalization

38

Yeah, good idea.

[xbolva00]

clang/include/clang/Basic/DiagnosticDriverKinds.td

279

We dont need to reuse everything at all cost. Clear and understandable messages are far more important.

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticDriverKinds.td

279

Can they? If they did, then flang would hard code "'none' or a positive integer" and " or greater" which I think defeats the purpose of what @rsmith asked for when referring to https://clang.llvm.org/docs/InternalsManual.html#the-format-string.

[nickdesaulniers]

• "expected one of: ..."

[xbolva00]

Looks better now, thanks.

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticDriverKinds.td

279

I've changed err_drv_invalid_value_with_suggestion since LGTM; PTAL. My main concern is that strings like "a positive integer" should be in the .td for translation (IIUC), not hardcoded in flang/lib/Frontend/CompilerInvocation.cpp.

clang/test/Driver/stack-protector-guard.c

38

I went with "expected one of". Reasoning: I don't think %select scales as simply in comparison. PTAL.

[xbolva00]

clang/test/Driver/stack-protector-guard.c

38

Your suggestion sounds good to me as well.

[xbolva00]

LG in the current state.

clang/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_ycyu_different_arg_clang_cl : Warning<
   "support for '/Yc' and '/Yu' with different filenames not implemented yet; flags ignored">,
   InGroup<ClangClPch>;
 def warn_drv_yc_multiple_inputs_clang_cl : Warning<
   "support for '/Yc' with more than one source file not implemented yet; flag ignored">,
   InGroup<ClangClPch>;
 
 def err_drv_invalid_value : Error<"invalid value '%1' in '%0'">;
 def err_drv_invalid_int_value : Error<"invalid integral value '%1' in '%0'">;
-def err_drv_invalid_value_with_suggestion : Error<"invalid value '%1' in '%0','%2'">;
+def err_drv_invalid_value_with_suggestion : Error<
+    "invalid value '%1' in '%0', expected one of: %2">;
 def err_drv_invalid_remap_file : Error<
     "invalid option '%0' not of the form <from-file>;<to-file>">;
 def err_drv_invalid_gcc_output_type : Error<
     "invalid output type '%0' for use with gcc tool">;
 def err_drv_cc_print_options_failure : Error<
     "unable to open CC_PRINT_OPTIONS file: %0">;
 def err_drv_lto_without_lld : Error<"LTO requires -fuse-ld=lld">;
 def err_drv_preamble_format : Error<

 def err_drv_omp_offload_target_cuda_version_not_support : Error<"NVPTX target requires CUDA 9.2 or above. CUDA %0 is detected.">;
 def warn_drv_omp_offload_target_duplicate : Warning<
   "The OpenMP offloading target '%0' is similar to target '%1' already specified - will be ignored.">,
   InGroup<OpenMPTarget>;
 def err_drv_unsupported_embed_bitcode
     : Error<"%0 is not supported with -fembed-bitcode">;
 def err_drv_bitcode_unsupported_on_toolchain : Error<
   "-fembed-bitcode is not supported on versions of iOS prior to 6.0">;
+def err_drv_negative_columns : Error<
+    "invalid value '%1' in '%0', value must be 'none' or a positive integer">;
+def err_drv_small_columns : Error<
+    "invalid value '%1' in '%0', value must be '%2' or greater">;
 
 def err_drv_invalid_malign_branch_EQ : Error<
   "invalid argument '%0' to -malign-branch=; each element must be one of: %1">;
 
 def warn_O4_is_O3 : Warning<"-O4 is equivalent to -O3">, InGroup<Deprecated>;
 def warn_drv_optimization_value : Warning<"optimization level '%0' is not supported; using '%1%2' instead">,
   InGroup<InvalidCommandLineArgument>;
 def warn_ignored_gcc_optimization : Warning<"optimization flag '%0' is not supported">,

clang/lib/Driver/ToolChains/Clang.cpp

static void RenderSSPOptions(const Driver &D, const ToolChain &TC,
   const std::string &TripleStr = EffectiveTriple.getTriple();
   if (Arg *A = Args.getLastArg(options::OPT_mstack_protector_guard_EQ)) {
     StringRef Value = A->getValue();
     if (!EffectiveTriple.isX86() && !EffectiveTriple.isAArch64())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
     if (Value != "tls" && Value != "global") {
       D.Diag(diag::err_drv_invalid_value_with_suggestion)
-      << A->getOption().getName() << Value
-      << "valid arguments to '-mstack-protector-guard=' are:tls global";
+          << A->getOption().getName() << Value << "tls global";
       return;
     }
     A->render(Args, CmdArgs);
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mstack_protector_guard_offset_EQ)) {
     StringRef Value = A->getValue();
     if (!EffectiveTriple.isX86())
static void RenderSSPOptions(const Driver &D, const ToolChain &TC,
 
   if (Arg *A = Args.getLastArg(options::OPT_mstack_protector_guard_reg_EQ)) {
     StringRef Value = A->getValue();
     if (!EffectiveTriple.isX86())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
     if (EffectiveTriple.isX86() && (Value != "fs" && Value != "gs")) {
       D.Diag(diag::err_drv_invalid_value_with_suggestion)
-      << A->getOption().getName() << Value
-      << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
+          << A->getOption().getName() << Value << "fs gs";
       return;
     }
     A->render(Args, CmdArgs);
   }
 }
 
 static void RenderSCPOptions(const ToolChain &TC, const ArgList &Args,
                              ArgStringList &CmdArgs) {

clang/test/Driver/stack-protector-guard.c

 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard=tls %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-TLS %s
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard=global %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-GLOBAL %s
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard=local %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-VALUE %s
 
 // CHECK-TLS: "-cc1" {{.*}}"-mstack-protector-guard=tls"
 // CHECK-GLOBAL: "-cc1" {{.*}}"-mstack-protector-guard=global"
-// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:tls global'
+// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=', expected one of: tls global
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-FS %s
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=gs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-GS %s
 
 // Invalid arch
 // RUN: not %clang -target arm-eabi-c -mstack-protector-guard=tls %s 2>&1 | \

 // Invalid option value
 // RUN: not %clang -target x86_64-unknown-unknown -c -mstack-protector-guard-reg=cs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-REG %s
 // RUN: not %clang -target x86_64-unknown-unknown -c -mstack-protector-guard-reg=ds %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-REG %s
 
 // CHECK-FS: "-cc1" {{.*}}"-mstack-protector-guard-reg=fs"
 // CHECK-GS: "-cc1" {{.*}}"-mstack-protector-guard-reg=gs"
-// INVALID-REG: error: invalid value {{.*}} in 'mstack-protector-guard-reg=','for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs'
+// INVALID-REG: error: invalid value {{.*}} in 'mstack-protector-guard-reg=', expected one of: fs gs
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-offset=30 %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OFFSET %s
 
 // CHECK-OFFSET: "-cc1" {{.*}}"-mstack-protector-guard-offset=30"

flang/lib/Frontend/CompilerInvocation.cpp

if (const auto *arg =
     llvm::StringRef argValue = llvm::StringRef(arg->getValue());
     std::int64_t columns = -1;
     if (argValue == "none") {
       columns = 0;
     } else if (argValue.getAsInteger(/*Radix=*/10, columns)) {
       columns = -1;
     }
     if (columns < 0) {
-      diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-          << arg->getOption().getName() << arg->getValue()
-          << "value must be 'none' or a non-negative integer";
+      diags.Report(clang::diag::err_drv_negative_columns)
+          << arg->getOption().getName() << arg->getValue();
     } else if (columns == 0) {
       opts.fixedFormColumns_ = 1000000;
     } else if (columns < 7) {
-      diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-          << arg->getOption().getName() << arg->getValue()
-          << "value must be at least seven";
+      diags.Report(clang::diag::err_drv_small_columns)
+          << arg->getOption().getName() << arg->getValue() << "7";
     } else {
       opts.fixedFormColumns_ = columns;
     }
   }
 
   if (const llvm::opt::Arg *arg =
           args.getLastArg(clang::driver::options::OPT_fimplicit_none,
               clang::driver::options::OPT_fno_implicit_none)) {

flang/test/Driver/fixed-line-length.f90

 ! EXPECTED OUTPUT WITH DEFAULT LENGTH
 !-------------------------------------
 ! The line should be trimmed to 72 characters when reading based on the default value of fixed line length.
 ! DEFAULTLENGTH: program{{(a{58})}}
 
 !-----------------------------------------
 ! EXPECTED OUTPUT WITH A NEGATIVE LENGTH
 !-----------------------------------------
-! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=','value must be 'none' or a non-negative integer'
+! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=', value must be 'none' or a positive integer
 
 !-----------------------------------------
 ! EXPECTED OUTPUT WITH LENGTH LESS THAN 7
 !-----------------------------------------
-! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=','value must be at least seven'
+! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=', value must be '7' or greater
 
 !---------------------------------------
 ! EXPECTED OUTPUT WITH UNLIMITED LENGTH
 !---------------------------------------
 ! The line should not be trimmed and so 73 characters (including spaces) should be read.
 ! UNLIMITEDLENGTH: program{{(a{59})}}
 
 !--------------------------------
 ! EXPECTED OUTPUT WITH LENGTH 13
 !--------------------------------
 ! LENGTH13: program