-
Notifications
You must be signed in to change notification settings - Fork 180
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
Add unittest for costModelResidualPairCollision #967
Conversation
@edantec The unittests are segfaulting. Please check where this segfault is coming from. |
The test also needs checking for whether Pinocchio is built with HPP-FCL |
This segfault is not coming from the code or the test, but after the tests. We have this kind of issues everywhere, ref eg. https://gitlab.laas.fr/loco-3d/crocoddyl/-/issues/288. So either we have no idea how to properly code in C++ and we are handling pointers in a bad way leading to possible double free or something like that ; or the segfault just come from Boost::unit_test_framework. It usually hit on Arch and Debian (CI on Debian is most of the time ignored because of that), but rarely on Ubuntu, no matter which versions. I was sometimes able to workaround that by splitting the failing tests in two different unit tests, and with exactly the same code, but not at the same place, the issue disappeared. |
I confirm that on my side, my tests are not segfaulting, and I'm working with Ubuntu |
eg. on Debian 9, with set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fno-omit-frame-pointer -fsanitize=address")
set (CMAKE_LINKER_FLAGS_DEBUG "${CMAKE_LINKER_FLAGS_DEBUG} -fno-omit-frame-pointer -fsanitize=address")
I don't see a single thing from our codebase in this backtrace. This doesn't mean that the bug is not from our side, but as I can't reproduce this on Ubuntu… I'm really wondering if this is worth it to investigate more, or just live with it, or even try the same test in another unit test framework. |
I think we should investigate it. I also spent (quite time ago) a bit of my time, but unfortunately, I have no clue and little bandwidth for it. If this task requires time (which I believe so) and everyone agree, then we could decide to allow failures in Buster. |
I tried: using clang 8 on 18.04, with or without ASan, doesn't trigger the issue. |
What is the current state of this PR? How can I help to pass it? |
@edantec I don't think you have anything to do immediately. This is a issue that we need to investigate first. What I can propose is that we leave this open for now, and I'll investigate this in the next week. |
@edantec -- could you rebase this branch? |
@wxmerkt -- it seems to me that we are not install Pinocchio with |
Correct, the ROS-Pinocchio-binary does not compile with HPP-FCL right now. I can make a HPP-FCL release soon - it would take a few weeks to become available though. In either case, we should guard the code with the |
It would be useful to release and integrate such kind of binaries and CI.
I agree. I've disabled the compilation of these unit-tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edantec -- I have made all the modifications needed to merge this PR.
I will merge it as soon as all the jobs pass.
Proper rebase of the unittest for pair collision