-
Notifications
You must be signed in to change notification settings - Fork 141
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 singleton::is_destroyed #69
Conversation
I hit the same problem:
Alas, this patch does not actually fix it. When applied, an assertion is hit:
|
@vasild Do you have a suitably small testcase? |
@masterleinad No :( I just built https://github.com/monero-project/monero/releases/tag/v0.11.1.0 and ran |
There you go:
|
@vasild In the code above you are trying to load from an empty archive and this is causing the error. If you fill the archive before as follows everything should be fine:
|
All of the above is with unmodified Boost 1.65.1. When the patch from this pull request is applied, both programs produce the same result during
|
I dug a little bit deeper into this. Test program
Notice that function Crashes as mentioned above with clang 5.0.0, clang 6.0.0 and gcc 7.2.1. Not compiler or OS dependent, although I have this option set to
Without that the bug may remain unnoticed because using a memory that has just been I confirmed that the bug was introduced in b0a794d. There is no crash with For some reason the patch in this pull request produces an assertion failure when applied to the released Boost 1.65.1 (see above), but when applied to the latest code from git daf20da it fixes the bug (no crash or assert). So, I guess that if @robertramey confirms it is ok to revert the main part of b0a794d, then the patch in thus pull request should be pushed and included in the next release. |
I'm interested in this but don't really understand what's being asked for.
|
@robertramey Hello, shortly - the question to you is - do you think that the patch in this pull request is ok to be merged - i.e. do you approve it? Longer story:
To me it looks like b0a794d has three unrelated changes - one MinGW change in |
it would probably be good to add this as a regression test in Boost::serialization. |
What does "this" refer to? The patch eliminates the changes intended to address issue related to "visibility" on various platforms. Have you actually run the test test suite with this patch included? With the shared build? On which platforms/compilers? |
I mean the test program by @vasild which shows the issue. So if the issue is fixed by this patch, I think it's good to have a regression test to make sure the same issue does not appear again later.
I have not. I just assume that prior to release boost runs the testsuite on for target compilers/os/etc and since this issue is present in 1.65.1. then most likely there was not a failing test to indicate it. |
I ran the test and it passes under Xcode on my machine and it passes with no problem - Of course. I'm running the static build with clang. This illustrates the problem with just merging in a PR. The whole "visibility" facility is required for a library of this size, but it's implemented with different syntax and rules among the various compilers. Fixing one usually breaks another so the whole thing turns into an endless game of whack-a-mole. For now, I'll add this to the test suite and check it in so it shows up on the Boost test matrix. Hopefully that will shed some light on the issue. Update: I just ran the test on all combinations of the following link=static,shared variant=debug,release toolset=darwin-6.2 The test is at the last line labeled test_z. Tests not run are labeled "missing" BTW there is currently a test in the boost test matrix labeled test_inclusion which pretty much does nothing and still fails. I'm thinking it may be the same test test we're looking at here. You might want to check that out see if it's the same. |
@robertramey so you ran the test program from #69 (comment) on unpatched Boost (without this PR) and got no crash? But did you instruct your allocator to wipe memory after |
yep
Nope, I actually don't know how to do either of these neither from Xcode nor from the command line. I did managed to enable the address sanitizer from Xcode and got a bunch of linker errors. I would like to enable the sanitizes in general. But I can't do that be cause the many of the tests for de-serialization of pointers have memory leaks so I get too many false positives. Update the tests to eliminate the false positives would be a big job give the number of tests we have. |
@robertramey Alright, then what you see is expected - the bug remains hidden because the code executed at program exit does something equivalent to:
this may work in some cases and remain unnoticed. Here is how to tell Mac OS X's malloc to rewrite the memory after
So, I guess it would be something like
About Valgrind, well that would be:
|
Per the instructions invoked:
|
@robertramey what about the test program I posted? Maybe the tests above are somewhat different?
|
it's the same program I ran, I copied from the post above. I just named it test_z.cpp. Note that boost test matrix here http://www.boost.org/development/tests/develop/developer/serialization.html Has a test named test_inclusion which I believe is showing similar problems on some platforms. I'm speculating that on some platforms some code doesn't get linked in properly or at all and this shows up as a linking error or as a runtime error when trying to run code from a shared library. Is your combination/platform on the boost regression tests. If so which one is it? Is the library passing all tests there? |
@robertramey I will try to reproduce this on Linux... |
The changes to singleton should only make a difference for a shared library implementation. The test matrix doesn't indicate whether the shared or static library is being used. So I don't know that the test matrix is helpful here. |
I can't seem to be able to reproduce the crash with my test prog on Linux with gcc 7.2.0 or clang 6.0.0. Also, So:
@masterleinad Can you demonstrate the bug in your environment with:
either under Valgrind, or by configuring malloc to wipe memory after PS yes, I am using shared libraries (libboost_serialization.so). |
@vasild No, I don't find any boost version for which this code would fail for me. Neither for The original cases I wrote this PR for are rather complex. I try to come up with a minimal example... |
@masterleinad I guess you tried with Valgrind? |
Yes, I tried with valgrind and |
Closed in favor of #79. |
Although this is closed I wanted to say you were on the right track. The issue is the following:
The problem is very complicated to reproduce, due to unknown init/finalize order. I assume: So one problem is the usage of the |
Wow, finally @Flamefire nailed this down! Thanks for the thorough analysis! |
@masterleinad What did you mean with
Every instance should be deleted exactly once. Not more, not less. Do you have anything to check this? |
This partially reverses commit b0a794d.
Apparently, the logic related to
causes problem with gcc. In particular, a nested
type
might get deleted multiple times causing a segmentation fault.Reverting back to the implementation in which this piece of code is outsourced in a
detail
namespace, solves the issues in dealii/dealii#5262 and the ones reported in https://svn.boost.org/trac10/ticket/13186.