Many of ubsan’s checks are recoverable, and it’d be great to support
doing so when possible. Example use-case might be trying
-fsanitize=undefined on a new code base that has many errors, where
recovery would allow fixing them in bulk and require fewer
build/test/fix cycles.
This has been discussed a bit previously, and one of the biggest
arguments against this was that making these paths recoverable might
have a negative impact on performance.
To help address this concern, attached are patches that add a
-fsanitize-recover /cc1/ flag to enable performance tests with and
without recovery enabled. Suggestions on how to proceed with such
testing would be appreciated :).
If performance testing proves the difference is not a concern, the
next step would be making recovery the default and
-fsanitize-no-recover a driver-level flag (with no need for
-fsanitize-recover). It’s certainly important to enable users to
specify they want program execution halted on the first detected error
as is presently the default.
As we discussed previously, I think this is a great direction. (Although at the driver level, we should have the no-recover flag, so that an earlier command-line argument can be overridden.) We would also need to make it clear in the documentation that we only make a best-effort attempt to recover, and that some sanitizers simply cannot recover (for instance, the vptr sanitizer might crash on an error, and asan doesn’t have the ability to recover).
The “recover” phrasing is used (as opposed to “abort” of “trap”
terminology) because many checks cannot be recovered from, so a flag
like -fsanitize-no-abort would be misleading what it does.
Review and feedback on this approach and its direction would be much
appreciated.
To keep the instrumented code size down, could you add separate handler entry points for the recovering and not-recovering cases? (Alternatively, find a spare bit in the static data and put the flag there.)
— a/include/clang/Basic/LangOptions.def
+++ b/include/clang/Basic/LangOptions.def
@@ -173,6 +173,8 @@ BENIGN_LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, “retain documentation comm
BENIGN_LANGOPT(Sanitize##ID, 1, 0, NAME " sanitizer”)
#include “clang/Basic/Sanitizers.def”
+BENIGN_LANGOPT(SanitizeNoRecover, 1, 1, “abort at runtime when a sanitizer check fails”)
This doesn’t need to be a language option. Just add it to CodeGenOpts?
— a/include/clang/Driver/CC1Options.td
+++ b/include/clang/Driver/CC1Options.td
@@ -450,6 +450,10 @@ def fdeprecated_macro : Flag<[“-”], “fdeprecated-macro”>,
HelpText<“Defines the __DEPRECATED macro”>;
def fno_deprecated_macro : Flag<[“-”], “fno-deprecated-macro”>,
HelpText<“Undefines the __DEPRECATED macro”>;
+def fsanitize_recover : Flag<[“-”], “fsanitize-recover”>,
- HelpText<“Attempt to recover if a sanitizer check fails at runtime”>;
+def fsanitize_no_recover : Flag<[“-”], “fsanitize-no-recover”>,
- HelpText<“Don’t attempt to recover if a sanitizer check fails at runtime”>;
Remove fsanitize_no_recover. -cc1 isn’t supposed to be user-friendly, and usually only a flag for the non-default value of an option.
— a/lib/CodeGen/CodeGenFunction.h
+++ b/lib/CodeGen/CodeGenFunction.h
@@ -2581,7 +2581,7 @@ public:
void EmitCheck(llvm::Value *Checked, StringRef CheckName,
llvm::ArrayRef<llvm::Constant *> StaticArgs,
llvm::ArrayRef<llvm::Value *> DynamicArgs,
- bool Recoverable = false);
- bool Recoverable = true, bool AlwaysRecover = false);
Please add a three-value enum for this.