From 1b6e2ef62cec9608d160ea25d99080f0e2964ee5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 May 2024 13:30:14 -0400 Subject: On Windows, defend test.cpp against structured exceptions too. Since August 2023, we've seen occasional GitHub Windows build test runs terminate with 0xC00000FD: stack overflow. We've usually responded by bumping up the default coroutine stack size. On closer examination, it's always llleap_test.cpp that blows up that way -- and llleap_test.cpp doesn't appear to use coroutines at all. So apparently we've been consuming more address space for ALL viewer coroutines without actually addressing the problem. Reset the default coroutine stack size to where it was before we started bumping it up in response to these llleap_test.cpp stack overflow failures. Note that LLCoros already catches and reports Windows structured exceptions, underscoring that the observed stack overflow is not from within a coroutine. While at it, restore the Windows llleap_test.cpp data volume to match Posix. We think the problem that led to reducing that data volume was an APR bug, which we hope has been fixed. Equip test.cpp, the test driver program for all our TUT unit and integration tests, with a Windows structured exception handler. Try to treat a Windows structured exception as a test failure -- instead of silently terminating with 0xC00000FD. Moreover, when a structured exception occurs, output a stack trace so we can try to track it down. --- indra/llcommon/llcoros.cpp | 4 +- indra/llcommon/tests/llleap_test.cpp | 2 +- indra/test/test.cpp | 125 +++++++++++++++++++++++++++++------ 3 files changed, 107 insertions(+), 24 deletions(-) diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index aa8eca7d90..a70e3d9ae7 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -123,7 +123,7 @@ LLCoros::LLCoros(): // Previously we used // boost::context::guarded_stack_allocator::default_stacksize(); // empirically this is insufficient. - mStackSize(1024*1024), + mStackSize(512*1024), // mCurrent does NOT own the current CoroData instance -- it simply // points to it. So initialize it with a no-op deleter. mCurrent{ [](CoroData*){} } @@ -155,7 +155,7 @@ void LLCoros::cleanupSingleton() // don't use llcoro::suspend() because that module depends // on this one // This will yield current(main) thread and will let active - // corutines run once + // coroutines run once boost::this_fiber::yield(); } printActiveCoroutines("after pumping"); diff --git a/indra/llcommon/tests/llleap_test.cpp b/indra/llcommon/tests/llleap_test.cpp index fa48bcdefd..3fb25b4cef 100644 --- a/indra/llcommon/tests/llleap_test.cpp +++ b/indra/llcommon/tests/llleap_test.cpp @@ -35,7 +35,7 @@ // causes Windows abdominal pain such that it later fails code-signing in some // mysterious way. Entirely suppressing these LLLeap tests pushes the failure // rate MUCH lower. Can we re-enable them with a smaller data size on Windows? -const size_t BUFFERED_LENGTH = 100*1024; +const size_t BUFFERED_LENGTH = 1023*1024; #else // not Windows const size_t BUFFERED_LENGTH = 1023*1024; // try wrangling just under a megabyte of data diff --git a/indra/test/test.cpp b/indra/test/test.cpp index 61a4eb07c5..0b2abbc650 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -68,10 +68,7 @@ #pragma warning (pop) #endif -#include -#include -#include -#include +#include #include @@ -181,10 +178,6 @@ public: LLTestCallback(bool verbose_mode, std::ostream *stream, std::shared_ptr replayer) : mVerboseMode(verbose_mode), - mTotalTests(0), - mPassedTests(0), - mFailedTests(0), - mSkippedTests(0), // By default, capture a shared_ptr to std::cout, with a no-op "deleter" // so that destroying the shared_ptr makes no attempt to delete std::cout. mStream(std::shared_ptr(&std::cout, [](std::ostream*){})), @@ -220,6 +213,8 @@ public: virtual void group_started(const std::string& name) { LL_INFOS("TestRunner")<<"Unit test group_started name=" << name << LL_ENDL; *mStream << "Unit test group_started name=" << name << std::endl; + mGroup = name; + mGroupTests = 0; super::group_started(name); } @@ -232,6 +227,7 @@ public: virtual void test_completed(const tut::test_result& tr) { ++mTotalTests; + ++mGroupTests; // If this test failed, dump requested log messages BEFORE stating the // test result. @@ -319,12 +315,15 @@ public: super::run_completed(); } + std::string mGroup; + int mGroupTests{ 0 }; + protected: - bool mVerboseMode; - int mTotalTests; - int mPassedTests; - int mFailedTests; - int mSkippedTests; + bool mVerboseMode{ false }; + int mTotalTests{ 0 }; + int mPassedTests{ 0 }; + int mFailedTests{ 0 }; + int mSkippedTests{ 0 }; std::shared_ptr mStream; std::shared_ptr mReplayer; }; @@ -520,6 +519,57 @@ void wouldHaveCrashed(const std::string& message) static LLTrace::ThreadRecorder* sMasterThreadRecorder = NULL; +// this is used in platform-generic code -- define outside #if LL_WINDOWS +struct Windows_SEH_exception: public std::runtime_error +{ + Windows_SEH_exception(const std::string& what): std::runtime_error(what) {} +}; + +#if LL_WINDOWS + +static const U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific + +U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS*) +{ + if (code == STATUS_MSC_EXCEPTION) + { + // C++ exception, go on -- but TUT is supposed to have caught those already?! + return EXCEPTION_CONTINUE_SEARCH; + } + else + { + // This is a non-C++ exception, e.g. hardware check. + // By the time the handler gets control, the stack has been unwound, + // so report the stack trace now at filter() time. + std::cerr << boost::stacktrace::stacktrace() << std::endl; + // pass control into the handler block + return EXCEPTION_EXECUTE_HANDLER; + } +} + +template +void seh_catcher(CALLABLE0&& trycode, CALLABLE1&& handler) +{ + __try + { + trycode(); + } + __except (seh_filter(GetExceptionCode(), GetExceptionInformation())) + { + handler(GetExceptionCode()); + } +} + +#else // not LL_WINDOWS + +template +void seh_catcher(CALLABLE0&& trycode, CALLABLE1&&) +{ + trycode(); +} + +#endif // not LL_WINDOWS + int main(int argc, char **argv) { // The following line must be executed to initialize Google Mock @@ -658,14 +708,47 @@ int main(int argc, char **argv) // a chained_callback subclass must be linked with previous mycallback->link(); - if(test_group.empty()) - { - tut::runner.get().run_tests(); - } - else - { - tut::runner.get().run_tests(test_group); - } + seh_catcher( + // __try + [test_group] + { + if(test_group.empty()) + { + tut::runner.get().run_tests(); + } + else + { + tut::runner.get().run_tests(test_group); + } + }, + // __except + [mycallback](U32 code) + { + static std::map codes = { + { 0xC0000005, "Access Violation" }, + { 0xC00000FD, "Stack Overflow" }, + // ... continue filling in as desired + }; + + auto found{ codes.find(code) }; + const char* name = ((found == codes.end())? "unknown" : found->second); + auto msg{ stringize("test threw ", std::hex, code, " (", name, ")") }; + + // Instead of bombing the whole test run, report this as a test + // failure. Arguably, catching structured exceptions should be + // hacked into TUT itself. + mycallback->test_completed(tut::test_result( + mycallback->mGroup, + mycallback->mGroupTests+1, // test within group + "unknown", // test name + tut::test_result::ex, // result: exception + // we don't have to throw this exception subclass to use it to + // populate the test_result struct + Windows_SEH_exception(msg))); + // we've left the TUT framework -- finish up by hand + mycallback->group_completed(mycallback->mGroup); + mycallback->run_completed(); + }); bool success = (mycallback->getFailedTests() == 0); -- cgit v1.2.3 From f10ec3073cde4810fe01753c1e5b82cb8591a1cb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 May 2024 14:19:20 -0400 Subject: Need magic #define for on Mac. --- indra/test/test.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/indra/test/test.cpp b/indra/test/test.cpp index 0b2abbc650..c4cf4b2d28 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -68,6 +68,11 @@ #pragma warning (pop) #endif +// On Mac, got: +// #error "Boost.Stacktrace requires `_Unwind_Backtrace` function. Define +// `_GNU_SOURCE` macro or `BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED` if +// _Unwind_Backtrace is available without `_GNU_SOURCE`." +#define BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED #include #include -- cgit v1.2.3 From 3c0710172e03329004a2962e0313d883b143d3ad Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 May 2024 16:05:43 -0400 Subject: Try to fix release_run logic in build.yaml. --- .github/workflows/build.yaml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index b60f787a9c..b301e88f48 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -27,7 +27,7 @@ jobs: # When you want to use a string variable as a workflow YAML boolean, it's # important to ensure it's the empty string when false. If you omit || '', # its value when false is "false", which is interpreted as true. - RELEASE_RUN: ${{ (github.event.inputs.release_run || github.ref_type == 'tag' && startsWith(github.ref_name, 'Second_Life')) && 'Y' || '' }} + RELEASE_RUN: ${{ (github.event.inputs.release_run || (github.ref_type == 'tag' && startsWith(github.ref_name, 'Second_Life'))) && 'Y' || '' }} steps: - name: Set Variable id: setvar @@ -403,7 +403,11 @@ jobs: release: needs: [setvar, build, sign-and-package-windows, sign-and-package-mac] runs-on: ubuntu-latest - if: needs.setvar.outputs.release_run + # action-gh-release requires a tag (presumably for automatic generation of + # release notes). Possible TODO: if we arrive here but do not have a + # suitable tag for github.sha, create one? If we do that, of course remove + # this == 'tag' condition. + if: needs.setvar.outputs.release_run && github.ref_type == 'tag' steps: - uses: actions/download-artifact@v4 with: -- cgit v1.2.3 From 7e381d62a8e7f64f1bae96a8ef6b38316427381b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 May 2024 16:12:23 -0400 Subject: Try determining which-branch once for all platforms. --- .github/workflows/build.yaml | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index b301e88f48..a33b4a88c7 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -21,6 +21,8 @@ jobs: runs-on: ubuntu-latest outputs: release_run: ${{ steps.setvar.outputs.release_run }} + branch: ${{ steps.which-branch.outputs.branch }} + relnotes: ${{ steps.which-branch.outputs.relnotes }} env: # Build with a tag like "Second_Life#abcdef0" to generate a release page # (used for builds we are planning to deploy). @@ -35,6 +37,13 @@ jobs: run: | echo "release_run=$RELEASE_RUN" >> "$GITHUB_OUTPUT" + - name: Determine source branch + id: which-branch + if: env.BUILD + uses: secondlife/viewer-build-util/which-branch@v2 + with: + token: ${{ github.token }} + build: needs: setvar strategy: @@ -52,8 +61,6 @@ jobs: outputs: viewer_channel: ${{ steps.build.outputs.viewer_channel }} viewer_version: ${{ steps.build.outputs.viewer_version }} - viewer_branch: ${{ steps.which-branch.outputs.branch }} - relnotes: ${{ steps.which-branch.outputs.relnotes }} imagename: ${{ steps.build.outputs.imagename }} env: AUTOBUILD_ADDRSIZE: 64 @@ -136,19 +143,12 @@ jobs: if: env.BUILD && runner.os == 'Windows' run: choco install nsis-unicode - - name: Determine source branch - id: which-branch - if: env.BUILD - uses: secondlife/viewer-build-util/which-branch@v2 - with: - token: ${{ github.token }} - - name: Build id: build if: env.BUILD shell: bash env: - AUTOBUILD_VCS_BRANCH: ${{ steps.which-branch.outputs.branch }} + AUTOBUILD_VCS_BRANCH: ${{ needs.setvar.outputs.branch }} RUNNER_OS: ${{ runner.os }} run: | # set up things the viewer's build.sh script expects @@ -437,7 +437,7 @@ jobs: Build ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} ${{ needs.build.outputs.viewer_channel }} ${{ needs.build.outputs.viewer_version }} - ${{ needs.build.outputs.relnotes }} + ${{ needs.setvar.outputs.relnotes }} prerelease: true generate_release_notes: true target_commitish: ${{ github.sha }} -- cgit v1.2.3 From 006b00b47d8fc0ea89ca799c191b91d49d8f6bb6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 May 2024 16:23:35 -0400 Subject: Add diagnostic output to setvar build step. --- .github/workflows/build.yaml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index a33b4a88c7..5854a512d1 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -35,11 +35,14 @@ jobs: id: setvar shell: bash run: | + echo "release_run = ${{ github.event.inputs.release_run }}" + echo "ref_type = ${{ github.ref_type }}" + echo "RELEASE_RUN = $RELEASE_RUN" echo "release_run=$RELEASE_RUN" >> "$GITHUB_OUTPUT" + exit 1 - name: Determine source branch id: which-branch - if: env.BUILD uses: secondlife/viewer-build-util/which-branch@v2 with: token: ${{ github.token }} -- cgit v1.2.3 From 189711f41cb262a958ccdabfe9687caa4eb17e79 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 May 2024 16:27:35 -0400 Subject: Try harder to interpret github.event.inputs.release_run checkbox. --- .github/workflows/build.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 5854a512d1..27516aacf0 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -29,7 +29,7 @@ jobs: # When you want to use a string variable as a workflow YAML boolean, it's # important to ensure it's the empty string when false. If you omit || '', # its value when false is "false", which is interpreted as true. - RELEASE_RUN: ${{ (github.event.inputs.release_run || (github.ref_type == 'tag' && startsWith(github.ref_name, 'Second_Life'))) && 'Y' || '' }} + RELEASE_RUN: ${{ (github.event.inputs.release_run != 'false' || (github.ref_type == 'tag' && startsWith(github.ref_name, 'Second_Life'))) && 'Y' || '' }} steps: - name: Set Variable id: setvar -- cgit v1.2.3 From fe14534c57b97c2ab3bfd9eade681f6a404f72a2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 May 2024 16:37:33 -0400 Subject: Fix test for github.event.inputs.release_run. --- .github/workflows/build.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 27516aacf0..4f35b2c813 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -26,20 +26,20 @@ jobs: env: # Build with a tag like "Second_Life#abcdef0" to generate a release page # (used for builds we are planning to deploy). + # Even though inputs.release_run is specified with type boolean, which + # correctly presents a checkbox, its *value* is a GH workflow string + # 'true' or 'false'. If you simply test github.event.inputs.release_run, + # it always evaluates as true because it's a non-empty string either way. # When you want to use a string variable as a workflow YAML boolean, it's # important to ensure it's the empty string when false. If you omit || '', - # its value when false is "false", which is interpreted as true. + # its value when false is "false", which (again) is interpreted as true. RELEASE_RUN: ${{ (github.event.inputs.release_run != 'false' || (github.ref_type == 'tag' && startsWith(github.ref_name, 'Second_Life'))) && 'Y' || '' }} steps: - name: Set Variable id: setvar shell: bash run: | - echo "release_run = ${{ github.event.inputs.release_run }}" - echo "ref_type = ${{ github.ref_type }}" - echo "RELEASE_RUN = $RELEASE_RUN" echo "release_run=$RELEASE_RUN" >> "$GITHUB_OUTPUT" - exit 1 - name: Determine source branch id: which-branch -- cgit v1.2.3 From f06f84aed26fa7ed294a14ed12dae58c063b0aa3 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 17 May 2024 09:16:53 -0400 Subject: Don't try to report a stack trace on stack overflow (sigh) --- indra/test/test.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/indra/test/test.cpp b/indra/test/test.cpp index c4cf4b2d28..1239b34d04 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -532,7 +532,8 @@ struct Windows_SEH_exception: public std::runtime_error #if LL_WINDOWS -static const U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific +static constexpr U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific +static constexpr U32 STATUS_STACK_FULL = 0xC00000FD; U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS*) { @@ -546,7 +547,13 @@ U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS*) // This is a non-C++ exception, e.g. hardware check. // By the time the handler gets control, the stack has been unwound, // so report the stack trace now at filter() time. - std::cerr << boost::stacktrace::stacktrace() << std::endl; + // Sadly, even though, at the time of this writing, stack overflow is + // the problem we would most like to diagnose, calling another + // function when the stack is already blown only terminates us faster. + if (code != STATUS_STACK_FULL) + { + std::cerr << boost::stacktrace::stacktrace() << std::endl; + } // pass control into the handler block return EXCEPTION_EXECUTE_HANDLER; } -- cgit v1.2.3 From 71d777ea126e7f02cb46c11bdb606094ca06f75c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 24 May 2024 17:34:04 -0400 Subject: Promote seh_catcher() et al. to llexception.{h,cpp} for general use. --- indra/llcommon/llcoros.cpp | 51 +--------------------- indra/llcommon/llexception.cpp | 50 ++++++++++++++++++---- indra/llcommon/llexception.h | 86 ++++++++++++++++++++++++++++++++++++-- indra/llwindow/llwindowwin32.cpp | 13 +----- indra/newview/llappviewerwin32.cpp | 36 ++-------------- indra/newview/llfeaturemanager.cpp | 36 ++-------------- indra/test/test.cpp | 68 +----------------------------- 7 files changed, 137 insertions(+), 203 deletions(-) diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index a70e3d9ae7..f3943b7138 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -286,55 +286,6 @@ std::string LLCoros::launch(const std::string& prefix, const callable_t& callabl return name; } -namespace -{ - -#if LL_WINDOWS - -static const U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific - -U32 exception_filter(U32 code, struct _EXCEPTION_POINTERS *exception_infop) -{ - if (code == STATUS_MSC_EXCEPTION) - { - // C++ exception, go on - return EXCEPTION_CONTINUE_SEARCH; - } - else - { - // handle it - return EXCEPTION_EXECUTE_HANDLER; - } -} - -void sehandle(const LLCoros::callable_t& callable) -{ - __try - { - callable(); - } - __except (exception_filter(GetExceptionCode(), GetExceptionInformation())) - { - // convert to C++ styled exception - // Note: it might be better to use _se_set_translator - // if you want exception to inherit full callstack - char integer_string[512]; - sprintf(integer_string, "SEH, code: %lu\n", GetExceptionCode()); - throw std::exception(integer_string); - } -} - -#else // ! LL_WINDOWS - -inline void sehandle(const LLCoros::callable_t& callable) -{ - callable(); -} - -#endif // ! LL_WINDOWS - -} // anonymous namespace - // Top-level wrapper around caller's coroutine callable. // Normally we like to pass strings and such by const reference -- but in this // case, we WANT to copy both the name and the callable to our local stack! @@ -348,7 +299,7 @@ void LLCoros::toplevel(std::string name, callable_t callable) // run the code the caller actually wants in the coroutine try { - sehandle(callable); + seh_catcher(callable); } catch (const Stop& exc) { diff --git a/indra/llcommon/llexception.cpp b/indra/llcommon/llexception.cpp index c0154a569f..74b33f1e3b 100644 --- a/indra/llcommon/llexception.cpp +++ b/indra/llcommon/llexception.cpp @@ -15,7 +15,12 @@ #include "llexception.h" // STL headers // std headers +#include +#include #include +#if LL_WINDOWS +#include +#endif // LL_WINDOWS // external library headers #include #include @@ -29,7 +34,6 @@ // On Windows, header-only implementation causes macro collisions -- use // prebuilt library #define BOOST_STACKTRACE_LINK -#include #endif // LL_WINDOWS #include @@ -94,15 +98,34 @@ void annotate_exception_(boost::exception& exc) // For windows SEH exception handling we sometimes need a filter that will // separate C++ exceptions from C SEH exceptions -static const U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific +static constexpr U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific +static constexpr U32 STATUS_STACK_FULL = 0xC00000FD; -U32 msc_exception_filter(U32 code, struct _EXCEPTION_POINTERS *exception_infop) +U32 ll_seh_filter( + std::string& stacktrace, + std::function filter, + U32 code, + struct _EXCEPTION_POINTERS* exception_infop) { - const auto stack = to_string(boost::stacktrace::stacktrace()); - LL_WARNS() << "SEH Exception handled (that probably shouldn't be): Code " << code - << "\n Stack trace: \n" - << stack << LL_ENDL; + // By the time the handler gets control, the stack has been unwound, + // so report the stack trace now at filter() time. + // Even though stack overflow is a problem we would very much like to + // diagnose, calling another function when the stack is already blown only + // terminates us faster. + if (code == STATUS_STACK_FULL) + { + stacktrace = "(stack overflow, no traceback)"; + } + else + { + stacktrace = boost::stacktrace::stacktrace().to_string(); + } + + return filter(code, exception_infop); +} +U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS*) +{ if (code == STATUS_MSC_EXCEPTION) { // C++ exception, go on @@ -110,9 +133,20 @@ U32 msc_exception_filter(U32 code, struct _EXCEPTION_POINTERS *exception_infop) } else { - // handle it + // This is a non-C++ exception, e.g. hardware check. return EXCEPTION_EXECUTE_HANDLER; } } +void seh_rethrow(U32 code, const std::string& stacktrace) +{ + std::ostringstream out; + out << "Windows exception 0x" << std::hex << code; + if (! stacktrace.empty()) + { + out << '\n' << stacktrace; + } + LLTHROW(Windows_SEH_exception(out.str())); +} + #endif //LL_WINDOWS diff --git a/indra/llcommon/llexception.h b/indra/llcommon/llexception.h index 68e609444e..3e50678b44 100644 --- a/indra/llcommon/llexception.h +++ b/indra/llcommon/llexception.h @@ -102,14 +102,94 @@ void crash_on_unhandled_exception_(const char*, int, const char*, const std::str log_unhandled_exception_(__FILE__, __LINE__, BOOST_CURRENT_FUNCTION, CONTEXT) void log_unhandled_exception_(const char*, int, const char*, const std::string&); +/***************************************************************************** +* Structured Exception Handling +*****************************************************************************/ +// this is used in platform-generic code -- define outside #if LL_WINDOWS +struct Windows_SEH_exception: public std::runtime_error +{ + Windows_SEH_exception(const std::string& what): std::runtime_error(what) {} +}; + +#if LL_WINDOWS //------------------------------------------------------------- + +#include + +// triadic variant specifies try(), filter(U32, struct _EXCEPTION_POINTERS*), +// handler(U32, const std::string& stacktrace) +// stacktrace may or may not be available +template +auto seh_catcher(TRYCODE&& trycode, FILTER&& filter, HANDLER&& handler) +{ + // don't try to construct a std::function at the moment of Structured Exception + std::function + filter_function(std::forward(filter)); + std::string stacktrace; + __try + { + return std::forward(trycode)(); + } + __except (ll_seh_filter( + stacktrace, + filter_function, + GetExceptionCode(), + GetExceptionInformation())) + { + return std::forward(handler)(GetExceptionCode(), stacktrace); + } +} + +// dyadic variant specifies try(), handler(U32, stacktrace), assumes default filter +template +auto seh_catcher(TRYCODE&& trycode, HANDLER&& handler) +{ + return seh_catcher( + std::forward(trycode), + seh_filter, + std::forward(handler)); +} -#if LL_WINDOWS +// monadic variant specifies try(), assumes default filter and handler +template +auto seh_catcher(TRYCODE&& trycode) +{ + return seh_catcher( + std::forward(trycode), + seh_filter, + seh_rethrow); +} // SEH exception filtering for use in __try __except // Separates C++ exceptions from C SEH exceptions // Todo: might be good idea to do some kind of seh_to_msc_wrapper(function, ARGS&&); -U32 msc_exception_filter(U32 code, struct _EXCEPTION_POINTERS *exception_infop); +U32 ll_seh_filter( + std::string& stacktrace, + std::function filter, + U32 code, + struct _EXCEPTION_POINTERS* exception_infop); +U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS* exception_infop); +void seh_rethrow(U32 code, const std::string& stacktrace); + +#else // not LL_WINDOWS ----------------------------------------------------- + +template +auto seh_catcher(TRYCODE&& trycode, FILTER&&, HANDLER&&) +{ + return std::forward(trycode)(); +} + +template +auto seh_catcher(TRYCODE&& trycode, HANDLER&&) +{ + return std::forward(trycode)(); +} + +template +auto seh_catcher(TRYCODE&& trycode) +{ + return std::forward(trycode)(); +} -#endif //LL_WINDOWS +#endif // not LL_WINDOWS ----------------------------------------------------- #endif /* ! defined(LL_LLEXCEPTION_H) */ diff --git a/indra/llwindow/llwindowwin32.cpp b/indra/llwindow/llwindowwin32.cpp index d6b93b93d9..1535d00d50 100644 --- a/indra/llwindow/llwindowwin32.cpp +++ b/indra/llwindow/llwindowwin32.cpp @@ -163,18 +163,7 @@ HGLRC SafeCreateContext(HDC &hdc) GLuint SafeChoosePixelFormat(HDC &hdc, const PIXELFORMATDESCRIPTOR *ppfd) { - __try - { - return ChoosePixelFormat(hdc, ppfd); - } - __except (EXCEPTION_EXECUTE_HANDLER) - { - // convert to C++ styled exception - // C exception don't allow classes, so it's a regular char array - char integer_string[32]; - sprintf(integer_string, "SEH, code: %lu\n", GetExceptionCode()); - throw std::exception(integer_string); - } + return seh_catcher(ChoosePixelFormat(hdc, ppfd)); } //static diff --git a/indra/newview/llappviewerwin32.cpp b/indra/newview/llappviewerwin32.cpp index 4c90a82fcb..a13e4de308 100644 --- a/indra/newview/llappviewerwin32.cpp +++ b/indra/newview/llappviewerwin32.cpp @@ -400,17 +400,10 @@ void ll_nvapi_init(NvDRSSessionHandle hSession) } } -//#define DEBUGGING_SEH_FILTER 1 -#if DEBUGGING_SEH_FILTER -# define WINMAIN DebuggingWinMain -#else -# define WINMAIN wWinMain -#endif - -int APIENTRY WINMAIN(HINSTANCE hInstance, - HINSTANCE hPrevInstance, - PWSTR pCmdLine, - int nCmdShow) +int APIENTRY wWinMain(HINSTANCE hInstance, + HINSTANCE hPrevInstance, + PWSTR pCmdLine, + int nCmdShow) { // Call Tracy first thing to have it allocate memory // https://github.com/wolfpld/tracy/issues/196 @@ -559,27 +552,6 @@ int APIENTRY WINMAIN(HINSTANCE hInstance, return 0; } -#if DEBUGGING_SEH_FILTER -// The compiler doesn't like it when you use __try/__except blocks -// in a method that uses object destructors. Go figure. -// This winmain just calls the real winmain inside __try. -// The __except calls our exception filter function. For debugging purposes. -int APIENTRY wWinMain(HINSTANCE hInstance, - HINSTANCE hPrevInstance, - PWSTR lpCmdLine, - int nCmdShow) -{ - __try - { - WINMAIN(hInstance, hPrevInstance, lpCmdLine, nCmdShow); - } - __except( viewer_windows_exception_handler( GetExceptionInformation() ) ) - { - _tprintf( _T("Exception handled.\n") ); - } -} -#endif - void LLAppViewerWin32::disableWinErrorReporting() { std::string executable_name = gDirUtilp->getExecutableFilename(); diff --git a/indra/newview/llfeaturemanager.cpp b/indra/newview/llfeaturemanager.cpp index 99139e6528..1f2ebd5092 100644 --- a/indra/newview/llfeaturemanager.cpp +++ b/indra/newview/llfeaturemanager.cpp @@ -40,6 +40,7 @@ #include "llappviewer.h" #include "llbufferstream.h" +#include "llexception.h" #include "llnotificationsutil.h" #include "llviewercontrol.h" #include "llworld.h" @@ -377,33 +378,6 @@ bool LLFeatureManager::parseFeatureTable(std::string filename) F32 gpu_benchmark(); -#if LL_WINDOWS - -F32 logExceptionBenchmark() -{ - // FIXME: gpu_benchmark uses many C++ classes on the stack to control state. - // SEH exceptions with our current exception handling options do not call - // destructors for these classes, resulting in an undefined state should - // this handler be invoked. - F32 gbps = -1; - __try - { - gbps = gpu_benchmark(); - } - __except (msc_exception_filter(GetExceptionCode(), GetExceptionInformation())) - { - // HACK - ensure that profiling is disabled - LLGLSLShader::finishProfile(false); - - // convert to C++ styled exception - char integer_string[32]; - sprintf(integer_string, "SEH, code: %lu\n", GetExceptionCode()); - throw std::exception(integer_string); - } - return gbps; -} -#endif - bool LLFeatureManager::loadGPUClass() { if (!gSavedSettings.getBOOL("SkipBenchmark")) @@ -413,14 +387,12 @@ bool LLFeatureManager::loadGPUClass() F32 gbps; try { -#if LL_WINDOWS - gbps = logExceptionBenchmark(); -#else - gbps = gpu_benchmark(); -#endif + gbps = seh_catcher(gpu_benchmark); } catch (const std::exception& e) { + // HACK - ensure that profiling is disabled + LLGLSLShader::finishProfile(false); gbps = -1.f; LL_WARNS("RenderInit") << "GPU benchmark failed: " << e.what() << LL_ENDL; } diff --git a/indra/test/test.cpp b/indra/test/test.cpp index 1239b34d04..d1c65d6aa7 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -36,6 +36,7 @@ #include "linden_common.h" #include "llerrorcontrol.h" +#include "llexception.h" #include "lltut.h" #include "chained_callback.h" #include "stringize.h" @@ -68,13 +69,6 @@ #pragma warning (pop) #endif -// On Mac, got: -// #error "Boost.Stacktrace requires `_Unwind_Backtrace` function. Define -// `_GNU_SOURCE` macro or `BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED` if -// _Unwind_Backtrace is available without `_GNU_SOURCE`." -#define BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED -#include - #include void wouldHaveCrashed(const std::string& message); @@ -524,64 +518,6 @@ void wouldHaveCrashed(const std::string& message) static LLTrace::ThreadRecorder* sMasterThreadRecorder = NULL; -// this is used in platform-generic code -- define outside #if LL_WINDOWS -struct Windows_SEH_exception: public std::runtime_error -{ - Windows_SEH_exception(const std::string& what): std::runtime_error(what) {} -}; - -#if LL_WINDOWS - -static constexpr U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific -static constexpr U32 STATUS_STACK_FULL = 0xC00000FD; - -U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS*) -{ - if (code == STATUS_MSC_EXCEPTION) - { - // C++ exception, go on -- but TUT is supposed to have caught those already?! - return EXCEPTION_CONTINUE_SEARCH; - } - else - { - // This is a non-C++ exception, e.g. hardware check. - // By the time the handler gets control, the stack has been unwound, - // so report the stack trace now at filter() time. - // Sadly, even though, at the time of this writing, stack overflow is - // the problem we would most like to diagnose, calling another - // function when the stack is already blown only terminates us faster. - if (code != STATUS_STACK_FULL) - { - std::cerr << boost::stacktrace::stacktrace() << std::endl; - } - // pass control into the handler block - return EXCEPTION_EXECUTE_HANDLER; - } -} - -template -void seh_catcher(CALLABLE0&& trycode, CALLABLE1&& handler) -{ - __try - { - trycode(); - } - __except (seh_filter(GetExceptionCode(), GetExceptionInformation())) - { - handler(GetExceptionCode()); - } -} - -#else // not LL_WINDOWS - -template -void seh_catcher(CALLABLE0&& trycode, CALLABLE1&&) -{ - trycode(); -} - -#endif // not LL_WINDOWS - int main(int argc, char **argv) { // The following line must be executed to initialize Google Mock @@ -734,7 +670,7 @@ int main(int argc, char **argv) } }, // __except - [mycallback](U32 code) + [mycallback](U32 code, const std::string& /*stacktrace*/) { static std::map codes = { { 0xC0000005, "Access Violation" }, -- cgit v1.2.3 From 5ed8df22cd59680a685c4ada7daa5555bf59d4fe Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 28 May 2024 13:22:05 -0400 Subject: Fix up llexception.h's cross-platform SEH wrapper. Introduce AlwaysReturn specialization, which always discards any result of calling the specified callable with specified args. Derive new Windows_SEH_exception from LLException, not std::runtime_error. Put the various SEH functions in LL::seh nested namespace, e.g. LL::seh::catcher() as the primary API. Break out more levels of Windows SEH handler to work around the restrictions on functions containing __try/__except. The triadic catcher() overload now does little save declare a std::string stacktrace before forwarding the call to catcher_inner(), passing a reference to stacktrace along with the trycode, filter and handler functions. catcher_inner() accepts the stacktrace and the three function template arguments. It contains the __try/__except logic. It calls a new filter_() wrapper template, which calls fill_stacktrace() before forwarding the call to the caller's filter function. fill_stacktrace(), in the .cpp file, contains the logic to populate the stacktrace string -- unless the Structured Exception is stack overflow, in which case it puts an explanatory string instead. catcher_inner()'s __except clause passes not only the code, but also the stacktrace string, to the caller's handler function. It wraps the caller's handler function in always_return(), where rtype is the type returned by the trycode function. This allows a handler to return a value, while also supporting the void handler case, e.g. one that throws a C++ exception. (This is why we need AlwaysReturn: some trycode() functions are themselves void.) For the dyadic catcher() overload, introduce common_filter() containing the logic to distinguish a C++ exception from any other kind of Structured Exception. The fact that the stacktrace is captured before the filter function is called should permit capturing a stacktrace for a C++ exception as well as for most other Structured Exceptions. As before, the monadic catcher() overload supplies the rethrow() handler, in the .cpp file. Change existing calls from seh_catcher() to LL::seh::catcher(). --- indra/llcommon/always_return.h | 16 ++++++ indra/llcommon/llcoros.cpp | 2 +- indra/llcommon/llexception.cpp | 24 +++------ indra/llcommon/llexception.h | 104 ++++++++++++++++++++++--------------- indra/llwindow/llwindowwin32.cpp | 2 +- indra/newview/llfeaturemanager.cpp | 2 +- indra/test/test.cpp | 2 +- 7 files changed, 91 insertions(+), 61 deletions(-) diff --git a/indra/llcommon/always_return.h b/indra/llcommon/always_return.h index a206471da5..b99eb49096 100644 --- a/indra/llcommon/always_return.h +++ b/indra/llcommon/always_return.h @@ -79,6 +79,22 @@ namespace LL DESIRED mDefault; }; + // specialize for AlwaysReturn + template <> + struct AlwaysReturn + { + public: + AlwaysReturn() {} + + // callable returns a type not convertible to DESIRED, return default + template + void operator()(CALLABLE&& callable, ARGS&&... args) + { + // discard whatever callable(args) returns + std::forward(callable)(std::forward(args)...); + } + }; + /** * always_return(some_function, some_args...) calls * some_function(some_args...). It is guaranteed to return a value of type diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index f3943b7138..7512bac88d 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -299,7 +299,7 @@ void LLCoros::toplevel(std::string name, callable_t callable) // run the code the caller actually wants in the coroutine try { - seh_catcher(callable); + LL::seh::catcher(callable); } catch (const Stop& exc) { diff --git a/indra/llcommon/llexception.cpp b/indra/llcommon/llexception.cpp index 74b33f1e3b..107fdc2b2d 100644 --- a/indra/llcommon/llexception.cpp +++ b/indra/llcommon/llexception.cpp @@ -101,44 +101,36 @@ void annotate_exception_(boost::exception& exc) static constexpr U32 STATUS_MSC_EXCEPTION = 0xE06D7363; // compiler specific static constexpr U32 STATUS_STACK_FULL = 0xC00000FD; -U32 ll_seh_filter( - std::string& stacktrace, - std::function filter, - U32 code, - struct _EXCEPTION_POINTERS* exception_infop) +void LL::seh::fill_stacktrace(std::string& stacktrace, U32 code) { - // By the time the handler gets control, the stack has been unwound, - // so report the stack trace now at filter() time. - // Even though stack overflow is a problem we would very much like to - // diagnose, calling another function when the stack is already blown only - // terminates us faster. + // Sadly, despite its diagnostic importance, trying to capture a + // stacktrace when the stack is already blown only terminates us faster. if (code == STATUS_STACK_FULL) { stacktrace = "(stack overflow, no traceback)"; } else { - stacktrace = boost::stacktrace::stacktrace().to_string(); + stacktrace = to_string(boost::stacktrace::stacktrace()); } - - return filter(code, exception_infop); } -U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS*) +U32 LL::seh::common_filter(U32 code, struct _EXCEPTION_POINTERS*) { if (code == STATUS_MSC_EXCEPTION) { - // C++ exception, go on + // C++ exception, don't stop at this handler return EXCEPTION_CONTINUE_SEARCH; } else { // This is a non-C++ exception, e.g. hardware check. + // Pass control into the handler block. return EXCEPTION_EXECUTE_HANDLER; } } -void seh_rethrow(U32 code, const std::string& stacktrace) +void LL::seh::rethrow(U32 code, const std::string& stacktrace) { std::ostringstream out; out << "Windows exception 0x" << std::hex << code; diff --git a/indra/llcommon/llexception.h b/indra/llcommon/llexception.h index 3e50678b44..f58a553eb3 100644 --- a/indra/llcommon/llexception.h +++ b/indra/llcommon/llexception.h @@ -12,6 +12,7 @@ #if ! defined(LL_LLEXCEPTION_H) #define LL_LLEXCEPTION_H +#include "always_return.h" #include #include #include @@ -106,90 +107,111 @@ void log_unhandled_exception_(const char*, int, const char*, const std::string&) * Structured Exception Handling *****************************************************************************/ // this is used in platform-generic code -- define outside #if LL_WINDOWS -struct Windows_SEH_exception: public std::runtime_error +struct Windows_SEH_exception: public LLException { - Windows_SEH_exception(const std::string& what): std::runtime_error(what) {} + Windows_SEH_exception(const std::string& what): LLException(what) {} }; +namespace LL +{ +namespace seh +{ + #if LL_WINDOWS //------------------------------------------------------------- -#include +void fill_stacktrace(std::string& stacktrace, U32 code); + +// wrapper around caller's U32 filter(U32 code, struct _EXCEPTION_POINTERS*) +// filter function: capture a stacktrace, if possible, before forwarding the +// call to the caller's filter() function +template +U32 filter_(std::string& stacktrace, FILTER&& filter, + U32 code, struct _EXCEPTION_POINTERS* exptrs) +{ + // By the time the handler gets control, the stack has been unwound, + // so report the stack trace now at filter() time. + fill_stacktrace(stacktrace, code); + return std::forward(filter)(code, exptrs); +} -// triadic variant specifies try(), filter(U32, struct _EXCEPTION_POINTERS*), -// handler(U32, const std::string& stacktrace) -// stacktrace may or may not be available template -auto seh_catcher(TRYCODE&& trycode, FILTER&& filter, HANDLER&& handler) +auto catcher_inner(std::string& stacktrace, + TRYCODE&& trycode, FILTER&& filter, HANDLER&& handler) { - // don't try to construct a std::function at the moment of Structured Exception - std::function - filter_function(std::forward(filter)); - std::string stacktrace; __try { return std::forward(trycode)(); } - __except (ll_seh_filter( - stacktrace, - filter_function, - GetExceptionCode(), - GetExceptionInformation())) + __except (filter_(stacktrace, + std::forward(filter), + GetExceptionCode(), GetExceptionInformation())) { - return std::forward(handler)(GetExceptionCode(), stacktrace); + return always_return( + std::forward(handler), GetExceptionCode(), stacktrace); } } -// dyadic variant specifies try(), handler(U32, stacktrace), assumes default filter +// triadic variant specifies try(), filter(U32, struct _EXCEPTION_POINTERS*), +// handler(U32, const std::string& stacktrace) +// stacktrace may or may not be available +template +auto catcher(TRYCODE&& trycode, FILTER&& filter, HANDLER&& handler) +{ + // Construct and destroy this stacktrace string in the outer function + // because we can't do either in the function with __try/__except. + std::string stacktrace; + return catcher_inner(stacktrace, + std::forward(trycode), + std::forward(filter), + std::forward(handler)); +} + +// common_filter() handles the typical case in which we want our handler +// clause to handle only Structured Exceptions rather than explicitly-thrown +// C++ exceptions +U32 common_filter(U32 code, struct _EXCEPTION_POINTERS*); + +// dyadic variant specifies try(), handler(U32, stacktrace), assumes common_filter() template -auto seh_catcher(TRYCODE&& trycode, HANDLER&& handler) +auto catcher(TRYCODE&& trycode, HANDLER&& handler) { - return seh_catcher( - std::forward(trycode), - seh_filter, - std::forward(handler)); + return catcher(std::forward(trycode), + common_filter, + std::forward(handler)); } // monadic variant specifies try(), assumes default filter and handler template -auto seh_catcher(TRYCODE&& trycode) +auto catcher(TRYCODE&& trycode) { - return seh_catcher( - std::forward(trycode), - seh_filter, - seh_rethrow); + return catcher(std::forward(trycode), rethrow); } -// SEH exception filtering for use in __try __except -// Separates C++ exceptions from C SEH exceptions -// Todo: might be good idea to do some kind of seh_to_msc_wrapper(function, ARGS&&); -U32 ll_seh_filter( - std::string& stacktrace, - std::function filter, - U32 code, - struct _EXCEPTION_POINTERS* exception_infop); -U32 seh_filter(U32 code, struct _EXCEPTION_POINTERS* exception_infop); -void seh_rethrow(U32 code, const std::string& stacktrace); +[[noreturn]] void rethrow(U32 code, const std::string& stacktrace); #else // not LL_WINDOWS ----------------------------------------------------- template -auto seh_catcher(TRYCODE&& trycode, FILTER&&, HANDLER&&) +auto catcher(TRYCODE&& trycode, FILTER&&, HANDLER&&) { return std::forward(trycode)(); } template -auto seh_catcher(TRYCODE&& trycode, HANDLER&&) +auto catcher(TRYCODE&& trycode, HANDLER&&) { return std::forward(trycode)(); } template -auto seh_catcher(TRYCODE&& trycode) +auto catcher(TRYCODE&& trycode) { return std::forward(trycode)(); } #endif // not LL_WINDOWS ----------------------------------------------------- +} // namespace LL::seh +} // namespace LL + #endif /* ! defined(LL_LLEXCEPTION_H) */ diff --git a/indra/llwindow/llwindowwin32.cpp b/indra/llwindow/llwindowwin32.cpp index 1535d00d50..12cd5320b8 100644 --- a/indra/llwindow/llwindowwin32.cpp +++ b/indra/llwindow/llwindowwin32.cpp @@ -163,7 +163,7 @@ HGLRC SafeCreateContext(HDC &hdc) GLuint SafeChoosePixelFormat(HDC &hdc, const PIXELFORMATDESCRIPTOR *ppfd) { - return seh_catcher(ChoosePixelFormat(hdc, ppfd)); + return LL::seh::catcher([hdc, ppfd]{ return ChoosePixelFormat(hdc, ppfd); }); } //static diff --git a/indra/newview/llfeaturemanager.cpp b/indra/newview/llfeaturemanager.cpp index 1f2ebd5092..765599bb82 100644 --- a/indra/newview/llfeaturemanager.cpp +++ b/indra/newview/llfeaturemanager.cpp @@ -387,7 +387,7 @@ bool LLFeatureManager::loadGPUClass() F32 gbps; try { - gbps = seh_catcher(gpu_benchmark); + gbps = LL::seh::catcher(gpu_benchmark); } catch (const std::exception& e) { diff --git a/indra/test/test.cpp b/indra/test/test.cpp index d1c65d6aa7..64ee702124 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -656,7 +656,7 @@ int main(int argc, char **argv) // a chained_callback subclass must be linked with previous mycallback->link(); - seh_catcher( + LL::seh::catcher( // __try [test_group] { -- cgit v1.2.3