Be more strict when checking the -flto option value

[yamaguchi]

Summary

It seems -flto must be either "thin" or "full". I think the use of
containValue is just a typo.

[tejohnson]

Make sure you add cfe-commits to the mailing list for clang changes (and llvm-commits for llvm changes, I accidentally added that one first and then fixed it), since all patches should go to the full mailing list.

This fix looks correct to me. Please add a test case though. Perhaps there should be an error if the value is something other than "thin" or "lto"? E.g. see the error that will get emitted if the value is not one of those in clang/lib/Driver/Driver.cpp.

[yamaguchi]

I think we don't need additional testcase, because this is non-functional change. Driver.cpp will emit error if value was not "thin" nor "full". This testcase is at clang/test/CodeGen/thinlto-backend-option.ll.

[tejohnson]

In D34055#777005, @yamaguchi wrote:

I think we don't need additional testcase, because this is non-functional change. Driver.cpp will emit error if value was not "thin" nor "full". This testcase is at clang/test/CodeGen/thinlto-backend-option.ll.

That test case is checking that -flto=thin is accepted by the driver. I don't see it checking for an unknown -flto= value. But in any case, you can go through the code you are fixing here without hitting the driver check by invoking clang -cc1 (%clang_cc1 in the test case). I just confirmed that -flto=thinfoo is flagged as an error by clang (when it goes through the driver), but is silently accepted if I pass directly to clang -cc1. So this is definitely a bug fix and it would be good to check for invalid -flto values and given an error, then have a test that uses %clang_cc1 and confirms that an unknown -flto= value gets that error.

[yamaguchi]

Update patch. Add testcase and check if argument is valid or not.

[tejohnson]

LGTM, thanks for the fix!

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
                                          OPT_fno_unique_section_names, true);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
 
   Opts.PrepareForLTO = Args.hasArg(OPT_flto, OPT_flto_EQ);
-  const Arg *A = Args.getLastArg(OPT_flto, OPT_flto_EQ);
-  Opts.EmitSummaryIndex = A && A->containsValue("thin");
+  Opts.EmitSummaryIndex = false;
+  if (Arg *A = Args.getLastArg(OPT_flto_EQ)) {
+    StringRef S = A->getValue();
+    if (S == "thin")
+      Opts.EmitSummaryIndex = true;
+    else if (S != "full")
+      Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << S;
+  }
   Opts.LTOUnit = Args.hasFlag(OPT_flto_unit, OPT_fno_lto_unit, false);
   if (Arg *A = Args.getLastArg(OPT_fthinlto_index_EQ)) {
     if (IK.getLanguage() != InputKind::LLVM_IR)
       Diags.Report(diag::err_drv_argument_only_allowed_with)
           << A->getAsString(Args) << "-x ir";
     Opts.ThinLTOIndexFile = Args.getLastArgValue(OPT_fthinlto_index_EQ);
   }
   Opts.ThinLinkBitcodeFile = Args.getLastArgValue(OPT_fthin_link_bitcode_EQ);

clang/test/CodeGen/thinlto-backend-option.ll

 ; Test to ensure -backend-options work when invoking the ThinLTO backend path.
 
 ; This test uses a non-existent backend option to test that backend options are
 ; being parsed. While it's more important that the existing options are parsed
 ; than that this error is produced, this provides a reliable way to test this
 ; scenario independent of any particular backend options that may exist now or
 ; in the future.
 
 ; RUN: %clang -flto=thin -c -o %t.o %s
 ; RUN: llvm-lto -thinlto -o %t %t.o
-; RUN: not %clang_cc1 -x ir %t.o -fthinlto-index=%t.thinlto.bc -backend-option -nonexistent -emit-obj -o /dev/null 2>&1 | FileCheck %s
+; RUN: not %clang_cc1 -x ir %t.o -fthinlto-index=%t.thinlto.bc -backend-option -nonexistent -emit-obj -o /dev/null 2>&1 | FileCheck %s -check-prefix=UNKNOWN
+; UNKNOWN: clang: Unknown command line argument '-nonexistent'
 
-; CHECK: clang: Unknown command line argument '-nonexistent'
+; RUN: not %clang_cc1 -flto=thinfoo 2>&1 | FileCheck %s -check-prefix=INVALID
+; INVALID: error: invalid value 'thinfoo' in '-flto=thinfoo'