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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
