Eliminate complex typed static variables #37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Complex typed static variables (e.g. maps) in dll-s are to avoid. On Windows they are not initialised in time (or at all) when the dll is dynamically loaded. The current use of static QHash data members results in crash on Windows when the PythonQt.dll is loaded. See details here: http://stackoverflow.com/questions/5114683/loading-dll-not-initializing-static-c-classes
|
Thanks for the updated PR. I will test and integrate later this afternoon (EST time) |
|
As suggested by Florian, may be a first patch would be to |
|
@espakm Based on the description I pushed a branch where I added a test attempting to reproduce the problem: See patched-5...jcfr:37-python-init-crash But it passes on both Linux and Windows (using either VS2008 or VS2013 for a Release build of PythonQt built against a release build of Qt and Python) |
|
Moving execution of PythonQt tests into the static initializer causes the test to fail. Failure is the following: |
|
Commit c891272 causes test to also fail on Linux |
|
"Moving execution of PythonQt tests into the static initializer causes the test to fail." Yes, that's exactly the point. The crash happens when PythonQt is initialised from the static initialiser of a library. (And PythonQt has not been loaded beforehand.) |
|
Could you try if my original fix that eliminated the complex typed static local variables lets the test pass? You can apply it cleanly after reverting c891272. Today I tried what happens if I declare the static maps as static locals and return a reference to them from the getters. It crashes. So, this tells me that we thought it wrong that the static locals are constructed at the first invocation. What means that compound typed static locals must be eliminated throughout the library. I might be wrong, but thanks to your new test, it is easy to check. |
Baseline results
To be exact, initializing PythonQt from within the static initializer is not causing the crash. But running the tests suite during static initialization is what is causing problem. I updated test framework to include these two tests:
Question: @espakm Does this capture the problem your are experiencing ? I want to make sure we work considering a valid scenario. The topic I will use to test your fixes (and mine) will be this one: jcfr:37-python-init-crash Documentation describing the two tests is here Within no specific fixes, we have the following baseline:
Results with espakm original fix
Applying your original fix NifTK/PythonQt@127d894 on top of
Results with espakm static maps and locals fix
Change integrated in jcfr:37-python-init-crash-espakm-static-maps-and-locals-fix
MiscellaneousAs a side note, here is a one-liner allowing to list all location where static variable are declared: |
|
More details about the failure. I further instrumented the test infrastructure to run all sub test cases for each scenario. That give a better idea of what is failing: |
|
Commit 949c056� (that is the HEAD of jcfr:37-python-init-crash now) crashes for me at this line: Line 1302 in 949c056 Tested on Windows with VS2012, release mode with debug symbols. Full stack trace: |
|
This is one of the variables that you found with your regex. Note that int-s and pointers are fine. I will make a patch and run the new tests as well. |
|
I tried to track down the tests_PythonQtDynamicLoading_RUN_TESTSUITE_IN_STATIC_INITIALIZER_1_Api_testCall crash. It is not related to the static variables. Something has not be initialised for the signal-slot connection. Call stack before the crash: Hope that helps. |
|
Thanks for the update. Do you have the result of all tests on your system ? |
|
|
It looks the same as your test results. |
|
I tried debug mode as well but then it crashes much earlier. But that's another issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Complex typed static variables (e.g. maps) in dll-s are to avoid.
On Windows they are initialised late when the dll
is being loaded dynamically. The current use of static QHash data members
results in crash on Windows when a PythonQt.dll static function is called from a static initialization code from a dependent library, and PythonQt.dll has not been loaded before, already.
See details here:
http://stackoverflow.com/questions/5114683/loading-dll-not-initializing-static-c-classes