[PATCH] Reimplement -fsanitize-recover family of flags.

[Alexey Samsonov]

Thank you! Addressed most of the comments and added docs about the new flag. Submitting.


================
Comment at: include/clang/Driver/Options.td:537-541
@@ -536,6 +536,7 @@
                                         HelpText<"Level of field padding for AddressSanitizer">;
-def fsanitize_recover : Flag<["-"], "fsanitize-recover">,
-                        Group<f_clang_Group>;
-def fno_sanitize_recover : Flag<["-"], "fno-sanitize-recover">,
-                           Group<f_clang_Group>, Flags<[CC1Option]>,
-                           HelpText<"Disable sanitizer check recovery">;
+def fsanitize_recover
+    : Flag<["-"], "fsanitize-recover">,
+      Group<f_clang_Group>,
+      HelpText<"Enable recovery for UBSan and integer checks. Deprecated. "
+               "Use -fsanitize-recover=undefined,integer instead.">;
+def fno_sanitize_recover
----------------
rsmith wrote:
> Maybe remove the HelpText and leave this undocumented now that it's deprecated.
Done

================
Comment at: lib/CodeGen/CGExpr.cpp:2308
@@ +2307,3 @@
+           "Runtime call required for AlwaysRecoverable kind!");
+    // FIXME: Shouldn't we assume that RecoverableKind should be null in this
+    // case?
----------------
rsmith wrote:
> Typo `RecoverableCond`?
> 
> I don't think it's obvious what we should do if the user turns on both `-fsanitize-recover=something` and also `-fsanitize-undefined-trap-on-error`. Since the latter is usually used in cases where the sanitizer runtime is not available (when building an OS kernel or the like), I think it should overrule `-fsanitize-recover` (as it does), and the answer to your FIXME is "no, we shouldn't".
Right. Replaced the FIXME with comment describing that `-fsanitize-undefined-trap-on-error` overrides `-fsanitize-recover=` options.

================
Comment at: lib/CodeGen/CGExpr.cpp:2363
@@ +2362,3 @@
+    emitCheckHandlerCall(*this, FnType, Args, CheckName, RecoverKind, true,
+                         NonFatalHandlerBB);
+    EmitBlock(NonFatalHandlerBB);
----------------
rsmith wrote:
> Passing `NonFatalHandlerBB` here seems strange: there should be no control flow path out of the fatal error handler; we should call the `_abort` runtime function, which doesn't return.
No, there is CheckRecoverableKind::AlwaysRecoverable (vptr sanitizer), so we may return even from __ubsan_handle_dynamic_type_cache_miss_abort.

================
Comment at: lib/Frontend/CompilerInvocation.cpp:335
@@ +334,3 @@
+                          .Default(SanitizerKind::Unknown);
+    if (K == SanitizerKind::Unknown)
+      Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
----------------
rsmith wrote:
> You should probably check for unrecoverable sanitizers for the `-fsanitize-recover=` case here, too, in case someone passes a bad argument to -cc1 directly.
Right. It's hard to fix it immediately, as we only support SANITIER_GROUP handling in driver. I've added a FIXME and will address this later.

http://reviews.llvm.org/D6302

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/