From 71b9515f7e1b730ae9fcfb23cfabe5c3a6e60bfa Mon Sep 17 00:00:00 2001
From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com>
Date: Tue, 12 Oct 2021 10:12:00 +0200
Subject: [PATCH] Better handling of errors during dynamic catalogue loading
(#1702)
* Add platform abstraction for loading DLL/SOs.
* Throw more informative exceptions when handling DLL/SOs.
* More informative errors when dealing with dynamically loaded catalogues.
* Translate `arb::file_not_found_error` to `FileNotFoundError` in Python.
---
CMakeLists.txt | 8 ++++-
arbor/arbexcept.cpp | 12 ++++---
arbor/include/arbor/arbexcept.hpp | 7 ++--
arbor/mechcat.cpp | 26 ++++++---------
arbor/util/dl.hpp | 5 +++
arbor/util/dl_platform_posix.hpp | 52 +++++++++++++++++++++++++++++
python/pyarb.cpp | 4 +++
python/test/unit/test_catalogues.py | 2 +-
test/unit/test_mechcat.cpp | 2 +-
9 files changed, 92 insertions(+), 26 deletions(-)
create mode 100644 arbor/util/dl.hpp
create mode 100644 arbor/util/dl_platform_posix.hpp
diff --git a/CMakeLists.txt b/CMakeLists.txt
index ccdfe0db..a2d3ce90 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -186,7 +186,13 @@ install(TARGETS arbor-config-defs EXPORT arbor-targets)
# Interface library `arbor-private-deps` collects dependencies, options etc.
# for the arbor library.
add_library(arbor-private-deps INTERFACE)
-target_link_libraries(arbor-private-deps INTERFACE arbor-config-defs ext-random123 ${CMAKE_DL_LIBS})
+target_link_libraries(arbor-private-deps INTERFACE arbor-config-defs ext-random123)
+if (UNIX)
+ target_compile_definitions(arbor-config-defs INTERFACE ARB_HAVE_DL)
+ target_link_libraries(arbor-private-deps INTERFACE ${CMAKE_DL_LIBS})
+else()
+ message(FATAL_ERROR "No support for dynamic loading on current platform.")
+endif ()
install(TARGETS arbor-private-deps EXPORT arbor-targets)
# Interface library `arborenv-private-deps` collects dependencies, options etc.
diff --git a/arbor/arbexcept.cpp b/arbor/arbexcept.cpp
index 185a0d5c..fc4ffe7e 100644
--- a/arbor/arbexcept.cpp
+++ b/arbor/arbexcept.cpp
@@ -124,14 +124,16 @@ range_check_failure::range_check_failure(const std::string& whatstr, double valu
{}
file_not_found_error::file_not_found_error(const std::string &fn)
- : arbor_exception(pprintf("Could not find file '{}'", fn)),
+ : arbor_exception(pprintf("Could not find readable file at '{}'", fn)),
filename{fn}
{}
-bad_catalogue_error::bad_catalogue_error(const std::string &fn, const std::string& call)
- : arbor_exception(pprintf("Error in '{}' while opening catalogue '{}'", call, fn)),
- filename{fn},
- failed_call{call}
+bad_catalogue_error::bad_catalogue_error(const std::string& msg)
+ : arbor_exception(pprintf("Error while opening catalogue '{}'", msg))
+{}
+
+bad_catalogue_error::bad_catalogue_error(const std::string& msg, const std::any& pe)
+ : arbor_exception(pprintf("Error while opening catalogue '{}'", msg)), platform_error(pe)
{}
unsupported_abi_error::unsupported_abi_error(size_t v):
diff --git a/arbor/include/arbor/arbexcept.hpp b/arbor/include/arbor/arbexcept.hpp
index 01d4e91e..2185c5df 100644
--- a/arbor/include/arbor/arbexcept.hpp
+++ b/arbor/include/arbor/arbexcept.hpp
@@ -146,10 +146,11 @@ struct file_not_found_error: arbor_exception {
std::string filename;
};
+//
struct bad_catalogue_error: arbor_exception {
- bad_catalogue_error(const std::string& fn, const std::string& call);
- std::string filename;
- std::string failed_call;
+ bad_catalogue_error(const std::string&);
+ bad_catalogue_error(const std::string&, const std::any&);
+ std::any platform_error;
};
// ABI errors
diff --git a/arbor/mechcat.cpp b/arbor/mechcat.cpp
index 3109f17e..829339fd 100644
--- a/arbor/mechcat.cpp
+++ b/arbor/mechcat.cpp
@@ -5,8 +5,6 @@
#include <vector>
#include <cassert>
-#include <dlfcn.h>
-
#include <arbor/version.hpp>
#include <arbor/arbexcept.hpp>
#include <arbor/mechcat.hpp>
@@ -17,6 +15,7 @@
#include "util/rangeutil.hpp"
#include "util/maputil.hpp"
#include "util/span.hpp"
+#include "util/dl.hpp"
/* Notes on implementation:
*
@@ -584,21 +583,18 @@ std::pair<mechanism_ptr, mechanism_overrides> mechanism_catalogue::instance_impl
mechanism_catalogue::~mechanism_catalogue() = default;
-static void check_dlerror(const std::string& fn, const std::string& call) {
- auto error = dlerror();
- if (error) { throw arb::bad_catalogue_error{fn, call}; }
-}
-
const mechanism_catalogue& load_catalogue(const std::string& fn) {
typedef const void* global_catalogue_t();
-
- auto plugin = dlopen(fn.c_str(), RTLD_LAZY);
- check_dlerror(fn, "dlopen");
- assert(plugin);
-
- auto get_catalogue = (global_catalogue_t*)dlsym(plugin, "get_catalogue");
- check_dlerror(fn, "dlsym");
-
+ global_catalogue_t* get_catalogue = nullptr;
+ try {
+ auto plugin = dl_open(fn);
+ get_catalogue = dl_get_symbol<global_catalogue_t*>(plugin, "get_catalogue");
+ } catch(dl_error& e) {
+ throw bad_catalogue_error{e.what(), {e}};
+ }
+ if (!get_catalogue) {
+ throw bad_catalogue_error{util::pprintf("Unusable symbol 'get_catalogue' in shared object '{}'", fn)};
+ }
/* NOTE We do not free the DSO handle here and accept retaining the handles
until termination since the mechanisms provided by the catalogue may have
a different lifetime than the actual catalogue itfself. This is not a
diff --git a/arbor/util/dl.hpp b/arbor/util/dl.hpp
new file mode 100644
index 00000000..8454036e
--- /dev/null
+++ b/arbor/util/dl.hpp
@@ -0,0 +1,5 @@
+#pragma once
+
+#ifdef ARB_HAVE_DL
+#include "util/dl_platform_posix.hpp"
+#endif
diff --git a/arbor/util/dl_platform_posix.hpp b/arbor/util/dl_platform_posix.hpp
new file mode 100644
index 00000000..e2442c15
--- /dev/null
+++ b/arbor/util/dl_platform_posix.hpp
@@ -0,0 +1,52 @@
+#pragma once
+
+#include <fstream>
+#include <string>
+
+#include <dlfcn.h>
+
+#include <arbor/arbexcept.hpp>
+
+#include "util/strprintf.hpp"
+
+namespace arb {
+
+struct dl_error: arbor_exception {
+ dl_error(const std::string& msg): arbor_exception{msg} {}
+};
+
+struct dl_handle {
+ void* dl = nullptr;
+};
+
+inline
+dl_handle dl_open(const std::string& fn) {
+ // Test if we can open file
+ if (!std::ifstream{fn.c_str()}.good()) throw file_not_found_error{fn};
+ // Call once to clear errors not caused by us
+ dlerror();
+ // Try to open shared object
+ auto result = dlopen(fn.c_str(), RTLD_LAZY);
+ // dlopen fails by returning NULL
+ if (!result) throw dl_error{util::pprintf("[POSIX] dl_open failed with: {}", dlerror())};
+ return {result};
+}
+
+template<typename T> inline
+T dl_get_symbol(const dl_handle& handle, const std::string& symbol) {
+ // Call once to clear errors not caused by us
+ dlerror();
+ // Get symbol from shared object, may return NULL if that is what symbol refers to
+ auto result = dlsym(handle.dl, symbol.c_str());
+ // ... so we can only ask for dlerror here
+ if (auto error = dlerror()) throw dl_error{util::pprintf("[POSIX] dl_get_symbol failed with: {}", error)};
+ return reinterpret_cast<T>(result);
+}
+
+inline
+void dl_close(dl_handle& handle) {
+ dlclose(handle.dl);
+ handle.dl = nullptr;
+}
+
+}
diff --git a/python/pyarb.cpp b/python/pyarb.cpp
index 9a6782b8..85871de6 100644
--- a/python/pyarb.cpp
+++ b/python/pyarb.cpp
@@ -3,6 +3,7 @@
#include <arbor/spike.hpp>
#include <arbor/common_types.hpp>
+#include <arbor/arbexcept.hpp>
#include <arbor/version.hpp>
#include "pyarb.hpp"
@@ -43,6 +44,9 @@ PYBIND11_MODULE(_arbor, m) {
m.doc() = "arbor: multicompartment neural network models.";
m.attr("__version__") = ARB_VERSION;
+ // Translate Arbor errors -> Python exceptions.
+ pybind11::register_exception<arb::file_not_found_error>(m, "FileNotFoundError", PyExc_FileNotFoundError);
+
pyarb::register_cable_loader(m);
pyarb::register_cable_probes(m, global_ptr);
pyarb::register_cells(m);
diff --git a/python/test/unit/test_catalogues.py b/python/test/unit/test_catalogues.py
index 4009192b..00cc2ff8 100644
--- a/python/test/unit/test_catalogues.py
+++ b/python/test/unit/test_catalogues.py
@@ -48,7 +48,7 @@ class recipe(arb.recipe):
class Catalogues(unittest.TestCase):
def test_nonexistent(self):
- with self.assertRaises(RuntimeError):
+ with self.assertRaises(FileNotFoundError):
arb.load_catalogue("_NO_EXIST_.so")
def test_shared_catalogue(self):
diff --git a/test/unit/test_mechcat.cpp b/test/unit/test_mechcat.cpp
index 488534d0..4d3d0cd3 100644
--- a/test/unit/test_mechcat.cpp
+++ b/test/unit/test_mechcat.cpp
@@ -280,7 +280,7 @@ TEST(mechcat, names) {
#ifdef USE_DYNAMIC_CATALOGUES
TEST(mechcat, loading) {
- EXPECT_THROW(load_catalogue(LIBDIR "/does-not-exist-catalogue.so"), bad_catalogue_error);
+ EXPECT_THROW(load_catalogue(LIBDIR "/does-not-exist-catalogue.so"), file_not_found_error);
EXPECT_THROW(load_catalogue(LIBDIR "/libarbor.a"), bad_catalogue_error);
const mechanism_catalogue* cat = nullptr;
EXPECT_NO_THROW(cat = &load_catalogue(LIBDIR "/dummy-catalogue.so"));
--
GitLab