Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: use canonical paths and a set when checking loaded dlls #2830

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

anilbey
Copy link
Contributor

@anilbey anilbey commented Apr 11, 2024

Uses a set of canonical path strings to fix the checking of already loaded dlls. It prevents the SEGFAULTs raised from trying to load the same dlls/mods using Python API.

Addresses a part of #2741.

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 67.15%. Comparing base (30844e7) to head (59b543e).

Files Patch % Lines
share/lib/python/neuron/tests/test_neuron.py 28.57% 15 Missing ⚠️
share/lib/python/neuron/__init__.py 66.66% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2830   +/-   ##
=======================================
  Coverage   67.15%   67.15%           
=======================================
  Files         562      562           
  Lines      104082   104102   +20     
=======================================
+ Hits        69896    69910   +14     
- Misses      34186    34192    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anilbey
Copy link
Contributor Author

anilbey commented Apr 11, 2024

The unit tests are added. ✔️

Copy link

sonarcloud bot commented Apr 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
6.4% Duplication on New Code

See analysis details on SonarCloud

@anilbey
Copy link
Contributor Author

anilbey commented Apr 11, 2024

Note: codecov is wrong that is pointing to a problem with the CI's coverage calculations. Perhaps that can be addressed in another PR.

@nrnhines
Copy link
Member

nrnhines commented Apr 11, 2024

Addresses a part of #2741.

I take it that this PR is not intended to deal with dlls loaded via h.nrn_load_dll(...), default loading of, e.g. x86_64/libnrnmech.so, or loading via the -dll path option when launching nrniv.

The final common function that dynamically loads libnrnmech(.dll, .so, .dylib) files is nrn/src/nrnoc/init.cpp: int mswin_load_dll(const char* cp1). On Darwin, cp1 will be converted to a full path before using it in the arch specific dlopen.

Given a /home/hines/neuron/nrn/build/install/share/nrn/demo/release/x86_64/libnrnmech.so
Then

$ cd /home/hines/neuron/nrn/build/install/share/nrn/demo/release
$ python
Python 3.12.2 (main, Feb 10 2024, 13:25:44) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from  neuron import h
>>> h.nrnversion()
'NEURON -- VERSION 9.0a-212-gd3d888692 master (d3d888692) 2024-04-11'
>>> h.nrn_load_dll("/home/hines/neuron/nrn/build/install/share/nrn/demo/release/x86_64/libnrnmech.so")
NEURON: The user defined name already exists: cadifpmp
 near line 0
 objref hoc_obj_[2]
                   ^
        nrn_load_dll("/home/hine...")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: hocobj_call error: hoc_execerror: The user defined name already exists: cadifpmp
>>>

can get the same effect from another path location with

$ cd $HOME
$ export NEURON_MODULE_OPTIONS="-dll /home/hines/neuron/nrn/build/install/share/nrn/demo/release/x86_64/libnrnmech.so"
$ python
Python 3.12.2 (main, Feb 10 2024, 13:25:44) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from neuron import h
>>> h.nrn_load_dll("/home/hines/neuron/nrn/build/install/share/nrn/demo/release/x86_64/libnrnmech.so")
NEURON: The user defined name already exists: cadifpmp
 near line 0
 objref hoc_obj_[2]
                   ^
        nrn_load_dll("/home/hine...")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: hocobj_call error: hoc_execerror: The user defined name already exists: cadifpmp
>>> 

@anilbey
Copy link
Contributor Author

anilbey commented Apr 12, 2024

I take it that this PR is not intended to deal with dlls loaded via h.nrn_load_dll(...),

Yes correct but it still prevents a source of segmentation fault errors that are raised due to the load_mechanisms Python function.

@anilbey
Copy link
Contributor Author

anilbey commented Apr 12, 2024

The final common function that dynamically loads libnrnmech(.dll, .so, .dylib) files is nrn/src/nrnoc/init.cpp: int mswin_load_dll(const char* cp1)

Thanks for the hint. I tried to modify that function in here. At the moment it is failing only for macOS-13 platform for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants