From 5430de1f233fde085ec42b455b8aa6ad9f3a1f4f Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Wed, 6 Oct 2021 19:45:06 +0200 Subject: [PATCH 01/13] Add platform abstraction for DLLs. --- arbor/arbexcept.cpp | 6 --- arbor/include/arbor/arbexcept.hpp | 6 --- arbor/include/arbor/dl.hpp | 13 ++++++ arbor/include/arbor/dl_platform_posix.hpp | 53 +++++++++++++++++++++++ arbor/mechcat.cpp | 17 ++------ 5 files changed, 70 insertions(+), 25 deletions(-) create mode 100644 arbor/include/arbor/dl.hpp create mode 100644 arbor/include/arbor/dl_platform_posix.hpp diff --git a/arbor/arbexcept.cpp b/arbor/arbexcept.cpp index 185a0d5cbc..f5d524dd62 100644 --- a/arbor/arbexcept.cpp +++ b/arbor/arbexcept.cpp @@ -128,12 +128,6 @@ file_not_found_error::file_not_found_error(const std::string &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} -{} - unsupported_abi_error::unsupported_abi_error(size_t v): arbor_exception(pprintf("ABI version is not supported by this version of arbor '{}'", v)), version{v} {} diff --git a/arbor/include/arbor/arbexcept.hpp b/arbor/include/arbor/arbexcept.hpp index 01d4e91e21..6677ebd6ed 100644 --- a/arbor/include/arbor/arbexcept.hpp +++ b/arbor/include/arbor/arbexcept.hpp @@ -146,12 +146,6 @@ 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; -}; - // ABI errors struct bad_alignment: arbor_exception { diff --git a/arbor/include/arbor/dl.hpp b/arbor/include/arbor/dl.hpp new file mode 100644 index 0000000000..ce2360aded --- /dev/null +++ b/arbor/include/arbor/dl.hpp @@ -0,0 +1,13 @@ +#pragma once + +#ifdef __APPLE__ +#include dl_platform_posix.hpp +#endif + +#ifdef __linux__ +#include dl_platform_posix.hpp +#endif + +#ifndef DL +#error "No platform with dynamic loading found" +#endif diff --git a/arbor/include/arbor/dl_platform_posix.hpp b/arbor/include/arbor/dl_platform_posix.hpp new file mode 100644 index 0000000000..1ff1c6f2d0 --- /dev/null +++ b/arbor/include/arbor/dl_platform_posix.hpp @@ -0,0 +1,53 @@ +#pragma once + +#include + +#include + +#include + +#include "util/strprintf.hpp" + +#define DL "POSIX" + +namespace arb { + +struct dl_error: arbor_exception { + dl_error(const std::string& msg): arbor_exception{msg} {} +}; + +struct dl_handle { + void* dl = nullptr; +}; + +dl_handle dl_open(const std::string& fn) { + // Call once to clear errors not caused by us + dlerror(); + auto result = dlopen(fn.c_str(), RTLD_LAZY); + // dlopen fails by returning NULL + if (nullptr == result) { + auto error = dlerror(); + throw dl_error{util::pprintf("[POSIX] dl_open failed with: {}", error)}; + } + return {result}; +} + +template +T dl_get_symbol(const dl_handle& handle, const std::string& symbol) { + // Call once to clear errors not caused by us + dlerror(); + auto result = dlsym(handle.dl, symbol.c_str()); + // dlsym mayb return NULL even if succeeding + auto error = dlerror(); + if (error) { + throw dl_error{util::pprintf("[POSIX] dl_get_symbol failed with: {}", error)}; + } + return reinterpret_cast(result); +} + +void dl_close(dl_handle& handle) { + dlclose(handle.dl); + handle.dl = nullptr; +} + +} diff --git a/arbor/mechcat.cpp b/arbor/mechcat.cpp index 3109f17e36..b6856b2ab6 100644 --- a/arbor/mechcat.cpp +++ b/arbor/mechcat.cpp @@ -5,8 +5,6 @@ #include #include -#include - #include #include #include @@ -18,6 +16,8 @@ #include "util/maputil.hpp" #include "util/span.hpp" +#include "dl.hpp" + /* Notes on implementation: * * The catalogue maintains the following data: @@ -584,20 +584,11 @@ std::pair 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"); + auto plugin = dl_open(fn); + auto get_catalogue = dl_get_symbol(plugin, "get_catalogue"); /* 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 From 82e31ee762c5ce5edfe19e1812c39cd50aba7b1f Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Wed, 6 Oct 2021 20:26:44 +0200 Subject: [PATCH 02/13] Add bad_catalogue_error, clean-up, mv dl to util. --- arbor/arbexcept.cpp | 10 +++++++++- arbor/include/arbor/arbexcept.hpp | 7 +++++++ arbor/mechcat.cpp | 17 +++++++++++------ arbor/{include/arbor => util}/dl.hpp | 4 ++-- .../arbor => util}/dl_platform_posix.hpp | 7 +++++++ test/unit/test_mechcat.cpp | 2 +- 6 files changed, 37 insertions(+), 10 deletions(-) rename arbor/{include/arbor => util}/dl.hpp (66%) rename arbor/{include/arbor => util}/dl_platform_posix.hpp (86%) diff --git a/arbor/arbexcept.cpp b/arbor/arbexcept.cpp index f5d524dd62..fc4ffe7ec5 100644 --- a/arbor/arbexcept.cpp +++ b/arbor/arbexcept.cpp @@ -124,10 +124,18 @@ 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& 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): arbor_exception(pprintf("ABI version is not supported by this version of arbor '{}'", v)), version{v} {} diff --git a/arbor/include/arbor/arbexcept.hpp b/arbor/include/arbor/arbexcept.hpp index 6677ebd6ed..2185c5dfbf 100644 --- a/arbor/include/arbor/arbexcept.hpp +++ b/arbor/include/arbor/arbexcept.hpp @@ -146,6 +146,13 @@ struct file_not_found_error: arbor_exception { std::string filename; }; +// +struct bad_catalogue_error: arbor_exception { + bad_catalogue_error(const std::string&); + bad_catalogue_error(const std::string&, const std::any&); + std::any platform_error; +}; + // ABI errors struct bad_alignment: arbor_exception { diff --git a/arbor/mechcat.cpp b/arbor/mechcat.cpp index b6856b2ab6..829339fd39 100644 --- a/arbor/mechcat.cpp +++ b/arbor/mechcat.cpp @@ -15,8 +15,7 @@ #include "util/rangeutil.hpp" #include "util/maputil.hpp" #include "util/span.hpp" - -#include "dl.hpp" +#include "util/dl.hpp" /* Notes on implementation: * @@ -586,10 +585,16 @@ mechanism_catalogue::~mechanism_catalogue() = default; const mechanism_catalogue& load_catalogue(const std::string& fn) { typedef const void* global_catalogue_t(); - - auto plugin = dl_open(fn); - auto get_catalogue = dl_get_symbol(plugin, "get_catalogue"); - + global_catalogue_t* get_catalogue = nullptr; + try { + auto plugin = dl_open(fn); + get_catalogue = dl_get_symbol(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/include/arbor/dl.hpp b/arbor/util/dl.hpp similarity index 66% rename from arbor/include/arbor/dl.hpp rename to arbor/util/dl.hpp index ce2360aded..8ffab9e574 100644 --- a/arbor/include/arbor/dl.hpp +++ b/arbor/util/dl.hpp @@ -1,11 +1,11 @@ #pragma once #ifdef __APPLE__ -#include dl_platform_posix.hpp +#include "dl_platform_posix.hpp" #endif #ifdef __linux__ -#include dl_platform_posix.hpp +#include "dl_platform_posix.hpp" #endif #ifndef DL diff --git a/arbor/include/arbor/dl_platform_posix.hpp b/arbor/util/dl_platform_posix.hpp similarity index 86% rename from arbor/include/arbor/dl_platform_posix.hpp rename to arbor/util/dl_platform_posix.hpp index 1ff1c6f2d0..8d09a60331 100644 --- a/arbor/include/arbor/dl_platform_posix.hpp +++ b/arbor/util/dl_platform_posix.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -21,6 +22,12 @@ struct dl_handle { }; dl_handle dl_open(const std::string& fn) { + try { + std::ifstream fd{fn.c_str()}; + if(!fd.good()) throw file_not_found_error{fn}; + } catch(...) { + throw file_not_found_error{fn}; + } // Call once to clear errors not caused by us dlerror(); auto result = dlopen(fn.c_str(), RTLD_LAZY); diff --git a/test/unit/test_mechcat.cpp b/test/unit/test_mechcat.cpp index 488534d00f..4d3d0cd31c 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")); From f119d12f20d7690b50fb3f05d5d7c101e6b20e46 Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Fri, 8 Oct 2021 08:45:22 +0200 Subject: [PATCH 03/13] Clarify compile error, inline for less collisions. --- arbor/util/dl.hpp | 2 +- arbor/util/dl_platform_posix.hpp | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/arbor/util/dl.hpp b/arbor/util/dl.hpp index 8ffab9e574..37937bddab 100644 --- a/arbor/util/dl.hpp +++ b/arbor/util/dl.hpp @@ -9,5 +9,5 @@ #endif #ifndef DL -#error "No platform with dynamic loading found" +#error "No platform support for dynamically loading libraries found." #endif diff --git a/arbor/util/dl_platform_posix.hpp b/arbor/util/dl_platform_posix.hpp index 8d09a60331..50695ded2f 100644 --- a/arbor/util/dl_platform_posix.hpp +++ b/arbor/util/dl_platform_posix.hpp @@ -21,6 +21,7 @@ struct dl_handle { void* dl = nullptr; }; +inline dl_handle dl_open(const std::string& fn) { try { std::ifstream fd{fn.c_str()}; @@ -39,7 +40,7 @@ dl_handle dl_open(const std::string& fn) { return {result}; } -template +template inline T dl_get_symbol(const dl_handle& handle, const std::string& symbol) { // Call once to clear errors not caused by us dlerror(); @@ -52,6 +53,7 @@ T dl_get_symbol(const dl_handle& handle, const std::string& symbol) { return reinterpret_cast(result); } +inline void dl_close(dl_handle& handle) { dlclose(handle.dl); handle.dl = nullptr; From 66bbdbd9c3bfc0fe87527ebe721b9cba636f615a Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Fri, 8 Oct 2021 08:58:58 +0200 Subject: [PATCH 04/13] Lean on CMake for platform bridges. --- CMakeLists.txt | 8 +++++++- arbor/util/dl.hpp | 10 +--------- arbor/util/dl_platform_posix.hpp | 2 -- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ccdfe0db06..982679fd56 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 ${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/util/dl.hpp b/arbor/util/dl.hpp index 37937bddab..68c44a1665 100644 --- a/arbor/util/dl.hpp +++ b/arbor/util/dl.hpp @@ -1,13 +1,5 @@ #pragma once -#ifdef __APPLE__ +#ifdef ARB_HAVE_DL #include "dl_platform_posix.hpp" #endif - -#ifdef __linux__ -#include "dl_platform_posix.hpp" -#endif - -#ifndef DL -#error "No platform support for dynamically loading libraries found." -#endif diff --git a/arbor/util/dl_platform_posix.hpp b/arbor/util/dl_platform_posix.hpp index 50695ded2f..e2eb2e41c8 100644 --- a/arbor/util/dl_platform_posix.hpp +++ b/arbor/util/dl_platform_posix.hpp @@ -9,8 +9,6 @@ #include "util/strprintf.hpp" -#define DL "POSIX" - namespace arb { struct dl_error: arbor_exception { From 12302c3a22d4702994c62d8c98608b7002a3d936 Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Fri, 8 Oct 2021 09:03:10 +0200 Subject: [PATCH 05/13] Appease CMake. --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 982679fd56..a2d3ce9076 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -189,7 +189,7 @@ add_library(arbor-private-deps INTERFACE) 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 ${CMAKE_DL_LIBS}) + target_link_libraries(arbor-private-deps INTERFACE ${CMAKE_DL_LIBS}) else() message(FATAL_ERROR "No support for dynamic loading on current platform.") endif () From 35edd7113ae35a3d45f2ace7f7f87988ac88a052 Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Fri, 8 Oct 2021 09:17:31 +0200 Subject: [PATCH 06/13] Add cxx->py exception translation. --- python/pyarb.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/python/pyarb.cpp b/python/pyarb.cpp index 9a6782b87f..cc4080b57e 100644 --- a/python/pyarb.cpp +++ b/python/pyarb.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include "pyarb.hpp" @@ -43,6 +44,9 @@ PYBIND11_MODULE(_arbor, m) { m.doc() = "arbor: multicompartment neural network models."; m.attr("__version__") = ARB_VERSION; + // Translate to Arbor error -> Python exceptions. + pybind11::register_exception(m, "FileNotFoundError"); + pyarb::register_cable_loader(m); pyarb::register_cable_probes(m, global_ptr); pyarb::register_cells(m); From 8c0dba8fd30b64dcb0dd85e9d9d0b1e5ef75b02f Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Fri, 8 Oct 2021 11:47:54 +0200 Subject: [PATCH 07/13] Fix py-test. --- python/test/unit/test_catalogues.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/test/unit/test_catalogues.py b/python/test/unit/test_catalogues.py index 4009192ba3..00cc2ff8af 100644 --- a/python/test/unit/test_catalogues.py +++ b/python/test/unit/test_catalogues.py @@ -48,7 +48,7 @@ def cell_description(self, gid): 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): From da3237e01c883ae70b382074895a973c7129b609 Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Fri, 8 Oct 2021 12:34:22 +0200 Subject: [PATCH 08/13] Use base class for exc tr. --- python/pyarb.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/pyarb.cpp b/python/pyarb.cpp index cc4080b57e..2f2de6dce9 100644 --- a/python/pyarb.cpp +++ b/python/pyarb.cpp @@ -45,7 +45,7 @@ PYBIND11_MODULE(_arbor, m) { m.attr("__version__") = ARB_VERSION; // Translate to Arbor error -> Python exceptions. - pybind11::register_exception(m, "FileNotFoundError"); + pybind11::register_exception(m, "FileNotFoundError", PyExc_FileNotFoundError); pyarb::register_cable_loader(m); pyarb::register_cable_probes(m, global_ptr); From a4acd1d6907e2bd90daf986618cc940128662e23 Mon Sep 17 00:00:00 2001 From: Brent Huisman Date: Fri, 8 Oct 2021 14:13:20 +0200 Subject: [PATCH 09/13] merge test of catalogue --- .github/workflows/basic.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/basic.yml b/.github/workflows/basic.yml index 5e22c96e1f..a7ac1531c0 100644 --- a/.github/workflows/basic.yml +++ b/.github/workflows/basic.yml @@ -180,5 +180,11 @@ jobs: run: mpirun -n 4 -oversubscribe python python/test/unit_distributed/runner.py -v2 - name: Run Python examples run: scripts/run_python_examples.sh - - name: Build a catalogue - run: build-catalogue -v default mechanisms/default + - name: Build and test a catalogue + run: | + build-catalogue -v default mechanisms/default + pwd + python3 -c 'import os; print(os.getcwd()); print(os.listdir())' + python3 -c 'from ctypes import CDLL;CDLL("default-catalogue.so")' + python3 -c 'open("default-catalogue.so")' + python3 -c 'import arbor; print([n for n in arbor.load_catalogue("default-catalogue.so").keys()])' From 4a68343c72c309245fcfc77a1f9b39f16b4421f3 Mon Sep 17 00:00:00 2001 From: bcumming Date: Mon, 8 Nov 2021 09:23:12 +0100 Subject: [PATCH 10/13] update pybind --- python/pybind11 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/pybind11 b/python/pybind11 index 750e38dcfd..f7b499615e 160000 --- a/python/pybind11 +++ b/python/pybind11 @@ -1 +1 @@ -Subproject commit 750e38dcfdac59231a615580d4e6cb3647b31779 +Subproject commit f7b499615e14d70ab098a20deb0cdb3889998a1a From 7e517abe50e24fbae969cbb7c33864c747e6de71 Mon Sep 17 00:00:00 2001 From: bcumming Date: Mon, 8 Nov 2021 09:55:29 +0100 Subject: [PATCH 11/13] rename dylib --- arbor/CMakeLists.txt | 2 +- arbor/mechcat.cpp | 2 +- arbor/util/{dl_platform_posix.cpp => dylib.cpp} | 2 +- arbor/util/{dl_platform_posix.hpp => dylib.hpp} | 0 4 files changed, 3 insertions(+), 3 deletions(-) rename arbor/util/{dl_platform_posix.cpp => dylib.cpp} (97%) rename arbor/util/{dl_platform_posix.hpp => dylib.hpp} (100%) diff --git a/arbor/CMakeLists.txt b/arbor/CMakeLists.txt index 80a226ceec..b4c1b99479 100644 --- a/arbor/CMakeLists.txt +++ b/arbor/CMakeLists.txt @@ -55,9 +55,9 @@ set(arbor_sources threading/threading.cpp thread_private_spike_store.cpp tree.cpp + util/dylib.cpp util/hostname.cpp util/unwind.cpp - util/dl_platform_posix.cpp version.cpp ) diff --git a/arbor/mechcat.cpp b/arbor/mechcat.cpp index 1ab96330ef..8bde96e4ba 100644 --- a/arbor/mechcat.cpp +++ b/arbor/mechcat.cpp @@ -12,7 +12,7 @@ #include #include -#include "util/dl_platform_posix.hpp" +#include "util/dylib.hpp" #include "util/maputil.hpp" #include "util/rangeutil.hpp" #include "util/span.hpp" diff --git a/arbor/util/dl_platform_posix.cpp b/arbor/util/dylib.cpp similarity index 97% rename from arbor/util/dl_platform_posix.cpp rename to arbor/util/dylib.cpp index dda60f02cc..ccc6a92438 100644 --- a/arbor/util/dl_platform_posix.cpp +++ b/arbor/util/dylib.cpp @@ -5,7 +5,7 @@ #include -#include "util/dl_platform_posix.hpp" +#include "util/dylib.hpp" #include "util/strprintf.hpp" namespace arb { diff --git a/arbor/util/dl_platform_posix.hpp b/arbor/util/dylib.hpp similarity index 100% rename from arbor/util/dl_platform_posix.hpp rename to arbor/util/dylib.hpp From de7cdf92d7d5087638800f44155930dd6252bbeb Mon Sep 17 00:00:00 2001 From: bcumming Date: Mon, 8 Nov 2021 14:43:57 +0100 Subject: [PATCH 12/13] address pr changes: remove handle wrapper --- arbor/mechcat.cpp | 8 ++++---- arbor/util/dylib.cpp | 15 +++------------ arbor/util/dylib.hpp | 8 +++++--- scripts/test-catalogue.py | 3 --- 4 files changed, 12 insertions(+), 22 deletions(-) diff --git a/arbor/mechcat.cpp b/arbor/mechcat.cpp index 8bde96e4ba..3796b46aa4 100644 --- a/arbor/mechcat.cpp +++ b/arbor/mechcat.cpp @@ -595,10 +595,10 @@ const mechanism_catalogue& load_catalogue(const std::string& fn) { 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 - leak proper as `dlopen` caches handles for us. + /* The DSO handle is not freed here: handles will be retained until + * termination since the mechanisms provided by the catalogue may have a + * different lifetime than the actual catalogue itfself. This is not a leak, + * as `dlopen` caches handles for us. */ return *((const mechanism_catalogue*)get_catalogue()); } diff --git a/arbor/util/dylib.cpp b/arbor/util/dylib.cpp index ccc6a92438..3f4826dfbd 100644 --- a/arbor/util/dylib.cpp +++ b/arbor/util/dylib.cpp @@ -11,11 +11,7 @@ namespace arb { namespace util { -struct dl_handle { - void* dl = nullptr; -}; - -dl_handle dl_open(const std::string& fn) { +void* dl_open(const std::string& fn) { try { std::ifstream fd{fn.c_str()}; if(!fd.good()) throw file_not_found_error{fn}; @@ -30,12 +26,7 @@ dl_handle dl_open(const std::string& fn) { auto error = dlerror(); throw dl_error{util::pprintf("[POSIX] dl_open failed with: {}", error)}; } - return {result}; -} - -void dl_close(dl_handle& handle) { - dlclose(handle.dl); - handle.dl = nullptr; + return result; } namespace impl{ @@ -46,7 +37,7 @@ void* dl_get_symbol(const std::string& fn, const std::string& symbol) { auto handle = dl_open(fn); // Get symbol from shared object, may return NULL if that is what symbol refers to - auto result = dlsym(handle.dl, symbol.c_str()); + auto result = dlsym(handle, symbol.c_str()); // dlsym mayb return NULL even if succeeding if (auto error = dlerror()) { throw dl_error{util::pprintf("[POSIX] dl_get_symbol failed with: {}", error)}; diff --git a/arbor/util/dylib.hpp b/arbor/util/dylib.hpp index 073002bcdd..5df6a4f2ef 100644 --- a/arbor/util/dylib.hpp +++ b/arbor/util/dylib.hpp @@ -8,16 +8,18 @@ namespace arb { namespace util { namespace impl{ -void* dl_get_symbol(const std::string& fn, const std::string& symbol); +void* dl_get_symbol(const std::string& filename, const std::string& symbol); } // namespace impl struct dl_error: arbor_exception { dl_error(const std::string& msg): arbor_exception{msg} {} }; +// Find and return a symbol from a dynamic library with filename. +// Throws dl_error on error. template -T dl_get_symbol(const std::string& fn, const std::string& symbol) { - return reinterpret_cast(impl::dl_get_symbol(fn, symbol)); +T dl_get_symbol(const std::string& filename, const std::string& symbol) { + return reinterpret_cast(impl::dl_get_symbol(filename, symbol)); } } // namespace util diff --git a/scripts/test-catalogue.py b/scripts/test-catalogue.py index 5f6a051863..7c4716d03e 100755 --- a/scripts/test-catalogue.py +++ b/scripts/test-catalogue.py @@ -1,7 +1,6 @@ #!/usr/bin/env python3 import argparse -import ctypes import os import sys @@ -19,6 +18,4 @@ print('ERROR: unable to open catalogue file') sys.exit(1) -print(ctypes.CDLL(catname)) - print([n for n in arbor.load_catalogue(catname).keys()]) From bffe46f65b5e92d0f77ffcf2a248c89e18304347 Mon Sep 17 00:00:00 2001 From: bcumming Date: Fri, 26 Nov 2021 11:06:10 +0100 Subject: [PATCH 13/13] fix small type: pr review --- arbor/util/dylib.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arbor/util/dylib.hpp b/arbor/util/dylib.hpp index 5df6a4f2ef..ba97976a6a 100644 --- a/arbor/util/dylib.hpp +++ b/arbor/util/dylib.hpp @@ -23,4 +23,4 @@ T dl_get_symbol(const std::string& filename, const std::string& symbol) { } } // namespace util -} // namespace arbo +} // namespace arbor