<div class="gmail_quote">On Fri, Nov 30, 2012 at 3:58 PM, Will Dietz <span dir="ltr"><<a href="mailto:willdtz@gmail.com" target="_blank">willdtz@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Many of ubsan's checks are recoverable, and it'd be great to support<br>
doing so when possible. Example use-case might be trying<br>
-fsanitize=undefined on a new code base that has many errors, where<br>
recovery would allow fixing them in bulk and require fewer<br>
build/test/fix cycles.<br>
<br>
This has been discussed a bit previously, and one of the biggest<br>
arguments against this was that making these paths recoverable might<br>
have a negative impact on performance.<br>
<br>
To help address this concern, attached are patches that add a<br>
-fsanitize-recover /cc1/ flag to enable performance tests with and<br>
without recovery enabled. Suggestions on how to proceed with such<br>
testing would be appreciated :).<br>
<br>
If performance testing proves the difference is not a concern, the<br>
next step would be making recovery the default and<br>
-fsanitize-no-recover a driver-level flag (with no need for<br>
-fsanitize-recover). It's certainly important to enable users to<br>
specify they want program execution halted on the first detected error<br>
as is presently the default.<br></blockquote><div><br></div><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The "recover" phrasing is used (as opposed to "abort" of "trap"<br>
terminology) because many checks cannot be recovered from, so a flag<br>
like -fsanitize-no-abort would be misleading what it does.<br>
<br>
Review and feedback on this approach and its direction would be much<br>
appreciated.<br></blockquote><div><br></div><div>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.)</div>
<div><br></div><div><br></div><div><div>--- a/include/clang/Basic/LangOptions.def</div><div>+++ b/include/clang/Basic/LangOptions.def</div><div>@@ -173,6 +173,8 @@ BENIGN_LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comm</div>
<div> BENIGN_LANGOPT(Sanitize##ID, 1, 0, NAME " sanitizer")</div><div> #include "clang/Basic/Sanitizers.def"</div><div> </div><div>+BENIGN_LANGOPT(SanitizeNoRecover, 1, 1, "abort at runtime when a sanitizer check fails")</div>
</div><div><br></div><div>This doesn't need to be a language option. Just add it to CodeGenOpts?</div><div><br></div><div><br></div><div><div>--- a/include/clang/Driver/CC1Options.td</div><div>+++ b/include/clang/Driver/CC1Options.td</div>
<div>@@ -450,6 +450,10 @@ def fdeprecated_macro : Flag<["-"], "fdeprecated-macro">,</div><div> HelpText<"Defines the __DEPRECATED macro">;</div><div> def fno_deprecated_macro : Flag<["-"], "fno-deprecated-macro">,</div>
<div> HelpText<"Undefines the __DEPRECATED macro">;</div><div>+def fsanitize_recover : Flag<["-"], "fsanitize-recover">,</div><div>+ HelpText<"Attempt to recover if a sanitizer check fails at runtime">;</div>
<div>+def fsanitize_no_recover : Flag<["-"], "fsanitize-no-recover">,</div><div>+ HelpText<"Don't attempt to recover if a sanitizer check fails at runtime">;</div><div><br></div>
</div><div>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.</div><div><br></div><div><br></div><div><div>--- a/lib/CodeGen/CodeGenFunction.h</div>
<div>+++ b/lib/CodeGen/CodeGenFunction.h</div><div>@@ -2581,7 +2581,7 @@ public:</div><div> void EmitCheck(llvm::Value *Checked, StringRef CheckName,</div><div> llvm::ArrayRef<llvm::Constant *> StaticArgs,</div>
<div> llvm::ArrayRef<llvm::Value *> DynamicArgs,</div><div>- bool Recoverable = false);</div><div>+ bool Recoverable = true, bool AlwaysRecover = false);</div></div><div>
<br></div><div>Please add a three-value enum for this.</div></div>