[cfe-dev] [ubsan] Check recovery

[Richard Smith]

On Fri, Nov 30, 2012 at 3:58 PM, Will Dietz <willdtz at gmail.com> wrote:

> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121130/6fdd14bb/attachment.html>