Comments (18)
In GitLab by @lettrich on Jul 2, 2017, 12:30
created branch 42-segfault-learnersgdeonofftest
from sgpp.
In GitLab by @lettrich on Jul 3, 2017, 08:51
First of all I was able to reproduce the error on my system and I am very surprised that I did not see it in the first place. Also my tests have shown, that it seems to be older then commit b8f5768 . I will need more time to look into that but I'm working on it and will keep you updated about my progress.
from sgpp.
In GitLab by @valentjn on Jul 3, 2017, 09:23
You know about git bisect
? It will tell you the right commit. If you have a script which tells good and bad commits apart, it even works automagically.
from sgpp.
In GitLab by @valentjn on Jul 3, 2017, 09:24
Is it possible to add a test (after you've found/fixed it) to avoid regression bugs?
from sgpp.
In GitLab by @lettrich on Jul 3, 2017, 09:47
I will see what I can do. I'm officially out of the game, but since it feels now that sgpp is also partially my baby I feel a certain responsibility ;) Nevertheless I want to point out, that tests for those classes should have been @maierjn 's work in the first place. My additions all provide unit tests. (however it is my stupidity in the second place to refactor untested code and wonder that its not bug free :D )
from sgpp.
In GitLab by @lettrich on Jul 22, 2017, 17:39
OK, so bisecting (very cool feature btw) hints me at commit merge commit 67ad260. This is something to start with but on the other hand terrible news, especially as both branches had working code before. So its back to opening up the debugger and finding out what went wrong
from sgpp.
In GitLab by @lettrich on Jul 22, 2017, 19:33
Ok so the first thing that looks very fishy is, that the pointer to the bounding box of the grid that's evaluated when this thing segfaults looks like it is invalid - the member variables all contain garbage values.
from sgpp.
In GitLab by @lettrich on Jul 22, 2017, 20:34
I found the source of the bug and have to admit, this is extremely neatly hidden. The base::Grid::clone()
member function is buggy. It was edited by me during the coding days to fix a shortcoming of the initial code by @fabianfranzelin. Despite writing unit tests for this function, the simple test was not enough to detect the error.
The quick fix for this is to revert the clone function to the state before 23ac235. Having tried this, I can confirm that it works. However this is only a quick fix, since just copying the grid points of the grid may work for most cases but doesn't work with stretching or bounding box. Unfortunately as far as I can see, there seems to be something wrong with the copy constructor of base::HashGridStorage
or one of its members. I suggest that this is done together while dealing with some of the issues I pointed out in #36.
I am sorry for taking so long to find this, but this took almost an entire day of intense work to find, trace back and debug, since I assumed the error anywhere but in a flawed copy constructor in code I did not write...
Please also note that while I am very happy to have worked with all of you, I have to admit that I am not very keen to implement the "real fix", since I am no longer affiliated with SGpp
.
@waegemans, please tell me if the quickfix works for you, so I can sleep better at night ;)
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 10:49
Wow--thank you so much for the thorough investigation!
Let's look at what you did in Grid::clone
...
newGrid->storage = HashGridStorage{this->storage};
Good lord! π€¦ Never ever do that. Seriously!
What happens if this line gets executed is the following:
- Create a temporary
HashGridStorage
object with the copy constructor, withthis->storage
as argument. - Assign the temporary object to
newGrid->storage
, either usingoperator=
ofHashGridStorage
or, if this doesn't exist (like in this case), using the implicit assignment operation. This copies pointers 1:1 to the new object. - Destruct the temporary object as its lifetime is limited to this single line. Member pointers of
HashGridStorage
aredelete
d. newGrid->storage
will now contain dangling pointers as the pointers were the same as in the temporary object.- Crash.
I've put together a simple test program if you want to experiment [1].
The real culprit is that HashGridStorage
doesn't have a proper assignment operator (the error would've been the same if you did newGrid->storage = this->storage;
). Does anybody know why? I'll just implement a straightforward one.
But even if there would be an operator=
, the line above is still bad code, as a temporary object gets created which could create much unnecessary overhead.
since I assumed the error anywhere but in a flawed copy constructor in code I did not write...
The copy constructor seems to be fine. You should've been looking what that clone
fix you wrote is actually doing, so you're not completely innocent π On the other hand, it's quite astonishing that nobody noticed so far...
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 10:54
mentioned in commit 24935f6
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 11:06
mentioned in merge request !21
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 11:06
Implemented HashGridStorage::operator=
, the example seems to work again. Discuss the fix in MR !21.
from sgpp.
In GitLab by @lettrich on Jul 24, 2017, 11:07
thank you so much for the feedback. Undoubtably you're right about the explicit call of the copy constructor, followed by an assignment. Also sorry for not taking a very detailed look at all of my code before starting to blame others.
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 11:09
You shall be forgiven π
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 16:27
closed via commit 24935f6
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 16:27
closed via commit ca2b9b9
from sgpp.
In GitLab by @valentjn on Jul 24, 2017, 16:27
closed via merge request !21
from sgpp.
In GitLab by @valentjn on Jul 26, 2017, 13:41
mentioned in commit ca2b9b9
from sgpp.
Related Issues (20)
- Issues Installing on Windoze HOT 4
- Travis Minimal. Job Boost Libraries Missing HOT 1
- SG++ lacks clear contributing policies
- Condense evalDx & evalDxDx for certain Bsplines
- Copyright & style (& linter?) checks for Python code
- Python 3.8 support on Windows HOT 2
- Issue With MatLab Binaries HOT 9
- error: βnβ not specified in enclosing βparallelβ HOT 4
- Better release automation with travis HOT 1
- MinGW and localtime_r HOT 3
- DataMatrix::resizeToSubMatrix off-by-two error
- DataMatrix::resizeRowsCols returns wrong matrix HOT 3
- pysgpp fails to import on linux HOT 3
- Avoiding LF/CRLF line terminations issues
- Missing extensions directory (compiling with GCC under Linux) HOT 1
- build fails on AArch64, Fedora 33
- pysgpp does not work on Windows with Python 3.8+ HOT 1
- Several compiler warnings with MSVC++ HOT 1
- Julia API HOT 2
- Compilation problem newer versions because of depreceated "distutils" HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from sgpp.