Merge pull request #1457 from DARMA-tasking/1456-make-throw-runtime-arg

1456: Change vt_throw_on_abort to runtime flag
DARMA-tasking · May 28, 2021 · 145a258 · 145a258
2 parents f1dc7bb + a4402be
commit 145a258
Show file tree

Hide file tree

Showing 36 changed files with 126 additions and 76 deletions.
diff --git a/ci/azure/azure-clang-10-alpine-mpich.yml b/ci/azure/azure-clang-10-alpine-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 0
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: alpine-clang-10-cache

diff --git a/ci/azure/azure-clang-10-ubuntu-mpich.yml b/ci/azure/azure-clang-10-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-clang-10-cache

diff --git a/ci/azure/azure-clang-3.9-ubuntu-mpich.yml b/ci/azure/azure-clang-3.9-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-clang-3.9-cache

diff --git a/ci/azure/azure-clang-5.0-ubuntu-mpich.yml b/ci/azure/azure-clang-5.0-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-clang-5.0-cache

diff --git a/ci/azure/azure-clang-9-ubuntu-mpich.yml b/ci/azure/azure-clang-9-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-clang-9-cache

diff --git a/ci/azure/azure-gcc-10-ubuntu-openmpi.yml b/ci/azure/azure-gcc-10-ubuntu-openmpi.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-gcc-10-cache

diff --git a/ci/azure/azure-gcc-5-ubuntu-mpich.yml b/ci/azure/azure-gcc-5-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-gcc-5-cache

diff --git a/ci/azure/azure-gcc-6-ubuntu-mpich.yml b/ci/azure/azure-gcc-6-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-gcc-6-cache

diff --git a/ci/azure/azure-gcc-7-ubuntu-mpich.yml b/ci/azure/azure-gcc-7-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-gcc-7-cache

diff --git a/ci/azure/azure-gcc-8-ubuntu-mpich.yml b/ci/azure/azure-gcc-8-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 1
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-gcc-8-cache

diff --git a/ci/azure/azure-gcc-9-ubuntu-mpich.yml b/ci/azure/azure-gcc-9-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 1
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-gcc-9-cache

diff --git a/ci/azure/azure-intel-18-ubuntu-mpich-extended.yml b/ci/azure/azure-intel-18-ubuntu-mpich-extended.yml
@@ -41,7 +41,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-intel-18-cache

diff --git a/ci/azure/azure-intel-18-ubuntu-mpich.yml b/ci/azure/azure-intel-18-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-intel-18-cache

diff --git a/ci/azure/azure-intel-19-ubuntu-mpich.yml b/ci/azure/azure-intel-19-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-intel-19-cache

diff --git a/ci/azure/azure-nvidia-10-ubuntu-mpich-extended.yml b/ci/azure/azure-nvidia-10-ubuntu-mpich-extended.yml
@@ -41,7 +41,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-nvidia-10-cache

diff --git a/ci/azure/azure-nvidia-10-ubuntu-mpich.yml b/ci/azure/azure-nvidia-10-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 0
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-nvidia-10-cache

diff --git a/ci/azure/azure-nvidia-11-ubuntu-mpich-extended.yml b/ci/azure/azure-nvidia-11-ubuntu-mpich-extended.yml
@@ -41,7 +41,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 1
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-nvidia-11-cache

diff --git a/ci/azure/azure-nvidia-11-ubuntu-mpich.yml b/ci/azure/azure-nvidia-11-ubuntu-mpich.yml
@@ -47,7 +47,6 @@ variables:
   VT_USE_STD_THREAD: 0
   VT_ZOLTAN: 0
   VT_CI_BUILD: 1
-  VT_THROW_ON_ABORT: 1
   VT_DIAGNOSTICS: 0
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: ubuntu-nvidia-11-cache

diff --git a/ci/build_cpp.sh b/ci/build_cpp.sh
@@ -140,7 +140,6 @@ cmake -G "${CMAKE_GENERATOR:-Ninja}" \
       -Dvt_ci_build="${VT_CI_BUILD:-0}" \
       -Dvt_debug_verbose="${VT_DEBUG_VERBOSE:-}" \
       -Dvt_tests_num_nodes="${VT_TESTS_NUM_NODES:-}" \
-      -Dvt_throw_on_abort="${VT_THROW_ON_ABORT:-0}" \
       "$VT"
 cmake_conf_ret=$?
 

diff --git a/cmake/configure_options.cmake b/cmake/configure_options.cmake
@@ -134,15 +134,6 @@ else()
     set(vt_feature_cmake_ci_build "0")
 endif()
 
-option(vt_throw_on_abort "Build VT with 'throw on vtAbort'" OFF)
-if (${vt_throw_on_abort})
-    message(STATUS "Building VT with 'throw on vtAbort'")
-    set (vt_feature_cmake_throw_on_abort "1")
-else()
-    message(STATUS "Building VT with `MPI_Abort on vtAbort`")
-    set (vt_feature_cmake_throw_on_abort "0")
-endif()
-
 if (LOWERCASE_CMAKE_BUILD_TYPE STREQUAL "release")
     option(vt_debug_verbose "Build VT with verbose debug printing enabled" OFF)
 else()

diff --git a/cmake_config.h.in b/cmake_config.h.in
@@ -68,7 +68,6 @@
 #define vt_feature_cmake_libfort             @vt_feature_cmake_libfort@
 #define vt_feature_cmake_production_build    @vt_feature_cmake_production_build@
 #define vt_feature_cmake_debug_verbose       @vt_feature_cmake_debug_verbose@
-#define vt_feature_cmake_throw_on_abort      @vt_feature_cmake_throw_on_abort@
 
 #define vt_detected_max_num_nodes @cmake_detected_max_num_nodes@
 

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -148,7 +148,6 @@ x-vtopts: &vtopts
   http_proxy: ${PROXY-}
   LSAN_OPTIONS: ${LSAN_OPTIONS-}
   VT_CI_BUILD: ${VT_CI_BUILD:-0}
-  VT_THROW_ON_ABORT: ${VT_THROW_ON_ABORT:-0}
   VT_DEBUG_VERBOSE: ${VT_DEBUG_VERBOSE-}
   VT_TESTS_NUM_NODES: ${VT_TESTS_NUM_NODES:-}
   CODECOV_TOKEN: ${CODECOV_TOKEN:-}

diff --git a/docs/md/building.md b/docs/md/building.md
@@ -69,7 +69,6 @@ build configuration:
 | `VT_BUILD_TESTS`                 | 1               | Build all VT tests |
 | `VT_BUILD_EXAMPLES`              | 1               | Build all VT examples |
 | `vt_debug_verbose`               | 1 (not Release) | Enable VT verbose debug prints at compile-time |
-| `vt_throw_on_abort`              | 0               | Throw an exception instead of calling `MPI_Abort` on `vtAbort` |
 
 \subsection using-the-build-script Using the Build Script
 
@@ -104,7 +103,6 @@ parameters.
 | `VT_DIAGNOSTICS_RUNTIME_ENABLED` | 0               | Enable VT component diagnostics at runtime by default |
 | `VT_DEBUG_VERBOSE`               | <empty>         | Enable VT verbose debug prints at compile-time |
 | `VT_TESTS_NUM_NODES`             | <empty>         | Maximum number of nodes used for tests. If empty, then the default value detected by CMake is used |
-| `VT_THROW_ON_ABORT`              | 0               | Throw an exception instead of calling `MPI_Abort` on `vtAbort` |
 
 With these set, invoke the script with two arguments: the path to the *vt* root
 directory and the build path. Here's an example assuming that *vt* is cloned

diff --git a/scripts/azure-workflow-template.yml b/scripts/azure-workflow-template.yml
@@ -37,7 +37,6 @@ variables:
   VT_USE_STD_THREAD: [% vt_use_std_thread %]
   VT_ZOLTAN: [% vt_zoltan %]
   VT_CI_BUILD: [% vt_ci_build %]
-  VT_THROW_ON_ABORT: [% vt_throw_on_abort %]
   VT_DIAGNOSTICS: [% vt_diagnostics %]
   CACHE: "$(Agent.TempDirectory)/cache/"
   cache_name: [% cache_name %]

diff --git a/scripts/workflows-azure.ini b/scripts/workflows-azure.ini
@@ -17,7 +17,6 @@ vt_use_openmp = 0
 vt_use_std_thread = 0
 vt_zoltan = 0
 vt_ci_build = 1
-vt_throw_on_abort = 1
 vt_tests_num_nodes = 2
 ulimit_core = 0
 code_coverage = 0
@@ -213,7 +212,6 @@ linux_env =""
 output_name = ci/azure/azure-clang-10-alpine-mpich.yml
 build_root = $(ARCH)-[% linux %]-$(COMPILER)-cache
 vt_production_build = 1
-vt_throw_on_abort = 0
 
 [PR-tests-clang-9]
 test_configuration = "clang-9, ubuntu, mpich"

diff --git a/src/vt/collective/collective_ops.cc b/src/vt/collective/collective_ops.cc
@@ -146,10 +146,13 @@ void CollectiveAnyOps<instance>::abort(
     myrt->theTrace->cleanupTracesFile();
 #endif
     myrt->abort(str, code);
+  } else if (vt::debug::preConfig()->vt_throw_on_abort) {
+    // Special case when preConfig has 'vt_throw_on_abort' option set
+    // This is meant to be used by nompi unit tests
+    throw std::runtime_error(str);
   } else {
     std::_Exit(code);
   }
-
 }
 
 template <runtime::RuntimeInstType instance>

diff --git a/src/vt/configs/arguments/app_config.h b/src/vt/configs/arguments/app_config.h
@@ -165,6 +165,7 @@ struct AppConfig {
 
   bool vt_pause = false;
   bool vt_no_assert_fail = false;
+  bool vt_throw_on_abort = false;
   std::size_t vt_max_mpi_send_size = 1ull << 30;
 
 #if (vt_feature_fcontext != 0)
@@ -318,6 +319,7 @@ struct AppConfig {
 
       | vt_pause
       | vt_no_assert_fail
+      | vt_throw_on_abort
       | vt_max_mpi_send_size
 
       | vt_debug_level

diff --git a/src/vt/configs/arguments/args.cc b/src/vt/configs/arguments/args.cc
@@ -501,6 +501,7 @@ void ArgConfig::addRuntimeArgs(CLI::App& app) {
   auto max_size = "Maximum MPI send size (causes larger messages to be split "
                   "into multiple MPI sends)";
   auto assert = "Do not abort the program when vtAssert(..) is invoked";
+  auto throw_on_abort = "Throw an exception when vtAbort(..) is called";
 
 
   auto a1 = app.add_option(
@@ -509,11 +510,15 @@ void ArgConfig::addRuntimeArgs(CLI::App& app) {
   auto a2 = app.add_flag(
     "--vt_no_assert_fail", config_.vt_no_assert_fail, assert
   );
+  auto a3 = app.add_flag(
+    "--vt_throw_on_abort", config_.vt_throw_on_abort, throw_on_abort
+  );
 
 
   auto configRuntime = "Runtime";
   a1->group(configRuntime);
   a2->group(configRuntime);
+  a3->group(configRuntime);
 }
 
 void ArgConfig::addThreadingArgs(CLI::App& app) {

diff --git a/src/vt/configs/debug/debug_colorize.h b/src/vt/configs/debug/debug_colorize.h
@@ -46,10 +46,12 @@
 #define INCLUDED_VT_CONFIGS_DEBUG_DEBUG_COLORIZE_H
 
 #include "vt/configs/arguments/app_config.h"
+#include "vt/configs/types/types_type.h"
 
 #include <string>
 
 namespace vt { namespace debug {
+arguments::AppConfig * preConfigRef();
 arguments::AppConfig const* preConfig();
 }} /* end namespace vt::debug */
 

diff --git a/src/vt/runtime/runtime.cc b/src/vt/runtime/runtime.cc
@@ -78,9 +78,7 @@
 
 #include <checkpoint/checkpoint.h>
 
-#if vt_check_enabled(throw_on_abort)
 #include <stdexcept>
-#endif
 #include <memory>
 #include <iostream>
 #include <functional>
@@ -518,24 +516,22 @@ void Runtime::reset() {
 }
 
 void Runtime::abort(std::string const abort_str, ErrorCodeType const code) {
-#if !vt_check_enabled(throw_on_abort)
-  aborted_ = true;
-#endif
-  output(abort_str,code,true,true,false);
-
-#if vt_check_enabled(throw_on_abort)
-  throw std::runtime_error(abort_str);
-#else
-  std::raise( SIGTRAP );
-  if (theContext) {
-    auto const comm = theContext->getComm();
-    MPI_Abort(comm, 129);
+  output(abort_str, code, true, true, false);
+
+  if (theConfig()->vt_throw_on_abort) {
+    throw std::runtime_error(abort_str);
   } else {
-    std::_Exit(code);
-    // @todo: why will this not compile with clang!!?
-    //quick_exit(code);
+    aborted_ = true;
+    std::raise(SIGTRAP);
+    if (theContext) {
+      auto const comm = theContext->getComm();
+      MPI_Abort(comm, 129);
+    } else {
+      std::_Exit(code);
+      // @todo: why will this not compile with clang!!?
+      //quick_exit(code);
+    }
   }
-#endif
 }
 
 void Runtime::output(

diff --git a/src/vt/runtime/runtime_get.cc b/src/vt/runtime/runtime_get.cc
@@ -157,6 +157,20 @@ namespace vt { namespace debug {
 // Dummy config that applies outside of RT initialization, much like preNode.
 static arguments::AppConfig preInitAppConfig{};
 
+
+/**
+ * \internal
+ * \brief Returns the preConfig, accessible OUTSIDE of VT initialization.
+ *
+ * This non-const version is used by 'nompi' tests, in order to customize
+ * the app config (mostly vt_throw_on_abort)
+ *
+ * \return A modifiable configuration
+ */
+arguments::AppConfig* preConfigRef(){
+  return &preInitAppConfig;
+}
+
 /**
  * \internal
  * \brief Returns the config, accessible OUTSIDE of VT initialization.

diff --git a/tests/unit/runtime/test_cli_arguments.extended.cc b/tests/unit/runtime/test_cli_arguments.extended.cc
@@ -56,17 +56,10 @@ struct TestCliArguments : TestParallelHarness { };
 TEST_F(TestCliArguments, test_vt_assert) {
   EXPECT_EQ(theConfig()->vt_no_assert_fail, false);
 
-#if vt_check_enabled(throw_on_abort)
   ASSERT_THROW(
     vtAssert(false, "Should throw."),
     std::runtime_error
   );
-#else
-  ASSERT_DEATH(
-    vtAssert(false, "Should abort."),
-    "Should abort."
-  );
-#endif
 }
 #endif
 

diff --git a/tests/unit/runtime/test_mpi_access_guards.cc b/tests/unit/runtime/test_mpi_access_guards.cc
@@ -75,17 +75,8 @@ static void attempt_mpi_access() {
 
 static void message_handler(DummyMsg* msg) {
   if (expected_to_fail_on_mpi_access) {
-#if vt_check_enabled(throw_on_abort)
-  ASSERT_THROW(
-    attempt_mpi_access(),
-    std::runtime_error
-  );
-#else
-    ASSERT_DEATH(
-      attempt_mpi_access(),
-      "MPI functions should not used inside user code invoked from VT handlers"
-    );
-#endif
+    ASSERT_THROW(attempt_mpi_access(), std::runtime_error) <<
+      "MPI functions should not used inside user code invoked from VT handlers";
   } else {
     attempt_mpi_access();
     SUCCEED();