[PATCH] Reimplement -fsanitize-recover family of flags.
vonosmas at gmail.com
Mon Jan 12 14:37:11 PST 2015
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">,
-def fno_sanitize_recover : Flag<["-"], "fno-sanitize-recover">,
- Group<f_clang_Group>, Flags<[CC1Option]>,
- HelpText<"Disable sanitizer check recovery">;
+ : Flag<["-"], "fsanitize-recover">,
+ HelpText<"Enable recovery for UBSan and integer checks. Deprecated. "
+ "Use -fsanitize-recover=undefined,integer instead.">;
> Maybe remove the HelpText and leave this undocumented now that it's deprecated.
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?
> 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,
> 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 @@
+ if (K == SanitizerKind::Unknown)
+ Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
> 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.
More information about the cfe-commits