<div dir="ltr">Should be fixed after r<span style="color:rgb(0,0,0)">303311. I filed </span><font color="#000000"><a href="https://bugs.llvm.org//show_bug.cgi?id=33081">https://bugs.llvm.org//show_bug.cgi?id=33081</a> after chatting with Richard about what ubsan should ideally do here.</font></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 17, 2017 at 1:13 PM, Vitaly Buka <span dir="ltr"><<a href="mailto:vitalybuka@google.com" target="_blank">vitalybuka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><a href="http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/5002/steps/check-llvm%20asan/logs/stdio" class="m_-1098524674459821206cremed" target="_blank">http://lab.llvm.org:8011/<wbr>builders/sanitizer-x86_64-<wbr>linux-fast/builds/5002/steps/<wbr>check-llvm%20asan/logs/stdio</a><div><br></div><div><pre style="font-family:"Courier New",courier,monotype,monospace;font-size:medium"><span class="m_-1098524674459821206inbox-inbox-stdout">SAN:DEADLYSIGNAL
==============================<wbr>==============================<wbr>=====
==24802==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000660cef bp 0x7fffa59551f0 sp 0x7fffa59551f0 T0)
==24802==The signal is caused by a WRITE memory access.
==24802==Hint: address points to the zero page.
#0 0x660cee in nullDeref() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/unittests/Support/<wbr>CrashRecoveryTest.cpp:24:52
#1 0xc7e51b in operator() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/include/llvm/ADT/<wbr>STLExtras.h:111:12
#2 0xc7e51b in llvm::CrashRecoveryContext::<wbr>RunSafely(llvm::function_ref<<wbr>void ()>) /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/lib/Support/<wbr>CrashRecoveryContext.cpp:359
#3 0x6614a1 in CrashRecoveryTest_Cleanup_<wbr>Test::TestBody() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/unittests/Support/<wbr>CrashRecoveryTest.cpp:59:5
#4 0xdc9a3e in HandleExceptionsInMethodIfSupp<wbr>orted<testing::Test, void> /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/src/gtest.cc:2458:<wbr>12
#5 0xdc9a3e in testing::Test::Run() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/src/gtest.cc:2474
#6 0xdce174 in testing::TestInfo::Run() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/src/gtest.cc:2656:<wbr>11
#7 0xdcf4a6 in testing::TestCase::Run() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/src/gtest.cc:2774:<wbr>28
#8 0xdef336 in testing::internal::<wbr>UnitTestImpl::RunAllTests() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/src/gtest.cc:4649:<wbr>43
#9 0xdee692 in HandleExceptionsInMethodIfSupp<wbr>orted<testing::internal::<wbr>UnitTestImpl, bool> /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/src/gtest.cc:2458:<wbr>12
#10 0xdee692 in testing::UnitTest::Run() /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/src/gtest.cc:4257
#11 0xdad59b in RUN_ALL_TESTS /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>googletest/include/gtest/<wbr>gtest.h:2233:46
#12 0xdad59b in main /mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm/utils/unittest/<wbr>UnitTestMain/TestMain.cpp:51
#13 0x7ff81ab8982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.<wbr>so.6+0x2082f)
#14 0x485d98 in _start (/mnt/b/sanitizer-buildbot3/<wbr>sanitizer-x86_64-linux-fast/<wbr>build/llvm_build_asan/<wbr>unittests/Support/<wbr>SupportTests+0x485d98)
</span></pre><br class="m_-1098524674459821206inbox-inbox-Apple-interchange-newline"></div></div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Wed, May 17, 2017 at 11:29 AM Reid Kleckner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rnk<br>
Date: Wed May 17 13:16:17 2017<br>
New Revision: 303279<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=303279&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=303279&view=rev</a><br>
Log:<br>
Re-land r303274: "[CrashRecovery] Use SEH __try instead of VEH when available"<br>
<br>
We have to check gCrashRecoveryEnabled before using __try.<br>
<br>
In other words, SEH works too well and we ended up recovering from<br>
crashes in implicit module builds that we weren't supposed to. Only<br>
libclang is supposed to enable CrashRecoveryContext to allow implicit<br>
module builds to crash.<br>
<br>
Added:<br>
llvm/trunk/unittests/Support/<wbr>CrashRecoveryTest.cpp<br>
Modified:<br>
llvm/trunk/lib/Support/<wbr>CrashRecoveryContext.cpp<br>
llvm/trunk/unittests/Support/<wbr>CMakeLists.txt<br>
<br>
Modified: llvm/trunk/lib/Support/<wbr>CrashRecoveryContext.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CrashRecoveryContext.cpp?rev=303279&r1=303278&r2=303279&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Support/CrashRecoveryContext.<wbr>cpp?rev=303279&r1=303278&r2=<wbr>303279&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Support/<wbr>CrashRecoveryContext.cpp (original)<br>
+++ llvm/trunk/lib/Support/<wbr>CrashRecoveryContext.cpp Wed May 17 13:16:17 2017<br>
@@ -78,6 +78,9 @@ static bool gCrashRecoveryEnabled = fals<br>
static ManagedStatic<sys::<wbr>ThreadLocal<const CrashRecoveryContext>><br>
tlIsRecoveringFromCrash;<br>
<br>
+static void installExceptionOrSignalHandle<wbr>rs();<br>
+static void uninstallExceptionOrSignalHand<wbr>lers();<br>
+<br>
CrashRecoveryContextCleanup::<wbr>~CrashRecoveryContextCleanup() {}<br>
<br>
CrashRecoveryContext::~<wbr>CrashRecoveryContext() {<br>
@@ -113,6 +116,23 @@ CrashRecoveryContext *CrashRecoveryConte<br>
return CRCI->CRC;<br>
}<br>
<br>
+void CrashRecoveryContext::Enable() {<br>
+ sys::ScopedLock L(*gCrashRecoveryContextMutex)<wbr>;<br>
+ // FIXME: Shouldn't this be a refcount or something?<br>
+ if (gCrashRecoveryEnabled)<br>
+ return;<br>
+ gCrashRecoveryEnabled = true;<br>
+ installExceptionOrSignalHandle<wbr>rs();<br>
+}<br>
+<br>
+void CrashRecoveryContext::Disable(<wbr>) {<br>
+ sys::ScopedLock L(*gCrashRecoveryContextMutex)<wbr>;<br>
+ if (!gCrashRecoveryEnabled)<br>
+ return;<br>
+ gCrashRecoveryEnabled = false;<br>
+ uninstallExceptionOrSignalHand<wbr>lers();<br>
+}<br>
+<br>
void CrashRecoveryContext::<wbr>registerCleanup(<wbr>CrashRecoveryContextCleanup *cleanup)<br>
{<br>
if (!cleanup)<br>
@@ -140,27 +160,56 @@ CrashRecoveryContext::<wbr>unregisterCleanup(<br>
delete cleanup;<br>
}<br>
<br>
-#ifdef LLVM_ON_WIN32<br>
+#if defined(_MSC_VER)<br>
+// If _MSC_VER is defined, we must have SEH. Use it if it's available. It's way<br>
+// better than VEH. Vectored exception handling catches all exceptions happening<br>
+// on the thread with installed exception handlers, so it can interfere with<br>
+// internal exception handling of other libraries on that thread. SEH works<br>
+// exactly as you would expect normal exception handling to work: it only<br>
+// catches exceptions if they would bubble out from the stack frame with __try /<br>
+// __except.<br>
<br>
-#include "Windows/WindowsSupport.h"<br>
+static void installExceptionOrSignalHandle<wbr>rs() {}<br>
+static void uninstallExceptionOrSignalHand<wbr>lers() {}<br>
+<br>
+bool CrashRecoveryContext::<wbr>RunSafely(function_ref<void()> Fn) {<br>
+ if (!gCrashRecoveryEnabled) {<br>
+ Fn();<br>
+ return true;<br>
+ }<br>
+<br>
+ bool Result = true;<br>
+ __try {<br>
+ Fn();<br>
+ } __except (1) { // Catch any exception.<br>
+ Result = false;<br>
+ }<br>
+ return Result;<br>
+}<br>
+<br>
+#else // !_MSC_VER<br>
<br>
-// On Windows, we can make use of vectored exception handling to<br>
-// catch most crashing situations. Note that this does mean<br>
-// we will be alerted of exceptions *before* structured exception<br>
-// handling has the opportunity to catch it. But that isn't likely<br>
-// to cause problems because nowhere in the project is SEH being<br>
-// used.<br>
+#if defined(LLVM_ON_WIN32)<br>
+// This is a non-MSVC compiler, probably mingw gcc or clang without<br>
+// -fms-extensions. Use vectored exception handling (VEH).<br>
//<br>
-// Vectored exception handling is built on top of SEH, and so it<br>
-// works on a per-thread basis.<br>
+// On Windows, we can make use of vectored exception handling to catch most<br>
+// crashing situations. Note that this does mean we will be alerted of<br>
+// exceptions *before* structured exception handling has the opportunity to<br>
+// catch it. Unfortunately, this causes problems in practice with other code<br>
+// running on threads with LLVM crash recovery contexts, so we would like to<br>
+// eventually move away from VEH.<br>
+//<br>
+// Vectored works on a per-thread basis, which is an advantage over<br>
+// SetUnhandledExceptionFilter. SetUnhandledExceptionFilter also doesn't have<br>
+// any native support for chaining exception handlers, but VEH allows more than<br>
+// one.<br>
//<br>
// The vectored exception handler functionality was added in Windows<br>
// XP, so if support for older versions of Windows is required,<br>
// it will have to be added.<br>
-//<br>
-// If we want to support as far back as Win2k, we could use the<br>
-// SetUnhandledExceptionFilter API, but there's a risk of that<br>
-// being entirely overwritten (it's not a chain).<br>
+<br>
+#include "Windows/WindowsSupport.h"<br>
<br>
static LONG CALLBACK ExceptionHandler(PEXCEPTION_<wbr>POINTERS ExceptionInfo)<br>
{<br>
@@ -203,14 +252,7 @@ static LONG CALLBACK ExceptionHandler(PE<br>
// non-NULL, valid VEH handles, or NULL.<br>
static sys::ThreadLocal<const void> sCurrentExceptionHandle;<br>
<br>
-void CrashRecoveryContext::Enable() {<br>
- sys::ScopedLock L(*gCrashRecoveryContextMutex)<wbr>;<br>
-<br>
- if (gCrashRecoveryEnabled)<br>
- return;<br>
-<br>
- gCrashRecoveryEnabled = true;<br>
-<br>
+static void installExceptionOrSignalHandle<wbr>rs() {<br>
// We can set up vectored exception handling now. We will install our<br>
// handler as the front of the list, though there's no assurances that<br>
// it will remain at the front (another call could install itself before<br>
@@ -219,14 +261,7 @@ void CrashRecoveryContext::Enable() {<br>
sCurrentExceptionHandle.set(<wbr>handle);<br>
}<br>
<br>
-void CrashRecoveryContext::Disable(<wbr>) {<br>
- sys::ScopedLock L(*gCrashRecoveryContextMutex)<wbr>;<br>
-<br>
- if (!gCrashRecoveryEnabled)<br>
- return;<br>
-<br>
- gCrashRecoveryEnabled = false;<br>
-<br>
+static void uninstallExceptionOrSignalHand<wbr>lers() {<br>
PVOID currentHandle = const_cast<PVOID>(<wbr>sCurrentExceptionHandle.get())<wbr>;<br>
if (currentHandle) {<br>
// Now we can remove the vectored exception handler from the chain<br>
@@ -237,7 +272,7 @@ void CrashRecoveryContext::Disable(<wbr>) {<br>
}<br>
}<br>
<br>
-#else<br>
+#else // !LLVM_ON_WIN32<br>
<br>
// Generic POSIX implementation.<br>
//<br>
@@ -289,14 +324,7 @@ static void CrashRecoverySignalHandler(i<br>
const_cast<<wbr>CrashRecoveryContextImpl*>(<wbr>CRCI)->HandleCrash();<br>
}<br>
<br>
-void CrashRecoveryContext::Enable() {<br>
- sys::ScopedLock L(*gCrashRecoveryContextMutex)<wbr>;<br>
-<br>
- if (gCrashRecoveryEnabled)<br>
- return;<br>
-<br>
- gCrashRecoveryEnabled = true;<br>
-<br>
+static void installExceptionOrSignalHandle<wbr>rs() {<br>
// Setup the signal handler.<br>
struct sigaction Handler;<br>
Handler.sa_handler = CrashRecoverySignalHandler;<br>
@@ -308,20 +336,13 @@ void CrashRecoveryContext::Enable() {<br>
}<br>
}<br>
<br>
-void CrashRecoveryContext::Disable(<wbr>) {<br>
- sys::ScopedLock L(*gCrashRecoveryContextMutex)<wbr>;<br>
-<br>
- if (!gCrashRecoveryEnabled)<br>
- return;<br>
-<br>
- gCrashRecoveryEnabled = false;<br>
-<br>
+static void uninstallExceptionOrSignalHand<wbr>lers() {<br>
// Restore the previous signal handlers.<br>
for (unsigned i = 0; i != NumSignals; ++i)<br>
sigaction(Signals[i], &PrevActions[i], nullptr);<br>
}<br>
<br>
-#endif<br>
+#endif // !LLVM_ON_WIN32<br>
<br>
bool CrashRecoveryContext::<wbr>RunSafely(function_ref<void()> Fn) {<br>
// If crash recovery is disabled, do nothing.<br>
@@ -339,6 +360,8 @@ bool CrashRecoveryContext::<wbr>RunSafely(fun<br>
return true;<br>
}<br>
<br>
+#endif // !_MSC_VER<br>
+<br>
void CrashRecoveryContext::<wbr>HandleCrash() {<br>
CrashRecoveryContextImpl *CRCI = (CrashRecoveryContextImpl *) Impl;<br>
assert(CRCI && "Crash recovery context never initialized!");<br>
<br>
Modified: llvm/trunk/unittests/Support/<wbr>CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CMakeLists.txt?rev=303279&r1=303278&r2=303279&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/unittests/<wbr>Support/CMakeLists.txt?rev=<wbr>303279&r1=303278&r2=303279&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/unittests/Support/<wbr>CMakeLists.txt (original)<br>
+++ llvm/trunk/unittests/Support/<wbr>CMakeLists.txt Wed May 17 13:16:17 2017<br>
@@ -11,6 +11,7 @@ add_llvm_unittest(SupportTests<br>
BlockFrequencyTest.cpp<br>
BranchProbabilityTest.cpp<br>
CachePruningTest.cpp<br>
+ CrashRecoveryTest.cpp<br>
Casting.cpp<br>
Chrono.cpp<br>
CommandLineTest.cpp<br>
<br>
Added: llvm/trunk/unittests/Support/<wbr>CrashRecoveryTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CrashRecoveryTest.cpp?rev=303279&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/unittests/<wbr>Support/CrashRecoveryTest.cpp?<wbr>rev=303279&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/unittests/Support/<wbr>CrashRecoveryTest.cpp (added)<br>
+++ llvm/trunk/unittests/Support/<wbr>CrashRecoveryTest.cpp Wed May 17 13:16:17 2017<br>
@@ -0,0 +1,83 @@<br>
+//===- llvm/unittest/Support/<wbr>CrashRecoveryTest.cpp ------------------------===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
+<br>
+#include "llvm/Support/<wbr>CrashRecoveryContext.h"<br>
+#include "llvm/Support/Compiler.h"<br>
+#include "gtest/gtest.h"<br>
+<br>
+#ifdef LLVM_ON_WIN32<br>
+#define WIN32_LEAN_AND_MEAN<br>
+#define NOGDI<br>
+#include <windows.h><br>
+#endif<br>
+<br>
+using namespace llvm;<br>
+using namespace llvm::sys;<br>
+<br>
+static int GlobalInt = 0;<br>
+static void nullDeref() { *(volatile int *)nullptr = 0; }<br>
+static void incrementGlobal() { ++GlobalInt; }<br>
+static void llvmTrap() { LLVM_BUILTIN_TRAP; }<br>
+<br>
+TEST(CrashRecoveryTest, Basic) {<br>
+ llvm::CrashRecoveryContext::<wbr>Enable();<br>
+ GlobalInt = 0;<br>
+ EXPECT_TRUE(<wbr>CrashRecoveryContext().<wbr>RunSafely(incrementGlobal));<br>
+ EXPECT_EQ(1, GlobalInt);<br>
+ EXPECT_FALSE(<wbr>CrashRecoveryContext().<wbr>RunSafely(nullDeref));<br>
+ EXPECT_FALSE(<wbr>CrashRecoveryContext().<wbr>RunSafely(llvmTrap));<br>
+}<br>
+<br>
+struct IncrementGlobalCleanup : CrashRecoveryContextCleanup {<br>
+ IncrementGlobalCleanup(<wbr>CrashRecoveryContext *CRC)<br>
+ : CrashRecoveryContextCleanup(<wbr>CRC) {}<br>
+ virtual void recoverResources() { ++GlobalInt; }<br>
+};<br>
+<br>
+static void noop() {}<br>
+<br>
+TEST(CrashRecoveryTest, Cleanup) {<br>
+ llvm::CrashRecoveryContext::<wbr>Enable();<br>
+ GlobalInt = 0;<br>
+ {<br>
+ CrashRecoveryContext CRC;<br>
+ CRC.registerCleanup(new IncrementGlobalCleanup(&CRC));<br>
+ EXPECT_TRUE(CRC.RunSafely(<wbr>noop));<br>
+ } // run cleanups<br>
+ EXPECT_EQ(1, GlobalInt);<br>
+<br>
+ GlobalInt = 0;<br>
+ {<br>
+ CrashRecoveryContext CRC;<br>
+ CRC.registerCleanup(new IncrementGlobalCleanup(&CRC));<br>
+ EXPECT_FALSE(CRC.RunSafely(<wbr>nullDeref));<br>
+ } // run cleanups<br>
+ EXPECT_EQ(1, GlobalInt);<br>
+}<br>
+<br>
+#ifdef LLVM_ON_WIN32<br>
+static void raiseIt() {<br>
+ RaiseException(123, EXCEPTION_NONCONTINUABLE, 0, NULL);<br>
+}<br>
+<br>
+TEST(CrashRecoveryTest, RaiseException) {<br>
+ llvm::CrashRecoveryContext::<wbr>Enable();<br>
+ EXPECT_FALSE(<wbr>CrashRecoveryContext().<wbr>RunSafely(raiseIt));<br>
+}<br>
+<br>
+static void outputString() {<br>
+ OutputDebugStringA("output for debugger\n");<br>
+}<br>
+<br>
+TEST(CrashRecoveryTest, CallOutputDebugString) {<br>
+ llvm::CrashRecoveryContext::<wbr>Enable();<br>
+ EXPECT_TRUE(<wbr>CrashRecoveryContext().<wbr>RunSafely(outputString));<br>
+}<br>
+<br>
+#endif<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</div></div></blockquote></div><br></div>