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

avoid python C API segfault on intel mac #2455

Merged
merged 2 commits into from Apr 21, 2023

Conversation

stchaker
Copy link
Contributor

@stchaker stchaker commented Apr 4, 2023

You can't reliably use extern "C" std::strings. The python C API ends up basically reinterpret casting path_statepoint, which is an std::string to char*. Depending on the libc++ implementation, this may segfault, as it does on my intel Mac. This failure can be seen in the cmfd_restart test, with this output below.

test.py Fatal Python error: Segmentation fault

Current thread 0x00007ff85fd97340 (most recent call first):
  File "/Users/sebastiantchakerian/Codes/openmc/openmc/lib/settings.py", line 59 in path_statepoint
  File "/Users/sebastiantchakerian/Codes/openmc/openmc/cmfd.py", line 1036 in _configure_cmfd
  File "/Users/sebastiantchakerian/Codes/openmc/openmc/cmfd.py", line 837 in init
  File "/Users/sebastiantchakerian/Codes/openmc/openmc/cmfd.py", line 812 in run_in_memory
  File "/usr/local/Cellar/python@3.9/3.9.16/Frameworks/Python.framework/Versions/3.9/lib/python3.9/contextlib.py", line 119 in __enter__
  File "/Users/sebastiantchakerian/Codes/openmc/openmc/cmfd.py", line 778 in run
  File "/Users/sebastiantchakerian/Codes/openmc/tests/regression_tests/cmfd_restart/test.py", line 29 in execute_test
  File "/Users/sebastiantchakerian/Codes/openmc/tests/testing_harness.py", line 42 in main
  File "/Users/sebastiantchakerian/Codes/openmc/tests/regression_tests/cmfd_restart/test.py", line 72 in test_cmfd_restart
  File "/usr/local/lib/python3.9/site-packages/_pytest/python.py", line 195 in pytest_pyfunc_call
  File "/usr/local/lib/python3.9/site-packages/pluggy/_callers.py", line 39 in _multicall
  File "/usr/local/lib/python3.9/site-packages/pluggy/_manager.py", line 80 in _hookexec
  File "/usr/local/lib/python3.9/site-packages/pluggy/_hooks.py", line 265 in __call__
  File "/usr/local/lib/python3.9/site-packages/_pytest/python.py", line 1789 in runtest
  File "/usr/local/lib/python3.9/site-packages/_pytest/runner.py", line 167 in pytest_runtest_call
  File "/usr/local/lib/python3.9/site-packages/pluggy/_callers.py", line 39 in _multicall
  File "/usr/local/lib/python3.9/site-packages/pluggy/_manager.py", line 80 in _hookexec
  File "/usr/local/lib/python3.9/site-packages/pluggy/_hooks.py", line 265 in __call__
  File "/usr/local/lib/python3.9/site-packages/_pytest/runner.py", line 260 in <lambda>
  File "/usr/local/lib/python3.9/site-packages/_pytest/runner.py", line 339 in from_call
  File "/usr/local/lib/python3.9/site-packages/_pytest/runner.py", line 259 in call_runtest_hook
  File "/usr/local/lib/python3.9/site-packages/_pytest/runner.py", line 220 in call_and_report
  File "/usr/local/lib/python3.9/site-packages/_pytest/runner.py", line 131 in runtestprotocol
  File "/usr/local/lib/python3.9/site-packages/_pytest/runner.py", line 112 in pytest_runtest_protocol
  File "/usr/local/lib/python3.9/site-packages/pluggy/_callers.py", line 39 in _multicall
  File "/usr/local/lib/python3.9/site-packages/pluggy/_manager.py", line 80 in _hookexec
  File "/usr/local/lib/python3.9/site-packages/pluggy/_hooks.py", line 265 in __call__
  File "/usr/local/lib/python3.9/site-packages/_pytest/main.py", line 349 in pytest_runtestloop
  File "/usr/local/lib/python3.9/site-packages/pluggy/_callers.py", line 39 in _multicall
  File "/usr/local/lib/python3.9/site-packages/pluggy/_manager.py", line 80 in _hookexec
  File "/usr/local/lib/python3.9/site-packages/pluggy/_hooks.py", line 265 in __call__
  File "/usr/local/lib/python3.9/site-packages/_pytest/main.py", line 324 in _main
  File "/usr/local/lib/python3.9/site-packages/_pytest/main.py", line 270 in wrap_session
  File "/usr/local/lib/python3.9/site-packages/_pytest/main.py", line 317 in pytest_cmdline_main
  File "/usr/local/lib/python3.9/site-packages/pluggy/_callers.py", line 39 in _multicall
  File "/usr/local/lib/python3.9/site-packages/pluggy/_manager.py", line 80 in _hookexec
  File "/usr/local/lib/python3.9/site-packages/pluggy/_hooks.py", line 265 in __call__
  File "/usr/local/lib/python3.9/site-packages/_pytest/config/__init__.py", line 167 in main
  File "/usr/local/lib/python3.9/site-packages/_pytest/config/__init__.py", line 190 in console_main
  File "/usr/local/bin/pytest", line 8 in <module>
[1]    14769 segmentation fault  pytest

Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, except this one thing. Just remove the empty_string.

src/settings.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix @stchaker. One more update you'll need in here is that we have an associated Python binding to this variable (hence why it is extern "C" in the first place). That one will need to be updated too:

@property
def path_statepoint(self):
path = c_char_p.in_dll(_dll, 'path_statepoint').value
return path.decode()

include/openmc/settings.h Outdated Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
@paulromano
Copy link
Contributor

@stchaker are you interested in getting this branch updated? The review comments are very minor and should be easy to address.

@stchaker
Copy link
Contributor Author

Sorry for the late reply/fix! I got swamped with some exams over the past two weeks. I went ahead and made those changes, thanks!

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stchaker!

@paulromano
Copy link
Contributor

@gridley Let us know if you have further requests on this PR. If not, please approve so that it can be merged.

@paulromano paulromano merged commit fffdedf into openmc-dev:develop Apr 21, 2023
17 checks passed
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

3 participants