Fix problem with exception handling in CPPScheduler

If an exception was thrown in the main thread, the child threads
were not being synchronised, leading to undefined behaviour (and
probably the program crashing instead of correctly propagating the
exception).

Add support to the build system for enabling ThreadSanitizer.
This needs a build system option rather than simply passing
extra_cxx/link_flags because the sanitizer options are
incompatible with -Wl,-undefined,error.

Add a unit test for throwing an exception within a CPPScheduler
workload.

Signed-off-by: Matthew Bentham <Matthew.Bentham@arm.com>
Change-Id: I7638272a5d43a24a861f3e6d63f3ee7b099483b5
Reviewed-on: https://eu-gerrit-1.euhpc.arm.com/c/VisualCompute/ComputeLibrary/+/538048
Comments-Addressed: bsgcomp <bsgcomp@arm.com>
Tested-by: bsgcomp <bsgcomp@arm.com>
Reviewed-by: Viet-Hoa Do <viet-hoa.do@arm.com>
Reviewed-on: https://review.mlplatform.org/c/ml/ComputeLibrary/+/9957
Tested-by: Arm Jenkins <bsgcomp@arm.com>
Comments-Addressed: Arm Jenkins <bsgcomp@arm.com>
Benchmark: Arm Jenkins <bsgcomp@arm.com>

4 files changed, 125 insertions, 11 deletions

diff --git a/SConscript b/SConscript
index b5ef6db067..da7683ed6c 100644
--- a/SConscript
+++ b/SConscript
@@ -534,7 +534,8 @@ arm_compute_env.Append(CPPDEFINES = [('ARM_COMPUTE_VERSION_MAJOR', LIBRARY_VERSI
 
 # Don't allow undefined references in the libraries:
 undefined_flag = '-Wl,-undefined,error' if 'macos' in arm_compute_env["os"] else '-Wl,--no-undefined'
-arm_compute_env.Append(LINKFLAGS=[undefined_flag])
+if not env['thread_sanitizer']:
+    arm_compute_env.Append(LINKFLAGS=[undefined_flag])
 arm_compute_env.Append(CPPPATH =[Dir("./src/core/").path] )
 
 if env['os'] != 'openbsd':
diff --git a/SConstruct b/SConstruct
index 4480c710d3..68c518a4a0 100644
--- a/SConstruct
+++ b/SConstruct
@@ -133,6 +133,7 @@ vars.AddVariables(
     ListVariable("data_layout_support", "Enable a list of data layout to support", "all", ["nhwc", "nchw"]),
     ("toolchain_prefix", "Override the toolchain prefix; used by all toolchain components: compilers, linker, assembler etc. If unspecified, use default(auto) prefixes; if passed an empty string '' prefixes would be disabled", "auto"),
     ("compiler_prefix", "Override the compiler prefix; used by just compilers (CC,CXX); further overrides toolchain_prefix for compilers; this is for when the compiler prefixes are different from that of the linkers, archivers etc. If unspecified, this is the same as toolchain_prefix; if passed an empty string '' prefixes would be disabled", "auto"),
+    BoolVariable("thread_sanitizer", "Enable ThreadSanitizer", False),
     ("extra_cxx_flags", "Extra CXX flags to be appended to the build command", ""),
     ("extra_link_flags", "Extra LD flags to be appended to the build command", ""),
     ("compiler_cache", "Command to prefix to the C and C++ compiler (e.g ccache)", ""),
@@ -613,6 +614,10 @@ if env['asserts']:
 if env['logging']:
     env.Append(CPPDEFINES = ['ARM_COMPUTE_LOGGING_ENABLED'])
 
+if env['thread_sanitizer']:
+    env.Append(CXXFLAGS = ['-fsanitize=thread'])
+    env.Append(LINKFLAGS = ['-fsanitize=thread'])
+
 env.Append(CPPPATH = ['#/include', "#"])
 env.Append(CXXFLAGS = env['extra_cxx_flags'])
 env.Append(LINKFLAGS = env['extra_link_flags'])
diff --git a/src/runtime/CPP/CPPScheduler.cpp b/src/runtime/CPP/CPPScheduler.cpp
index 39811ec156..45e872428f 100644
--- a/src/runtime/CPP/CPPScheduler.cpp
+++ b/src/runtime/CPP/CPPScheduler.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016-2022 Arm Limited.
+ * Copyright (c) 2016-2023 Arm Limited.
  *
  * SPDX-License-Identifier: MIT
  *
@@ -172,7 +172,7 @@ public:
     void start();
 
     /** Wait for the current kernel execution to complete. */
-    void wait();
+    std::exception_ptr wait();
 
     /** Function ran by the worker thread. */
     void worker_thread();
@@ -244,17 +244,13 @@ void Thread::start()
     _cv.notify_one();
 }
 
-void Thread::wait()
+std::exception_ptr Thread::wait()
 {
     {
         std::unique_lock<std::mutex> lock(_m);
         _cv.wait(lock, [&] { return _job_complete; });
     }
-
-    if(_current_exception)
-    {
-        std::rethrow_exception(_current_exception);
-    }
+    return _current_exception;
 }
 
 void Thread::worker_thread()
@@ -509,15 +505,34 @@ void CPPScheduler::run_workloads(std::vector<IScheduler::Workload> &workloads)
         thread_it->start();
     }
     info.thread_id = t;                         // Set main thread's thread_id
-    process_workloads(workloads, feeder, info); // Main thread processes workloads
+    std::exception_ptr last_exception = nullptr;
 #ifndef ARM_COMPUTE_EXCEPTIONS_DISABLED
     try
     {
 #endif /* ARM_COMPUTE_EXCEPTIONS_DISABLED */
+        process_workloads(workloads, feeder, info); // Main thread processes workloads
+#ifndef ARM_COMPUTE_EXCEPTIONS_DISABLED
+    }
+    catch (...)
+    {
+        last_exception = std::current_exception();
+    }
+
+    try
+    {
+#endif /* ARM_COMPUTE_EXCEPTIONS_DISABLED */
         thread_it = _impl->_threads.begin();
         for(unsigned int i = 0; i < num_threads_to_use - 1; ++i, ++thread_it)
         {
-            thread_it->wait();
+            std::exception_ptr current_exception = thread_it->wait();
+            if (current_exception)
+            {
+                last_exception = current_exception;
+            }
+        }
+        if (last_exception)
+        {
+            std::rethrow_exception(last_exception);
         }
 #ifndef ARM_COMPUTE_EXCEPTIONS_DISABLED
     }
diff --git a/tests/validation/UNIT/CPPScheduler.cpp b/tests/validation/UNIT/CPPScheduler.cpp
new file mode 100644
index 0000000000..52431653b5
--- /dev/null
+++ b/tests/validation/UNIT/CPPScheduler.cpp
@@ -0,0 +1,93 @@
+/*
+ * Copyright (c) 2023 Arm Limited.
+ *
+ * SPDX-License-Identifier: MIT
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+#include "arm_compute/runtime/CPP/CPPScheduler.h"
+
+#include "arm_compute/core/CPP/ICPPKernel.h"
+#include "tests/framework/Asserts.h"
+#include "tests/framework/Macros.h"
+
+#include <stdexcept>
+
+using namespace arm_compute;
+using namespace arm_compute::test;
+
+namespace
+{
+class TestException: public std::exception
+{
+public:
+    const char* what() const noexcept override
+    {
+        return "Expected test exception";
+    }
+};
+
+class TestKernel: public ICPPKernel
+{
+public:
+    TestKernel()
+    {
+        Window window;
+        window.set(0, Window::Dimension(0, 2));
+        configure(window);
+    }
+
+    const char* name() const override
+    {
+        return "TestKernel";
+    }
+
+    void run(const Window &, const ThreadInfo &) override
+    {
+        throw TestException();
+    }
+
+};
+}
+
+TEST_SUITE(UNIT)
+TEST_SUITE(CPPScheduler)
+
+#if !defined(BARE_METAL)
+TEST_CASE(RethrowException, framework::DatasetMode::ALL)
+{
+    CPPScheduler scheduler;
+    CPPScheduler::Hints hints(0);
+    TestKernel kernel;
+
+    scheduler.set_num_threads(2);
+    try
+    {
+        scheduler.schedule(&kernel, hints);
+    }
+    catch(const TestException&)
+    {
+        return;
+    }
+    ARM_COMPUTE_EXPECT_FAIL("Expected exception not caught", framework::LogLevel::ERRORS);
+}
+#endif // !defined(BARE_METAL)
+
+TEST_SUITE_END()
+TEST_SUITE_END()