Fix a memory leak when creating Python3 modules. #2019
Conversation
T045T
force-pushed
the
master
branch
2 times, most recently
from
bf30d61 to
0f5a7bc
Compare
|
Is there an easy way to repro the Python3.5 Travis failure? I'd like to run that with a debugger attached. |
|
So, unrelated to this PR, I've been playing with valgrind and pybind this morning. I can confirm that this solves the 104 byte leak. As for the travis/python3.5 environment, take a look at https://github.com/pybind/pybind11/blob/master/.travis.yml#L70-L76 |
|
Hi @T045T, @bstaletic, would you mind explaining in a bit more detail what the original problem is? Best, |
|
I'm not sure how this PR fixes the leak, but I can provide valgrind output to confirm that it does. Running So pybind is indeed leaking 104 bytes: The "possibly lost" and "still reachable" are from CPython itself, but the "definitely lost" is pybind's fault. Again, I don't know how this pull request works, but the above valgrind command runs clean with this PR. |
|
So, The original code then called It seemed like nothing called So this is how I called So there is a risk that using |
|
I also just pushed an updated version that might resolve the Segfault in the Travis builds – in case either of the pointers is |
|
@T045T In case you haven't, rebase onto latest master, since some travis fixes have been merged. |
include/pybind11/pybind11.h
Outdated
| std::memset(def, 0, sizeof(PyModuleDef)); | ||
| def->m_name = name; | ||
| def->m_doc = doc; | ||
| def->m_size = -1; | ||
| def->m_free = [](void* module) { | ||
| if (module != nullptr) { | ||
| Py_XDECREF( PyModule_GetDef((PyObject*) module)); |
There was a problem hiding this comment.
Fixed. Note that neither of these were reported by tools/check-style.sh
|
Merged, thanks! |
|
Hmm, I realize now that Python 3.5 segfaults likely due to this PR. I'll revert the commit for now until this issue is resolved. |
|
Fair. I hope to get to the bottom of that by the end of the week.
…On Wed, Dec 11, 2019, 20:26 Wenzel Jakob ***@***.***> wrote:
Hmm, I realize now that Python 3.5 segfaults likely due to this PR. I'll
revert the commit for now until this issue is resolved.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2019?email_source=notifications&email_token=AAGUX3IBWBH66QH7OMHQ3U3QYFEHVA5CNFSM4JYMEA7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGUOT7Q#issuecomment-564718078>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGUX3NSUK7DZGKFZPGBZMLQYFEHVANCNFSM4JYMEA7A>
.
|
|
@T045T I was able to repro the segfault with either gcc or clang. The only requirement was a debug version of python. Cmake configuration output: After running
Running the same in gdb: |
|
Here's a minimal repro:
#include <pybind11/pybind11.h>
PYBIND11_MODULE(foo, m){}
|
|
I cannot repro this, either via CMake or your minimal repro case. These are the steps I used to set up the dev environment: # install prerequisites
sudo apt install -y make build-essential libssl-dev zlib1g-dev libbz2-dev libreadline-dev libsqlite3-dev wget curl llvm libncurses5-dev libncursesw5-dev xz-utils tk-dev libffi-dev liblzma-dev python-openssl git valgrind
# set up pyenv
git clone https://github.com/pyenv/pyenv.git ~/.pyenv
echo 'export PYENV_ROOT="$HOME/.pyenv"' >> ~/.bashrc
echo 'export PATH="$PYENV_ROOT/bin:$PATH"' >> ~/.bashrc
echo -e 'if command -v pyenv 1>/dev/null 2>&1; then\n eval "$(pyenv init -)"\nfi' >> ~/.bashrc
source ~/.bashrc
# install Python 3.5.9 with the same build settings as the arch config
PYTHON_CONFIGURE_OPTS="--enable-shared --with-threads --with-computed-gotos --enable-ipv6 --with-valgrind --with-system-expat --with-dbmliborder=gdbm:ndbm --with-system-ffi OPT=\"-fPIC\"" pyenv install 3.5.9
# Tell pyenv to use 3.5.9 in the pybind11 folder (not strictly necessary, except for the python call at the very end).
cd $HOME/pybind11
pyenv local 3.5.9
# build pybind
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Debug -DPYTHON_EXECUTABLE=$(pyenv root)/versions/3.5.9/bin/python3 -DPYBIND11_PYTHON_VERSION=3.5 -DPYBIND11_CPP_STANDARD=-std=c++14 -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON ..
# run test
make pytest -j4
# Alternatively, manually compile repro example
cd $HOME/pybind11
echo -e "#include <pybind11/pybind11.h>\nPYBIND11_MODULE(foo, m){}" > foo.cpp
c++ -shared foo.cpp -o foo.so -fPIC `$(pyenv root)/versions/3.5.9/bin/python3.5-config --cflags --ldflags` -isystem include
python -c 'import foo' |
|
That's strange... I don't see anything different compared to what I have and I can consistently repro. I've actually started with a "debian:stretch" docker container, since that's what Travis is actually running. The following is what travis is executing for the problematic build: export CXX=g++-6 CC=gcc-6
docker pull debian:stretch
containerid=$(docker run --detach --tty \
--volume="$PWD":/pybind11 --workdir=/pybind11 \
--env="CC=$CC" --env="CXX=$CXX" \
debian:stretch)
SCRIPT_RUN_PREFIX="docker exec --tty $containerid"
$SCRIPT_RUN_PREFIX sh -c 'for s in 0 15; do sleep $s; apt-get update && apt-get -qy dist-upgrade && break; done'
cmake --version
$SCRIPT_RUN_PREFIX sh -c "for s in 0 15; do sleep \$s; \
apt-get -qy --no-install-recommends install \
python3.5-dbg python3-scipy-dbg python3.5-dev python3-pytest python3-scipy \
libeigen3-dev libboost-dev cmake make g++-6 && break; done"
$SCRIPT_RUN_PREFIX cmake \
-DCMAKE_BUILD_TYPE=Debug \
-DPYTHON_EXECUTABLE=/usr/bin/python3.5dm \
-DPYBIND11_PYTHON_VERSION=3.5 \
-DPYBIND11_CPP_STANDARD=-std=c++14 \
-DPYBIND11_WERROR=ON \
-DDOWNLOAD_CATCH=ON \
.
$SCRIPT_RUN_PREFIX make pytest -j 2 VERBOSE=1
$SCRIPT_RUN_PREFIX make cpptest -j 2The |
|
@T045T Actually, forget that docker. I know what's wrong with your build step... I had to edit locally the PKGBUILD I linked. Add |
|
That did it, thanks! Confirmed that changing to |
Trying a different approach to pybind#2019. EXPERIMENTAL, PROOF OF CONCEPT, please do not review. If this approach works out additional work is needed to avoid code duplication.
This PR solves the same issue as pybind#2019 (rolled back), but in a way that is certain to be portable and will work for any leak checker. The Python 3 documentation suggests `static` allocation for `PyModuleDef`: * https://docs.python.org/3/c-api/module.html#initializing-c-modules * The module definition struct, which holds all information needed to create a module object. There is usually only one statically initialized variable of this type for each module. This PR changes the `PYBIND11_MODULE` macro accordingly: `static PyModuleDef mdef;` The `pybind11::module::module` code is slightly refactored, with the idea to make the future removal of Python 2 support straightforward.
This PR solves the same issue as pybind#2019 (rolled back), but in a way that is certain to be portable and will work for any leak checker. The Python 3 documentation suggests `static` allocation for `PyModuleDef`: * https://docs.python.org/3/c-api/module.html#initializing-c-modules * The module definition struct, which holds all information needed to create a module object. There is usually only one statically initialized variable of this type for each module. This PR changes the `PYBIND11_MODULE` macro accordingly: `static PyModuleDef mdef;` The `pybind11::module::module` code is slightly refactored, with the idea to make the future removal of Python 2 support straightforward.
This PR solves the same issue as pybind#2019 (rolled back), but in a way that is certain to be portable and will work for any leak checker. The Python 3 documentation suggests `static` allocation for `PyModuleDef`: * https://docs.python.org/3/c-api/module.html#initializing-c-modules * The module definition struct, which holds all information needed to create a module object. There is usually only one statically initialized variable of this type for each module. This PR changes the `PYBIND11_MODULE` macro accordingly: `static PyModuleDef mdef;` The `pybind11::module::module` code is slightly refactored, with the idea to make the future removal of Python 2 support straightforward.
No description provided.