[llvm] r303279 - Re-land r303274: "[CrashRecovery] Use SEH __try instead of VEH when available"

Reid Kleckner via llvm-commits llvm-commits at lists.llvm.org
Wed May 17 15:56:05 PDT 2017


Should be fixed after r303311. I filed
https://bugs.llvm.org//show_bug.cgi?id=33081 after chatting with Richard
about what ubsan should ideally do here.

On Wed, May 17, 2017 at 1:13 PM, Vitaly Buka <vitalybuka at google.com> wrote:

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


More information about the llvm-commits mailing list