Using an invalid -O falls back on -O3 instead of an error

[sylvestre.ledru]

Summary

Currently with clang:
$ clang -O20 foo.c
error: invalid value '20' in '-O20'

With the patch:
$ clang -O20 foo.c
warning: optimization level '-O20' is unsupported; using '-O3' instead.
1 warning generated.

This matches the gcc behavior (with a warning added)

Pass all tests:
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
Testing Time: 94.14s

Expected Passes    : 6721
Expected Failures  : 20
Unsupported Tests  : 17

(which was not the case of http://llvm-reviews.chandlerc.com/D2125)

[sylvestre.ledru]

I am open to change to -O2 if you think it is more relevant...

[rengolin]

I think -O3 is the correct level. LGTM.

[sylvestre.ledru]

Document the change in the release

[rengolin]

Sylvestre, you got a profile-sample-use change together with your doc change. ;)

[rengolin]

Oh, or maybe you didn't re-base, and phabricator got crazy... I think...

[sylvestre.ledru]

Chandler, Rafael, is that OK with you?

[sylvestre.ledru]

I just committed to make sure it will be in the 3.4 branch.
However, if there is a consensus to revert this change, I won't mind doing it :)
(as you can imagine, this commit goal is to simplify the transition from gcc to clang)

[ygao]

There is a problem in this implementation. At Line#302 of lib/Frontend/CompilerInvocation.cpp:
Opts.OptimizationLevel = getOptimizationLevel(Args, IK, Diags);

Notice that Opts.OptimizationLevel is a 3-bit bitfield, so any optimization bigger than 8 are truncated, so
for example, in test/Driver/invalid-o-level.c, you have checked that you get a nice diagnostic for -O900, but
if you try -O899, you will not get any diagnostics.

I think one needs to diagnose on the unsigned optimization level before assigning it to the bitfield.

docs/ReleaseNotes.rst

 
 -  ...
 
 New Compiler Flags
 ------------------
 
 - Clang no longer special cases -O4 to enable lto. Explicitly pass -flto to
   enable it.
-- Command line "clang -O3 -flto a.c -c" and "clang -emit-llvm a.c -c" 
+- Clang no longer fails on >= -O5. Uses -O3 instead.
+- Command line "clang -O3 -flto a.c -c" and "clang -emit-llvm a.c -c"
   are no longer equivalent.
 - Clang now errors on unknown -m flags (``-munknown-to-clang``),
   unknown -f flags (``-funknown-to-clang``) and unknown
   options (``-what-is-this``).
 
 C Language Changes in Clang
 ---------------------------
 

include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_unknown_objc_runtime : Error<
   "unknown or ill-formed Objective-C runtime '%0'">;
 def err_drv_emit_llvm_link : Error<
    "-emit-llvm cannot be used when linking">;
 def err_drv_unknown_toolchain : Error<
   "cannot recognize the type of the toolchain">;
 
 def warn_O4_is_O3 : Warning<"-O4 is equivalent to -O3">, InGroup<Deprecated>;
+def warn_drv_optimization_value : Warning<"optimization level '%0' is unsupported; using '%1%2' instead.">,
+  InGroup<InvalidCommandLineArgument>;
 def warn_c_kext : Warning<
   "ignoring -fapple-kext which is valid for C++ and Objective-C++ only">;
 def warn_drv_input_file_unused : Warning<
   "%0: '%1' input unused%select{ when '%3' is present|}2">,
   InGroup<UnusedCommandLineArgument>;
 def warn_drv_input_file_unused_by_cpp : Warning<
   "%0: '%1' input unused in cpp mode">,
   InGroup<UnusedCommandLineArgument>;

include/clang/Basic/DiagnosticGroups.td

 def IgnoredAttributes : DiagGroup<"ignored-attributes">;
 def UnnamedTypeTemplateArgs : DiagGroup<"unnamed-type-template-args",
                                         [CXX98CompatUnnamedTypeTemplateArgs]>;
 def UnsupportedFriend : DiagGroup<"unsupported-friend">;
 def UnusedArgument : DiagGroup<"unused-argument">;
 def UnusedSanitizeArgument : DiagGroup<"unused-sanitize-argument">;
 def UnusedCommandLineArgument : DiagGroup<"unused-command-line-argument",
                                           [UnusedSanitizeArgument]>;
+def InvalidCommandLineArgument : DiagGroup<"invalid-command-line-argument">;
 def UnusedComparison : DiagGroup<"unused-comparison">;
 def UnusedExceptionParameter : DiagGroup<"unused-exception-parameter">;
 def UnneededInternalDecl : DiagGroup<"unneeded-internal-declaration">;
 def UnneededMemberFunction : DiagGroup<"unneeded-member-function">;
 def UnusedPrivateField : DiagGroup<"unused-private-field">;
 def UnusedFunction : DiagGroup<"unused-function", [UnneededInternalDecl]>;
 def UnusedMemberFunction : DiagGroup<"unused-member-function",
                                      [UnneededMemberFunction]>;

lib/Driver/Tools.cpp

if (types::getPreprocessedType(InputType) != types::TY_INVALID)
     AddPreprocessingOptions(C, JA, D, Args, CmdArgs, Output, Inputs);
 
   // Don't warn about "clang -c -DPIC -fPIC test.i" because libtool.m4 assumes
   // that "The compiler can only warn and ignore the option if not recognized".
   // When building with ccache, it will pass -D options to clang even on
   // preprocessed inputs and configure concludes that -fPIC is not supported.
   Args.ClaimAllArgs(options::OPT_D);
 
-  // Manually translate -O4 to -O3; let clang reject others.
+  // Manually translate -O4 to -O3; let clang fall back on -O3 for others
   if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {
     if (A->getOption().matches(options::OPT_O4)) {
       CmdArgs.push_back("-O3");
       D.Diag(diag::warn_O4_is_O3);
     } else {
       A->render(Args, CmdArgs);
     }
   }

lib/Frontend/CompilerInvocation.cpp

static void ParseCommentArgs(CommentOptions &Opts, ArgList &Args) {
   Opts.ParseAllComments = Args.hasArg(OPT_fparse_all_comments);
 }
 
 static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
                              DiagnosticsEngine &Diags) {
   using namespace options;
   bool Success = true;
 
-  unsigned OptLevel = getOptimizationLevel(Args, IK, Diags);
-  if (OptLevel > 3) {
-    Diags.Report(diag::err_drv_invalid_value)
-      << Args.getLastArg(OPT_O)->getAsString(Args) << OptLevel;
-    OptLevel = 3;
-    Success = false;
+  Opts.OptimizationLevel = getOptimizationLevel(Args, IK, Diags);
+  unsigned MaxOptLevel = 3;
+  if (Opts.OptimizationLevel > MaxOptLevel) {
+    // If the optimization level is not supported, fall back on the default optimization
+    Diags.Report(diag::warn_drv_optimization_value)
+        << Args.getLastArg(OPT_O)->getAsString(Args) << "-O" << MaxOptLevel;
+    Opts.OptimizationLevel = MaxOptLevel;
   }
-  Opts.OptimizationLevel = OptLevel;
 
   // We must always run at least the always inlining pass.
   Opts.setInlining(
     (Opts.OptimizationLevel > 1) ? CodeGenOptions::NormalInlining
                                  : CodeGenOptions::OnlyAlwaysInlining);
   // -fno-inline-functions overrides OptimizationLevel > 1.
   Opts.NoInline = Args.hasArg(OPT_fno_inline);
   Opts.setInlining(Args.hasArg(OPT_fno_inline_functions) ?

test/Driver/clang_f_opts.c

 
 // RUN: %clang -### -S -fpascal-strings -mno-pascal-strings %s 2>&1 | FileCheck -check-prefix=CHECK-NO-M-PASCAL-STRINGS %s
 // CHECK-NO-M-PASCAL-STRINGS-NOT: "-fpascal-strings"
 
 // RUN: %clang -### -S -O4 %s 2>&1 | FileCheck -check-prefix=CHECK-MAX-O %s
 // CHECK-MAX-O: warning: -O4 is equivalent to -O3
 // CHECK-MAX-O: -O3
 
+// RUN: %clang -S -O20 %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-O %s
+// CHECK-INVALID-O: warning: optimization level '-O20' is unsupported; using '-O3' instead.
+
 // Test that we don't error on these.
 // RUN: %clang -### -S -Werror                                                \
 // RUN:     -falign-functions -falign-functions=2 -fno-align-functions        \
 // RUN:     -fasynchronous-unwind-tables -fno-asynchronous-unwind-tables      \
 // RUN:     -fbuiltin -fno-builtin                                            \
 // RUN:     -fdiagnostics-show-location=once                                  \
 // RUN:     -ffloat-store -fno-float-store                                    \
 // RUN:     -feliminate-unused-debug-types -fno-eliminate-unused-debug-types  \

test/Driver/invalid-o-level.c

-// RUN: not %clang_cc1 %s -O900 2> %t.log
+// RUN: %clang_cc1 %s -O900 2> %t.log
 // RUN: FileCheck %s -input-file=%t.log
 
-// CHECK: invalid value '900' in '-O900'
+// CHECK: warning: optimization level '-O900' is unsupported; using '-O3' instead.
+
+